LG-7195 | log_reproof_event is now reachable by n1zyy · Pull Request #6982 · 18F/identity-idp

n1zyy · 2022-09-16T21:17:33Z

changelog: Internal, Attempts API, Fixes log_reproof_event

n1zyy · 2022-09-16T21:19:27Z

app/controllers/idv/review_controller.rb

+
      init_profile
+
+      log_reproof_event if idv_session.profile.has_proofed_before?


This was the bug: The user has no active idv_session.profile_id until we call init_profile, so log_reproof_event would never get called. I had moved this check above init_profile while working in the test and didn't come back to run through it manually. 🤦‍♂️

The user will always be have idv_session.profile.active? here (unless something went wrong proofing), so we lose the ability to tell if they had been proofed off of that. Hence adding a new method.

n1zyy · 2022-09-16T21:24:21Z

app/models/profile.rb

+  def has_proofed_before?
+    threshold = active? ? 2 : 1
+    Profile.where(user_id: user_id).where.not(activated_at: nil).count >= threshold
+  end


@zachmargolis @mdiarra3 This code works for my purposes, but is this sound logic?

Proofing and "activated" appear synonymous but this is something I'd really appreciate confirmation on. It is possible that "active" and "proofed" are just coincidentally changed as similar times.

When we deactivate an account, we set active: false but leave activated_at alone. So if an account has activated_at set, we know they have been activated (proofed?) previously.

That said, I think if I were to implement a reproofed? method, it would really be has_proofed_before && active?. I may want to change the usage above.

n1zyy · 2022-09-16T21:25:01Z

spec/models/profile_spec.rb

+      expect(existing_profile.active).to be_falsey
+      expect(existing_profile.activated_at).to_not be_nil
+      expect(profile.active).to be_truthy
+      expect(profile.activated_at).to_not be_nil


This little block is probably unnecessary, but was useful for me proving out the concept. Would you like me to remove 305-309?

seems ok to keep for now

n1zyy · 2022-09-16T21:30:42Z

Note to self: Commit message has a typo. Fix that on squash and merge!

zachmargolis · 2022-09-19T17:16:29Z

app/models/profile.rb

+  def has_proofed_before?
+    threshold = active? ? 2 : 1
+    Profile.where(user_id: user_id).where.not(activated_at: nil).count >= threshold
+  end


what if instead of checking for a hard count based on active, we did a count, excluding this one's ID?

Suggested change

def has_proofed_before?

threshold = active? ? 2 : 1

Profile.where(user_id: user_id).where.not(activated_at: nil).count >= threshold

end

def has_proofed_before?

Profile.where(user_id: user_id).where.not(activated_at: nil, id: self.id).exists?

end

I like this. Multiple conditions in a .not were getting interpreted non-intuitively so I broke it down to be more explicit, but this is definitely a better way than using a count.

zachmargolis

LGTM

