LG-10157 Profiles should have activation characterization tests by kbighorse · Pull Request #8629 · 18F/identity-idp

kbighorse · 2023-06-21T17:41:24Z

🎫 Ticket

Profile de-/activation methods should have characterization tests

🛠 Summary of changes

add before and after tests around activation calls, including deactivation methods
prefer pre-defined profile factories with traits whenever possible

…vation

also: - use active profile factory - replace double negatives with positive checks

…tion

TODO: target new `active` trait on profile factory

kbighorse · 2023-06-22T23:16:28Z

spec/controllers/sign_up/completions_controller_spec.rb

+
+      # TODO: this preserves the previous passing behavior, but should be re-implemented
+      # to rely on the `Profile#active` boolean rather than the `Profile#activated_at` timestamp
+      let!(:user) { create(:user, profiles: [create(:profile, activated_at: Time.zone.now)]) }


note: Here is the spec below that fails:
e08d8f8#diff-6bf8b5abaee036aff5e33c715c093fd644b2a81cd370915848679e2914ce8fdeR365

@theabrad @soniaconnolly

TODO: this change accounts for the following non-active profile scenarios: - deactivated - password_reset - encryption_error There may also be other possible non-active scenarios.

…-tests

theabrad · 2023-06-27T17:04:38Z

spec/controllers/sign_up/completions_controller_spec.rb

          request_url: 'http://example.com',
        }
+
+        expect(unverified_profile.activated_at).to be_present


if the profile is unverified should there be an activated_at?

I agree that this is an issue to investigate if we keep this code around.

kbighorse · 2023-06-27T18:53:37Z

spec/controllers/sign_up/completions_controller_spec.rb

+        expect(unverified_profile.fraud_review_pending?).to eq(false)
+        expect(unverified_profile.gpo_verification_pending_at).to be_nil
+        expect(unverified_profile.initiating_service_provider).to be_nil
+        expect(unverified_profile.verified_at).to be_nil


@soniaconnolly

kbighorse · 2023-06-27T19:29:24Z

spec/controllers/sign_up/completions_controller_spec.rb

      it 'logs a reproofing event upon reproofing' do
-        user.profiles.create(verified_at: Time.zone.now, active: true, activated_at: Time.zone.now)
+        unverified_profile = user.profiles.first
+        verified_profile = create(:profile, :verified, user: user)


@theabrad note: is this equivalent to what's on main?

kbighorse · 2023-06-27T19:33:01Z

spec/controllers/sign_up/completions_controller_spec.rb

+        expect(verified_profile.initiating_service_provider).to be_nil
+        expect(verified_profile.verified_at).to be_present
+
        expect(@irs_attempts_api_tracker).to receive(:idv_reproof)


@soniaconnolly: @theabrad and I think that Profile#has_proofed_before? may need to be refactored to rely on activated_at AND verified_at. It currently considers activated_at only.

@benjaminchait this is IRS-related code, should we loop in anyone else?

…-tests

This reverts commit 9d30a93.

This reverts commit 2dcc5e8.

…-tests

This spec should expect a raised exception

TODOs: - call activate on 215 - remove `has_proofed_before` describe block - 324 should also be `verified` - 357 should store `activated_at`

add 1 TODO and clarify another

soniaconnolly

Thank you for all this work and analysis! Approving, with the strong recommendation to add one more commit to automatically include :verified in the :active trait. And some other comments.

soniaconnolly · 2023-06-29T22:41:05Z

spec/controllers/sign_up/completions_controller_spec.rb

          request_url: 'http://example.com',
        }
+
+        expect(unverified_profile.activated_at).to be_present


I agree that this is an issue to investigate if we keep this code around.

soniaconnolly · 2023-06-29T22:43:20Z

spec/factories/profiles.rb

+      # `activated_at` will be always defined for active profiles,
+      # but it is not defined here so tests will not rely on it
+      # instead of the `active` boolean above as they should


Recommend adding activated_at and verified_at fields to accurately reflect what's required in the code.

soniaconnolly · 2023-06-29T22:43:44Z

spec/factories/profiles.rb

+
+    trait :deactivated do
+      active { false }
      activated_at { Time.zone.now }


If it's deactivated, activated_at should be in the past, and this should also have a matching verified_at.

soniaconnolly · 2023-06-29T22:45:47Z

spec/models/profile_spec.rb

  describe '#activate' do
    it 'activates current Profile, de-activates all other Profile for the user' do
-      active_profile = user.profiles.create(active: true)
+      # TODO: should also be verified


As noted above, :active should automatically imply :verified.

soniaconnolly · 2023-06-29T22:47:58Z

spec/models/profile_spec.rb

  describe '#activate_after_password_reset' do
-    it 'activates a profile after password reset' do
+    it 'activates a non-verified, non-active, non-activated profile after password reset' do
+      # TODO: this profile scenario shouldn't be possible


As discussed, this scenario should expect a RuntimeError, and the code should raise.

soniaconnolly · 2023-06-29T22:50:59Z

spec/models/profile_spec.rb

    end

+    # ??? This method still nils out deactivation_reason even though it raises
    it 'does not activate a profile if it has a pending reason' do


Test title could be more descriptive.

Suggested change

it 'does not activate a profile if it has a pending reason' do

it 'does not activate a profile after password reset if it has fraud review pending' do

