Skip to content

LG-9050: Add password error handling#7989

Merged
jc-gsa merged 11 commits intomainfrom
LG-9050-add-password-error-handling
Mar 16, 2023
Merged

LG-9050: Add password error handling#7989
jc-gsa merged 11 commits intomainfrom
LG-9050-add-password-error-handling

Conversation

@jc-gsa
Copy link
Contributor

@jc-gsa jc-gsa commented Mar 15, 2023

🎫 Ticket

LG-9050

🛠 Summary of changes

This code changes password validation feedback when a user creates a new password for their account. The submit button is no longer disabled and error feedback is immediately applied. Please see the attached Figma for more information.

📜 Testing Plan

Create a new account. When entering a new password that is invalid please observe the changes in feedback.

@jc-gsa jc-gsa force-pushed the LG-9050-add-password-error-handling branch from 11e9bdb to b368755 Compare March 15, 2023 14:37
jc-gsa added 5 commits March 15, 2023 10:06
changelog: User-Facing Improvements, Registration, Add additional feedback to password error handling
@jc-gsa jc-gsa force-pushed the LG-9050-add-password-error-handling branch from b368755 to 1f73641 Compare March 15, 2023 15:06
@jc-gsa jc-gsa requested a review from a team March 15, 2023 19:46
@jc-gsa jc-gsa force-pushed the LG-9050-add-password-error-handling branch from 1039169 to 787e1da Compare March 15, 2023 21:06
Copy link
Contributor

@aduth aduth left a comment

Choose a reason for hiding this comment

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

I left a few comments, but functionally this appears to be working well 👍

Comment on lines 84 to 88
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks to be repeated from getStrength. Can we either remove the identical code from getStrength since the check happens here, or alternatively remove this code and update the call to validatePasswordField to pass strength as the argument below instead of z.score?

https://github.com/18F/identity-idp/blob/787e1da7029b131032b86d68de6f51856a98d707/app/javascript/packs/pw-strength.js#L136-L139

Also, if we did choose to keep this code, I think it's safe to assume that the password argument will always be a string, so we could simplify this and flatten the two conditions?

