[HOLD for payment 2024-10-25] Contact method - unlinkLoginForm.succesfullyUnlinkedLogin after unlinking secondary login #50256

lanitochka17 · 2024-10-04T16:43:57Z

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

Version Number: 9.0.44-7
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail: N/A
Email or phone of affected tester (no customers): applausetester+kh021001@applause.expensifail.com
Issue reported by: Applause - Internal Team

Action Performed:

Go to staging.new.expensify.com
Go to Account settings > Profile > Contact method
Click New contact method
Add a secondary email and enter magic code, but do not verify it
Log in as the secondary email
Click Unlink
Open email with subject "Unlink secondary Expensify login"
Click on the link "the validation link here"

Expected Result:

App will show the correct successful unlink message

Actual Result:

App shows unlinkLoginForm.succesfullyUnlinkedLogin after unlinking secondary login

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

Screenshots/Videos

Add any screenshot/video evidence

Bug6624569_1728055712579.20241004_232118.mp4

View all open jobs on GitHub

Issue Owner

Current Issue Owner: @trjExpensify / @trjExpensify

melvin-bot · 2024-10-04T16:44:08Z

Triggered auto assignment to @trjExpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

lanitochka17 · 2024-10-04T16:45:20Z

@trjExpensify FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

twilight2294 · 2024-10-04T17:26:45Z

Proposal

Please re-state the problem that we are trying to solve in this issue.

unlinkLoginForm.succesfullyUnlinkedLogin after unlinking secondary login

What is the root cause of that problem?

We are pasting unlinkLoginForm.succesfullyUnlinkedLogin in the success data here:

App/src/libs/actions/Session/index.ts

