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

add the org user expiry feature #3602

Merged
merged 5 commits into from
Sep 23, 2024
Merged

Conversation

teovin
Copy link
Contributor

@teovin teovin commented Sep 18, 2024

This PR adds the ability to associate users with organizations with an affiliation expiration date.

Registrars and admins can create organization users with affiliation expiration dates. By default, the form will open up with an indefinite affiliation selection. (manage/organization-users/add-user?email={email})
Deleted organizations are filtered out from the select options.

image

If the Indefinite affiliation checkbox is unchecked, the date selection field will display.

image

If the checkbox is checked again, the date selection field will hide. Form can be submitted in each state.

If the user already has an account, the form will only come with the organization and expiration fields, with the select options not displaying the organizations that the user is already affiliated with.

image

Registrars and admins can view the date on the user details page. (manage/organization-users/{id}) Registrars can only see the affiliations with organizations that are associated with them. Admins can see all of the affiliations.

image

Users can view their affiliations on the user settings along with the expiry dates. (settings/affiliations)

image

Admins can view and modify the date via the admin dashboard. (admin/perma/linkuser/{id}/change/)

image

Two scheduled tasks will run once a day checking the organization users' expiration dates:

  • The removal task will remove users whose affiliation has expired.
  • The warning task will send users an email telling them how many days they have left in their affiliation period.

Sample notification email sent to the user:

image

Note: This PR changes the initial behavior of the form where we were able to add multiple affiliations to the user. Since now we are adding an expiry date to each, I changed the behavior to choose one.

@teovin teovin requested a review from a team as a code owner September 18, 2024 14:32
@teovin teovin requested review from cmsetzer and removed request for a team September 18, 2024 14:32
Copy link

codecov bot commented Sep 18, 2024

Codecov Report

Attention: Patch coverage is 68.88889% with 28 lines in your changes missing coverage. Please review.

Project coverage is 68.68%. Comparing base (bde0aa3) to head (e20d975).
Report is 5 commits behind head on develop.

Files with missing lines Patch % Lines
perma_web/perma/celery_tasks.py 15.62% 27 Missing ⚠️
perma_web/perma/forms.py 96.77% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #3602      +/-   ##
===========================================
- Coverage    68.84%   68.68%   -0.17%     
===========================================
  Files           48       48              
  Lines         7004     7063      +59     
===========================================
+ Hits          4822     4851      +29     
- Misses        2182     2212      +30     

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

@teovin
Copy link
Contributor Author

teovin commented Sep 18, 2024

I just realized I added a new section to the profile page to display the affiliations, and there is already a page for it accessible through the left-hand menu Your Affiliations item. I will make the change to update that instead.

*This is complete and I updated the pr description accordingly.

Copy link
Contributor

@cmsetzer cmsetzer left a comment

Choose a reason for hiding this comment

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

Nice work on this! (And thanks for your patience with this review.)

Everything looks good for the most part. One point of UX feedback: the checkbox label "Indefinite affiliation" feels a bit engineer-ish to me. If I were an organization/registrar admin, "Permanent affiliation" or something like that might feel more intuitive. (Clare may have already weighed in on this, in which case feel free to ignore me.)

Comment on lines 1631 to 1632
except Exception as e:
logger.error(f"Error emailing user {affiliation.user_id}: {e}")
Copy link
Contributor

Choose a reason for hiding this comment

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

We might consider using logger.exception so the full traceback gets included:

Suggested change
except Exception as e:
logger.error(f"Error emailing user {affiliation.user_id}: {e}")
except Exception:
logger.exception(f"Error emailing user {affiliation.user_id}")

The error might be obvious in most cases, though — I'll leave this up to you.


deleted_affiliations = UserOrganizationAffiliation.objects.filter(expires_at__lt=expiration_date).delete()

if deleted_affiliations[0] > 0:
Copy link
Contributor

@cmsetzer cmsetzer Sep 20, 2024

Choose a reason for hiding this comment

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

Should we add another condition here to avoid erroring out when deleted_affiliations is empty? (Or maybe I'm misunderstanding the object returned by delete()?)

Suggested change
if deleted_affiliations[0] > 0:
if deleted_affiliations and deleted_affiliations[0] > 0:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great suggestion on the label naming. I made the change for this and also for a similar label I had created for the registrar sponsorship form. I also made the change to both to use logger.exception().

When it comes to deleting affiliations, if no db objects are impacted, meaning nothing is deleted, then deleted_affiliations[0] will be 0. delete() returns a tuple with its first item being the number of objects deleted.

@teovin teovin merged commit a466172 into harvard-lil:develop Sep 23, 2024
2 checks passed
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