LG-8697 Use StringRedacter instead of copied redact method by soniaconnolly · Pull Request #7683 · 18F/identity-idp

soniaconnolly · 2023-01-23T19:20:21Z

NOTE: @zachmargolis worked on this together and finalized the code changes. I'll be closing this branch and making a new one off main with the same commits as soon as #7686 is merged.

Change VerifyBaseStep and NewPhoneForm to use the StringRedacterConcern introduced while refactoring the Flow State Machine. Reduces repetition in code and creates a place to add more redaction methods as needed.

This PR depends on the PR for LG-7400, merge that one first!

🎫 Ticket

LG-8697, subtask of LG-7401

🛠 Summary of changes

Updated VerifyBaseStep and NewPhoneForm and tests. Add new string_redacter_spec.rb with a test formerly in new_phone_form_spec.rb

Co-authored-by: Jonathan Hooper <jonathan.hooper@gsa.gov>

Co-authored-by: Eric Gade <eric.gade@gsa.gov> Co-authored-by: Jonathan Hooper <jonathan.hooper@gsa.gov>

Replace resolution_info_verified with already existing resolution_info_verified Co-authored-by: Eric Gade <eric.gade@gsa.gov>

Look for proofing results in :stages, :resolution rather than :extra Co-authored-by: Eric Gade <eric.gade@gsa.gov>

Redirect back to verify_info if needed from review controller before action Proofing results exception found deeper in data structure on failure Co-authored-by: Eric Gade <eric.gade@gsa.gov> Co-authored-by: Alex Bradley <alexander.bradley@gsa.gov>

per PR feedback. This is now available for a future PR to use in PhoneController and VerifyBaseStep. Co-authored-by: Alex Bradley <alexander.bradley@gsa.gov> Co-authored-by: Eric Gade <eric.gade@gsa.gov>

changelog: Internal improvements, Flow State Machine, Add update method to new VerifyInfoController

Co-authored-by: Jonathan Hooper <jonathan.hooper@gsa.gov>

Co-authored-by: Eric Gade <eric.gade@gsa.gov> Co-authored-by: Jonathan Hooper <jonathan.hooper@gsa.gov>

Replace resolution_info_verified with already existing resolution_info_verified Co-authored-by: Eric Gade <eric.gade@gsa.gov>

Look for proofing results in :stages, :resolution rather than :extra Co-authored-by: Eric Gade <eric.gade@gsa.gov>

Redirect back to verify_info if needed from review controller before action Proofing results exception found deeper in data structure on failure Co-authored-by: Eric Gade <eric.gade@gsa.gov> Co-authored-by: Alex Bradley <alexander.bradley@gsa.gov>

per PR feedback. This is now available for a future PR to use in PhoneController and VerifyBaseStep. Co-authored-by: Alex Bradley <alexander.bradley@gsa.gov> Co-authored-by: Eric Gade <eric.gade@gsa.gov>

…lt as expected

…a-lg-7400

changelog: Internal, Flow State Machine, Add update action for new VerifyInfoController

Also adding the wait template Co-authored-by: Sonia Connolly <sonia.connolly@gsa.gov> Co-authored-by: Alex Bradley <alexander.bradley@gsa.gov> [skip changelog]

[skip changelog]

Move test from NewPhoneForm to new StringRedacter spec

mitchellhenke · 2023-01-23T19:22:37Z

app/controllers/idv/review_controller.rb

      params.fetch(:user, {})[:password].presence
    end

+    def confirm_verify_info_complete


Is this related to the changes for string redaction?

Nope, base branch issues like @zachmargolis pointed out below.

zachmargolis · 2023-01-23T19:23:47Z

I'd recommend changing the base on this Pull Request (to sonia-lg-7400) so it's easier to view the changes (how to link just in case)

zachmargolis · 2023-01-23T19:42:53Z

app/controllers/concerns/string_redacter.rb

@@ -0,0 +1,7 @@
+module StringRedacter
+  extend ActiveSupport::Concern


Suggested change

extend ActiveSupport::Concern

module_function

Personally I'd go with a static method so we'd reference this directly like StringRedacter.redact_alphanumeric('abcdef') instead of just redact_alphanumeric, one of the advantages is that it's easier to see where this comes from at call sites.

