LG-12159: Refactor selfie_success to selfie_status and make it return a symbol by eileen-nava · Pull Request #9953 · 18F/identity-idp

eileen-nava · 2024-01-22T19:22:01Z

🎫 Ticket

LG-12159: Change selfie_success to return a symbol

🛠 Summary of changes

renamed selfie_success to selfie_status in the TrueIDResponse, AcuantResponse, Mock::ResultResponse, and DocAuth::Response
refactored selfie_status to return :success, :fail, :not_processed instead of true, false, and nil
refactored the all_passed? method introduced by LG-12038 to use this new logic

…o use selfie_status instead of selfie_success

…::Response

…ocAuth::Response usage.

…esponse

app/forms/idv/api_image_upload_form.rb

app/models/document_capture_session.rb

zachmargolis · 2024-01-22T19:29:30Z

app/services/document_capture_session_result.rb

-  :doc_auth_success, :selfie_success,
+  :doc_auth_success, :selfie_status,
  keyword_init: true,
  allowed_members: [:id, :success, :attention_with_barcode, :failed_front_image_fingerprints,
                    :failed_back_image_fingerprints, :captured_at, :selfie_check_performed,
-                    :doc_auth_success, :selfie_success]
+                    :doc_auth_success, :selfie_status]


this struct is serialized in redis, which means it will get persisted across hosts in a deploy. We need to add some 50-50 logic if we're renaming this, such as allowing both properties to be read, etc etc, then eventually dropping the old name

Thanks, I'll make a follow-up ticket to remove the old field and read from both fields in this PR. 👍🏻

Follow-up ticket: LG-12200

app/services/doc_auth/acuant/responses/get_results_response.rb

amirbey · 2024-01-23T14:35:57Z

app/models/document_capture_session.rb

    session_result.doc_auth_success = doc_auth_response.doc_auth_success?
-    # nil(selfie not required) or true/false
-    session_result.selfie_success = doc_auth_response.selfie_success
+    session_result.selfie_status = doc_auth_response.respond_to?(:selfie_status) ?


@eileen-nava - whenever reading the selfie_status from DocumentCaptureResult, a string will be retrieved, yes? is there a way to return a symbol when retrieving the selfie_status attribute from DocumentCaptureResult?

@eileen-nava and @amirbey , one thought is that it can be achieved with a kind of subclass, or some kind of call backs
For example subclassing with prepend

require 'redacted_struct' module TestPropSymbolizer def test_prop super.to_sym end end Result = RedactedStruct.new( :test_prop, allowed_members: [:test_prop], ) do prepend TestPropSymbolizer end t = Result.new(test_prop: 'abc') puts t.test_prop puts t.test_prop.class

Though it's debatable to include complex behaviors with struct, which basically a data holder.

Like hash in ruby, many cases it get overused it if not abuses.

a bit simpler ... i tried defining the method #selfie_status as follows in DocumentCaptureSessionResult which seemed to work ...

def selfie_status self[:selfie_status].to_sym end

obs document_capture_session_result[:selfie_status] will still return a string, but using the member to fetch is not stylistically the way we're accessing attributes in this struct 🤔

@amirbey , that works too.

@dawei-nava @amirbey I implemented the first suggestion, then refreshed this thread and saw the second one. Is there a strong preference between the two? I agree that both work.

@eileen-nava , no!

@eileen-nava , we may need to consider accessing using result[:selfie_status] or result['selfie_status'] method calls
For example(draft):

def [](key) return super.to_sym if key.to_s == :test_prop super end

@dawei-nava @amirbey I implemented the first suggestion, then refreshed this thread and saw the second one. Is there a strong preference between the two? I agree that both work.

I agree both work ... i prefer the latter approach for reasons stated here ... https://github.com/18F/identity-idp/pull/9953/files#r1465155665

…tatus

eileen-nava · 2024-01-23T17:51:55Z

app/models/concerns/symbolizer.rb

@@ -0,0 +1,5 @@
+module Symbolizer


Other names considered: StringToSymbolConverter or FieldSymbolizer.

would this concern be of use outside of DocumentCaptureSessionResult? I ask since Symbolizer doesn't seem closely tied to DocumentCaptureSessonResult; and, if other methods are added for use by other classes, the methods intended for use by other classes would be available to DocumentCaptureSessionResult and we would then maybe have to check the class of the caller. It almost seems like adding the #selfie_status method directly in DocumentCaptureSessionResult is a very simple solution and avoids this potential complication.

amirbey · 2024-01-24T15:57:29Z

app/forms/idv/api_image_upload_form.rb

+          selfie_status: client_response.respond_to?(:selfie_status) ?
+            client_response.selfie_status : :not_processed,


to help maintain consistency, this could be a private method reused on line 461:

... selfie_status: selfie_status(client_response), .... def selfie_status(client_response) return client_response.selfie_status if client_response.respond_to?(:selfie_status) :not_processed end

@amirbey I like this idea and took it one further by extracting the helper method into ApplicationHelper, so that we can use the method in ApiImageUploadForm, DocumentCaptureSession and DocAuth::Response.

dawei-nava · 2024-01-24T17:57:55Z

app/services/document_capture_session_result.rb

    'dcs:result'
  end

+  def selfie_status


similarly , result[:selfie_status] or result['selfie_status'] may have problem. But not critical.

Are there any scenarios in which we read session_result[:selfie_status] or session_result['selfie_status']? I thought we always used session_result.selfie_status.

@eileen-nava , no just for completeness purpose.

dawei-nava

LGTM

