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

MNTOR-3971: Use last optout date instead of last update #5591

Merged
merged 1 commit into from
Feb 12, 2025

Conversation

Vinnl
Copy link
Collaborator

@Vinnl Vinnl commented Feb 5, 2025

References:

Jira: MNTOR-3971
Figma: N/A

Description

This newly-added API response property accurately reflects the last scan date (which we had incorrectly hoped the last update date to do).

Note that I'm not familiar with the QA customs tool, so I mostly followed existing patterns and added last_optout_at wherever I could find optout_attempts. I think that covers it, but it uses quite a few custom types and otherwise lives fairly separate from the rest of the codebase, so I'm not particularly confident.

Screenshot (if applicable)

Not applicable.

How to test

Make sure the DataBrokerRemovalAttempts and AdditionalRemovalStatuses flags are on, which in Storybook you can do by setting it to:

["DataBrokerRemovalAttempts", "AdditionalRemovalStatuses"]

(You might have to refresh, because the initial value Storybook sets is {}, which is not an array and thus causes an error.)

A Plus user (e.g. in the above story) should then see the removal attempt in the Fixed tab for any "Requested removal" scan results. If you want to be sure that it's showing the last optout date as returned from the API, you can set it to some recognisable value in /src/apiMocks/mockData.ts.

Checklist (Definition of Done)

  • Localization strings (if needed) have been added.
  • Commits in this PR are minimal and have descriptive commit messages.
  • I've added or updated the relevant sections in readme and/or code comments
  • I've added a unit test to test for potential regressions of this bug.
  • If this PR implements a feature flag or experimentation, I've checked that it still works with the flag both on, and with the flag off.
  • If this PR implements a feature flag or experimentation, the Ship Behind Feature Flag status in Jira has been set
  • Product Owner accepted the User Story (demo of functionality completed) or waived the privilege.
  • All acceptance criteria are met.
  • Jira ticket has been updated (if needed) to match changes made during the development process.
  • Jira ticket has been updated (if needed) with suggestions for QA when this PR is deployed to stage.

@Vinnl Vinnl added the Review: XS Code review time: up to 30min label Feb 5, 2025
@Vinnl Vinnl requested review from codemist, flozia and mansaj February 5, 2025 11:06
@Vinnl Vinnl self-assigned this Feb 5, 2025
Copy link

github-actions bot commented Feb 5, 2025

@mansaj
Copy link
Collaborator

mansaj commented Feb 5, 2025

It's best practice to separate out the db migration part of the PR with the business logic on top.

This way, in the event of a rollback, we can rollback the part 2 without running into the similar issue we had before

@Vinnl Vinnl force-pushed the MNTOR-3971-last-optout-at branch from bdafa7a to 8748975 Compare February 6, 2025 12:12
@Vinnl
Copy link
Collaborator Author

Vinnl commented Feb 6, 2025

@mansaj I did actually think of that, and then promptly forgot about it 😅 I split the commits up now, with a separate migration commit in 9670eb8.

Copy link
Collaborator

@flozia flozia left a comment

Choose a reason for hiding this comment

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

Looks good.

@Vinnl
Copy link
Collaborator Author

Vinnl commented Feb 10, 2025

@mansaj Could you also review to make sure the migration and back-end parts are OK?

@mansaj
Copy link
Collaborator

mansaj commented Feb 11, 2025

@mansaj I did actually think of that, and then promptly forgot about it 😅 I split the commits up now, with a separate migration commit in 9670eb8.

Sorry, I actually meant to for this PR to be split into 2 PRs. The reason is stated above, but with commits, we'd still have to do cherry picking etc in the event of a rollback. Just my 2c

Copy link
Collaborator

@mansaj mansaj left a comment

Choose a reason for hiding this comment

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

Just the comment about splitting this PR into two. Otherwise LGTM

@Vinnl
Copy link
Collaborator Author

Vinnl commented Feb 12, 2025

PRs aren't separately recorded in Git history; individual commits can be rolled back regardless of whether they're a PR merge commit or a regular commit (i.e. reverting a PR through the GitHub UI is just executing git reverts as well). But happy to create a separate PR containing just the first commit :) #5618

This newly-added API response property accurately reflects the
last scan date (which we had incorrectly hoped the last update date
to do).

Note that I'm not familiar with the QA customs tool, so I mostly
followed existing patterns and added `last_optout_at` wherever I
could find `optout_attempts`. I think that covers it, but it uses
quite a few custom types and otherwise lives fairly separate from
the rest of the codebase, so I'm not particularly confident.
@Vinnl Vinnl force-pushed the MNTOR-3971-last-optout-at branch from 20c66e7 to 3d18a6a Compare February 12, 2025 11:05
@Vinnl Vinnl merged commit a8a38fa into main Feb 12, 2025
16 checks passed
@Vinnl Vinnl deleted the MNTOR-3971-last-optout-at branch February 12, 2025 11:20
Copy link

Cleanup completed - database 'blurts-server-pr-5591' destroyed, cloud run service 'blurts-server-pr-5591' destroyed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Review: XS Code review time: up to 30min
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants