[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
• [x] MacOS: Chrome / Safari
• [ ] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/78819774/7bd85fee-8702-4f2b-a2a2-d5b2ddac8662

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[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. https://github.com/Expensify/App/blob/15cf6adceb3a209a28cbafe1d460580afa2a433e/src/libs/actions/Policy.ts#L1094

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

https://github.com/Expensify/App/assets/85894871/36570ab6-1e6c-44b2-8c8a-e90c488bed7d

[melvin-bot[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[bot]]

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

[melvin-bot[bot]]

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

[sonialiap]

Might be related to https://github.com/Expensify/App/issues/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 https://github.com/Expensify/App/blob/15cf6adceb3a209a28cbafe1d460580afa2a433e/src/libs/actions/Policy.ts#L1094
• updateWorkspaceDescription https://github.com/Expensify/App/blob/15cf6adceb3a209a28cbafe1d460580afa2a433e/src/libs/actions/Policy.ts#L1178

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:

https://github.com/Expensify/App/assets/12425932/3e05a7fc-6f44-4450-9b20-6f73e01301a3

[Krishna2323]

We need to use Onyx.METHOD.SET for the reason mentioned here: https://github.com/Expensify/App/blob/15cf6adceb3a209a28cbafe1d460580afa2a433e/src/libs/actions/Policy.ts#L1093-L1094

[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

https://github.com/Expensify/App/blob/15cf6adceb3a209a28cbafe1d460580afa2a433e/src/libs/actions/Policy.ts#L1094

https://github.com/Expensify/App/blob/15cf6adceb3a209a28cbafe1d460580afa2a433e/src/libs/actions/Policy.ts#L1100

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,
},

https://github.com/Expensify/App/blob/15cf6adceb3a209a28cbafe1d460580afa2a433e/src/libs/actions/Policy.ts#L1100

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() - https://github.com/Expensify/App/issues/37254#issuecomment-1983788129

[melvin-bot[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.

https://github.com/Expensify/App/issues/37254#issuecomment-1983788129, 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

https://github.com/Expensify/App/issues/37254#issuecomment-1983788129, 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

[Krishna2323]

https://github.com/Expensify/App/issues/37254#issuecomment-1983810198, I posted this because I updated the root cause and I agree that at first my root cause was not 100% correct but partially it was. The solution remained same from start, there was no change in the solution, I just removed redundant parts.

Please wait on @rlinoz's final decision.

[dukenv0307]

@rushatgabhane @rlinoz TLDR, Here's the summary timeline:

• On Feb 27, @Krishna2323 posted his first proposals, subsequently make 3 more edits, but the core root cause solution is the same, which highlights the root cause is in here (updateWorkspaceDescription) and the fix will be there.

• On March 7 12:46 UTC, the issue was made external, @Krishna2323 comes in and make another edit, which enforces the change they want to make in updateWorkspaceDescription (if you try to apply the solution at this point, it won't fix the issue, because both the root cause and solution are not correct)

• On March 7 15:41 UTC, @dukenv0307 posted a proposal highlighting the root cause in updateGeneralSettings and the update required to make there

• 6 minutes later, at 15:47 UTC, @Krishna2323 updates his proposal that make changes changes both the root cause and solution, now they are both on the updateGeneralSettings, I attached the diff below for you to evaluate if it's "removed redundant parts" or a "complete rewrite". They could have made the changes in any of the 4 edits in the past 1.5 weeks headstart since they posted the first proposal (including 1 edit after the issue was made External), but didn't, they only did 6 minutes after I posted my proposal with the correct fix on updateGeneralSettings

Let's keep discussion minimal and wait for @rlinoz and @rushatgabhane to evaluate the who to assign.

Thank you!

[Krishna2323]

@dukenv0307, that's not a fair evaluation, I already agreed many times that I made change in the root cause and I even posted Proposal Updated after updating, and in my initial proposal I did mentioned that we need to make changes to updateGeneralSettings, you intentionally cropped that part in you last comment I don't know why 😕, updating updateGeneralSettings was mentioned in my proposal's second edit which came 10days before your initial propsoal. Here is a screenshot (second edit on 27th feb). Even in the recordings you can check my solution was working, and that recording was also posted 10days before your proposal.

The redundant part that I removed is to update updateWorkspaceDescription and that's what I removed, I didn't even removed it, just crossed it so it can be seen that I removed only that part and updateGeneralSettings wasn't added after your change it was there from the initial proposal.

and this needs to be updated in updateWorkspaceDescription & updateGeneralSettings.

[sonialiap]

@rushatgabhane @rlinoz who do you think we should assign to this?

[rushatgabhane]

I gave this a look again. I think we should assign @Krishna2323 because they were first - https://github.com/Expensify/App/issues/37254#issuecomment-1984502868

[dukenv0307]

Based on ProposalPolice's investigation here, it was also found that there's significant proposal change of Krishna2323's proposal after I posted my proposal 🥲

they were first

@rushatgabhane Could you clarify more on this? Do you agree with this timeline, if not, can give more explanation on your preference, it will help clear everyone's doubt.

Would you select the version of Krishna2323's proposal right before I posted my proposal, as it is (before it was edited with significant proposal change detected by ProposalPolice)?

Thank you!

[Krishna2323]

Proposal police clearly mentions the change was in the RCA, pls don't use incomplete sentences.

significant proposal change detected (RCA)

Before update: We update the pendingFields of the policy without passing previous pendingFields fields that should be there even after the change.

After update: Here we use SET which removes the other pending fields.

Also, I don't think this is a significant change because previous RCA was also correct and after updating I posted proposal updated here.

Important points that were mentioned in my proposal before your proposal was posted:

1. We update the pendingFields of the policy without passing previous pendingFields fields that should be there even after the change.
2. needs to be updated updateGeneralSettings
3. The recording clearly shows my solution was working
4. This comment came before your proposal, by this we can clearly understand that I knew what I mentioned in my proposal.

I believe they are enough for anyone to understand the solution. Again, all these points I mentioned came before dukenv0307's proposal.

[rlinoz]

I agree with @rushatgabhane , so I am assigning @Krishna2323

The RCA could have been better explained in the first post, and we didn't need to update updateWorkspaceDescription, but the proposal already points to fix updateGeneralSettings.

[melvin-bot[bot]]

❌ There was an error making the offer to @Krishna2323 for the Contributor role. The BZ member will need to manually hire the contributor.

[rlinoz]

Hey @sonialiap if you could help with the comment above