Suggested change
// override the strength value to 2 if the password is < 12
if (!(z && z.password.length && z.password.length >= 12)) {
if (z.score >= 3) {
z.score = 2;
}
if (password.length < 12 && z.score >= 3) {
z.score = 2;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will make some changes. I refrained from modifying old code too much, as it provokes the question when do we just do a total rewrite of old code?

Comment on lines 183 to 184
Copy link
Contributor

Choose a reason for hiding this comment

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

For a feature spec, I think Capybara's default locator methods are a bit more straight-forward to use, and also match the user's experience in their interaction with the content on the page.

Ideally, if we were wanting to test that the field is associated with a specific error message, we could use our custom have_description helper:

Suggested change
message = find('input.password-toggle__input').native.attribute('validationMessage')
expect(message).to eq t('errors.messages.stronger_password')
expect(find(':focus')).to have_description t('errors.messages.stronger_password')

...but it looks like the have_description would need to a small revision to support use in feature specs:

diff --git a/spec/support/matchers/accessibility.rb b/spec/support/matchers/accessibility.rb
index 5a991666b..65f7bb687 100644
--- a/spec/support/matchers/accessibility.rb
+++ b/spec/support/matchers/accessibility.rb
@@ -98,3 +98,3 @@ RSpec::Matchers.define :have_description do |description|
       split(' ')&.
-      map { |descriptor_id| rendered.at_css("##{descriptor_id}")&.text }
+      map { |descriptor_id| page.find("##{descriptor_id}")&.text }
   end

As a fallback, we could test that the error content exists, and optionally is displayed as an error.

Suggested change
message = find('input.password-toggle__input').native.attribute('validationMessage')
expect(message).to eq t('errors.messages.stronger_password')
expect(page).to have_content t('errors.messages.stronger_password')
Suggested change
message = find('input.password-toggle__input').native.attribute('validationMessage')
expect(message).to eq t('errors.messages.stronger_password')
expect(page).to have_css('.usa-error-message', text: t('errors.messages.stronger_password'))

@jc-gsa jc-gsa force-pushed the LG-9050-add-password-error-handling branch from 787e1da to 72d5546 Compare March 16, 2023 14:12
Copy link
Contributor

@aduth aduth left a comment

Choose a reason for hiding this comment

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

LGTM 👍

jc-gsa and others added 2 commits March 16, 2023 15:47
Co-authored-by: Andrew Duthie <andrew.duthie@gsa.gov>
@jc-gsa jc-gsa merged commit 85ffbe7 into main Mar 16, 2023
@jc-gsa jc-gsa deleted the LG-9050-add-password-error-handling branch March 16, 2023 21:28
@zachmargolis zachmargolis mentioned this pull request Mar 17, 2023
zachmargolis added a commit that referenced this pull request Mar 17, 2023
* Add columns to duplicate SSN reporting (LG-9191) (#7995)

* Add profile_active column
* Add count_active_ssn_fingerprint column
* Handle nil activated_at values

changelog: Internal, Reporting, Add report for duplicate SSNs

* LG-9138: Allow state ID address fields in user PII (#7961)

* Allow state ID address fields in user PII

changelog: Upcoming features, Double address verification, Allow
state ID address fields in user PII

* Add more Pii attribute comments

* Use existing state_id_jurisdiction field

* Allow state_id_jurisdiction in feature tests

* Allow PII for other events

* Update brakeman gem (#7998)

changelog: Internal, Security, Update brakeman gem

* Clean up `ResolutionProofingJob` tests (#7996)

The `ResolutionProofingJob` is the class that does much of the legwork during the verify step. Specifically, it does the following:

1. Communicates with ThreatMetrix if necessary
2. Communicates with InstantVerify
3. Communicates with AAMVA
4. Combines results through the `ResolutionResultAdjudicator`

The tests for this class have been sophisticated and have combined a number of different ways of mocking out responses from vendors (e.g. webmocks or stubs of the vendor classes).

This change attempts to simplify some of this by creating a helper for stubbing responses from vendors. Those helpers are used to re-implement simpler examples for the logic the tests test. Ideally this will allow us to more quickly make changes to the `ResolutionProofingJob`.

[skip changelog]

* Reduce device/event transactions by wrapping multiple writes in one transaction (#7999)

changelog: Internal, Database, Reduce device/event transactions by wrapping multiple writes in one transaction

* Don't make a request to AAMVA if verification is guaranteed to fail (#7997)

* Don't make a request to AAMVA if verification is guaranteed to fail

We implemented a feature that allowed us to use the response we get from AAMVA to cover attributes that fail the LexisNexis check during resolution. Specifically, the following attributes can be covered by AAMVA:

- Address
- Dob
- State ID Number

As part of this change we started sending requests to AAMVA even if the LexisNexis response failed. Previously this occurred even if the failed LexisNexis attributes could not be covered by the AAMVA response.

This commit attempts to shed some load on AAMVA's service by not making requests to them if the attributes in a failed LexisNexis response cannot be covered by AAMVA.

[skip changelog]

* Disable primary database prepared statements if using database socket connection (#7990)

changelog: Internal, Database, Disable primary database prepared statements if using database socket connection

* Change the regular expression we use to determine if get to yes is viable (#8002)

Before we use the get-to-yes feature we need to make sure that the base lexis nexis error refers to the scoring model verification. This is what implies that the checks failed and we can fallback to AAMVA.

LexisNexis uses the code priority.scoring.model.verification.fail for some of its workflows. This commit changes the regex we use to match against the code to match that code.

[skip changelog]

* Disable advisory locks when using pgbouncer socket (#8003)

Related PRs #7990 and #7967

This disables advisory locks when a DB socket is used as `pgbouncer` does
not support advisory lock use in transaction mode.

changelog: Internal, Database, Disable primary database advisory locks if using database socket connection

* Remove `skip_legacy_state` method (#7991)

* Remove `skip_legacy_state` method

The `skip_legacy_state` method had 2 major problems:

1. The name implies that the new state is somehow a legacy state
2. It is hidden in the `check_ssn` code which makes it difficult to understand how a user moves from the verify info controller to the phone controller

This method attempts to clean this up by removing `skip_legacy_state` method and moving its logic into more reasonable places in the verify info concern or into the `Idv::Session`.

[skip changelog]

* Update app/services/idv/session.rb

Co-authored-by: Sonia Connolly <sonia.connolly@gsa.gov>

* delint

---------

Co-authored-by: Sonia Connolly <sonia.connolly@gsa.gov>

* Add additional prompts to UUID lookup and review rake tasks (#8001)

changelog: Internal, utilities, Reuqest more information from users when calling investigative rake tasks

Co-authored-by: Paul Hirsch <paul.hirsch@gsa.gov>

* LG-9020 update ssn content (#8004)

* add strings

* update ssn views with changed content

* changelog: User-Facing Improvements, ssn page, update text

* Update have_description accessibility matcher for feature spec compatibility (#8010)

changelog: Internal, Automated Testing, Improve compatibility of accessibility description matcher

* Remove send link step (#7929)

* attempt to squash everything

* Adding clear_and_fill_in helper

-- What
This new helper method will first clear an input before inserting text
into it during capybara tests.

* Removing unused analytics event methods

Co-authored-by: Andrew Duthie <andrew.duthie@gsa.gov>

* Changelog

changelog: User-Facing Improvements, Updating Hybrid Handoff,
finishing removal of feature-flagged combined hybrid handoff

* changelog: User-Facing Improvements, Updating Hybrid Handoff, finishing removal of feature-flagged combined hybrid handoff (LG-8903)

* Restore `pgcrypto` extension

* Remove redundant assignment

* Decrease find wait time in feature spec

---------

Co-authored-by: Jonathan Hooper <jonathan.hooper@gsa.gov>
Co-authored-by: eric-gade <eric.gade@gsa.gov>
Co-authored-by: Andrew Duthie <andrew.duthie@gsa.gov>

* LG-9050: Add password error handling (#7989)

* Add additional feedback to password error handling

changelog: User-Facing Improvements, Registration, Add additional feedback to password error handling

* LG-9023: Remove phone helper text (#7917)

* Remove phone example number text

changelog: User-Facing Improvements, Authentication, Remove phone helper text

* LG-9206 | Analytics logs irs_session_id (#8007)


changelog: Internal, Analytics, Log irs_session_id with analytics

* Lg 9089 user data ack (#7974)

* LG-9089 Add IRS text to localdev.yml for testing

changelog: Internal, Analytics, Add Policy Redirect

* Remove arcgis_search_enabled feature flag (#8006)

* Remove arcgis_search_enabled feature flag

Why: This feature is now enabled in production and would not be expected to be reverted to pilot facilities, and its continued existence in the code creates confusion for other developers.

* changelog: Internal, Feature Flags, Remove code associated with live features

* Remove unused locale strings

* LG-8887: Update Ready to Verify language to be simpler, more branded (#8009)

* changelog: Improvements, In-Person Proofing, Update language to be simpler and branded

* Add param

* Remove custom initializers from GoodJob classes (#8015)

**Why**: They interfere with the jobs being instantiated by the scheduler

changelog: Internal, Reporting, Fix scheduled SSN and Daily Dropoff report jobs

---------

Co-authored-by: Sheldon Bachstein <sheldon.bachstein@gsa.gov>
Co-authored-by: Mitchell Henke <mitchell.henke@gsa.gov>
Co-authored-by: Jonathan Hooper <jonathan.hooper@gsa.gov>
Co-authored-by: Paul Hirsch <59626817+pauldoomgov@users.noreply.github.com>
Co-authored-by: Sonia Connolly <sonia.connolly@gsa.gov>
Co-authored-by: Stephen Shelton <stephen.shelton@gsa.gov>
Co-authored-by: Paul Hirsch <paul.hirsch@gsa.gov>
Co-authored-by: Shannon A <20867088+svalexander@users.noreply.github.com>
Co-authored-by: Andrew Duthie <andrew.duthie@gsa.gov>
Co-authored-by: Kimball Bighorse <kimball.bighorse@gsa.gov>
Co-authored-by: eric-gade <eric.gade@gsa.gov>
Co-authored-by: jc-gsa <104452882+jc-gsa@users.noreply.github.com>
Co-authored-by: Matt Wagner <mattwagner@navapbc.com>
Co-authored-by: Luis H. Matos <ThatSpaceGuy@users.noreply.github.com>
Co-authored-by: Matt Gardner <wilburnforce@gmail.com>
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.

2 participants