Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

deduplicate ilios user records before processing their enrollment. #78

Open
wants to merge 7 commits into
base: MOODLE_404_STABLE
Choose a base branch
from

Conversation

stopfstedt
Copy link
Member

fixes #72

while at it, i reworked how ilios user records without campus ids are
filter out - i made that a pre-processing step to deduplication.
@stopfstedt stopfstedt force-pushed the 72_deal_with_duplicate_user_accounts branch from 2076130 to c793b2c Compare January 29, 2025 14:47
- corrects docblock.
- adds assertion to confirm that no enrolment happened.
@stopfstedt stopfstedt force-pushed the 72_deal_with_duplicate_user_accounts branch from 5fbf4a8 to 32c4c1e Compare January 31, 2025 23:43
@stopfstedt stopfstedt force-pushed the 72_deal_with_duplicate_user_accounts branch from 32c4c1e to 925c48a Compare January 31, 2025 23:52
@stopfstedt stopfstedt changed the base branch from main to MOODLE_404_STABLE February 1, 2025 00:00
@stopfstedt stopfstedt marked this pull request as ready for review February 1, 2025 00:02
@ctam
Copy link
Member

ctam commented Feb 3, 2025

Thanks @stopfstedt. The code looks good to me. I just wonder that it is really impossible or too hard to implement this on the API's endpoint. It seems to me that if the Ilios's user API is returning these users with missing or duplicate campusid that we don't really need, it just creates unnecessary network traffic and impedes overall performance.

@jrjohnson
Copy link
Member

@ctam we could add a filter to remove the users with a missing ID, but I doubt it would save much. I wouldn't want to remove duplicates with any sort of filter because, as shown here, the decision about which account to use remains a local one and is up to the API consumer.

Given this is an automated, and not a user facing process, this seems like a good way forward.

Copy link
Member

@jrjohnson jrjohnson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there would be value in trapping these duplicates and cleaning up the data in Ilios over time. Could we log them for inspection?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add deduplication step to sync
3 participants