* Replace id_token_hint with client_id in OIDC Logout (#6936) Resolves LG-7433 **Why:** We don't want partners sending us ID tokens as query parameters. We initially permit both client_id and id_token_hint, but also include two feature flags so that we can extend the rollout of both support for client_id as well as the deprecation of id_token_hint through the sandbox. changelog: Bug Fixes, Authentication, Replace id_token_hint with client_id in OIDC logout * Fix a typo in the step indicator constants for inherited proofing (#6985) [skip changelog] * Implement basic Please Verify page UI for Inherited Proofing (#6988) Why: Inherited Proofing users will need to verify that the information we receive from the partner organization is correct changelog: Internal, Inherited Proofing, Adding basic Please Verify UI * LG-7152: A/B testing native camera only (#6915) * LG-7152: Setting up A/B testing for native camera vs Acuant SDK changelog: Internal, Document Capture, Set up A/B testing for native camera vs Acuant SDK * include a feature flag to enable/disable test completely * first cut at AbTestBucket * flesh out AbTestBucket * apply the AbTestBucket to the DocumentCaptureStep * Pull the specifics around this A/B test into its own class. * Log the bucket in the image upload vendor submitted event. * use a fully deterministic spec to test bucket distribution * check for nativeCameraOnly as part of shouldStartAcuantCapture * adds the name of the experiment to the percent generator * better logic on when to block SDK for A/B test * adds a spec for the native camera A/B test * LG-7123 Normalize arguments for enrollments (#6987) **Why:** - We were sometimes passing Pii::Attribute structs and other times passing hashes to this function. While it wasn't causing a problem now it is confusing changelog: Upcoming Features, In-person proofing, Normalize arguments for creating an enrollment * LG-7195 | log_reproof_event is now reachable (#6982) changelog: Internal, Attempts API, Fixes log_reproof_event * Scope the NativeCameraABTest to the Idv module (#6989) [skip changelog] * LG-7364 Return specific attributes that fail from LexisNexis proofer (#6956) * LG-7364 Return specific attributes that fail from LexisNexis proofer This commit aims to refactor the LexisNexis proofer to user a plain old ruby object and to have it return specifically which attributes fail if only certain attributes fail * i can't even write psuedocode * still cannot code * add failing specs for the proofer * put resolution job back the way we found it for now * [skip changelog] * make the lexisnexis proofer look like the phonefinder one * get started on the mock proofer * get mock proofer resolution client passing * start mapping checks to attributes * User proofer_result directly * Punt on merging with state_id_proofer result * Punt on mutating the callback_log_data result * Punt on context field and other proofer results * Group transaction_id and reference with other fields and mark TODO field * Test expected fields in turn * Group fields and mark TODO context field * Test fields in turn * Group and mark TODO * Group and mark TODO * Test fields in turn * Test fields in turn * Consolidate different result hash logic * use match instead of eq * some things passing and some things failing * example of how to fix nomethoderror * Defer when resolution result is only a proofer result * Implement methods on proofing result class * Test result fields in turn * Rename local variable name * Rename threatmetrix entities * Add back resolution tests * Test result fields in turn * Test expected value directly * Test threatmetrix disabled * Test lexisnexis failure response * Restore threatmetrix nil response test * Consolidate logic * Improve format * Improve format * Test against result hash methods * delint * spec cleanup * state id result is not quite ready * clean up agent spec * Define first_name on pii test object * delint Co-authored-by: Kimball Bighorse <kbighorse@yahoo.com> * Log timing info for TMX inside ResolutionProofingJob (#6991) [skip changelog] * LG-7469 Standardize naming conventions (#6992) changelog: Internal, Attempts API, Standardize events name * Allow logging of emailage fields including confidence scores (#6993) * Allow logging of emailage fields including confidence scores * changelog: Internal, ThreatMetrix API, allow non-PII fields * Only fetch all email addresses when requested for OIDC user info (#6999) changelog: Internal, Performance, Only fetch all email addresses when requested for OIDC user info * Add ESLint enforcement of awaited userEvent interaction (#6995) * Add ESLint enforcement of awaited userEvent **Why**: Avoid developer confusion associated with race conditions caused by not properly awaiting the completion of a userEvent interaction. changelog: Internal, Automated Testing, Improve developer experience for writing interaction tests * Refactor password reset button spec to avoid Mocha "done" API * Refactor PasswordResetButton spec to use Chai promise helprs * Clean up some AB Test bucket code (#6994) - Move to initializer so we're not constantly re-allocating and checking - Remove ActiveModel::Model, it was only half-used - Update DocAuthRouter to use buckets * Update document_capture_step spec and create new FakeAbTestBucket [skip changelog] Co-authored-by: Doug Price <douglas.price@gsa.gov> * Update Rails (#7000) * Update Rails changelog: Internal, Dependencies, Update Rails * Fix patched behavior for redirects and unsafe redirects * Cache phone_configuration queries during OTP authentication (#6998) changelog: Internal, Performance, Cache phone_configuration queries during OTP authentication * Handle zip+0 at GPO verification letter export (#6970) * Handle zip+0 at GPO verification letter export [skip changelog] * Fix short-circuiting in OTP confirmation (#7002) [skip changelog] * Revert strscan version upgrade (#7003) [skip changelog] Co-authored-by: Oren Kanner <oren.kanner@gsa.gov> Co-authored-by: Jonathan Hooper <jonathan.hooper@gsa.gov> Co-authored-by: Melissa Miller <melissa.miller@gsa.gov> Co-authored-by: Doug Price <douglas.price@gsa.gov> Co-authored-by: Sheldon Bachstein <sheldon.bachstein@gsa.gov> Co-authored-by: Matt Wagner <mattwagner@navapbc.com> Co-authored-by: Kimball Bighorse <kbighorse@yahoo.com> Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com> Co-authored-by: olatifflexion <109746710+olatifflexion@users.noreply.github.com> Co-authored-by: John Skiles Skinner <john.skinner@gsa.gov> Co-authored-by: Andrew Duthie <andrew.duthie@gsa.gov> Co-authored-by: John Maxwell <john.maxwell@gsa.gov>

LG-71985 | log_reproof_event is now reachable

e9ef879

changelog: Internal, Attempts API, Fixes log_reproof_event

n1zyy commented Sep 16, 2022

View reviewed changes

n1zyy requested review from a team and mdiarra3 September 16, 2022 21:29

n1zyy changed the title ~~LG-71985 | log_reproof_event is now reachable~~ LG-7195 | log_reproof_event is now reachable Sep 16, 2022

zachmargolis reviewed Sep 19, 2022

View reviewed changes

n1zyy added 2 commits September 20, 2022 12:01

Some fixes

f66bc1b

Merge branch 'main' into mattw/LG-7195-redux

ab3b9cd

zachmargolis approved these changes Sep 20, 2022

View reviewed changes

n1zyy merged commit bfed3e0 into main Sep 20, 2022

n1zyy deleted the mattw/LG-7195-redux branch September 20, 2022 16:43

mitchellhenke mentioned this pull request Sep 22, 2022

Deploy RC 211 to Production #6997

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

LG-7195 | log_reproof_event is now reachable#6982

LG-7195 | log_reproof_event is now reachable#6982
n1zyy merged 3 commits intomainfrom
mattw/LG-7195-redux

n1zyy commented Sep 16, 2022

Uh oh!

n1zyy Sep 16, 2022 •

edited

Loading

Uh oh!

n1zyy Sep 16, 2022

Uh oh!

n1zyy Sep 16, 2022

Uh oh!

zachmargolis Sep 19, 2022

Uh oh!

n1zyy commented Sep 16, 2022

Uh oh!

zachmargolis Sep 19, 2022

Uh oh!

n1zyy Sep 20, 2022

Uh oh!

zachmargolis left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants


		init_profile

		log_reproof_event if idv_session.profile.has_proofed_before?

Conversation

n1zyy commented Sep 16, 2022

Uh oh!

n1zyy Sep 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

n1zyy Sep 16, 2022

Choose a reason for hiding this comment

Uh oh!

n1zyy Sep 16, 2022

Choose a reason for hiding this comment

Uh oh!

zachmargolis Sep 19, 2022

Choose a reason for hiding this comment

Uh oh!

n1zyy commented Sep 16, 2022

Uh oh!

zachmargolis Sep 19, 2022

Choose a reason for hiding this comment

Uh oh!

n1zyy Sep 20, 2022

Choose a reason for hiding this comment

Uh oh!

zachmargolis left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

n1zyy Sep 16, 2022 •

edited

Loading