Skip to content

LG-9192 remove dav flag#9207

Merged
svalexander merged 42 commits intomainfrom
shannon/lg-9192-remove-dav-flag
Oct 10, 2023
Merged

LG-9192 remove dav flag#9207
svalexander merged 42 commits intomainfrom
shannon/lg-9192-remove-dav-flag

Conversation

@svalexander
Copy link
Contributor

@svalexander svalexander commented Sep 14, 2023

🎫 Ticket

LG-9192

🛠 Summary of changes

This pr makes double address verification the permanent view for ipp. The flags and conditionals associated with dav are removed from the verify page, address page and state id page. It also updates the in person spec, including to change the happy path to reflect a user who does live at the address on their id.

📜 Testing Plan

Provide a checklist of steps to confirm the changes.

  • Create account and go through ipp flow
  • Make sure everything works as expected and that the state id form reflects the dav version
  • Run specs that were changed in this pr and make sure they all pass

@svalexander svalexander marked this pull request as draft September 14, 2023 16:22
@svalexander svalexander changed the title Shannon/lg 9192 remove dav flag - Draft pr LG-9192 remove dav flag Sep 18, 2023
@gina-yamada
Copy link
Contributor

gina-yamada commented Sep 18, 2023

  • Ran through scenarios in this FIGMA locally and all scenarios are what I'd expect as described in FIGMA
  • Flow is what I'd expect depending on radio button I pick (what I edit, etc)
  • Selecting cancel and then No, keep going redirects me back to the view I was previously on (for both address & state id)
  • Validation for both state id and address page working as I'd expect.
  • Every field observed in mocks is on residential and state id form
  • Continue button/ Cancel link (initial visit) vs Update button /Back link (after first visit) is correct on both views
  • The Verify your information mocks are a little different than what I observe locally. You may see if mocks are current. There is no Issuing state and no extra line after ID number. (SSN section also does not have a title in mocks- but bet they should and mocks are just not current)
  • I did not test all the way to the end (will do when this is ready)
  • Team Ada's flow (Remote flow) does not seem affected- I am able to edit on Verify Info page- navigation is what I'd expect
  • many more instances of in_person_capture_secondary_id_enabled in code but imagine you are still working through it

@gina-yamada
Copy link
Contributor

Hi @svalexander, this is a huge undertaking! Let me know if you'd like pair or look at the failing tests for this PR. I have availability

@svalexander svalexander marked this pull request as ready for review September 27, 2023 22:07
capture_secondary_id_enabled || false
def double_address_verification
# if in person return true else return false
current_user.has_in_person_enrollment?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated this line of code but the previous comment still stands so will re-paste here:

temporarily using double_address_verification to check if we're in the ipp flow in files related to the proofing job and result adjudication. Will add a ticket that adds flag for enrollments that are created in the ipp flow in order to distinguish those enrollments in the files that still reference double_address_verification

@NavaTim NavaTim requested a review from sheldon-b September 28, 2023 23:12
Copy link
Contributor

@sheldon-b sheldon-b left a comment

Choose a reason for hiding this comment

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

Just a few minor cleanup / clarification sorts of suggestions to consider. I'm going to test it out locally before approving

current_user.establishing_in_person_enrollment&.
capture_secondary_id_enabled || false
def double_address_verification
# if in person return true else return false
Copy link
Contributor

@sheldon-b sheldon-b Oct 2, 2023

Choose a reason for hiding this comment

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

Not a blocking comment but I would love the comment to say that it's temporary code, and ideally with a reference to the ticket where it'll be removed (even if the ticket is just a placeholder you create in Jira and isn't fully defined just yet)

Suggested change
# if in person return true else return false
# if in person return true else return false. this is temporary logic until we remove the rest of the DAV logic
# todo (LG-#####): remove this method when the DAV flag has been removed

Copy link
Contributor

@sheldon-b sheldon-b left a comment

Choose a reason for hiding this comment

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

@svalexander I'm having issues with my local environment that I need to sort out but I don't want to block this PR in the mean time. Once I get my local environment setup correctly I'll test this out and will trust that you've run through it as well

@gina-yamada gina-yamada self-requested a review October 4, 2023 16:26
Copy link
Contributor

@gina-yamada gina-yamada left a comment

Choose a reason for hiding this comment

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

Ran through different scenarios (Yes, I live.. No, I don't live..., Yes- with edits, No - with edits), Cancel- then continue, etc- I am being directed as I'd expect for all scenarios. I observed Puerto Rick extras on both State and Address View. Form Validation (required fields) working as expected. Every field observed on mocks for state & residential. Bad SSN reduces attempts and locks out account after 5 failed attempts. Remote flow (Team Ada's) is not affected by your changes. The Verify your information mocks are what I observe locally. Issuing state is on Verify info and there is an extra line after ID number. (I may have been looking at outdated mocks for a comment I made last week.) Looking at tests- updates all seem reasonable. Lots of changes, I think reasonable to ask for another approver but this looks good to me.

@svalexander svalexander merged commit 66029ef into main Oct 10, 2023
@svalexander svalexander deleted the shannon/lg-9192-remove-dav-flag branch October 10, 2023 13:31
@jmhooper jmhooper mentioned this pull request Oct 10, 2023
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.

7 participants