amirbey · 2024-01-24T18:41:45Z

@eileen-nava - LGTM ... do we want to add a quick test to ensure that document_capture_session_result.selfie_status returns a symbol? so this doesn't get mistakenly changed at a later time

zachmargolis · 2024-01-24T18:57:46Z

app/helpers/application_helper.rb

+  def selfie_status_from_response(client_response)
+    return client_response.selfie_status if client_response.respond_to?(:selfie_status)
+
+    :not_processed
+  end


why was this helper added here? this is a very generic helper, but this is a specific method? What if we made a mixin just for this?

zachmargolis · 2024-01-24T19:01:39Z

app/helpers/application_helper.rb

  end
+
+  def selfie_status_from_response(client_response)
+    return client_response.selfie_status if client_response.respond_to?(:selfie_status)


Is the respond_to? check how we are managing the 50-50 state?

If this code is running, it will be on a new box, so in that context, the selfie_status property will exist no matter what. I think we need to check the return values of the properties (because we really want to check the keys of the serialized JSON but that's hidden at this layer) so all we can see is which values are nil

Is the respond_to? check how we are managing the 50-50 state?

No, it is not.

I referred to the existing documentation for renaming a field in a data structure in Redis, which states that in the first deploy, you add the new name and write to the new name everywhere the old name is written. I added selfie_success back to DocumentCaptureSessionResult in this commit and we will remove it when working on this ticket LG-12200

Please let me know if a different approach would be preferable.

Also, selfie_status_from_response takes a Response object like DocAuth::Response or Mock::ResultResponse. I thought the greatest risk around a 50/50 state error for this PR related to the change of a session field?

ah yup that's right, I lost track of which objects were deserialized from redis and which were just in-memory

eileen-nava added 13 commits January 18, 2024 14:50

refactor true id response to represent selfie_status with symbols

f2f2f44

changelog: Internal, DocAuth, refactor how we represent selfie outcomes

fd30f4a

refactor document_capture_session & document_capture_session_result t…

2c437fd

…o use selfie_status instead of selfie_success

include selfie_status method in both Mock::ResultResponse and DocAuth…

9ec4c57

…::Response

update image uploads controller spec to use selfie_status. Maintain D…

032a7a5

…ocAuth::Response usage.

remove selfie_status from DocAuth::Response, keep it in Mock::ResultR…

0411d6a

…esponse

Merge branch 'main' into em/12159-refactor-selfie-success

43eff2c

fix tests

26d53f5

fix more tests

ddd5443

add selfie_passed? & continue selfie_success to selfie_status refactor

7341f69

fix failing test

594ca5a

Merge branch 'main' into em/12159-refactor-selfie-success

9f65bd0

revert doc capture session spec to pre-refactor expectations

9c79595

eileen-nava requested review from a team, charleyf and zachmargolis and removed request for a team January 22, 2024 19:22

eileen-nava changed the title ~~Em/12159 refactor selfie success~~ LG-12159: Refactor selfie_success to selfie_status and make it return a symbol Jan 22, 2024

zachmargolis reviewed Jan 22, 2024

View reviewed changes

app/services/doc_auth/acuant/responses/get_results_response.rb Show resolved Hide resolved

amirbey reviewed Jan 23, 2024

View reviewed changes

ensure that DocCaptureSessionResult will return a symbol for selfie_s…

d261e54

…tatus

eileen-nava commented Jan 23, 2024

View reviewed changes

handle 50/50 state in DocCaptureSessionResult

b7e797a

eileen-nava requested review from amirbey, dawei-nava and zachmargolis January 24, 2024 15:08

amirbey reviewed Jan 24, 2024

View reviewed changes

change approach to ensuring that selfie_status returns a symbol

3dcfc7e

amirbey approved these changes Jan 24, 2024

View reviewed changes

dawei-nava reviewed Jan 24, 2024

View reviewed changes

extract selfie_status_from_response method into ApplicationHelper

1a6c532

dawei-nava approved these changes Jan 24, 2024

View reviewed changes

eileen-nava added 2 commits January 24, 2024 13:53

test selfie_status return type

d88acb2

Merge branch 'main' into em/12159-refactor-selfie-success

19da555

zachmargolis reviewed Jan 24, 2024

View reviewed changes

eileen-nava added 2 commits January 24, 2024 16:44

Merge branch 'main' into em/12159-refactor-selfie-success

d964cb9

lint

169c5c6

eileen-nava merged commit 32826b1 into main Jan 24, 2024

eileen-nava deleted the em/12159-refactor-selfie-success branch January 24, 2024 22:24

aduth mentioned this pull request Jan 29, 2024

Deploy RC 351 to Production #9994

Merged

amirbey mentioned this pull request Jan 30, 2024

LG-12041: Prevent user from resubmitting a selfie that already failed portrait matching #9976

Merged

4 tasks

eileen-nava mentioned this pull request Feb 5, 2024

LG-12200: Remove selfie_success field from DocumentCaptureSessionResult #10035

Merged

		selfie_status: client_response.respond_to?(:selfie_status) ?
		client_response.selfie_status : :not_processed,

Conversation

eileen-nava commented Jan 22, 2024

🎫 Ticket

🛠 Summary of changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dawei-nava Jan 23, 2024 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dawei-nava Jan 24, 2024 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dawei-nava left a comment

Choose a reason for hiding this comment

Uh oh!

amirbey commented Jan 24, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dawei-nava Jan 23, 2024 •

edited

Loading

dawei-nava Jan 24, 2024 •

edited

Loading