Done 70c79e6 and moved to services 7f3a351

zachmargolis · 2023-01-23T21:40:09Z

spec/controllers/concerns/string_redacter_spec.rb

@@ -0,0 +1,15 @@
+require 'rails_helper'
+
+describe 'StringRedacter' do


looks like in the branch base switch, the StrictRedacter definition itself seems to have disappeared?

Thanks for keeping an eye on this! I think this is going to have to wait until PR #7672 gets merged and I can branch directly from main.

I also realized now, StringRedacter is added on that PR, but the specs, refactoring etc are all here

zachmargolis · 2023-01-24T19:52:54Z

spec/controllers/concerns/string_redacter_spec.rb

+  module Idv
+    class RedactTestController < ApplicationController
+      include StringRedacter
+    end
+  end


so ruby blocks are weird, this is going to define Idv::RedactRestController in the global namespace.

When making classes in specs, I'd consider using the Class.new syntax so that the classes are anonymous and don't stay around:

let(:controller) do Class.new(ApplicationController.new) do include StringRedacter end.new end # ... expect(controller.redact_alphanumeric)...

(also this mixin/concern business... another reason I'd consider just a static method StringRedacter.redact_alphanumeric)

Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>

soniaconnolly · 2023-01-25T20:10:00Z

Replaced with cleaned-up version #7699

soniaconnolly and others added 30 commits January 11, 2023 13:00

Add test for Continue from verify_info screen

e2f0c5e

Co-authored-by: Jonathan Hooper <jonathan.hooper@gsa.gov>

Add scaffolding to remove verify_wait from flow state machine

9d7bb48

Co-authored-by: Jonathan Hooper <jonathan.hooper@gsa.gov>

Add more methods to make verify_info#update work

25a5151

Co-authored-by: Eric Gade <eric.gade@gsa.gov> Co-authored-by: Jonathan Hooper <jonathan.hooper@gsa.gov>

Update test to go to next step, now passes

7085cd8

New specs and Idv::Session field change

60f5117

Replace resolution_info_verified with already existing resolution_info_verified Co-authored-by: Eric Gade <eric.gade@gsa.gov>

Small ssn_step cleanup

d3dfc1a

throttle and continue spec passes

f0ed81f

Separate kinds of throttles

64254dc

Look for proofing results in :stages, :resolution rather than :extra Co-authored-by: Eric Gade <eric.gade@gsa.gov>

Verify_info feature tests passing

0d8334d

Redirect back to verify_info if needed from review controller before action Proofing results exception found deeper in data structure on failure Co-authored-by: Eric Gade <eric.gade@gsa.gov> Co-authored-by: Alex Bradley <alexander.bradley@gsa.gov>

Remove line disabling lint and rearrange.

3a2c085

Create Concern for redact method

41a6339

per PR feedback. This is now available for a future PR to use in PhoneController and VerifyBaseStep. Co-authored-by: Alex Bradley <alexander.bradley@gsa.gov> Co-authored-by: Eric Gade <eric.gade@gsa.gov>

Clean up comments and lint for verify_info show.html.erb

98c7232

Add changelog

fcd3ebe

changelog: Internal improvements, Flow State Machine, Add update method to new VerifyInfoController

Add test for Continue from verify_info screen

a37b795

Co-authored-by: Jonathan Hooper <jonathan.hooper@gsa.gov>

Add scaffolding to remove verify_wait from flow state machine

b4ea51d

Co-authored-by: Jonathan Hooper <jonathan.hooper@gsa.gov>

Add more methods to make verify_info#update work

1d1f1d2

Co-authored-by: Eric Gade <eric.gade@gsa.gov> Co-authored-by: Jonathan Hooper <jonathan.hooper@gsa.gov>

Update test to go to next step, now passes

e41beaf

New specs and Idv::Session field change

5658495

Replace resolution_info_verified with already existing resolution_info_verified Co-authored-by: Eric Gade <eric.gade@gsa.gov>

Small ssn_step cleanup

cd03803

throttle and continue spec passes

0d97dab

Separate kinds of throttles

f3c3d86

Look for proofing results in :stages, :resolution rather than :extra Co-authored-by: Eric Gade <eric.gade@gsa.gov>

Verify_info feature tests passing

383b95f

Redirect back to verify_info if needed from review controller before action Proofing results exception found deeper in data structure on failure Co-authored-by: Eric Gade <eric.gade@gsa.gov> Co-authored-by: Alex Bradley <alexander.bradley@gsa.gov>

Remove line disabling lint and rearrange.

63ce60b

Create Concern for redact method

8a9d0c0

per PR feedback. This is now available for a future PR to use in PhoneController and VerifyBaseStep. Co-authored-by: Alex Bradley <alexander.bradley@gsa.gov> Co-authored-by: Eric Gade <eric.gade@gsa.gov>

Clean up comments and lint for verify_info show.html.erb

4c4068e

fix an issue where exceptions were not appearing in the proofing resu…

4efe550

…lt as expected

Merge branch 'sonia-lg-7400' of github.com:18F/identity-idp into soni…

02c29ac

…a-lg-7400

Add changelog

6c8669d

changelog: Internal, Flow State Machine, Add update action for new VerifyInfoController

Add spinner button to verify_info_controller#show

9e3bc33

Also adding the wait template Co-authored-by: Sonia Connolly <sonia.connolly@gsa.gov> Co-authored-by: Alex Bradley <alexander.bradley@gsa.gov> [skip changelog]

Show error alerts when verify info is throttled

1f24aeb

soniaconnolly added 2 commits January 23, 2023 11:12

Use StringRedacter instead of redact method

254b239

[skip changelog]

Use StringRedacter instead of redact method

ac053c8

Move test from NewPhoneForm to new StringRedacter spec

mitchellhenke reviewed Jan 23, 2023

View reviewed changes

zachmargolis reviewed Jan 23, 2023

View reviewed changes

soniaconnolly changed the base branch from main to sonia-lg-7400 January 23, 2023 21:32

soniaconnolly changed the base branch from sonia-lg-7400 to sonia-lg-7401 January 23, 2023 21:32

zachmargolis reviewed Jan 23, 2023

View reviewed changes

soniaconnolly added 3 commits January 24, 2023 10:43

Lint

4c5a72a

Make redact_alphanumeric a static method per PR feedback

70c79e6

move string_redacter under services

7f3a351

zachmargolis reviewed Jan 24, 2023

View reviewed changes

soniaconnolly and others added 2 commits January 24, 2023 13:19

Remove unnecessary controller from spec

9b5458f

remove unneeded includes

3bda211

Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>

soniaconnolly changed the base branch from sonia-lg-7401 to main January 24, 2023 21:34

soniaconnolly mentioned this pull request Jan 25, 2023

LG-8697 Clean up StringRedacter and replace duplicated redact() methods #7699

Merged

3 tasks

soniaconnolly closed this Jan 25, 2023

soniaconnolly deleted the sonia-lg-8697 branch February 14, 2023 22:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

LG-8697 Use StringRedacter instead of copied redact method#7683

LG-8697 Use StringRedacter instead of copied redact method#7683
soniaconnolly wants to merge 37 commits intomainfrom
sonia-lg-8697

soniaconnolly commented Jan 23, 2023 •

edited

Loading

Uh oh!

mitchellhenke Jan 23, 2023

Uh oh!

soniaconnolly Jan 23, 2023

Uh oh!

zachmargolis commented Jan 23, 2023

Uh oh!

zachmargolis Jan 23, 2023

Uh oh!

soniaconnolly Jan 24, 2023

Uh oh!

zachmargolis Jan 23, 2023

Uh oh!

soniaconnolly Jan 23, 2023

Uh oh!

zachmargolis Jan 23, 2023

Uh oh!

zachmargolis Jan 24, 2023

Uh oh!

soniaconnolly commented Jan 25, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

		@@ -0,0 +1,7 @@
		module StringRedacter
		extend ActiveSupport::Concern

		@@ -0,0 +1,15 @@
		require 'rails_helper'

		describe 'StringRedacter' do

Conversation

soniaconnolly commented Jan 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🎫 Ticket

🛠 Summary of changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zachmargolis commented Jan 23, 2023

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!

soniaconnolly commented Jan 25, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

soniaconnolly commented Jan 23, 2023 •

edited

Loading