Skip to content

LG-12270: Audit ipp mock data and test helpers#11573

Merged
eileen-nava merged 11 commits intomainfrom
em/12270-audit-test-helpers
Dec 5, 2024
Merged

LG-12270: Audit ipp mock data and test helpers#11573
eileen-nava merged 11 commits intomainfrom
em/12270-audit-test-helpers

Conversation

@eileen-nava
Copy link
Contributor

🎫 Ticket

LG-12270: Audit and update test(s) mock data and helper functions for the entire ipp flow

🛠 Summary of changes

  • Delete mock data and helper methods that weren't being used.
  • Consolidate helper methods for ipp service specs into usps ipp service helper.
  • Fix bug in user factory's :proofed_in_person_enrollment trait.
  • Review idp/constants.rb.
  • Fix incorrect setup and expectations in specs.

📜 Testing Plan

  • Run automated tests

@eileen-nava eileen-nava requested review from a team and gina-yamada November 27, 2024 22:54
@eileen-nava eileen-nava changed the title Em/12270 audit test helpers LG-12270: Audit ipp mock data and test helpers Nov 27, 2024
identity_doc_address2:,
identity_doc_city:,
identity_doc_address_state:,
identity_doc_zipcode:,
Copy link
Contributor

@gina-yamada gina-yamada Dec 3, 2024

Choose a reason for hiding this comment

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

This test looks like it checks the data before the PUT. What do you think about confirming that the residential address data is included in pii_from_user at this point? (in addition to the state ID address)

expect(page).to have_field(
t('in_person_proofing.form.state_id.identity_doc_address_state'),
with: Idp::Constants::MOCK_IDV_APPLICANT[:state_id_jurisdiction],
with: Idp::Constants::MOCK_IDV_APPLICANT_STATE_ID_JURISDICTION,
Copy link
Contributor

Choose a reason for hiding this comment

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

I know you just changed how we were getting this data but rather than getting state_id_jurisdiction should it be MOCK_IDV_APPLICANT_STATE or Idp::Constants::MOCK_IDV_APPLICANT[:state]?

Also, do you think we should have one test (when state jurisdiction is different than state id (doc auth), when address is same as id, we confirm we are using state id (doc auth) and not the state jurisdiction? I didn't see this test and thought it might be a good one around DAV.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for catching that. I changed L113 to use MOCK_IDV_APPLICANT_STATE for identity_doc_address_state and also changed the helper method fill_out_state_id_form_ok to use MOCK_IDV_APPLICANT_STATE for filling in this field. I locally ran all the specs that directly called this method and they didn't break after the refactor. I didn't locally run all the specs that call helper methods which use this method, like complete_idv_steps_before_address, but I'll keep an eye on them in the build.

After making this change, the current spec now tests for a scenario where same_address_as_id is true and state id jurisdiction differs from identity_doc_address_state. I think that satisfies your suggestion.

Copy link
Contributor

@gina-yamada gina-yamada Dec 4, 2024

Choose a reason for hiding this comment

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

It does! The two values are different now.

@gina-yamada gina-yamada self-requested a review December 3, 2024 20:51
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.

Nice clean up! I added some comments. Nothing I suggested in blocking so don't feel pressure to make changes if you disagree.

@eileen-nava eileen-nava requested a review from a team December 4, 2024 15:56
Copy link
Contributor

@shanechesnutt-ft shanechesnutt-ft left a comment

Choose a reason for hiding this comment

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

The clean up looks great! Great job! I love that you removed more than was added.

Comment on lines +47 to +48
end
it 'redirects to state id page if not complete' do
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: Add a newline after the end and it

Suggested change
end
it 'redirects to state id page if not complete' do
end
it 'redirects to state id page if not complete' do

@eileen-nava eileen-nava merged commit 240606a into main Dec 5, 2024
@eileen-nava eileen-nava deleted the em/12270-audit-test-helpers branch December 5, 2024 16:41
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.

4 participants