[$500] Request Money – Edited fields are not saved grayed out on the distance report page Offline

[lanitochka17]

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

---

Version Number: 1.4.8-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):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: Applause - Internal Team
Slack conversation:

Action Performed:

1. Go to https://staging.new.expensify.com/
2. Log in
3. Navigate to a distance request details conversation or create it
4. Disable the internet connection
5. On the distance report page select the Amount, Description or Date fields
6. Edit some of the fields
7. Select save

Expected Result:

Edited fields are grayed out on the distance report page Offline

Actual Result:

Edited fields are not saved grayed out on the distance report page 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

Bug6302246_1701802787866.Edited_fields_are_not_saved_grey.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0181753718dd096504
• Upwork Job ID: 1732115829983993856
• Last Price Increase: 2023-12-12
• Automatic offers:
• situchan | Contributor | 28033443

[melvin-bot]

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

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~0181753718dd096504

[melvin-bot]

Bug0 Triage Checklist (Main S/O)

• This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• This bug is not a duplicate report (check E/App issues and #expensify-bugs)
  • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
• This bug is reproducible using the reproduction steps in the OP. S/O
  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
• This issue is filled out as thoroughly and clearly as possible
  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
• I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[melvin-bot]

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

[paultsimura]

This is a regression from #30290, where it was decided to use the SET operation for updating the transaction.
This now results in overwriting the pending fields with only the ones from the last offline modification, which did not happen when using the MERGE operation.

More explanation here: #30290 (comment)

[dukenv0307]

Proposal

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

Request Money – Edited fields are not saved grayed out on the distance report page Offline

What is the root cause of that problem?

I don't think this is a regression from #30290. Using the SET method is correct to update transactions because we are using the getUpdatedTransaction function to retain fields that are not updated
The RCA is here

App/src/libs/actions/IOU.js

Line 749 in 5a0b7fd

pendingFields,

We don't retain pendingFields of old transaction

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

In here

App/src/libs/TransactionUtils.ts

Line 177 in 5a0b7fd

updatedTransaction.pendingFields = {

we should add ...transaction.pendingFields, to cover pendingFields of old transaction

Then, we need to remove this line

App/src/libs/actions/IOU.js

Line 749 in 5a0b7fd

pendingFields,

because updatedTransaction has updated pendingFields

What alternative solutions did you explore? (Optional)

[paultsimura]

I don't think this is a regression from #30290. Using the SET method is correct to update transactions

It is a correct method, otherwise, it wouldn't have been approved by the C+ in the first place.
However, it doesn't change the fact that this issue is caused by the linked PR, which technically means this is a regression, and the linked issue is still in the regression period, which means the author would have to fix it as a follow-up.

[dukenv0307]

@paultsimura Please take a look at my proposal first, I explained the details and the RCA of this issue differs from the linked issue. Even if we revert the PR of the linked issue, we still need to fix this issue. We should do the perfect work to fix the root issue instead of working around to fix some cases by reverting the old PR. Let's wait for the comment from C+

[DylanDylann]

Hi @paultsimura, I agree with the above explanation

[mananjadhav]

I can only take this up in 4-5 hours. Feel free to reassign if this requires more urgency.

[melvin-bot]

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

[melvin-bot]

📣 @situchan 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

[mountiny]

@situchan please proceed with the proposal review

[melvin-bot]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[situchan]

reviewing proposals

[paultsimura]

Just in case, I updated the original comment with more details of why this is a regression: #32511 (comment)

[melvin-bot]

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

[melvin-bot]

@JmillsExpensify, @situchan Eep! 4 days overdue now. Issues have feelings too...

[situchan]

The PR fixing #30290 was reverted.

[paultsimura]

After the revert, this issue is not reproducible anymore.
@situchan should we handle the proper fix of #30290 here as suggested in #33098 (comment)?

[situchan]

I am not sure which GH is the best to handle. Maybe #30290 which is original?

[situchan]

@mountiny what do you think?

[mountiny]

@situchan sorry just to make sure I understand, the og issue here is fixed but there is another issue which is not logged? if we had to revert some PR then I think the changes should be handled in the issue the reverted PR was linked to

[situchan]

This issue is regression and the offending PR is reverted so not reproducible anymore.
I think we can close this

[situchan]

Btw, do you think it's worth fixing #32511 (comment)?

[paultsimura]

According to this thread, it's recommended to put non-critical issues related to updates of Distance requests on hold until #28358 is done

[mountiny]

I see ok I think that if this is the case we should just close this issue which is not reproducible and just make sure that whenever the next attempt on fixing the original issue will be done, we need to test for this bug.

But I dont think we should keep this open especially since its not repro