Expiring Access do not renew #281
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There was a problem hiding this comment.
Pull Request Overview
This PR introduces functionality to mark expiring access as “do not renew” (i.e. reviewed without renewal) by adding a new boolean field (should_expire) and updating both the frontend bulk renewal dialogs and backend operations/notifications. Key changes include modifications to test cases and API schemas, new UI toggles in the Bulk Renewal dialogs, and updates to the database migration and business logic for expiring access.
Reviewed Changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/* | Added tests for “do not renew” functionality |
| src/pages/* | Updated BulkRenewal and Expiring pages with new toggle and review display |
| api/* | Adjusted API schemas, models, operations and notifications to support the new flag |
| migrations/* | Added migration for the new should_expire field in relevant tables |
savathoon
reviewed
Jun 23, 2025
somethingnew2-0
requested changes
Jun 24, 2025
somethingnew2-0
approved these changes
Jun 25, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Still maybe making minor changes (checking functionality for bugs atm) but more or less ready for review
This new feature makes it so that expiring access can be marked as reviewed without renewing the access. This allows the number of notifications to be reduced (we previously had no way to indicate that something had been reviewed so notifications for reviewed-but-not-renewed access would still be sent) and hopefully reduce conflicts that may come up where one group owner intends to allow access to expire while another owner of the group goes later and renews it anyways.
The biggest change is that the bulk renewal dialogs now feature a Yes/No toggle instead of checkbox selection in order to indicate whether access should be renewed or allowed to expire. Access that has been marked as 'should expire' can still be renewed later if needed.
The default page size for the expiring access pages is still 20 rows. I just reduced it to 10 for the sake of more compact screenshots.
Default expiring access page (doesn't show items marked as 'do not review')
View showing items marked as 'do not renew'
Bulk renewal dialog with new toggle (only showing unreviewed items)
Bulk renewal dialog showing all access (items marked 'do not renew' have 'No' in the toggle pre-selected)
