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

Bulk organization user creation/addition feature #3651

Open
wants to merge 18 commits into
base: develop
Choose a base branch
from

Conversation

teovin
Copy link
Contributor

@teovin teovin commented Nov 7, 2024

This feature adds a new button to the organization user management view (/manage/organization-users):

image

Upon clicking, the bulk upload form comes up:

image

User makes the organization, affiliation expiration, and CSV file selections. Form cannot be submitted without an organization selection or with a non .csv file extension. It is also invalid if it doesn't include all of the headers rows and data in the email field.

image

image

image

image

The affiliation field can be toggled just like in the single user flow.

image

Once submitted, users that don't exist will be created; those that do will have their organization affiliation updated. New users will receive new user email with account activation email. Existing users will receive the user added to organization email.

image

image

A generic success message will be displayed at the end if there are no validation errors:

image

If some of the users get processed, but some can't due to those being an admin or registrar, below will be displayed:

image

If all users in the CSV are admins or registrars, the below error message will be displayed:

image

This PR also fixes a bug where the existing user email wasn't displaying the organization name.

image

Disclaimer: Any behavior and the wording here are open to feedback. This was my initial take on it :)

EDIT:

As of Nov 20th, I put this feature behind a feature flag so we can do some more testing on it in stage before a possible production deployment.

Copy link

codecov bot commented Nov 7, 2024

Codecov Report

Attention: Patch coverage is 88.07339% with 13 lines in your changes missing coverage. Please review.

Project coverage is 69.33%. Comparing base (e189f29) to head (6488bfe).
Report is 1 commits behind head on develop.

Files with missing lines Patch % Lines
perma_web/perma/views/user_management.py 68.42% 12 Missing ⚠️
perma_web/perma/forms.py 98.57% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #3651      +/-   ##
===========================================
+ Coverage    69.08%   69.33%   +0.25%     
===========================================
  Files           54       54              
  Lines         7397     7500     +103     
===========================================
+ Hits          5110     5200      +90     
- Misses        2287     2300      +13     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

@teovin teovin marked this pull request as ready for review November 7, 2024 17:06
@teovin teovin requested a review from a team as a code owner November 7, 2024 17:06
@teovin teovin requested review from bensteinberg and removed request for a team November 7, 2024 17:06
@christiansmith
Copy link

Reviewing this with @bensteinberg (running locally), we noticed a few minor details with the CSV handling form.

  1. The CSV upload fails without a header row or with incorrect column names.
    1. We could provide a template for users to download and edit
    2. We could also handle the case of a missing header row and validate the structure of the provided data rows.
  2. When there is a validation error, instructions should remain on the screen along with the error message.
  3. If a malformed CSV is uploaded, it would be good to have better feedback for the user than the current stack trace.

Copy link
Contributor

@bensteinberg bensteinberg left a comment

Choose a reason for hiding this comment

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

As discussed, @christiansmith and I just looked at this together. I see this is added to the test for permissions, but otherwise has no tests.

Also, we were just discussing this with @rebeccacremona, who pointed out that we might prefer to assemble all the work and run it in a single database query, rather than going line-by-line. It might also make sense to fail if there are any validation errors, rather than create some users and not others.

@teovin
Copy link
Contributor Author

teovin commented Nov 15, 2024

@christiansmith @bensteinberg Pushed a couple of commits for the desired behavior and also added a test. I am checking with Clare about below to see what her preference would be. My initial approach was that it is good to process the good data and ignore the rest, as in something is better than nothing if it makes sense. Will check with Becky on the point of running a single db query.

It might also make sense to fail if there are any validation errors, rather than create some users and not others.

@teovin
Copy link
Contributor Author

teovin commented Nov 18, 2024

@christiansmith @bensteinberg I made some changes to the PR since you last looked, I'd appreciate it if you could look again. Clare preferred to process the valid users and notify the requestor for those that were invalid, and I updated the PR description and UI messaging to reflect that.

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.

3 participants