Add significantly_updated_at attribute to User
#1991
Merged
Conversation
This file contains 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
ethax-ross
force-pushed
the
3747-add-significant-updated-at-to-users
branch
2 times, most recently
from
81c3632 to
b225009
Compare
|
Review app deployed to https://npq-registration-review-1991-web.test.teacherservices.cloud/ |
ethax-ross
force-pushed
the
3747-add-significant-updated-at-to-users
branch
from
e362d21 to
dddd259
Compare
cwrw
reviewed
Nov 21, 2024
ethax-ross
force-pushed
the
3747-add-significant-updated-at-to-users
branch
from
dddd259 to
9e7bb5f
Compare
| @@ -58,12 +58,12 @@ def call | |||
| active_alert: npq_application.active_alert || user.active_alert, | |||
| trn_verified:, | |||
| created_at: ecf_user.created_at, | |||
| updated_at: ecf_user.updated_at, | |||
| significantly_updated_at: ecf_user.updated_at, | |||
There was a problem hiding this comment.
Maybe we still can keep migrating updated_at over or not?
There was a problem hiding this comment.
discussed in person: let's keep both
cwrw
reviewed
Nov 22, 2024
cwrw
reviewed
Nov 22, 2024
We have an `updated_at` on the `User` model, however this is updated frequently and for actions that lead providers don't need to care about (such as when a user signs in). We want to add a second `updarted_at` attribute that only updates when something significant changes with the `User`, such as their name, TRN, etc. As the `Application` can touch a `User` we also need this attribute to update when the user is touched. I opted for `significantly_updated_at` as the attribute name as opposed to `api_updated_at` to try and keep it more generic in case we want to surface it elsewhere. Index the field to ensure endpoints that filter by `updated_at` will be performant when we switch them to `significantly_updated_at`.
We want to populate `significantly_udpated_at` instead of `updated_at` as we will be exposing this attribute in the serializers for the lead provider API.
We want to filter by the `significantly_updated_at` attribute instead of the `updated_at` attribute when querying users for the lead provider API. This will prevent insignificant changes to a `User` resulting in them being re-surfaced in the API. Update `Participants::Query` and `Applications::Query` to use `significantly_updated_at`.
We want to surface the `sigificant_updated_at` attribute of participants in the lead provider API responses so that providers only see changes/updates when something they care about has changed with the user.
Exclude `significantly_updated_at` from these feature specs.
We want to retain updating `updated_at` on users as well as populating the new `significantly_updated_at` during the migration.
We want to ensure we don't touch `significantly_updated_at` from the callback in `User` when performing a migration.
We want to backfill the `significantly_updated_at` using the `updated_at` attribute of each user.
ethax-ross
force-pushed
the
3747-add-significant-updated-at-to-users
branch
from
a51f25b to
8abfaf7
Compare
|
leandroalemao
approved these changes
Nov 22, 2024
cwrw
approved these changes
Nov 22, 2024
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.
Jira-3747
Context
At the moment, we use the
User#updated_atto surface changes to lead providers via the API. The issue with this is that actions/changes that lead providers don't care about result in the user being marked as 'updated'. Instead, we want to have an additionalupdated_atattribute that we can use to surface the changes lead providers care about to the underlyingUsermodel.Changes proposed in this pull request
We have an
updated_aton theUsermodel, however this is updated frequently and for actions that lead providers don't need to care about (such as when a user signs in).We want to add a second
updarted_atattribute that only updates when something significant changes with theUser, such as their name, TRN, etc.As the
Applicationcan touch aUserwe also need this attribute to update when the user is touched.I opted for
significantly_updated_atas the attribute name as opposed toapi_updated_atto try and keep it more generic in case we want to surface it elsewhere.Index the field to ensure endpoints that filter by
updated_atwill be performant when we switch them to significantly_updated_at`.We want to populate
significantly_udpated_atinstead ofupdated_atas we will be exposing this attribute in the serializers for the lead provider API.We want to filter by the
significantly_updated_atattribute instead of theupdated_atattribute when querying users for the lead provider API. This will prevent insignificant changes to aUserresulting in them being re-surfaced in the API.We want to surface the
sigificant_updated_atattribute of participants in the lead provider API responses so that providers only see changes/updates when something they care about has changed with the user.Guidance to review
As it stands any users already in the NPQ reg database that aren't touched as part of the migration will end up with a default
significantly_updated_attime of 'now' (when we roll this out). Do we want to backfill to match with the userupdated_atfor these records?