fix: Change password hint color to red when invalid, else grey #5130

mananjadhav · 2021-09-08T05:27:39Z

@Jag96 Can you please review this?

Details

Show the password hint color to red when it is invalid otherwise always show the message in grey

Fixed Issues

$ #4995

Tests

Blank password field should show grey hint
Invalid Password should show the hint in red
Valid Password should show grey hint

QA Steps

Open the Change Password
Check the empty New Password field to show grey hint
Enter the invalid password and hint text should show it in red
Enter the valid password and the hint should show back in grey.

Tested On

Screenshots

Web

v1

https://user-images.githubusercontent.com/3069065/135293821-e1f56e42-5e7c-4e49-b211-a1799de306eb.mov

Latest
https://user-images.githubusercontent.com/3069065/137942020-5fefb22e-6dd2-4962-82a1-4aa981841e57.mov

Mobile Web

v1

Latest

Desktop

v1

Latest

iOS

v1

Latest

Android

v1

Latest

…s invalid, else grey

marcaaron · 2021-09-08T15:50:21Z

src/pages/settings/PasswordPage.js

@@ -86,8 +80,8 @@ class PasswordPage extends Component {
        }
    }

-    isValidPassword() {
-        return this.state.newPassword.match(CONST.PASSWORD_COMPLEXITY_REGEX_STRING);
+    isInvalidPassword() {


Why change from isValidPassword to isInvalidPassword? We usually use the "positive" version of things rather than check if something is "not" something like isNotValid() or isInvalid().

Okay, earlier this function was used for a flag that we don’t use anymore. I’ll update the logic to use ‘isValidPassword’

@marcaaron Are you suggesting we have

isValidPassword() { return this.state.newPassword.match(CONST.PASSWORD_COMPLEXITY_REGEX_STRING); } and then `this.state.newPassword && !isValidPassword()`

That is what we initially thought to use but wanted to move this && to a separate function. Let me know and I'll accordingly update.

Good point on keeping the function positive, in that case, yes I think we'd have to leave that logic in the render function the way you specified (maybe split out the style array into separate lines like we do here to avoid a giant line).

Alternatively, you could change the function to be called something like shouldApplyPasswordErrorStyles and include the empty state check, but I don't like that solution as much.

Yeah agree and hence, lets just split into lines with an && check

I don't know why we need two separate lines. If there's no password it's not valid, right?

return this.state.newPassword && this.state.newPassword.match(CONST.PASSWORD_COMPLEXITY_REGEX_STRING);

Or maybe we just mean syntactically use a line break? That seems fine.

We wanted to match the same behavior from the screenshots in #5093 where the field shows as grey initially if the user hasn't typed anything.

Changed to isValidPassword

…d to isValidPassword

mananjadhav · 2021-09-09T00:27:11Z

@Jag96 @marcaaron PR updated

Jag96

Just a couple of style nitpicks, otherwise looks good!

Jag96 · 2021-09-09T19:46:50Z

src/pages/settings/PasswordPage.js

+                            >
+                                {this.props.translate('passwordPage.newPasswordPrompt')}
+                            </Text>
+


Remove spaces between elements

Jag96 · 2021-09-09T19:50:12Z

src/pages/settings/PasswordPage.js

-                            )}
+
+                            <Text style={[
+                                styles.textLabelSupporting, styles.mt1,


Can you put each of these on their own line?

Can you move the style prop to its own line and indent accordingly? There's an example in the // Good section of https://github.com/Expensify/App/blob/main/STYLING.md#when-to-create-a-new-style

Okay. Thanks for the share. I wasn't sure if every style will be moved to a new line. Do you think this should be added as a lint config?

That sounds like a good improvement, I think it's worth bringing up in slack to at least get a discussion going, but we'll keep that separate from this PR.

…ssword-hint-color

mananjadhav · 2021-09-10T01:01:44Z

PR updated

Jag96 · 2021-09-10T22:33:04Z

Thanks for the update @mananjadhav!

In slack, we just agreed on some updated ways to handle displaying errors, so if you'd like to update this PR to match the new standard we're happy to provide a bonus (equal to the current price) for the updated scope. Otherwise, we can merge this PR as it is and open a separate issue to update the code to the new standard. Either is fine, just let us know what you prefer!

The updates we're looking to implement are:

Update the form so the Save button is always enabled/clickable.
Always show the new password requirements text, and only update it to have formError styles when the user submits the form and the new password doesn't meet the requirements.
The onBlur functions can be removed, if the passwords don't match after the user submits the form display the Passwords must match error where it is now.

Feel free to ask any questions as well!

mananjadhav · 2021-09-11T03:16:58Z

Hi it makes sense to update the code in this PR only. I’ll do the needful.

mananjadhav · 2021-09-11T03:40:08Z

@Jag96 Two questions based on the slack discussion here

Should I add border color to formError or that is already handled?
If I type the valid password after the error message is shown, should we have onBlur to remove the message?

…ssword-hint-color

mananjadhav · 2021-09-12T15:02:42Z

@Jag96 I started with the implementation and have one more question. Once we get rid of the following block, we'll also have to show Required field validations, right?

isDisabled={!this.state.currentPassword || !this.state.newPassword
                                || !this.state.confirmNewPassword
                                || (this.state.newPassword !== this.state.confirmNewPassword)
                                || (this.state.currentPassword === this.state.newPassword)
                                || !this.state.newPassword.match(CONST.PASSWORD_COMPLEXITY_REGEX_STRING)}

Jag96 · 2021-09-14T21:27:56Z

@mananjadhav sounds great! To answer your questions

The errorText prop on the ExpensiTextInput component is what adds the red border I believe, but in this case since we want the message to show up as gray as well as red, it may not be as simple as just using errorText. If that is the case, I think it's fine to leave the field without a red border for the new password input for now and we can update it later when we create a generic form component. If you can get it working w/ ExpensiTextInput though, that works!
onBlur isn't required so I'd say let's leave it out for now per this message.

@Jag96 I started with the implementation and have one more question. Once we get rid of the following block, we'll also have to show Required field validations, right?

Yes, that's correct, the required field validations will have to move into handleChangePassword or another function. We can use the errorText prop on the ExpensiTextInput for displaying the error messages, and if you need to add a new error for something like Password is a required field you can add it to this obj and ask for a Spanish translation in slack.

kadiealexander · 2021-09-16T03:49:21Z

Hi @mananjadhav, I've placed this issue on hold as per this update, as we are prioritising issues related to a feature release scheduled for 9/24. As an apology for the delay, we will add a $100 bonus as a thank you for waiting.

…ssword-hint-color

Jag96

Just one request to update JS docs, otherwise looks good!

Jag96 · 2021-10-18T18:41:46Z

src/pages/settings/PasswordPage.js


-        this.setState(stateToUpdate);
+    clearErrorAndSetValue(field, value) {


Add jsdocs here and to other functions

Updated. Sorry, not sure how I missed these 🤔

Jag96

Conflicts and a couple of things, otherwise looking good

Jag96 · 2021-11-03T18:59:25Z

src/languages/en.js

+            currentPassword: 'Current password is required',
+            confirmNewPassword: 'Confirm password is required',
+            newPasswordSameAsOld: 'New password must be different than your old password',
+            newPassword: 'Your password must have at least 8 characters,\n1 capital letter, 1 lowercase letter, 1 number.',


Suggested change

newPassword: 'Your password must have at least 8 characters,\n1 capital letter, 1 lowercase letter, 1 number.',

newPassword: 'Your password must have at least 8 characters,\n1 capital letter, 1 lowercase letter, and 1 number.',

Same for the Spanish translation

Jag96 · 2021-11-03T19:00:04Z

src/libs/ValidationUtils.js

@@ -248,6 +258,7 @@ function isValidLengthForFirstOrLastName(name) {
    return name.length <= 50;
 }

+


Unnecessary extra space

Jag96 · 2021-11-03T19:04:06Z

src/pages/settings/PasswordPage.js

-        } else {
-            this.setState({shouldShowPasswordConfirmError: false});
+    /**
+     * Validate all fields and submit the form if no errors


Update comment, this function no longer submits the form

…ssword-hint-color # Conflicts: # src/pages/settings/PasswordPage.js

mananjadhav · 2021-11-04T15:05:03Z

@Jag96 PR updated with the changes.

Jag96

LGTM, @marcaaron all yours

marcaaron

This looks pretty good, but I did have some suggestions. I think the logic around which errors are getting set is a lot more difficult to suss out now - but I don't have a great suggestion for that other than extract some of the conditions into variables so there's less to parse mentally.

marcaaron · 2021-11-04T23:05:59Z

src/libs/ValidationUtils.js

@@ -230,6 +230,16 @@ function isValidUSPhone(phoneNumber) {
    return CONST.REGEX.PHONE_E164_PLUS.test(phoneNumber.replace(CONST.REGEX.NON_ALPHA_NUMERIC, '')) && CONST.REGEX.US_PHONE.test(phoneNumber);
 }

+/**
+ * Checks if the password matches regex


Remove this comment please

marcaaron · 2021-11-04T23:06:45Z

src/pages/settings/PasswordPage.js

-        } else {
-            stateToUpdate.isPasswordRequirementsVisible = false;
+    /**
+     * Return the error message for the field


Remove this comment please

marcaaron · 2021-11-04T23:07:54Z

src/pages/settings/PasswordPage.js

        }
+        return null;


If the doc says we are returning a string then don't return null return an empty string.

marcaaron · 2021-11-04T23:08:40Z

src/pages/settings/PasswordPage.js

-            stateToUpdate.shouldShowPasswordConfirmError = false;
+
+    /**
+     * Set value for the field in state and clear error flag


Remove this comment

marcaaron · 2021-11-04T23:09:01Z

src/pages/settings/PasswordPage.js

-        } else {
-            this.setState({shouldShowPasswordConfirmError: false});
+    /**
+     * Validate all fields


marcaaron · 2021-11-04T23:10:19Z

src/pages/settings/PasswordPage.js

+     * @param {String} value
+     * @param {String[]} additionalErrorFlags
+     */
+    clearErrorAndSetValue(field, value, additionalErrorFlags) {


additionalErrorsToClear would be a better name

marcaaron · 2021-11-04T23:17:05Z

src/pages/settings/PasswordPage.js

-                                onFocus={() => this.setState({isPasswordRequirementsVisible: true})}
-                                onBlur={() => this.onBlurNewPassword()}
+                                hasError={this.state.errors.newPassword || this.state.errors.newPasswordSameAsOld}
+                                errorText={!this.state.errors.newPassword && this.state.errors.newPasswordSameAsOld


Do we need to check both of these conditions to determine which errorText to show? If there's no error it will not return anything?

marcaaron · 2021-11-04T23:18:37Z

src/pages/settings/PasswordPage.js

-                                </Text>
-                            )}
+                            {
+                              !this.state.errors.newPassword && !this.state.errors.newPasswordSameAsOld


NAB, I would probably extract this to a variable for readability like const shouldShowNewPasswordPrompt

marcaaron · 2021-11-04T23:25:47Z

src/pages/settings/PasswordPage.js

                            />
                        </View>
-                        {!this.state.shouldShowPasswordConfirmError && !isEmpty(this.props.account.error) && (
+                        {!_.some(_.values(this.state.errors)) && !_.isEmpty(this.props.account.error) && (


Inverting _.some() is a bit confusing and the _.values() isn't necessary.

Maybe clearer to just do something like....

_.every(this.state.errors, error => !error);

…rompt exp to var

…bility

mananjadhav · 2021-11-08T07:53:10Z

@marcaaron PR updated

marcaaron · 2021-11-08T15:09:35Z

These changes look good, however there is one strange thing happening... after changing the password the fields are not cleared and we see this:

Should we clear the state here? I think my expectation is that the old passwords would not be sticking around in these fields, but it looks like we are pushing the settings page on top of the change password page instead of popping it off the stack and returning us to the settings page.

Maybe things already work this way on production, but should be fixed. Thoughts @Jag96 ?

mananjadhav · 2021-11-08T15:34:53Z

Should we clear the state here

Agreed. Let me know what you and @Jag96 decide. Will do the needful.

mananjadhav · 2021-11-09T12:15:53Z

Any updates for me on this one?

Jag96 · 2021-11-09T14:55:57Z

Should we clear the state here? I think my expectation is that the old passwords would not be sticking around in these fields, but it looks like we are pushing the settings page on top of the change password page instead of popping it off the stack and returning us to the settings page.

I think going back is better than navigating to the settings page, so I agree with the improvement being proposed.

Also, per this message we're putting all non-critical issues on merge hold including this one, and adding a $250 bonus.

mananjadhav · 2021-11-09T17:25:36Z

@marcaaron Can you tell me in which scenario are the fields retained? I just tried and it didn't. I tried clicking on back, etc. too. One area where I saw the values being retained in the fields is when there's an error. Can you help me with the steps so that I can fix and do the needful.

change-password-redirect.mov

Jag96 · 2021-11-09T21:53:54Z

Hmm, it looks like if you load the page by typing http://localhost:8080/settings/password (or whatever URL you use to test with settings/password at the end) in the URL bar and then submit the form, you get the behavior where the previous values stick around. If you load up the app without having the settings page already open the bug doesn't happen. Using onBackButtonPress={() => Navigation.goBack()} will navigate back if there is a settings menu to go back to, and if there isn't then it will just close the modal, so I think that is an ok solution here.

For the actual submit, we can do the same thing, since it seems to behave the same.

Jag96 · 2021-11-18T21:44:04Z

This issue has been taken off hold! @mananjadhav if you have any questions on #5130 (comment) let me know!

mananjadhav · 2021-11-20T19:49:05Z

@Jag96 PR was updated yesterday

Jag96

@marcaaron all yours

marcaaron · 2021-11-22T19:45:32Z

src/pages/settings/PasswordPage.js

-    isValidPassword() {
-        return this.state.newPassword.match(CONST.PASSWORD_COMPLEXITY_REGEX_STRING);
-    }
+        if (!this.state.newPassword || !isValidPassword(this.state.newPassword)) {


NAB, !this.state.newPassword check should be performed inside isValidPassword()?

OSBotify · 2021-11-22T21:11:47Z

🚀 Deployed to staging by @marcaaron in version: 1.1.15-19 🚀

platform	result
🤖 android 🤖	failure ❌
🖥 desktop 🖥	failure ❌
🍎 iOS 🍎	success ✅
🕸 web 🕸	failure ❌

OSBotify · 2021-11-29T18:47:48Z

🚀 Deployed to production by @roryabraham in version: 1.1.16-10 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

fix(password-hint-color): Change password hint color to red when it i…

254e1d3

…s invalid, else grey

mananjadhav requested a review from a team as a code owner September 8, 2021 05:27

MelvinBot requested review from marcaaron and removed request for a team September 8, 2021 05:27

marcaaron requested changes Sep 8, 2021

View reviewed changes

marcaaron assigned mananjadhav Sep 8, 2021

fix(password-hint-color): Changed function flow from isInvalidPasswor…

5a3e33b

…d to isValidPassword

Jag96 suggested changes Sep 9, 2021

View reviewed changes

mananjadhav added 3 commits September 10, 2021 06:21

Merge branch 'main' of https://github.com/mananjadhav/App into fix/pa…

d33a958

…ssword-hint-color

style(password-hint-color): Fixed style changes

25e16ad

style(password-hint-color): Moved style prop to newline

821a6b1

mananjadhav requested a review from Jag96 September 10, 2021 20:51

PrashantMangukiya mentioned this pull request Sep 11, 2021

Confirm password box moves after setting removing focus from set new password box #5189

Closed

Merge branch 'main' of https://github.com/mananjadhav/App into fix/pa…

226a6cd

…ssword-hint-color

fix(password-hint-color): Changed logic from onBlur to onClick

8a8e16b

kadiealexander added the n6-hold label Sep 16, 2021

Jag96 mentioned this pull request Sep 22, 2021

User is able to change the password after entering incorrect information in the confirm password field #5431

Closed

mananjadhav added 3 commits September 26, 2021 22:03

Merge branch 'main' of https://github.com/mananjadhav/App into fix/pa…

52501c6

…ssword-hint-color

fix(password-onsubmit-error): Changed all errors to onSubmit

5cfca27

fix(password-onsubmit-error): Clear error when user starts typing

7e90a49

mananjadhav mentioned this pull request Sep 29, 2021

[HOLD for payment 2021-12-06] Change Password - Error message not displayed in red for incorrect new password #4995

Closed

Jag96 reviewed Oct 18, 2021

View reviewed changes

Jag96 suggested changes Nov 3, 2021

View reviewed changes

mananjadhav added 2 commits November 4, 2021 20:30

fix(password-hinit-color): Fixed lint issues and minor typos

c6b2766

Merge branch 'main' of https://github.com/mananjadhav/App into fix/pa…

e9399f3

…ssword-hint-color # Conflicts: # src/pages/settings/PasswordPage.js

mananjadhav requested a review from Jag96 November 4, 2021 15:07

Jag96 previously approved these changes Nov 4, 2021

View reviewed changes

marcaaron requested changes Nov 4, 2021

View reviewed changes

mananjadhav added 3 commits November 8, 2021 00:34

fix(password-hint-color): Removed unwanted comments and renamed params

8d97c12

fix(password-hint-color): Updated conditions and moved new password p…

d3c5afc

…rompt exp to var

fix(password-hinit-color): Changed _.some to _.every for better reada…

7be90c8

…bility

mananjadhav dismissed Jag96’s stale review via 7be90c8 November 8, 2021 07:52

mananjadhav requested a review from marcaaron November 8, 2021 07:56

marcaaron previously approved these changes Nov 8, 2021

View reviewed changes

fix(password-hint-color): Changed redirect from Settings to back

c94d639

mananjadhav dismissed marcaaron’s stale review via c94d639 November 19, 2021 14:30

Jag96 approved these changes Nov 22, 2021

View reviewed changes

marcaaron approved these changes Nov 22, 2021

View reviewed changes

marcaaron merged commit e27a916 into Expensify:main Nov 22, 2021

OSBotify mentioned this pull request Nov 22, 2021

Deploy Checklist: New Expensify 2021-11-22 #6399

Closed


		this.setState(stateToUpdate);
		clearErrorAndSetValue(field, value) {

	newPassword: 'Your password must have at least 8 characters,\n1 capital letter, 1 lowercase letter, 1 number.',
	newPassword: 'Your password must have at least 8 characters,\n1 capital letter, 1 lowercase letter, and 1 number.',

		@@ -248,6 +258,7 @@ function isValidLengthForFirstOrLastName(name) {
		return name.length <= 50;
		}

fix: Change password hint color to red when invalid, else grey #5130

fix: Change password hint color to red when invalid, else grey #5130

Conversation

mananjadhav commented Sep 8, 2021 • edited

Details

Fixed Issues

Tests

QA Steps

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

Choose a reason for hiding this comment

mananjadhav Sep 8, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mananjadhav commented Sep 9, 2021

Jag96 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mananjadhav commented Sep 10, 2021

Jag96 commented Sep 10, 2021

mananjadhav commented Sep 11, 2021

mananjadhav commented Sep 11, 2021

mananjadhav commented Sep 12, 2021 • edited

Jag96 commented Sep 14, 2021

kadiealexander commented Sep 16, 2021

Jag96 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jag96 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mananjadhav commented Nov 4, 2021

Jag96 left a comment

Choose a reason for hiding this comment

marcaaron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mananjadhav commented Nov 8, 2021

marcaaron commented Nov 8, 2021

mananjadhav commented Nov 8, 2021

mananjadhav commented Nov 9, 2021

Jag96 commented Nov 9, 2021

mananjadhav commented Nov 9, 2021

Jag96 commented Nov 9, 2021 • edited

Jag96 commented Nov 18, 2021

mananjadhav commented Nov 20, 2021

Jag96 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OSBotify commented Nov 22, 2021

OSBotify commented Nov 29, 2021

mananjadhav commented Sep 8, 2021 •

edited

mananjadhav Sep 8, 2021 •

edited

mananjadhav commented Sep 12, 2021 •

edited

Jag96 commented Nov 9, 2021 •

edited