Custom units are broken

[jasperhuangg]

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:

1. Open or create any workspace.
2. Attempt to update the custom unit rates.
3. Notice how it just doesn't work.

Screen.Recording.2022-09-13.at.2.01.02.PM.mov

Expected Result:

Updates to the custom unit rate should be persisted.

Actual Result:

Updates to the custom unit rate aren't persisted. The policy.customUnits is null.

Workaround:

Reverting changes from #10873 fixes things locally

Platform:

Where is this issue occurring?

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

Version Number: v1.2.0-0
Reproducible in staging?: yes
Reproducible in production?: no
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation
Expensify/Expensify Issue URL:
Issue reported by:
Slack conversation:

View all open jobs on GitHub

[melvin-bot]

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

[melvin-bot]

Current assignee @mallenexpensify is eligible for the AutoAssignerTriage assigner, not assigning anyone new.

[jasperhuangg]

@francoisl

The biggest thing I'm noticing from #10873 is that we're not calling Policy.loadFullPolicy anymore. This means that we'll only have loaded the policy from AppInit, which sets customUnits to be null. The only way we can grab a policy's custom units is if we load the full policy.

This is definitely what's causing the custom unit rate to be 0.000 when we first load the page, and any subsequent updates to it to fail. If you log this.props.policy.customUnits it's actually null when it shouldn't be.

I'm not entirely sure what the decision behind getting rid of the call to Policy.loadFullPolicy was, maybe we can wait for @arosiclair to clarify?

[jasperhuangg]

@francoisl After reading the design doc it looks like not calling Policy.loadFullPolicy was an intentional design choice to prevent additional chaining, it seems this was an unintended consequence that wasn't considered previously.

I say we revert #10873 for now to avoid blocking the deploy.

[arosiclair]

For the refactor we moved syncing customUnits to the new OpenWorkspaceReimburseView command here. I'm curious how reverting that PR fixes this issue, because I've noticed that changing the custom unit was already very wonky the last time I tried it.

[jasperhuangg]

@arosiclair The custom unit was fixed recently, but I just compared staging and prod and found it was broken. I also reverted the changes from your PR locally and it fixed the issue.

[OSBotify]

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open StagingDeployCash deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

1. Identify the pull request that introduced this issue and revert it.
2. Find someone who can quickly fix the issue.
3. Fix the issue yourself.

[francoisl]

Yeah from my tests, the root of the issue is that on this line, props.policy.customUnits is null, so we don't find the right customUnitRate. In turn, when we call the API, this error is thrown because we can't find the right custom rate to update.

I'm still looking if there's an easy fix for a bit.

[francoisl]

#10873 was also the last place we called Policy.loadFullPolicy(), so there might also be additional places in the code where we rely on the whole policy being loaded that would now be broken and that we haven't seen yet.

Let's revert the PR to unblock the deploy so that we can look into a proper long-term fix.

[mallenexpensify]

@jasperhuangg , if you can confirm that this worked properly on production in the past, can you add details to the RCA sheet so we can track the regression? Thx

[arosiclair]

So it looks like OpenWorkspaceReimburseView is syncing the policy's customUnit correctly and this is getting passed along to the WorkspaceReimburseView without issue.

However this format is not what the page is expecting. The page is expecting the rates to be set under onyxRates not rates. Since this is undefined, the field displays as 0.000 and if you try to change it, we send mostly empty data to UpdateWorkspaceCustomUnitRate which causes the failure.

@jasperhuangg @yuwenmemon did the customUnits format change recently? And which way is the right way? Should the rates be set under rates or onyxRates?

[arosiclair]

When creating a new workspace, the CreateWorkspace command actually sets the custom unit rates under both properties and the reimburse view works with this data.

[jasperhuangg]

@arosiclair

Ah that was my doing, sorry if this wasn't made clear! We had to move the rates into onxyRates temporarily to ensure that my new custom unit rate changes weren't breaking older clients, which were still accessing from rates.

Notice how in the screenshot onyxRates is keyed by ID, while rates is just an array of rates. We need the rates to be keyed by ID for them to work with any Onyx updates.

This will actually be removed in #10974, so things should just work afterwards?

[arosiclair]

Notice how in the screenshot onyxRates is keyed by ID, while rates is just an array of rates. We need the rates to be keyed by ID for them to work with any Onyx updates.

This will actually be removed in #10974, so things should just work afterwards?

Yeah that sounds about right. https://github.com/Expensify/Web-Expensify/pull/34862 should get the rates keyed by ID and then our form should be working correctly again. I'll re-test the withFullPolicy refactor after that stuff gets merged in

[jasperhuangg]

@arosiclair Sounds like a plan, thanks for waiting on this!

[francoisl]

The fix was tested on staging #10974 (comment), closing.