[HOLD for payment 2023-02-15] [$1000] Settings - Changed name is not saved under Display name page #14784

kbecciv · 2023-02-03T03:45:30Z

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

Action Performed:

Access staging.new.expensify.com
Sign into a valid account
Click on >Setting>Profile > Display name
Change name>Click Save
Click the back arrow
Click the Display name again
The new name does not match the old name

Expected Result:

Changed name is saved under Display name page

Actual Result:

Changed name is not saved under Display name page.

Workaround:

Unknown

Platforms:

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

Version Number: 1.2.64.3

Reproducible in staging?: Yes

Reproducible in production?: No

If this was caught during regression testing, add the test name, ID and link from TestRail:

Email or phone of affected tester (no customers):

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos: Any additional supporting documentation

Bug5922983_Recording__1974.mp4

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team & @priya-zha per #14773 (comment)

Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1675348988953739

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~01bc1f33f46287e85e
Upwork Job ID: 1621400923786579968
Last Price Increase: 2023-02-03

melvin-bot · 2023-02-03T03:45:36Z

Triggered auto assignment to @Christinadobrzyn (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

kbecciv · 2023-02-03T03:47:02Z

Issue is not reproduced in Production

Recording.1976.mp4

Christinadobrzyn · 2023-02-03T06:41:27Z

I can repeat this in staging on MacOS / Chrome / Safari following these steps (I updated the OP with these steps)

Access staging.new.expensify.com
Sign into a valid account
Click on > Setting > Profile > Display name
Change name > Click Save
Click the back arrow
Click the Display name again
The new name does not match the old name

I can repeat this on iOS / native
I can't repeat this on the staging MacOS / Desktop
First reported here Display name is not updated after changing it to a new name #14773
I think this can be External but I think we should pay this reporter here since they first reported this

melvin-bot · 2023-02-03T06:51:21Z

Job added to Upwork: https://www.upwork.com/jobs/~01bc1f33f46287e85e

melvin-bot · 2023-02-03T06:51:25Z

Current assignee @Christinadobrzyn is eligible for the External assigner, not assigning anyone new.

melvin-bot · 2023-02-03T06:51:32Z

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

melvin-bot · 2023-02-03T06:51:38Z

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

Christinadobrzyn · 2023-02-03T06:55:52Z

Reading over our documentation, I don't know if external contributors can fix deploy blockers? @techievivek so you know?

Asking the team here

sobitneupane · 2023-02-03T07:33:06Z

This is not limited to Profile Page Display Name only. I can reproduce the same issue on Workspace> General Settings> Name.

It is not limited to TextInput only. I can reproduce the same issue on <Picker /> as well (Workspace > General Settings > Default currency).

sobitneupane · 2023-02-03T07:45:09Z

Change in the following line in #13501 PR has caused this regression.

App/src/components/Form.js

Line 83 in d27dc93

inputValues: this.props.draftValues,

It used to be:

inputValues: {},

tienifr · 2023-02-03T08:12:06Z

Proposal

Problem

As @sobitneupane mentioned above, the root cause is inputValues: this.props.draftValues. It's reference type

In https://github.com/tienifr/App/blob/8f754df0ddea4249eb5d1f94ea6642a8a797d08c/src/components/Form.js#L239
we update this.state.inputValues[inputID] = defaultValue, that makes this.props.draftValues change too.
In this case at first draftValues is empty object and we assign inputValues = draftValues (reference type), if inputValue is undefined we update this.state.inputValues[inputID] = defaultValue; => draftValues is updated to equal defaultValues too. That why when we open the display name form again, the input contains the stale defaultValues

Solution 1

        this.state = {
            errors: {},
-            inputValues: props.draftValues,
+             inputValues: {
+                ...props.draftValues
+            },
        };

Solution 2

I use lodashCloneDeep to clone the new draft object. That can make sure props.draftValues can't be change unexpectedly

...
+import lodashCloneDeep from 'lodash/cloneDeep';
...
        this.state = {
            errors: {},
-            inputValues: props.draftValues,
+            inputValues: lodashCloneDeep(props.draftValues)
        };

Result

Screen.Recording.2023-02-03.at.15.10.51.mov

rushatgabhane · 2023-02-10T05:49:42Z

What should be the next steps? @techievivek @sobitneupane @Christinadobrzyn

Beamanator · 2023-02-10T07:28:57Z

@rushatgabhane normally the next steps would be for the PR author (@tienifr ) to fix the regression, but it looks like there's some other proposals on the linked issue? I don't see any "approved / accepted" proposals though

techievivek · 2023-02-10T07:30:35Z

Can we ask @tienifr to look into the regression and fix it before we go with any new proposals?

Beamanator · 2023-02-10T07:34:13Z

Makes sense to me!

tienifr · 2023-02-10T08:16:14Z

@Beamanator @techievivek Thank for your feedback, but In

App/src/pages/ReimbursementAccount/BankAccountManualStep.js

Lines 114 to 132 in 14ebe26

    
           <CheckboxWithLabel 
        
               style={styles.mt4} 
        
               inputID="acceptedTerms" 
        
               LabelComponent={() => ( 
        
                   <Text> 
        
                       {this.props.translate('common.iAcceptThe')} 
        
                       <TextLink 
        
                           href="https://use.expensify.com/terms" 
        
                           // to call the onPress in the TextLink before the input blur is fired and shift the link element 
        
                           onMouseDown={e => e.preventDefault()} 
        
                       > 
        
                           {`Expensify ${this.props.translate('common.termsOfService')}`} 
        
                       </TextLink> 
        
                   </Text> 
        
               )} 
        
               defaultValue={this.props.getDefaultStateForField('acceptTerms', false)} 
        
               shouldSaveDraft 
        
           />

we use two different names: acceptedTerms for inputID but we get acceptTerms from defaultState
So my suggestion is to change this line

App/src/pages/ReimbursementAccount/BankAccountManualStep.js

Line 130 in 14ebe26

defaultValue={this.props.getDefaultStateForField('acceptTerms', false)}

to

defaultValue={this.props.getDefaultStateForField('acceptedTerms', false)}

My result after applying this change:

Screen.Recording.2023-02-10.at.15.14.17.mov

tienifr · 2023-02-10T08:17:21Z

But I'm not sure this problem comes from my PR, or we should create other issue for this? Thanks.

Beamanator · 2023-02-10T08:26:11Z

@rushatgabhane huh if the problem is that the inputIDs are not matching, why are we thinking the PR for this issue caused the regression? 😅

fedirjh · 2023-02-10T11:13:58Z

@rushatgabhane huh if the problem is that the inputIDs are not matching, why are we thinking the PR for this issue caused the regression? 😅

@Beamanator that's not the problem. I have posted an explanation in the other issue. #14972 (comment)

@tienifr acceptTerms is returned from the server and we use it to initialize the defaultValue . However the Form is not using the defaultValue and instead uses draftValue

tienifr · 2023-02-13T07:22:08Z

I think the problem here is so simple and my PR doesn't cause the regression. We should keep the preceded PR #13501 because it's complex implementation and works well. This is normal if there're some minor issues when we release the new feature. So I pushed the proposal here: #14972 (comment).

rushatgabhane · 2023-02-13T12:51:05Z

@Beamanator yeah my bad 😅
https://github.com/Expensify/App/pull/14866/files isn't causing the regression (can be verified by reverting the PR). Sorry for the trouble

Beamanator · 2023-02-13T17:25:49Z

Okkkkk great good to know, thanks for the investigation @rushatgabhane ! No trouble at all 🙏

Beamanator · 2023-02-13T17:26:24Z

@rushatgabhane looks like you will continue reviewing proposals for the other issue (#14972) over there so I believe this PR can be payed out, right? (in 2 days, i guess)

fedirjh · 2023-02-13T17:30:47Z

We should keep the preceded PR #13501 because it's complex implementation and works well.

No, we should not continue using incorrect patterns just to avoid a bug. In #13501, consistency should have been maintained by using the proper pattern. Instead of passing draftValue down from the Form, it would have been better to subscribe to draftValues with Onyx.

In other components, this is the pattern that we use to retrieve the draftValues.

App/src/pages/ReimbursementAccount/ReimbursementAccountPage.js

Lines 393 to 395 in 6fcad7b

    
           reimbursementAccountDraft: { 
        
               key: ONYXKEYS.REIMBURSEMENT_ACCOUNT_DRAFT, 
        
           },

Christinadobrzyn · 2023-02-15T07:36:43Z

Hi! Just checking on this, @techievivek @Beamanator @fedirjh @tienifr are we waiting on #14972 before issuing payment on this job?

Beamanator · 2023-02-15T10:26:43Z

My understanding is that the other issue (#14972) is unrelated to the fix in this issue. The fix in this issue (#14866) was merely fixing a bug from #13501, so if anything the regression #14972 would have been caused by #13501

So my understanding is we are good to pay this one out, but @rushatgabhane and @fedirjh should continue discussing and investigating a solid fix for #14972 in that issue - I like the discussion, but I don't think it belongs here anymore as this fix was very small, and it tests well.

techievivek · 2023-02-16T05:12:16Z

@Beamanator Hi, can you confirm if @sobitneupane would receive a 50% bonus as the PR was merged within 3 business days?

Christinadobrzyn · 2023-02-16T06:09:23Z

Yes, my understanding is the Contributor and C+ both get the bonus if the PR is merged within 3 days.

paid @priya-zha for reporting
paid @sobitneupane + 50% bonus for C+
paid @tienifr + 50% bonus for fix

I think we just need to finalise this BuzZero checklist

#14784 (comment)

Beamanator · 2023-02-16T09:03:14Z

Makes sense to me @Christinadobrzyn and @techievivek 👍

rushatgabhane · 2023-02-16T20:04:39Z

so I believe this PR can be payed out, right

yes yes

Christinadobrzyn · 2023-02-20T05:32:24Z

@sobitneupane @tienifr can you please complete this checklist so we can close this out?

#14784 (comment)

sobitneupane · 2023-02-20T05:53:35Z

The PR that introduced the bug has been identified. Link to the PR:

#13501

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:

https://github.com/Expensify/App/pull/13501/files#r1111477927

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:

https://expensify.slack.com/archives/C049HHMV9SM/p1676872397051299

tienifr · 2023-02-20T07:11:05Z

@Christinadobrzyn if you mean the regression test step, it was already added here #14784 (comment)

Christinadobrzyn · 2023-02-21T08:06:12Z

Ah thank you @tienifr I missed that. Proposed Regression Test here - https://expensify.slack.com/archives/C01SKUP7QR0/p1676967958834629

@tienifr Should the below Regression Test steps be part of this GH instead?

Verify the form refactor for ACH Contract Step of Add Bank Account form works expectedly
Go to Setting > Workspace > Connect bank account > Connect manually
Fill the form and go to step 4
Check "Somebody else owns more than 25% of" and filling out the Identity Form part way
Refresh the page to ensure that the checkbox is still checked and the identify form is rendered with the values you entered

Christinadobrzyn · 2023-02-22T08:12:16Z

I think this can be closed so I'm going to close it but let me know if it needs to remain open for anything

kbecciv added DeployBlockerCash This issue or pull request should block deployment Hourly KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Feb 3, 2023

melvin-bot bot assigned Christinadobrzyn Feb 3, 2023

melvin-bot bot locked and limited conversation to collaborators Feb 3, 2023

Christinadobrzyn mentioned this issue Feb 3, 2023

Display name is not updated after changing it to a new name #14773

Closed

6 tasks

Christinadobrzyn added the External Added to denote the issue can be worked on by a contributor label Feb 3, 2023

melvin-bot bot unlocked this conversation Feb 3, 2023

melvin-bot bot changed the title ~~Settings - Changed name is not saved under Display name page~~ [$1000] Settings - Changed name is not saved under Display name page Feb 3, 2023

melvin-bot bot assigned eVoloshchak Feb 3, 2023

melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Feb 3, 2023

melvin-bot bot assigned techievivek Feb 3, 2023

melvin-bot bot mentioned this issue Feb 3, 2023

[$1000] [HOLD for payment 2023-02-10] [Form Refactor] ACHContractStep #9578

Closed

8 tasks

melvin-bot bot added Daily KSv2 and removed Hourly KSv2 labels Feb 3, 2023

OSBotify mentioned this issue Feb 3, 2023

Deploy Checklist: New Expensify 2023-02-02 #14771

Closed

30 tasks

Beamanator mentioned this issue Feb 3, 2023

[N7 Account Settings] Create personal details pages #14290

Merged

53 tasks

techievivek assigned sobitneupane and unassigned eVoloshchak Feb 3, 2023

melvin-bot bot added the Reviewing Has a PR in review label Feb 10, 2023

Beamanator removed the Reviewing Has a PR in review label Feb 13, 2023

melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Feb 15, 2023

sobitneupane mentioned this issue Feb 20, 2023

[Form Refactor] ACHContractStep #13501

Merged

57 tasks

Christinadobrzyn closed this as completed Feb 22, 2023

[HOLD for payment 2023-02-15] [$1000] Settings - Changed name is not saved under Display name page #14784

[HOLD for payment 2023-02-15] [$1000] Settings - Changed name is not saved under Display name page #14784

Comments

kbecciv commented Feb 3, 2023 • edited by Christinadobrzyn Loading

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

melvin-bot bot commented Feb 3, 2023

kbecciv commented Feb 3, 2023

Christinadobrzyn commented Feb 3, 2023 • edited Loading

melvin-bot bot commented Feb 3, 2023

melvin-bot bot commented Feb 3, 2023

melvin-bot bot commented Feb 3, 2023

melvin-bot bot commented Feb 3, 2023

Christinadobrzyn commented Feb 3, 2023

sobitneupane commented Feb 3, 2023

sobitneupane commented Feb 3, 2023

tienifr commented Feb 3, 2023 • edited Loading

Proposal

Problem

Solution 1

Solution 2

Result

rushatgabhane commented Feb 10, 2023 • edited Loading

Beamanator commented Feb 10, 2023

techievivek commented Feb 10, 2023

Beamanator commented Feb 10, 2023

tienifr commented Feb 10, 2023

tienifr commented Feb 10, 2023

Beamanator commented Feb 10, 2023

fedirjh commented Feb 10, 2023 • edited Loading

tienifr commented Feb 13, 2023

rushatgabhane commented Feb 13, 2023 • edited Loading

Beamanator commented Feb 13, 2023

Beamanator commented Feb 13, 2023 • edited Loading

fedirjh commented Feb 13, 2023 • edited Loading

Christinadobrzyn commented Feb 15, 2023

Beamanator commented Feb 15, 2023 • edited Loading

techievivek commented Feb 16, 2023

Christinadobrzyn commented Feb 16, 2023

Beamanator commented Feb 16, 2023

rushatgabhane commented Feb 16, 2023

Christinadobrzyn commented Feb 20, 2023

sobitneupane commented Feb 20, 2023 • edited Loading

tienifr commented Feb 20, 2023

Christinadobrzyn commented Feb 21, 2023 • edited Loading

Christinadobrzyn commented Feb 22, 2023

kbecciv commented Feb 3, 2023 •

edited by Christinadobrzyn

Loading

Christinadobrzyn commented Feb 3, 2023 •

edited

Loading

tienifr commented Feb 3, 2023 •

edited

Loading

rushatgabhane commented Feb 10, 2023 •

edited

Loading

fedirjh commented Feb 10, 2023 •

edited

Loading

rushatgabhane commented Feb 13, 2023 •

edited

Loading

Beamanator commented Feb 13, 2023 •

edited

Loading

fedirjh commented Feb 13, 2023 •

edited

Loading

Beamanator commented Feb 15, 2023 •

edited

Loading

sobitneupane commented Feb 20, 2023 •

edited

Loading

Christinadobrzyn commented Feb 21, 2023 •

edited

Loading