Lines 890 to 897 in 4e12268

    
           const successData: OnyxUpdate[] = [ 
        
               { 
        
                   onyxMethod: Onyx.METHOD.MERGE, 
        
                   key: ONYXKEYS.ACCOUNT, 
        
                   value: { 
        
                       isLoading: false, 
        
                       message: 'unlinkLoginForm.succesfullyUnlinkedLogin', 
        
                   },

Then in the UnlinkLoginForm below, we check

What changes do you think we should make in order to solve the problem?

Pass a message prop to unlinkLogin and set it as message of successData, the message can be formed in UnlinkLoginPage here and then sent to the function.

We would also ned to update the message below:

App/src/pages/signin/UnlinkLoginForm.tsx

Line 35 in 4e12268

    
           const unlinkMessage = account?.message === 'unlinkLoginForm.linkSent' || account?.message === 'unlinkLoginForm.succesfullyUnlinkedLogin' ? translate(account?.message) : account?.message;

as now we will set the actual message instead of the key

If there is same bug with unlinkLoginForm.linkSent we need to fix that too, also need to check if any BE changes are needed

Implementation:

In UnlinkLoginPage, we need to create a new const as UnlinkLoginMessage:

const unlinkLoginMessage = translate('unlinkLoginForm.succesfullyUnlinkedLogin`)

    useEffect(() => {
        Session.unlinkLogin(Number(accountID), validateCode, unlinkLoginMessage);

    }, []);

then for unlinkLogin function, we would update it to:

function unlinkLogin(accountID: number, validateCode: string, unlinkMessage: string) {
...
...
...
const successData: OnyxUpdate[] = [
        {
            onyxMethod: Onyx.METHOD.MERGE,
            key: ONYXKEYS.ACCOUNT,
            value: {
                isLoading: false,
                message: unlinkMessage,
            },
        },

What alternative solutions did you explore? (Optional)

Nodebrute · 2024-10-04T17:47:49Z

Edited by proposal-police: This proposal was edited at 2024-10-04 17:49:43 UTC.

Proposal

Please re-state the problem that we are trying to solve in this issue.

unlinkLoginForm.succesfullyUnlinkedLogin after unlinking secondary login

What is the root cause of that problem?

We are passing this as a string 'unlinkLoginForm.succesfullyUnlinkedLogin' and we are not using translate here

App/src/libs/actions/Session/index.ts

Line 896 in 896bc78

message: 'unlinkLoginForm.succesfullyUnlinkedLogin',

What changes do you think we should make in order to solve the problem?

let's use translate here translate('unlinkLoginForm.succesfullyUnlinkedLogin')

App/src/libs/actions/Session/index.ts

Line 896 in 896bc78

message: 'unlinkLoginForm.succesfullyUnlinkedLogin',

Let's fix these two value too translate('unlinkLoginForm.linkSent') and translate('unlinkLoginForm.succesfullyUnlinkedLogin')

App/src/pages/signin/UnlinkLoginForm.tsx

Line 35 in 896bc78

    
           const unlinkMessage = account?.message === 'unlinkLoginForm.linkSent' || account?.message === 'unlinkLoginForm.succesfullyUnlinkedLogin' ? translate(account?.message) : account?.message;

We should look for other places too where we are not using translate and fix the issue

Some other places where we should fix this

App/src/libs/actions/Session/index.ts

Line 307 in 896bc78

message: 'resendValidationForm.linkHasBeenResent',

App/src/libs/actions/Session/index.ts

Line 858 in 896bc78

message: 'unlinkLoginForm.linkSent',

Note

We should use the translate method wherever we are using these variables. This will address the issue at its core. Additionally, we have a reviewer checklist item emphasizing the importance of using the translate method with variables.

I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the [translation method](https://github.com/Expensify/App/blob/4bd99402cebdf4d7394e0d1f260879ea238197eb/src/components/withLocalize.js#L60)

What alternative solutions did you explore? (Optional)

Or we can also use Localize.translateLocal in Session/index.ts file and translate in UnlinkLoginForm.tsx file

Nodebrute · 2024-10-04T17:51:45Z

Proposal Updated
Added another place where the fix is needed

Nodebrute · 2024-10-04T18:04:05Z

Updated Proposal
Added Alternate solution

melvin-bot · 2024-10-04T18:31:48Z

Unable to auto-create job on Upwork. The BZ team member should create it manually for this issue.

melvin-bot · 2024-10-04T18:31:53Z

Triggered auto assignment to Contributor-plus team member for initial proposal review - @rojiphil (External)

bernhardoj · 2024-10-05T04:28:28Z

Proposal

Please re-state the problem that we are trying to solve in this issue.

Unlink login form success message is shown with the translation key.

What is the root cause of that problem?

The message is shown without being translated here.

App/src/pages/signin/LoginForm/BaseLoginForm.tsx

Lines 274 to 281 in 99f280b

    
           {(!!closeAccount?.success || !!account?.message) && ( 
        
               <DotIndicatorMessage 
        
                   style={[styles.mv2]} 
        
                   type="success" 
        
                   // eslint-disable-next-line @typescript-eslint/naming-convention,@typescript-eslint/prefer-nullish-coalescing 
        
                   messages={{0: closeAccount?.success ? closeAccount.success : account?.message || ''}} 
        
               /> 
        
           )}

This happens after #42970. Previously, DotIndicatorMessage handles the translation if it needs translation. So, the account message will always be translated, but not with the closeAccount.success because we mark it as translated already.

But after the PR, DotIndicatorMessage will show any message it gets, so the translation responsibility is from the component that uses it.

What changes do you think we should make in order to solve the problem?

To follow the previous behavior, we need to translate the account.message when passing it down to DotIndicatorMessage.

messages={{0: closeAccount?.success ? closeAccount.success : translate(account?.message) || ''}}

melvin-bot · 2024-10-07T18:48:51Z

@rojiphil, @trjExpensify Whoops! This issue is 2 days overdue. Let's get this updated quick!

trjExpensify · 2024-10-08T00:28:44Z

@rojiphil can you review the proposals, please? Thanks!

Nodebrute · 2024-10-08T00:37:56Z

Proposal Updated
Added a note

rojiphil · 2024-10-08T08:56:21Z

@bernhardoj Is there any distinct advantage of using translate just before display as per your proposal vis-a-vis using it at the source itself as proposed by @Nodebrute

bernhardoj · 2024-10-08T10:20:07Z

If we save the translated text to Onyx, then changing the app language won't re-translate it. I believe it's a common pattern to save the translation key to Onyx.

Nodebrute · 2024-10-08T10:24:24Z

@bernhardoj Could you provide any reference where we are saving a translation key to Onyx?

twilight2294 · 2024-10-08T10:41:38Z

@bernhardoj Is there any distinct advantage of using translate just before display as per your proposal vis-a-vis using it at the source itself as proposed by @Nodebrute

@rojiphil FYI, the source is a ts file so we cannot use directly save it there, we need to pass it as a prop

Nodebrute · 2024-10-08T10:48:12Z

@twilight294 We can use Localize.translateLocal, just like we do in other TypeScript files

App/src/libs/PaymentUtils.ts

Line 33 in 867bbb4

    
           return `${Localize.translateLocal('paymentMethodList.accountLastFour')} ${account.accountNumber?.slice(-4)}`;

bernhardoj · 2024-10-08T10:56:12Z

I was going to refer to the IOU file where there are a lot of getMicroSecondOnyxErrorWithTranslationKey usage such as below,

App/src/libs/actions/IOU.ts

Line 863 in 867bbb4

    
           errors: ErrorUtils.getMicroSecondOnyxErrorWithTranslationKey('iou.error.genericCreateFailureMessage'),

but I just realized getMicroSecondOnyxErrorWithTranslationKey translate the key.

App/src/libs/ErrorUtils.ts

Lines 44 to 46 in 867bbb4

    
           function getMicroSecondOnyxErrorWithTranslationKey(error: TranslationPaths, errorKey?: number): Errors { 
        
               return {[errorKey ?? DateUtils.getMicroseconds()]: Localize.translateLocal(error)}; 
        
           }

And again, this happens after #42970. Previously, we saved the translation key (with isTranslated is false, so we know that it needs to be translated).

This means, any errors that use getMicroSecondOnyxErrorWithTranslationKey is not translatable.

Btw, here is an example where we translate the key before showing it.

App/src/pages/signin/UnlinkLoginForm.tsx

Line 35 in 8b27c8d

    
           const unlinkMessage = account?.message === 'unlinkLoginForm.linkSent' || account?.message === 'unlinkLoginForm.succesfullyUnlinkedLogin' ? translate(account?.message) : account?.message;

Maybe we should align first whether a client-side error message should be translatable or not.

Nodebrute · 2024-10-09T15:21:36Z

@bernhardoj The error you mentioned, The provided phone number belongs to landline, is coming from the backend and not from the en.ts/es.ts files. That's why it's not being translated.

Nodebrute · 2024-10-09T15:30:28Z

Here we set error using getMicroSecondOnyxErrorWithTranslationKey which uses Localize.translateLocal(error) under the hood

App/src/libs/actions/TaxRate.ts

Line 148 in 8e53105

    
           errors: ErrorUtils.getMicroSecondOnyxErrorWithTranslationKey('workspace.taxes.error.createFailureMessage'),

This is the error we get when the create tax rate request fails. It is translated into Spanish, and I believe this is a common pattern that we use in our app.

puneetlath · 2024-10-14T15:33:58Z

Ah ok. In that case, I agree with you on saving the key and translating at the time of usage. Is that what we do in other similar scenarios?

rojiphil · 2024-10-14T16:14:56Z

Ah ok. In that case, I agree with you on saving the key and translating at the time of usage. Is that what we do in other similar scenarios?

Well, both approaches are used. But a similar scenario here uses this approach.

puneetlath · 2024-10-14T16:21:24Z

Cool, thanks for the explanation. Let's do it.

bernhardoj · 2024-10-15T05:12:07Z

PR is ready

cc: @rojiphil

melvin-bot · 2024-10-18T18:55:34Z

Reviewing label has been removed, please complete the "BugZero Checklist".

melvin-bot · 2024-10-18T18:55:39Z

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

Fix unlink login success isn't translated #50769

If no regressions arise, payment will be issued on 2024-10-25. 🎊

For reference, here are some details about the assignees on this issue:

@rojiphil requires payment (Needs manual offer from BZ)
@bernhardoj requires payment through NewDot Manual Requests

melvin-bot · 2024-10-18T18:55:40Z

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

[@rojiphil] The PR that introduced the bug has been identified. Link to the PR:
[@rojiphil] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
[@rojiphil] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
[@rojiphil] Determine if we should create a regression test for this bug.
[@rojiphil] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
[@trjExpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

rojiphil · 2024-10-24T19:03:19Z

[@rojiphil] The PR that introduced the bug has been identified. Link to the PR: Offending PR
[@rojiphil] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment: Added comment
[@rojiphil] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion: Not Required. Existing checklist is good enough to capture such issues.
[@rojiphil] Determine if we should create a regression test for this bug. : Yes. We can
[@rojiphil] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.

Steps:

Go to Settings->Profile and add a secondary email.
Login with secondary email and click on Unlink
Open Unlink Validation link email and click on the link
Verify that the unlink success message is displayed.

rojiphil · 2024-10-24T19:06:05Z

@trjExpensify BZ Checklist is done

melvin-bot · 2024-10-25T18:06:21Z

Payment Summary

Upwork Job

ROLE: @rojiphil paid $(AMOUNT) via Upwork (LINK)
Reviewer: @bernhardoj owed $250 via NewDot

BugZero Checklist (@puneetlath)

I have verified the correct assignees and roles are listed above and updated the neccesary manual offers
I have verified that there are no duplicate or incorrect contracts on Upwork for this job (https://www.upwork.com/ab/applicants//hired)
I have paid out the Upwork contracts or cancelled the ones that are incorrect
I have verified the payment summary above is correct

trjExpensify · 2024-10-25T18:44:52Z

Great, thanks! Payment summary as follows:

$250 to @bernhardoj for the fix
$250 to @rojiphil for the C+ review

@rojiphil sent you an offer. @bernhardoj, go ahead and request.

rojiphil · 2024-10-25T18:47:31Z

@rojiphil sent you an offer.

Thanks @trjExpensify. Accepted offer.

trjExpensify · 2024-10-25T18:59:43Z

Perfect, paid!

bernhardoj · 2024-10-26T02:56:22Z

Requested in ND.

JmillsExpensify · 2024-10-28T10:53:56Z

$250 approved for @bernhardoj

lanitochka17 added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Oct 4, 2024

melvin-bot bot assigned trjExpensify Oct 4, 2024

trjExpensify added the External Added to denote the issue can be worked on by a contributor label Oct 4, 2024

melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Oct 4, 2024

melvin-bot bot assigned rojiphil Oct 4, 2024

trjExpensify added this to [#whatsnext] #wave-collect Oct 4, 2024

trjExpensify moved this to Polish in [#whatsnext] #wave-collect Oct 4, 2024

melvin-bot bot added the Overdue label Oct 7, 2024

melvin-bot bot removed the Overdue label Oct 8, 2024

puneetlath assigned bernhardoj Oct 14, 2024

melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Oct 14, 2024

bernhardoj mentioned this issue Oct 15, 2024

Fix unlink login success isn't translated #50769

Merged

50 tasks

melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Oct 15, 2024

melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Oct 18, 2024

melvin-bot bot changed the title ~~Contact method - unlinkLoginForm.succesfullyUnlinkedLogin after unlinking secondary login~~ [HOLD for payment 2024-10-25] Contact method - unlinkLoginForm.succesfullyUnlinkedLogin after unlinking secondary login Oct 18, 2024

melvin-bot bot removed the Reviewing Has a PR in review label Oct 18, 2024

rojiphil mentioned this issue Oct 24, 2024

fix crash when trying to unlink secondary contact #23561

Merged

58 tasks

melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Oct 25, 2024

trjExpensify closed this as completed Oct 25, 2024

[HOLD for payment 2024-10-25] Contact method - unlinkLoginForm.succesfullyUnlinkedLogin after unlinking secondary login #50256

[HOLD for payment 2024-10-25] Contact method - unlinkLoginForm.succesfullyUnlinkedLogin after unlinking secondary login #50256

Comments

lanitochka17 commented Oct 4, 2024 • edited by puneetlath Loading

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

Screenshots/Videos

melvin-bot bot commented Oct 4, 2024

lanitochka17 commented Oct 4, 2024

twilight2294 commented Oct 4, 2024 • edited Loading

Proposal

Please re-state the problem that we are trying to solve in this issue.

What is the root cause of that problem?

What changes do you think we should make in order to solve the problem?

What alternative solutions did you explore? (Optional)

Nodebrute commented Oct 4, 2024 • edited Loading

Proposal

Please re-state the problem that we are trying to solve in this issue.

What is the root cause of that problem?

What changes do you think we should make in order to solve the problem?

What alternative solutions did you explore? (Optional)

Nodebrute commented Oct 4, 2024

Nodebrute commented Oct 4, 2024

melvin-bot bot commented Oct 4, 2024

melvin-bot bot commented Oct 4, 2024

bernhardoj commented Oct 5, 2024

Proposal

Please re-state the problem that we are trying to solve in this issue.

What is the root cause of that problem?

What changes do you think we should make in order to solve the problem?

melvin-bot bot commented Oct 7, 2024

trjExpensify commented Oct 8, 2024

Nodebrute commented Oct 8, 2024

rojiphil commented Oct 8, 2024

bernhardoj commented Oct 8, 2024

Nodebrute commented Oct 8, 2024

twilight2294 commented Oct 8, 2024

Nodebrute commented Oct 8, 2024

bernhardoj commented Oct 8, 2024 • edited Loading

Nodebrute commented Oct 9, 2024 • edited Loading

Nodebrute commented Oct 9, 2024

puneetlath commented Oct 14, 2024

rojiphil commented Oct 14, 2024

puneetlath commented Oct 14, 2024

bernhardoj commented Oct 15, 2024

melvin-bot bot commented Oct 18, 2024

melvin-bot bot commented Oct 18, 2024

melvin-bot bot commented Oct 18, 2024 • edited by puneetlath Loading

rojiphil commented Oct 24, 2024

rojiphil commented Oct 24, 2024

melvin-bot bot commented Oct 25, 2024

Payment Summary

BugZero Checklist (@puneetlath)

trjExpensify commented Oct 25, 2024

rojiphil commented Oct 25, 2024

trjExpensify commented Oct 25, 2024

bernhardoj commented Oct 26, 2024

JmillsExpensify commented Oct 28, 2024

lanitochka17 commented Oct 4, 2024 •

edited by puneetlath

Loading

twilight2294 commented Oct 4, 2024 •

edited

Loading

Nodebrute commented Oct 4, 2024 •

edited

Loading

bernhardoj commented Oct 8, 2024 •

edited

Loading

Nodebrute commented Oct 9, 2024 •

edited

Loading

melvin-bot bot commented Oct 18, 2024 •

edited by puneetlath

Loading