[Backwards compatibility] IOU preview not been updated when one expense was moved to the other report

[izarutskaya]

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.52-5
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/4426987&group_by=cases:section_id&group_order=asc&group_id=296773
Email or phone of affected tester (no customers): [email protected]
Logs: https://stackoverflow.com/c/expensify/questions/4856
Issue reported by: Applause-Internal team

Action Performed:

Preconditions:

1. Create a new Gmail account in OD
2. Create collect workspace
3. Set the workspace policy to USD
4. Open the JS console and run the following script in the collect policy settings page:

if (Url.getCurrentPageName()!="policy" ){ alert('Only run this snippet from a policy editor!'); } else { var p = Policy.getCurrent(); p.policy.isPolicyExpenseChatEnabled = "true"; p.save().done(function(){$.jGrowl('Workspace chat creation enabled!')}); }

5. Invite an employee account to the workspace

Steps:

1. Sign In with employee account in ND and OD
2. Create two empty reports in OD
3. Add two expenses to one of the empty reports
4. Navigate to workspace conversation in OD
5. Tap on the IOU preview with two added expenses
6. Navigate back to the workspace conversation
7. Open the report with two expenses in OD
8. Tap on on of the expense on the report page in OD
9. Change the report the report to other one and click on Save button
10. Navigate to LHN in ND
11. Tap on the workspace conversation

Expected Result:

The IOU preview in workspace conversation should be updated since one of the expenses was moved to other report in OD

Actual Result:

The IOU preview remained exactly the same as before when one expense was moved to another report

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

Bug6414392_1710463848514.screen-20240315-024436.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0133592dd5880e808e
• Upwork Job ID: 1775967485159059456
• Last Price Increase: 2024-04-04

[melvin-bot]

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

[izarutskaya]

@JmillsExpensify 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.

[izarutskaya]

We think that this bug might be related to #vip-vsb
CC @quinthar

[melvin-bot]

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

[JmillsExpensify]

This is actually a #wave-collect backward compatibility issue, though I think it's going to be solved by Maxence's PRs that will update the report/preview when expenses are moved between reports. @trjExpensify Do you agree?

[trjExpensify]

The steps in the OP are a bit sloppy. Can we make sure to proof read them before posting?

4. Navigate to workspace conversation in OD

Is this supposed to say ND?

8. Tap on on of the expense on the report page in OD
9. Change the report the report to other one and click on Save button

These don't make sense.

This is actually a #wave-collect backward compatibility issue, though I think it's going to be solved by Maxence's PRs that will update the report/preview when expenses are moved between reports. @trjExpensify Do you agree?

Max is working on moving a report to a different workspace. Moving an expense to a different report should be done already: #33177 CC: @yuwenmemon.

The tester in this video OP might have a bit of a broken account. Notice how there's an empty avatar and Submitter & in the header of the expense report preview in the workspace chat? Might be worth a fresh repro on this, Jason. 👍

[JmillsExpensify]

Not going to spend more time on this, as I don't think it's a priority. Further, the empty avatars is already an existing issue reported and covered elsewhere. That said, I'll highlight in QA for them to re-test.

[JmillsExpensify]

Requested a retest

[kavimuru]

@JmillsExpensify Issue is still reproducible Pixel 6/Android 14

screen-20240403-163730.mp4

[JmillsExpensify]

Thank you!

[yuwenmemon]

From the looks of it seems like we're probably missing sending an Onyx update somewhere in Auth. This is internal.

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~0133592dd5880e808e

[melvin-bot]

Triggered auto assignment to Contributor Plus for review of internal employee PR - @mollfpr (Internal)

[mallenexpensify]

Added to #wave-collect because we want all bugs and feature requests closed or put on a roadmap project. Added back @mollfpr , we have C+ on Internal issues in case they can review the associated PR.

[melvin-bot]

@JmillsExpensify, @yuwenmemon, @mollfpr Whoops! This issue is 2 days overdue. Let's get this updated quick!

[JmillsExpensify]

Still in the queue

[melvin-bot]

@JmillsExpensify, @yuwenmemon, @mollfpr Huh... This is 4 days overdue. Who can take care of this?

[yuwenmemon]

Got back from OOO last week, going to take a look at this again today/tommorow

[melvin-bot]

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

[melvin-bot]

@JmillsExpensify, @yuwenmemon, @mollfpr 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

[JmillsExpensify]

Same as above.

[melvin-bot]

@JmillsExpensify, @yuwenmemon, @mollfpr Huh... This is 4 days overdue. Who can take care of this?

[yuwenmemon]

Argh, my bad on this I have been prioritizing some #wave-collect items over this one. Giving this the midnight look now.

[yuwenmemon]

Okay, so ReportTransactions the API command is not very 1:1:1 at all. Only after it calls Auth's ReportTransactions, does it call UpdateReport to update the new total here: https://github.com/Expensify/Web-Expensify/blob/1cd3b857b978170c71233f812ef517d0040c17ad/lib/ReportAPI.php#L3230-L3237

This led me to some of the code we use to update the report in Onyx whenever it is updated here. However, there's a filter so that we only do this when the previous report was the Auto Report ID (PR):
https://github.com/Expensify/Auth/blame/main/auth/command/UpdateReport.cpp#L149-L181

@deetergp / @Beamanator / @lakchote -- I'm curious as to why we need this check in place. Shouldn't we update the report preview whenever a policy if expense chat is enabled and the report is updated? Or am I missing something here?

[yuwenmemon]

And just as a note for myself, the proposed change I'd make is this very simple diff:

diff --git a/auth/command/UpdateReport.cpp b/auth/command/UpdateReport.cpp
index 0bc0df539e..eba95667c0 100644
--- a/auth/command/UpdateReport.cpp
+++ b/auth/command/UpdateReport.cpp
@@ -146,7 +146,7 @@ void UpdateReport::process()
         }
         DB::write(db, "UPDATE reports SET " + setFields + " WHERE reportID=" + SQ(reportID) + cachedDataStaleCheck + ";");
 
-        if (previousReportID == Transaction::IMPORTMERGE_REPORTID && Policy::isPolicyExpenseChatEnabled(db, Report::getPolicyID(db, reportID))) {
+        if (Policy::isPolicyExpenseChatEnabled(db, Report::getPolicyID(db, reportID))) {
             // Initialize the updates
             JSON::Value expenseReportPreviewOnyxUpdates = JSON::Value(JSON::ARRAY);
             JSON::Value expenseReportOnyxUpdates = JSON::Value(JSON::ARRAY);

[Beamanator]

In theory that makes sense to me! I haven't touched the previousReportID == Transaction::IMPORTMERGE_REPORTID stuff though, so not sure why that was needed / added 😬

[lakchote]

I think @deetergp will have more context on this, as he has added this check with if (previousReportID == Transaction::IMPORTMERGE_REPORTID

[deetergp]

Just talked through this on a call with @yuwenmemon. That check was only put in place to make sure I wasn't adversely affecting other report id changes — I was trying to keep it focused solely on reports going from unreported to reported without creating any side effects. If removing it works for this GH, then by all means, let's get rid of it.

[yuwenmemon]

Draft PR up, will want to most likely add a test

[yuwenmemon]

Ready for review @deetergp !

[melvin-bot]

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

[melvin-bot]

@JmillsExpensify, @yuwenmemon, @mollfpr 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

[melvin-bot]

@JmillsExpensify, @yuwenmemon, @mollfpr 8 days overdue is a lot. Should this be a Weekly issue? If so, feel free to change it!

[JmillsExpensify]

Pretty sure that we should close this issue, as the linked PR above is merged. Additionally, no C+ reviews apply.