[Student import] Refactor getting a person object from the repo

[netsensei]

Story

The current code loops over the OtherId property, and then tries to query the database via the repo.GetPersonByOtherId method. At the same time, it also performs a check whether or not the id is a historic_ugent_id.

It's a lot of ceremony which could be simplified. Moreover, there's room to make the repo.GetPersonByOtherId do more of the heavy lifting.

Success criteria

• [ ] Simplify getting a person by historic_ugent_id

Screenshots

n/a

Implementation suggestion

np.OtherID is a slice of IdRef structs. Looping across it and fetching the id could / should become part of the repo.GetPersonByOtherId method. This would simplify the code while importing students to this:

oldperson, err = repo.GetPersonByOtherId(ctx, "historic_ugent_id", np.OtherId)
if err != nil {
    return err
}

Note how the entire slice of id's is passed to the function, instead of a single historic_ugent_id id.

Automatic testing scenario

n/a

Additional information

This issue might be deprecated by #40

Related issues

• 31

[nicolasfranck]

@netsensei why would that make it simpler? In that case you would have to guess/know what the method will be doing. Plus you would not be able use the method if you would just have a string value at your disposal.

[netsensei]

@nicolasfranck

• Far less code that needs to be repeated if you want to "get the person via its OtherId".
• That method should accept a []string{}

Additionally, storing all id's as an array of key/value objects makes it far harder to do lookups. Way easier is a hashmap of arrays. With each array being identified by its type:

{
  "historic_ugent_id": [ 111, 222, 333 ],
  "uzgent_id": [ 444, 555, 666]
}

If we store the id's like this, we can just fetch the corresponding array and use functions like sort.Search or the newer slices.BinarySearch to do lookups; instead of iterating over a range.

[netsensei]

This issue might become deprecated / stale due to #40. Review #40 first before working on this one.

[nicolasfranck]

@netsensei I see indeed in my code that historic ugent id is required when fetching data from GISMO, because any linking would be impossible otherwise. See https://github.com/ugent-library/people-service/blob/main/subscribers/gismo_person.go#L72 where it returns an error for that incoming message