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: correctly assign existing users as institutional mangers #30

Merged
merged 3 commits into from
Feb 7, 2024

Conversation

fdalcin
Copy link
Contributor

@fdalcin fdalcin commented Feb 7, 2024

Issue #29

This PR aims to fix an issue when trying to assign a user as institutional manager. The issue happened when the user was already assigned as a regular user in the institution. So when trying to make them institutional managers we would receive an error for duplicate keys. The update removes the issue by updating existing records instead of trying to create new ones.

In addition, this PR also updates users to regular assigned users when removing them form institutional mangers. Previously, they would be unassigned from the institution.

How to test

  1. Assign users to an institution as regular users (you could use the auto assign feature or manually insert them in the database for now)
  2. Visit the institution edit page and select a regular user already assigned to the institution
  3. The user should be marked as a manager without issues
  4. Try removing them from institutional managers
  5. They should still be assigned to the institution but not as managers anymore

Note

No tests were added for now. The entire CRUD need tests, so they will be added in the same ticket.

Note

If a user is assigned to a different institution as a regular user we currently allow them to be assigned to both institutions, that will be updated in a separate ticket (will link it here later)

Copy link
Contributor

@arzola arzola left a comment

Choose a reason for hiding this comment

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

LGTM, it works as expected

@arzola arzola merged commit d131544 into dev Feb 7, 2024
6 checks passed
@arzola arzola deleted the fix/assign-managers-for-existing-users branch February 7, 2024 16:20
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.

2 participants