Maybe it's removing password_reset as the deactivation reason because they used their Personal Key to recover the PII bundle? Do they have a Personal Key if they're fraud review pending? Maybe these password reset specs are left over from when the user saw the Personal Key before entering their GPO code.

soniaconnolly · 2023-06-29T22:52:44Z

spec/models/profile_spec.rb

+        :fraud_review_pending,
+        :in_person_verification_pending,


This is an invalid combination of characteristics. Fraud review pending doesn't apply to in person proofing.

soniaconnolly · 2023-06-29T22:54:30Z

spec/models/profile_spec.rb

+  # TODO: does deactivating make sense for a non-active profile? Should we prevent it?
+  # TODO: related: should we test against an active profile here?


We used to activate and immediately deactivate. I think Amir fixed that, but the method names still include "deactivate."

…-tests

…tory

kbighorse added 2 commits June 21, 2023 09:39

Test profile and active_profile before and after

a488264

Set up for active factory use

ce8dbaa

kbighorse marked this pull request as draft June 21, 2023 17:41

kbighorse added 16 commits June 21, 2023 10:48

Use active profile factory

a58835c

Correction: activated_at remains not nil

306d13e

changelog: Internal, Testing, Add tests for profile activation/deacti…

7a96894

…vation

Use profile factory in profile spec

1e72ca0

Test before and after activate in reproof spec

7ae11e5

also: - use active profile factory - replace double negatives with positive checks

Activate rather than update single field

cfde4de

Test fields before and after activation

0ceab79

Test Profile#has_proofed_before? explicitly before and after activa…

05413be

…tion

Mark unchanged field

ec78561

Use profile factory

534532a

Test both profiles before and after activation

6f4b097

Test Profile#has_proofed_before? before and after

fe6647a

Removed activated_at from active trait

d715e86

Shorten lines

b64201b

Remove trailing whitespace

bb821ff

Document spec that relies on the wrong profile field

e08d8f8

TODO: target new `active` trait on profile factory

kbighorse commented Jun 22, 2023

View reviewed changes

kbighorse added 10 commits June 22, 2023 17:12

Test user's profiles

5c174dc

Use profile factory with verified, active traits

3363be4

Material spec still passes without active profile under new active trait

04845d1

Name test objects better

e86a05c

Test activating a non-active profile

780ec76

TODO: this change accounts for the following non-active profile scenarios: - deactivated - password_reset - encryption_error There may also be other possible non-active scenarios.

Test activate in reproof spec

9cacd84

Test activate raise error

eed74c6

Create and use verify_by_mail profile

f4bae68

Test prevent activate on a fraud pending profile

83b0e0b

Use fraud review pending factory

900f1e0

Merge branch 'main' into LG-10157-profile-activation-characterization…

d7cbd0a

…-tests

kbighorse marked this pull request as ready for review June 26, 2023 15:15

kbighorse changed the title ~~Profiles should have activation characterization tests~~ LG-10157 Profiles should have activation characterization tests Jun 26, 2023

kbighorse requested review from jmhooper and theabrad June 26, 2023 15:16

Merge branch 'main' into LG-10157-profile-activation-characterization…

2ed2645

…-tests

theabrad reviewed Jun 27, 2023

View reviewed changes

kbighorse commented Jun 27, 2023

View reviewed changes

kbighorse added 11 commits June 27, 2023 12:35

Merge branch 'main' into LG-10157-profile-activation-characterization…

3f90d9f

…-tests

Merge branch 'main' into LG-10157-profile-activation-characterization…

9891b34

…-tests

Merge branch 'main' into LG-10157-profile-activation-characterization…

0f52aa3

…-tests

Remove /sign_up/completed routes to see what fails

2dcc5e8

Delete SignUp::CompletionsController and spec

9d30a93

Revert "Delete SignUp::CompletionsController and spec"

22320f6

This reverts commit 9d30a93.

Revert "Remove /sign_up/completed routes to see what fails"

dc7e3b7

This reverts commit 2dcc5e8.

Merge branch 'main' into LG-10157-profile-activation-characterization…

5111a94

…-tests

Mark where a profile is being activated mistakenly

81c9371

This spec should expect a raised exception

Update comments

66239bb

TODOs: - call activate on 215 - remove `has_proofed_before` describe block - 324 should also be `verified` - 357 should store `activated_at`

Add comments

60ae162

add 1 TODO and clarify another

soniaconnolly approved these changes Jun 29, 2023

View reviewed changes

kbighorse added 4 commits June 30, 2023 09:14

Merge branch 'main' into LG-10157-profile-activation-characterization…

085f890

…-tests

Don't call unsupported method in unused spec for unused controller

5ac7e51

Restore activated_at to active trait

8685535

Rename local variables, account for present activated_at from new fac…

c90702d

…tory

kbighorse merged commit 77311e9 into main Jul 3, 2023

kbighorse deleted the LG-10157-profile-activation-characterization-tests branch July 3, 2023 17:05

kbighorse restored the LG-10157-profile-activation-characterization-tests branch July 3, 2023 17:07

soniaconnolly mentioned this pull request Jul 3, 2023

Deploy RC 293 to Prod #8717

Merged

	it 'does not activate a profile if it has a pending reason' do
	it 'does not activate a profile after password reset if it has fraud review pending' do

		# TODO: does deactivating make sense for a non-active profile? Should we prevent it?
		# TODO: related: should we test against an active profile here?

Conversation

kbighorse commented Jun 21, 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!

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 left a comment

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kbighorse commented Jun 21, 2023 •

edited

Loading