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

Add option to add and update additional scan profile details #5660

Closed
wants to merge 24 commits into from

Conversation

flozia
Copy link
Collaborator

@flozia flozia commented Feb 25, 2025

References:

Jira: MNTOR-4065
Figma: https://www.figma.com/design/2iCgADpFXKOFZTEgCPkQsR/Settings?node-id=597-10052&t=c2UkGyKPdbvneuz5-4

Description

Adds additional scan profile details and the option to update them:

  1. The migration file for the changes in this PR is split out to PR Migration for PR #5660 #5664.
  2. Updates the /admin/dev panel to add the option to test the backend changes.

How to test

  1. Run the migrations: npm run db:migrate
  2. Run an initial welcome scan
  3. Enable the feature flag EditScanProfileDetails
  4. Make sure you can still run an initial scan
  5. Visit /admin/dev
  6. Make changes to the scan profile
  7. Confirm any profile updates are applied to the table onerep_profiles and also show for the remote profile

Checklist (Definition of Done)

  • Commits in this PR are minimal and have descriptive commit messages.
  • I've added or updated the relevant sections in code comments
  • I've added a unit test to test for potential regressions of this bug.
  • All acceptance criteria are met.
  • Jira ticket has been updated with suggestions for QA when this PR is deployed to stage.

@flozia flozia requested review from rhelmer and mansaj and removed request for rhelmer and mansaj February 25, 2025 17:22
@flozia flozia changed the title Add option to add and update additional scan profile details WIP: Add option to add and update additional scan profile details Feb 25, 2025
@flozia flozia marked this pull request as ready for review February 26, 2025 10:53
@flozia flozia requested review from rhelmer, Vinnl and mansaj February 26, 2025 10:53
@flozia flozia changed the title WIP: Add option to add and update additional scan profile details Add option to add and update additional scan profile details Feb 26, 2025
Copy link

@flozia flozia self-assigned this Feb 26, 2025
@flozia flozia mentioned this pull request Feb 26, 2025
@flozia flozia marked this pull request as draft February 26, 2025 13:06
@flozia flozia removed request for rhelmer and Vinnl February 26, 2025 13:06
Copy link
Collaborator

@Vinnl Vinnl left a comment

Choose a reason for hiding this comment

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

I didn't closely review the admin UI code since it's running locally only anyway, but it's looking really neat! And the functionality seemed to work here.

Copy link
Collaborator

@rhelmer rhelmer left a comment

Choose a reason for hiding this comment

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

lgtm overall, I reviewed but did not test this locally, @flozia do you want me to help with that?

The only really important question I have is how you intend to enforce the limits on the number of allowable fields, since this involves both DB storage and external API calls it is probably more complex, but I included a reference to some prior art we have in this area (the limit on secondary emails in the settings screen).

"phone_numbers",
"addresses",
] as const;
export const CONST_DATA_BROKER_PROFILE_DETAIL_LIMITS = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

How are these limits enforced? We found in the past that is was insufficient to do this on the front-end, and we ended up enforcing this at the database level using transactions with a strong isolation level

await trx.raw("SET TRANSACTION ISOLATION LEVEL SERIALIZABLE");

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The database entries are checked for their limits before writing the updates here. Since this still leaves a window of opportunity to insert more entries than the limit allows for setting the transaction isolation level is a good idea. Added in 27b9cc8.

@@ -320,7 +320,6 @@ declare module "knex/types/tables" {
interface OnerepProfileRow {
id: number;
onerep_profile_id: null | number;
name_suffix: null | string;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think I missed the memo on this, are we deprecating the name_suffix field entirely?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, we are keeping it for now: 41912ff.

@flozia flozia requested a review from rhelmer March 10, 2025 10:43
Copy link
Collaborator

@Vinnl Vinnl left a comment

Choose a reason for hiding this comment

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

The changes look good to me too. (Except the transaction level stuff, since I don't know what that is, but I'll leave that to you and Rob :) )

@flozia flozia mentioned this pull request Mar 10, 2025
flozia added a commit that referenced this pull request Mar 11, 2025
Base automatically changed from mntor-4065-migrations to main March 11, 2025 10:06
@flozia
Copy link
Collaborator Author

flozia commented Mar 11, 2025

The Deploy Preview Action is also stuck for this PR. Closing in favor of PR #5697.

@flozia flozia closed this Mar 11, 2025
Copy link

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

Copy link

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

@flozia flozia deleted the mntor-4065 branch March 11, 2025 11:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants