mirror of
https://github.com/GSA/notifications-api.git
synced 2026-02-03 01:41:05 -05:00
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 <carlo.costino@gsa.gov>
This commit is contained in:
1
.github/ISSUE_TEMPLATE/config.yml
vendored
Normal file
1
.github/ISSUE_TEMPLATE/config.yml
vendored
Normal file
@@ -0,0 +1 @@
|
|||||||
|
blank_issues_enabled: true
|
||||||
166
docs/adrs/0002-how-to-handle-timezones.md
Normal file
166
docs/adrs/0002-how-to-handle-timezones.md
Normal file
@@ -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
|
||||||
116
docs/adrs/0003-implementing-invite-expirations.md
Normal file
116
docs/adrs/0003-implementing-invite-expirations.md
Normal file
@@ -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.
|
||||||
@@ -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
|
This is the log of all of our ADRs in reverse chronological order (newest is up
|
||||||
top!).
|
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
|
- [ADR-0001](./0001-establishing-adrs-for-us-notify.md) - Establishing ADRs for US Notify
|
||||||
|
|||||||
Reference in New Issue
Block a user