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

Fix import duplicate usernames #11899

Conversation

nick2432
Copy link
Contributor

@nick2432 nick2432 commented Feb 18, 2024

Summary

This pull request introduces a modification to the get_username method, enabling case-insensitive checking for duplicate usernames.

Changes made

Updated the get_username method to convert usernames to lowercase before checking for duplicates.
Ensured that usernames with different letter cases are treated as the same to prevent case-related duplicates.

Issue Resolved

Fixes #11890

##Screenshots

2024-02-18.17-00-56.mp4

@github-actions github-actions bot added DEV: backend Python, databases, networking, filesystem... SIZE: small labels Feb 18, 2024
@nick2432 nick2432 changed the title fix:import-duplicate-usernames Fix import duplicate usernames Feb 18, 2024
@radinamatic radinamatic requested a review from pcenov February 18, 2024 19:55
@radinamatic
Copy link
Member

@nick2432 We'll proceed with the manual QA, please try to fix the linting errors in the meantime.

Copy link
Member

@pcenov pcenov left a comment

Choose a reason for hiding this comment

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

Hi @nick2432, there's one more case to handle here - if the username is already existing in the facility and I attempt to import several duplicated usernames or just the same username as the one which is already in the facility, you are now checking that they are duplicated but you are still allowing one of the usernames to be imported:

2024-02-19_11-19-23.mp4

It shouldn't be possible to import a duplicated username which is without a UUID.

@github-actions github-actions bot added SIZE: very small DEV: dev-ops Continuous integration & deployment APP: Learn Re: Learn App (content, quizzes, lessons, etc.) APP: Coach Re: Coach App (lessons, quizzes, groups, reports, etc.) APP: User Re: User app (sign-in, sign-up, user profile, etc.) APP: Setup Wizard Re: Setup Wizard (facility import, superuser creation, settings, etc.) DEV: frontend SIZE: very large labels Feb 19, 2024
Copy link
Member

@akolson akolson left a comment

Choose a reason for hiding this comment

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

Hi @nick2432, the current changes makes make sense to me. I will however hold off the approval until the feedback from @pcenov has been added. Thanks. Also, noting that your most recent commit seems to have brought in a really huge number of changes. Is this intended?

@nick2432
Copy link
Contributor Author

Hi @nick2432, the current changes makes make sense to me. I will however hold off the approval until the feedback from @pcenov has been added. Thanks. Also, noting that your most recent commit seems to have brought in a really huge number of changes. Is this intended?

no

@akolson
Copy link
Member

akolson commented Feb 19, 2024

Please be sure to let us know how we can help in case you encounter any challenges while reverting the changes. Thanks

@nick2432 nick2432 force-pushed the fix-import-duplicate-usernames branch from 89a62ac to 9969834 Compare February 20, 2024 10:43
@rtibbles
Copy link
Member

Retargeting this to release-v0.16.x as the issue is for Planned Patch 1.

@rtibbles rtibbles changed the base branch from develop to release-v0.16.x February 20, 2024 15:38
@rtibbles
Copy link
Member

Hi @nick2432 - sorry, after you sorted out your previous rebase woes, the retarget is going to give you more issues.

Could you please rebase this onto release-v0.16.x? See here for instructions: https://github.com/learningequality/kolibri/blob/develop/docs/howtos/rebasing_a_pull_request.md

@github-actions github-actions bot added DEV: renderers HTML5 apps, videos, exercises, etc. APP: Device Re: Device App (content import/export, facility-syncing, user permissions, etc.) APP: Facility Re: Facility App (user/class management, facility settings, csv import/export, etc.) labels Feb 22, 2024
@github-actions github-actions bot added DEV: tools Internal tooling for development SIZE: medium labels Feb 22, 2024
@nick2432 nick2432 force-pushed the fix-import-duplicate-usernames branch from 41143b2 to 7b08d47 Compare February 22, 2024 09:58
@nick2432
Copy link
Contributor Author

Hey @pcenov , could you please check my new changes?

@nick2432 nick2432 requested review from pcenov and akolson February 23, 2024 12:03
Copy link
Member

@pcenov pcenov left a comment

Choose a reason for hiding this comment

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

Thanks @nick2432 - I confirm that now it's not possible to import duplicate usernames.

Copy link
Member

@akolson akolson left a comment

Choose a reason for hiding this comment

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

Hi @nick2432. This looks correct to me. It would however be nice to add some tests to https://github.com/learningequality/kolibri/blob/7fba0d2a8edd9e2edda5df9ae196865a8c624172/kolibri/core/auth/test/test_bulk_import.py as as well covering the scenarios that @pcenov raised. Thanks again

@nick2432
Copy link
Contributor Author

nick2432 commented Feb 23, 2024

Hi @nick2432. This looks correct to me. It would however be nice to add some tests to https://github.com/learningequality/kolibri/blob/7fba0d2a8edd9e2edda5df9ae196865a8c624172/kolibri/core/auth/test/test_bulk_import.py as as well covering the scenarios that @pcenov raised. Thanks again

I will add the tests and have resolved the scenarios raised by @pcenov.

@akolson
Copy link
Member

akolson commented Feb 23, 2024

Thanks @nick2432.

@rtibbles
Copy link
Member

Also worth noting that there is a test failure now of an existing test, might be worth checking if something has broken, or if an assumption of the test is now being violated:

kolibri.core.auth.test.test_bulk_import.ImportTestCase testMethod=test_asterisk_in_password

@nick2432
Copy link
Contributor Author

@akolson @rtibbles , I have added the tests and resolved the test failure.

@nick2432 nick2432 requested a review from akolson February 24, 2024 17:14
Copy link
Member

@akolson akolson left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for seeing this through @nick2432.

@akolson
Copy link
Member

akolson commented Feb 26, 2024

Assigning this to Marcella for merge purposes.

@marcellamaki marcellamaki merged commit c2d3ece into learningequality:release-v0.16.x Feb 27, 2024
34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
APP: Coach Re: Coach App (lessons, quizzes, groups, reports, etc.) APP: Device Re: Device App (content import/export, facility-syncing, user permissions, etc.) APP: Facility Re: Facility App (user/class management, facility settings, csv import/export, etc.) APP: Learn Re: Learn App (content, quizzes, lessons, etc.) APP: Setup Wizard Re: Setup Wizard (facility import, superuser creation, settings, etc.) APP: User Re: User app (sign-in, sign-up, user profile, etc.) DEV: backend Python, databases, networking, filesystem... DEV: dev-ops Continuous integration & deployment DEV: frontend DEV: renderers HTML5 apps, videos, exercises, etc. DEV: tools Internal tooling for development SIZE: medium SIZE: small SIZE: very large SIZE: very small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants