From 5c83ed0643c2c2708ec46d8023e4f60e26be5d94 Mon Sep 17 00:00:00 2001 From: Carlo Costino Date: Wed, 7 Jun 2023 12:37:36 -0400 Subject: [PATCH] Add timezone and invite expiration ADRs (#292) This changeset adds two new ADRs: - ADR-0002: Determine How to Handle Timezones in US Notify - ADR-0003: Implementing Invite Expirations It also includes a config.yml file for GitHub that was missing in a previous PR to enable the new ADR issue template and form. Signed-off-by: Carlo Costino --- .github/ISSUE_TEMPLATE/config.yml | 1 + docs/adrs/0002-how-to-handle-timezones.md | 166 ++++++++++++++++++ .../0003-implementing-invite-expirations.md | 116 ++++++++++++ docs/adrs/README.md | 2 + 4 files changed, 285 insertions(+) create mode 100644 .github/ISSUE_TEMPLATE/config.yml create mode 100644 docs/adrs/0002-how-to-handle-timezones.md create mode 100644 docs/adrs/0003-implementing-invite-expirations.md diff --git a/.github/ISSUE_TEMPLATE/config.yml b/.github/ISSUE_TEMPLATE/config.yml new file mode 100644 index 000000000..0086358db --- /dev/null +++ b/.github/ISSUE_TEMPLATE/config.yml @@ -0,0 +1 @@ +blank_issues_enabled: true diff --git a/docs/adrs/0002-how-to-handle-timezones.md b/docs/adrs/0002-how-to-handle-timezones.md new file mode 100644 index 000000000..d46e4bf50 --- /dev/null +++ b/docs/adrs/0002-how-to-handle-timezones.md @@ -0,0 +1,166 @@ +# TITLE: Determine How to Handle Timezones in US Notify + + +| CREATED DATE | LAST UPDATED | STATUS | AUTHOR | STAKEHOLDERS | +| :---: | :---: | :---: | :---: | :---: | +| 06/06/2023 | N/A | Accepted | @terrazoon, @ccostino | @GSA/notify-contributors | + + +## CONTEXT AND PROBLEM STATEMENT + +**OPEN ISSUE(S):** https://github.com/GSA/notifications-api/issues/260, along +with a related pull request: https://github.com/GSA/notifications-api/pull/272 + +Currently, the application converts back and forth to Eastern Time in a few +places, using utilities provided by `notifications-utils`. This adds complexity +and possible confusion since we'll actually be working over multiple timezones. + +We are currently linked to timezones in the backend, and we want to unlink it, +but we quickly find places where things do not match up. + + +## DECISION DRIVERS + +We're looking for these primary outcomes with this work: + +- Find all the time-dependent pieces of the application. +- Decide what we can tackle now versus later. +- Determine what the return on this is. + +We've also identified the following areas as pieces of the application and +service that could be impacted by any timezone changes: + +- Reports by day (or specific month/year) +- Jobs running at a reasonable time +- Job schedules (we want users to understand when things will happen) +- Scheduling sending of messages +- UI listing of time messages were sent + +Ultimately, we're looking for the least disruption possible while maximimizing +our ability to operate the service consistently with predictable results. + + +### SECURITY COMPLIANCE CONSIDERATIONS + +None at this time, given that the nature of this work is strictly changing the +way timezones are handled in the existing application. + + +## CONSIDERED OPTIONS + +As a team, we've gone through the following options: + +- **Backend UTC, frontend explicitly ET**: We convert the backend to UTC and + keep the frontend as Eastern Time. + +- **Backend UTC, frontend UTC**: We convert both the backend and frontend to + UTC time. + +- **Backend UTC, frontend configurable at service level**: We convert the + backend to UTC and make the frontend configurable at the eservice level. + +- **Backend UTC, frontend configurable at user level**: We convert the backend + to UTC and make the frontend configurable at the user level. + +- **Backend UTC, frontend verbose (various options)**: We convert the backend + to UTC and strive for maximum flexibility on the frontend with a variety of + configuration options. + +For all of these options, we've settled on the need to adjust the backend +service to operate and manage timezones with UTC only. + +Pros of converting the backend to UTC: + +- Eliminates entire classes of bugs trying to synchronize jobs, reports, + scheduling of sending messages, etc., and ensures things are always running + when expected. + +- This is a fairly standard industry practice when dealing with any timezone + management in the applicationo; have the backend operate strictly with UTC + and leave the display and formatting of timezones in local time to the client. + +Cons of converting the backend to UTC: + +- There's a decent amount of work involved in the conversation, and tests need + to be updated to ensure they're accounting for the timezone change as well. + +For the frontend choices we have, it comes down to level of effort, time +involved, and what is a higher priority for us now versus later. + +Pros of converting parts of the frontend now: + +- It provides a bit of consistency with the backend change, and accounts for the + work now instead of later. + +- It offers a level of configuration not currently available in the app, which + would allow users to interact with and customize it in ways that better suite + their needs and preferences. + +Cons of converting parts of the frontend now: + +- There is a lot of additional work involved, not all touch points are known, + and there is a signficant effort underway at the moment to update the + frontend design and information architecture. + +- We're still not entirely sure at which level of granularity we'd like to offer + customization, if any. + + +## PROPOSED OR CHOSEN OPTION: Proposed/Chosen Option Title Here + +After talking through each of these options together as a team, we have decided +to move forward with converting the backend to UTC fully and pairing that work +with displaying UTC in the frontend where need be. + +@terrazoon had an [open PR](https://github.com/GSA/notifications-api/pull/272) +with most of the work already accounted for, and explained the rationale for +making the change based on previous work and project experience. + +Multiple team members also spoke about the benefits of storing, processing, and +managaging timezones as only UTC in the backend of the system and that it's +worth the additional work to implement. The challenges inherent in trying to +manage timezones directly are too many and greatly increase the risk of new bugs +and undesired behavior and side-effects being introduced into the system. + + +### Consequences + +- Positive + - Work was already partially complete for the backend adjustment. + - Consistent handling of timezones with just UTC across the system in the + backend. + - Provides support to adjust the frontend and other clients as desired going + forward. + +- Negative + - The additional work includes updating tests to make sure they all continue + to work properly. + - User testing will also have to be conducted to account for both the app + still functioning properly and noting where in the UI/frontend things will + need to change. + + +## VALIDATION AND NEXT STEPS + +With the decision to move the backend to UTC, the following actions need to be +taken: + +- **Change the backend to use UTC:** Remove all references to specific + timezones and switch everything to use UTC. + - Accounted for in https://github.com/GSA/notifications-api/pull/272 + +- **Update tests to account for the UTC change:** All of the tests that have + anything to do with a timezone will need to be updated to continue to work + properly. + - Accounted for in https://github.com/GSA/notifications-api/pull/272 + +We also need to update the frontend to account for these changes. This will be +done in two parts: + +1. We'll update the UI to make sure everything reflects UTC where necessary for + any timzone displays. This work will be tracked in this issue: + https://github.com/GSA/notifications-admin/issues/525 + +1. We need to create an ADR for future frontend work for how we'd like to handle + timezones in the UI going forward. This is currently noted in this issue: + https://github.com/GSA/notifications-api/issues/286 diff --git a/docs/adrs/0003-implementing-invite-expirations.md b/docs/adrs/0003-implementing-invite-expirations.md new file mode 100644 index 000000000..3a9fd0bb3 --- /dev/null +++ b/docs/adrs/0003-implementing-invite-expirations.md @@ -0,0 +1,116 @@ +# TITLE: Implementing User Invite Expirations + + +| CREATED DATE | LAST UPDATED | STATUS | AUTHOR | STAKEHOLDERS | +| :---: | :---: | :---: | :---: | :---: | +| 06/06/2023 | N/A | Proposed | @ccostino | @GSA/notify-contributors | + + +## CONTEXT AND PROBLEM STATEMENT + +**OPEN ISSUE(S):** https://github.com/GSA/notifications-admin/issues/96 + +We've run into a situation where we want to re-invite users when their previous +invites have expired. However, we're not currently able to do that because +there is no mechanism in the app (specifically the API and the data model) to +support expired invites. + +Right now, users who are invited to the system receive an email invitation that +includes a note that the invitation will expire after 24 hours. + +However, on the backend side of things, no such expiration exists. Instead, +there is a scheduled job that runs every 66 minutes to check for all +`InvitedUser` objects that are older than 2 days and deletes them. + +([Issue #96 in `notifications-admin`](https://github.com/GSA/notifications-admin/issues/96) +has more specific details.) + + +## DECISION DRIVERS + +We'd like to adjust the API and data model so that invited users are no longer +deleted from the system and are instead tracked as active or expired. When an +invite is expired, we'd like to be able to re-invite the person. + + +### SECURITY COMPLIANCE CONSIDERATIONS + +The system currently has a data model for capturing an invited user +(`InvitedUser`), which is based on an authorized user of the system having the +permission to invite others to it. + +These changes should not deviate from the existing structures and contraints +that are already in place, which prevent the following: + +- Unauthorized users from accessing the system +- Users without the proper permissions from inviting others + + +## CONSIDERED OPTIONS + +These are the different approaches we're considering for implementing this +change: + +- **Adjust `InvitedUser` management in the API:** Instead of deleting + `InvitedUser` objects, we manage them instead and track their `created_at` + dates for when they need to expire. This would involve the following + potential changes: + + - Add an `expired` flag to the `InvitedUser` model + + - Change the `delete_invitations` scheduled job to `expire_invitations` and + change its behavior to check for `InvitedUser` objects that are older than + 24 hours and flip the `expired` flag to `True`. + + - Add an additional `INVITE_EXPIRED` status to the API and include it in the + `INVITED_USER_STATUS_TYPES` enum. This will be necessary for future UI + changes. + + - Make sure the API responses that provided `InvitedUser` objects/data + included the new `expired` field and status. + + - Update all tests related to `InvitedUsers` to account for the new behavior. + + The pros in making this change: + + - This will enable us to support expiring invites in the system, including + frontend changes to enable seeing and managing expired invites. + + The cons in making this change: + + - Updated the tests might be a bit challenging depending on how many there are + (especially any related to scheduled jobs). + + +## PROPOSED OR CHOSEN OPTION: Proposed/Chosen Option Title Here + +I am proposing we adjust the `InvitedUser` management in the API and get these +updates in place first for future UI changes, because without them we cannot +display any expired invites nor offer a way of managing them or providing an +option to re-invite folks. + +After looking through the code and researching how the existing user invite +flow works, these changes seem straight-forward and would yield us a lot of +value for the effort. + + +### Consequences + +- Positive + - Allows us to support expired invites + - We could allow for custom expiration periods (either now or in the future) + - Provides the mechanisms needed in the frontend to display and manage + expired invites + +- Negative + - We might end up having to adjust a lot of tests; that's currently unclear. + + +## VALIDATION AND NEXT STEPS + +TBD, pending additional ideas and discussion! + +Once a decision is made though, a seperate issue should be written up for the +API changes that need to take place, and then follow-on work will be needed on +the admin side in https://github.com/GSA/notifications-admin/issues/96 to make +the UI adjustments. diff --git a/docs/adrs/README.md b/docs/adrs/README.md index c1f5a4b40..408ab8696 100644 --- a/docs/adrs/README.md +++ b/docs/adrs/README.md @@ -148,4 +148,6 @@ our ADRs in reverse chronological order so we have a convenient index of them. This is the log of all of our ADRs in reverse chronological order (newest is up top!). +- [ADR-0003](./0003-implementing-invite-expirations.md) - Implementing User Invite Expirations +- [ADR-0002](./0002-how-to-handle-timezones.md) - Determine How to Handle Timezones in US Notify - [ADR-0001](./0001-establishing-adrs-for-us-notify.md) - Establishing ADRs for US Notify