[HOLD for payment 2022-01-11] [Network] No error messages shown to user when setpassword gets an error from the API

[anthony-hull]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Action Performed:

sign up as a new user and click the link. Delete the last character of the url and navigate.

Expected Result:

An error should be shown to the user explaining why the request didn't work.
In this case say the key has likely expired and a new one will be waiting in their emails.
I would also expect the email from expensify to be another link to new.expensify.com to finish the set password flow

Actual Result:

https://user-images.githubusercontent.com/47184118/129949187-00c5b590-ca4e-4a7a-abee-54b49ece9952.mp4
the second email has a link to the old expensify password reset flow, not to the new.expensify.com/setpassword/.../...

Workaround:

User would have to guess what went wrong (I'm simulating an expired token).
User would have to check emails finish the flow in the old site and then navigate to the new site.

Platform:

• Web
• iOS
• Android
• Desktop App
• Mobile Web

View all open jobs on GitHub

[MelvinBot]

Triggered auto assignment to @tjferriss (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

[anthony-hull]

should I made another issue for the API not sending the correct link out or will that get one made internally somewhere else?

[akshayasalvi]

I reported a duplicate issue it seems. I am proposing the following solution.

Proposed Solution

Add a catch block to setPassword API:

function setPassword(password, validateCode, accountID) {
  Onyx.merge(ONYXKEYS.ACCOUNT, {...CONST.DEFAULT_ACCOUNT_DATA, loading: true});

  API.SetPassword({
    password,
    validateCode,
    accountID,
  })
    .then((response) => {
         /// existing code
    })
    .catch((err) => { 
        Onyx.merge(ONYXKEYS.ACCOUNT, {error: err.message});
    })
   // existing code
}

[MelvinBot]

@tjferriss Huh... This is 4 days overdue. Who can take care of this?

[anthony-hull]

I would modify:

App/src/libs/actions/Session.js

Lines 261 to 281 in 8a5cf2d

	function setPassword(password, validateCode, accountID) {
	Onyx.merge(ONYXKEYS.ACCOUNT, {...CONST.DEFAULT_ACCOUNT_DATA, loading: true});
	API.SetPassword({
	password,
	validateCode,
	accountID,
	})
	.then((response) => {
	if (response.jsonCode === 200) {
	createTemporaryLogin(response.authToken, response.encryptedAuthToken, response.email);
	return;
	}
	// This request can fail if the password is not complex enough
	Onyx.merge(ONYXKEYS.ACCOUNT, {error: response.message});
	})
	.finally(() => {
	Onyx.merge(ONYXKEYS.ACCOUNT, {loading: false});
	});
	}

To resemble:

App/src/libs/actions/Session.js

Lines 229 to 231 in 8a5cf2d

	.catch((error) => {
	Onyx.merge(ONYXKEYS.ACCOUNT, {error: translateLocal(error.message), loading: false});
	});

Where the error handling would be in a catch block.
I'm not familiar with translateLocal yet. I would look at using this on the error as well.

[anthony-hull]

I had a look at how API.js handles it and we have a bunch of switch cases to cover off giving the right translated strings in the Errors to give to translateLocal:

App/src/libs/API.js

Lines 222 to 251 in 8a5cf2d

	.then((response) => {
	// If we didn't get a 200 response from Authenticate we either failed to Authenticate with
	// an expensify login or the login credentials we created after the initial authentication.
	// In both cases, we need the user to sign in again with their expensify credentials
	if (response.jsonCode !== 200) {
	switch (response.jsonCode) {
	case 401:
	throw new Error('passwordForm.error.incorrectLoginOrPassword');
	case 402:
	// If too few characters are passed as the password, the WAF will pass it to the API as an empty
	// string, which results in a 402 error from Auth.
	if (response.message === '402 Missing partnerUserSecret') {
	throw new Error('passwordForm.error.incorrectLoginOrPassword');
	}
	throw new Error('passwordForm.error.twoFactorAuthenticationEnabled');
	case 403:
	throw new Error('passwordForm.error.invalidLoginOrPassword');
	case 404:
	throw new Error('passwordForm.error.unableToResetPassword');
	case 405:
	throw new Error('passwordForm.error.noAccess');
	case 413:
	throw new Error('passwordForm.error.accountLocked');
	default:
	throw new Error('passwordForm.error.fallback');
	}
	}
	return response;
	});
	}

Do we have such sophistication of HTTP codes for this API call?

For a simple fix I can just use the message in the API response to display. But to bring this upto standard with the rest of the app with translations I need a mapping of the different HTTP error codes to copy.

[anthony-hull]

The API response not being translated should be its own issue. I'll make one later :)

[MelvinBot]

@tjferriss 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

[MelvinBot]

@tjferriss 8 days overdue is a lot. Should this be a Weekly issue? If so, feel free to change it!

[tjferriss]

@akshayasalvi can you link to the duplicate issue you previously mentioned? If you're working on a solution there then can we close this issue?

[akshayasalvi]

@tjferriss the issue is #4745 . I am not working on it at the moment. One of your team member asked me to apply to this issue and closed the other one .

here’s my proposal for the same #4737 (comment)

[tjferriss]

Thanks for referencing it and sorry for the delay here. I'm going to tag engineering so someone can take a look and remove my assignment.

[MelvinBot]

Triggered auto assignment to @chiragsalian (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

[MelvinBot]

Triggered auto assignment to @kadiealexander (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

[kadiealexander]

Hired you in Upwork @akshayasalvi, sorry for the oversight there!

[akshayasalvi]

Thanks a lot @kadiealexander. No worries :)

[kadiealexander]

Hi @akshayasalvi, 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.

[MelvinBot]

@chiragsalian, @akshayasalvi, @kadiealexander Whoops! This issue is 2 days overdue. Let's get this updated quick!

[chiragsalian]

On n-6 hold melvin. Shoo.

Edit i just noticed its daily, doesn't make sense for something thats on hold to be daily. Demoting to weekly.

[kadiealexander]

Please refer to this post for updated information on the n6 hold, we've raised the bonus to $250 for all issues where a PR is created before the N6 hold is lifted.

[kadiealexander]

Hold has been removed, please go for gold @akshayasalvi!!

[MelvinBot]

This issue has not been updated in over 15 days. @chiragsalian, @akshayasalvi, @kadiealexander eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

[botify]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.1.24-19 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Allow redirect to MagicLink after Invalid Validation Code #5150

If no regressions arise, payment will be issued on 2022-01-11. 🎊

[kadiealexander]

Paid, thanks @akshayasalvi!!

[akshayasalvi]

@kadiealexander This PR was on n6-hold and company offsite hold. Bonus for the same wasn’t added

[kadiealexander]

Oops, sorry for missing that one! Have issued the bonus now. :)

[akshayasalvi]

Thanks a lot, @kadiealexander for helping here.

[anthony-hull]

@kadiealexander Should I have had a reporting bonus for this issue?

[kadiealexander]

Hey Anthony! The reporting bonus came into effect in September last year, a few weeks after this issue was raised. (Link to Slack post). The post is clear about issues raised before this date not qualifying for the reporting bonus, but if you think this should be reassessed for this issue please raise the question in #expensify-open-source to have the team review this. :)

[anthony-hull]

no worries! thanks for the info :)