[HOLD for payment 2024-04-05] [$500] Workspace - If title is changed offline, the description is no longer greyed out

[lanitochka17]

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

---

Version Number: 1.4.44-0
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers): [email protected]
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: Applause - Internal Team
Slack conversation:

Action Performed:

1. Access staging.new.expensify.com
2. Sign into a valid account
3. Go to any Workspace setting
4. Turn off the internet and change the Description (Note that the text will be greyed out)
5. Still offline, change the title of the WS and observe the description

Expected Result:

User expects the Description to continued greyed out until internet is back online

Actual Result:

The description text turns "normal black" after editing the WS name offline

Workaround:

Unknown

Platforms:

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

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6393511_1709000031734.WS_Description_is_no_longer_greyed_out.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01a5b48695496946bf
• Upwork Job ID: 1765718526440808448
• Last Price Increase: 2024-03-07

Issue Owner

Current Issue Owner: @rushatgabhane

[melvin-bot]

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

[lanitochka17]

We think that this bug might be related to #wave8
CC @zanyrenney

[lanitochka17]

@sonialiap 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

[Krishna2323]

Proposal

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

Workspace - If title is changed offline, the description is no longer greyed out

What is the root cause of that problem?

Here we use SET which removes the other pending fields.

App/src/libs/actions/Policy.ts

Line 1094 in 15cf6ad

onyxMethod: Onyx.METHOD.SET,

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

Spread current pendingFields along with the new one.

                pendingFields: {
                    ...policy.pendingFields,
                    ...updatedFields
                },

We should also do the same with the errors field and this needs to be updated in updateWorkspaceDescription & updateGeneralSettings. There might be other util functions also so we will also make sure that they are also updated like this. In updateWorkspaceDescription we need to get policy using const policy = allPolicies?.[${ONYXKEYS.COLLECTION.POLICY}${policyID}];.

Also, this need to be done for optimisticData , finallyData & failureData.

Result

pending_fields_grayed_out.mp4

[melvin-bot]

@sonialiap Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[sonialiap]

Reproducible. I've asked in slack if we want to add it to #vip-split

[melvin-bot]

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

[melvin-bot]

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

[sonialiap]

Might be related to #36598

[dragnoir]

Proposal

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

Workspace setting page, when offline, editing settings make description opacity back to 1

What is the root cause of that problem?

The issue is that we are using Onyx.set for updating name and currency BUT we are using Onyx.merge for updating the description. As discussed on slack many times, Onyx.set can't be used aside with Onyx.merge, and it can defect data like removing previous data)

• updateGeneralSettings
  

  App/src/libs/actions/Policy.ts

  Line 1094 in 15cf6ad

  onyxMethod: Onyx.METHOD.SET,

• updateWorkspaceDescription
  

  App/src/libs/actions/Policy.ts

  Line 1178 in 15cf6ad

  onyxMethod: Onyx.METHOD.MERGE,

Links to slack discussions:

• https://expensify.slack.com/archives/C01GTK53T8Q/p1702656897746359?thread_ts=1702656464.396679&cid=C01GTK53T8Q
• https://expensify.slack.com/archives/C01GTK53T8Q/p1706541752212389?thread_ts=1705484273.717249&cid=C01GTK53T8Q

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

We need to use Onyx.merge with updateGeneralSettings here

-  onyxMethod:  Onyx.METHOD.SET,
+  onyxMethod:  Onyx.METHOD.MERGE,

POC:

20240307_170631.mp4

[Krishna2323]

We need to use Onyx.METHOD.SET for the reason mentioned here:

App/src/libs/actions/Policy.ts

Lines 1093 to 1094 in 15cf6ad

	// We use SET because it's faster than merge and avoids a race condition when setting the currency and navigating the user to the Bank account page in confirmCurrencyChangeAndHideModal
	onyxMethod: Onyx.METHOD.SET,

[dukenv0307]

Proposal

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

The description text turns "normal black" after editing the WS name offline

What is the root cause of that problem?

We're using the set method here to avoid a race condition when setting the currency and navigating the user to the Bank account page, but we forgot to merge other pending fields here

App/src/libs/actions/Policy.ts

Line 1094 in 15cf6ad

onyxMethod: Onyx.METHOD.SET,

App/src/libs/actions/Policy.ts

Line 1100 in 15cf6ad

generalSettings: CONST.RED_BRICK_ROAD_PENDING_ACTION.UPDATE,

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

We should spread other pending fields here, and shouldn't change the method to merge because we use set to avoid race condition

pendingFields: {
    ...policy.pendingFields
    generalSettings: CONST.RED_BRICK_ROAD_PENDING_ACTION.UPDATE,
},

App/src/libs/actions/Policy.ts

Line 1100 in 15cf6ad

generalSettings: CONST.RED_BRICK_ROAD_PENDING_ACTION.UPDATE,

What alternative solutions did you explore? (Optional)

NA

[Krishna2323]

Proposal updated
@dragnoir's analysis is more accurate, though my solution remains the same.

[rushatgabhane]

C+ reviewed 🎀 👀 🎀

I like @Krishna2323's proposal because they were first.

Reason to use Onyx.set() - #37254 (comment)

[melvin-bot]

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

[dukenv0307]

@rushatgabhane Can you please check again, the selected proposal is edited to correct solution and root cause after my proposal is posted

[Krishna2323]

@dukenv0307, My solution was the same from initial proposal, I just added change in root cause and removed some extra changes that we don't require. You can check my initial proposal. Also I commented here about the update.

[dukenv0307]

Here is the last edited of the selected proposal before I post my proposal, the root cause is wrong (It doesn't mention the current root cause is we're using SET method in updateGeneralSetting function which makes the pendingField of description is removed), and the pseudo-code is updated in the wrong place.

[Krishna2323]

@dukenv0307, you can clearly see thats a pseudo-code and below that I already mentioned about updating updateGeneralSettings, I already mentioned that I updated root cause. BTW you posted your solution after @dragnoir's, we can clearly see you have used their root cause and part of my solution to build yours.

#37254 (comment), I commented this before your proposal.

[dukenv0307]

1. Let's the the first root cause in your proposal, it doesn't point to the correct root cause and it's updated after I posted my proposal

#37254 (comment), I commented this before your proposal.

2. It's only pointed out that the reason we use the SET method, does not mean you updated anything in your proposal

BTW you posted your solution after @dragnoir's, we can clearly see you have used their root cause and part of my solution to build yours.

3. The root cause and the solution are straightforward, I don't need to use any existing solution, when I'm typing my proposal dragnoir's proposal is posted

[melvin-bot]

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

[melvin-bot]

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

• fix: Workspace - If title is changed offline, the description is no Ionger greyed out. #38249

If no regressions arise, payment will be issued on 2024-04-05. 🎊

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

• @rushatgabhane requires payment through NewDot Manual Requests
• @Krishna2323 requires payment (Needs manual offer from BZ)

[melvin-bot]

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:

• [@rushatgabhane] The PR that introduced the bug has been identified. Link to the PR:
• [@rushatgabhane] 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:
• [@rushatgabhane] 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:
• [@rushatgabhane] Determine if we should create a regression test for this bug.
• [@rushatgabhane] 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.
• [@sonialiap] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[sonialiap]

@rushatgabhane $500 - please request in ND - please complete the checklist
@Krishna2323 $500 - offer sent - paid ✔️

[Krishna2323]

@sonialiap accepted, thanks!

[melvin-bot]

Payment Summary

Upwork Job

• Reviewer: @rushatgabhane owed $250 via NewDot
• ROLE: @Krishna2323 paid $(AMOUNT) via Upwork (LINK)

BugZero Checklist (@sonialiap)

• 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/1765718526440808448/hired)
• I have paid out the Upwork contracts or cancelled the ones that are incorrect
• I have verified the payment summary above is correct

[JmillsExpensify]

$500 approved for @rushatgabhane

[sonialiap]

@rushatgabhane bump on completing the checklist 🙏

[sonialiap]

@rushatgabhane bump for the checklist #37254 (comment)

[rushatgabhane]

1. The PR that introduced the bug has been identified. Link to the PR: Ask non-USD Workspaces to update currency before adding VBBA #19638

2. 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/19638/files#r1578904212

3. 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: N.A.

4. Determine if we should create a regression test for this bug. Yes

5. 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

   1. Go offline.
   2. Go to any Workspace setting
   3. Change the description
   4. Change the title of the WS
   5. Verify the description is greyed out

[sonialiap]

Morning, my favorite type of bump, @rushatgabhane could we get the checklist completed so that we can close this out? 😁

[melvin-bot]

@rlinoz, @sonialiap, @rushatgabhane, @Krishna2323 Whoops! This issue is 2 days overdue. Let's get this updated quick!

[rushatgabhane]

soon soon soon 🔜

[sonialiap]

@rushatgabhane Looking forward to it 😜

[melvin-bot]

@rlinoz, @sonialiap, @rushatgabhane, @Krishna2323 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[melvin-bot]

@rlinoz, @sonialiap, @rushatgabhane, @Krishna2323 Huh... This is 4 days overdue. Who can take care of this?

[rlinoz]

@rushatgabhane friendly bump

[rushatgabhane]

Updated #37254 (comment)

we can close after regression test is added