[HOLD for payment 2023-10-23] [HOLD for payment 2023-10-23] [HOLD for payment 2023-10-20] [HOLD for payment 2023-10-20] [$500] MEDIUM: Web - Only one map is displayed for two distance requests if they are identical #27567

kbecciv · 2023-09-15T21:00:35Z

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

Action Performed:

Create a new workspace
Click on the FAB and request money
Click distance and choose your start and end points and click next
Choose the workspace we created and click request
Repeat the steps from 2 to 4 - Make sure to choose the same start and end point
Now we request the same distance twice > Notice the distance request message

Expected Result:

Two maps should be displayed because there are two requests

Actual Result:

Two maps should be displayed because there are two requests

Workaround:

Unknown

Platforms:

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

Version Number: 1.3.70.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:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation

Screencast.from.14-09-23.17_44_59.webm

Recording.4509.mp4

Expensify/Expensify Issue URL:
Issue reported by:@Ahmed-Abdella
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1694704076992449

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~01bd660634ca8e76ff
Upwork Job ID: 1702789545084084224
Last Price Increase: 2023-09-15

melvin-bot · 2023-09-15T21:00:40Z

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

melvin-bot · 2023-09-15T21:00:40Z

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

melvin-bot · 2023-09-15T21:00:42Z

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 · 2023-09-15T21:00:44Z

Triggered auto assignment to @MitchExpensify (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

melvin-bot · 2023-09-15T21:00:50Z

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

akinwale · 2023-09-15T23:41:06Z

This looks like it's intended to me. @hayata-suenaga Could you please confirm if that is the case?

Ahmed-Abdella · 2023-09-15T23:44:28Z

@akinwale the two maps will be displayed if you reload the page, so I think it is not intended. And if it is intended It should display one map after reloading. There is still something need to be fixed

akinwale · 2023-09-15T23:50:22Z

@akinwale the two maps will be displayed if you reload the page, so I think it is not intended. And if it is intended It should display one map after reloading. There is still something need to be fixed

Ah, gotcha. Makes sense. Looking 👀

zukilover · 2023-09-16T00:14:10Z

Proposal

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

Only one map is displayed for two distance requests if they are identical.

What is the root cause of that problem?

There is unnecessary re-render that caused ReportPreview not to render the last created transaction. This is because in the re-render, action's childLastReceiptTransactionIDs is getting overridden by the race condition.

App/src/components/ReportActionItem/ReportPreview.js

Lines 122 to 125 in 76c1559

    
           const lastThreeTransactionsWithReceipts = ReportUtils.getReportPreviewDisplayTransactions(props.action); 
        
           const lastThreeReceipts = _.map(lastThreeTransactionsWithReceipts, ({receipt, filename, receiptFilename}) => 
        
               ReceiptUtils.getThumbnailAndImageURIs(receipt.source, filename || receiptFilename || ''), 
        
           );

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

Prevent re-render by adding another condition in ReportActionItem memo function. It could be by checking whether the report action has the same reportActionTimestamp and the request count is different, because it still needs to populate the report preview to be rendered in the view, e.g.:

           const shouldSkipProcessingDistanceRequest = (
                // Skip re-render if this is the same distance request 
                // that is still in progress of resolving the report view action
                nextProps.action.reportActionTimestamp === prevProps.action.reportActionTimestamp && 
                (nextProps.action.childMoneyRequestCount || 0) < (prevProps.action.childMoneyRequestCount || 0)
            )
            return shouldSkipProcessingDistanceRequest || (current-memo-conditions-here)

What alternative solutions did you explore? (Optional)

N/A

zukilover · 2023-09-16T00:59:15Z

Proposal

Updated

hoangzinh · 2023-09-16T02:40:03Z

Seem BE issue, The API command CreateDistanceRequest returns incorrect values of childLastReceiptTransactionIDs and childMoneyRequestCount in this case

It should be

sakluger · 2023-09-17T23:30:04Z

Added to the wave5 project as requested by @tgolen.

tgolen · 2023-09-17T23:46:12Z

Still looking for proposals. I don't think it is a back-end issue because it should only be merging the new incremental data to the front-end, so I would expect to see it only trying to merge a single transaction.

tgolen · 2023-09-17T23:47:12Z

I'm curious about the re-render proposal from @zukilover. What do you think of it @ArekChr?

ArekChr · 2023-09-18T12:11:41Z

@zukilover, thanks for the proposal! Your current fix seems like a short-term solution. In the future, different props might cause the component to re-render, bringing back this bug. Could you please explore fixing the race condition for a more robust, long-term fix?

paultsimura · 2023-09-18T19:46:30Z

I'm with @hoangzinh on this.
It's a BE issue: the childLastReceiptTransactionIDs is a string, not a list, so it's being overwritten instead of pushing the new value into the list.

Proof:
You can debug this chunk of code on the last API response Onyx operation:

https://github.com/Expensify/react-native-onyx/blob/d6824308f7afa7299502f500d60e9d12b9e119f7/lib/Onyx.js#L1110-L1159

First, you have 2 values in existingData (coming from optimistic data):

Then, the operation from API Response:

And finally, the merge result:

FYI @tgolen

tgolen · 2023-09-19T04:19:16Z

Ah, I see what you mean. That is definitely not ideal that it is a string of comma separated IDs. Not good at all :(

Ideally, the fix would be to make this a nested object like:

childLastReceiptTransactionIDs: {
    id1: id1,
    id2: id2,
}

(which is also better than an array).

cc @Gonals Could you take a look at this bug please and give your opinion on it? It looks like you were the one that implemented the Onyx response for this in Web-E.

Gonals · 2023-09-19T04:25:18Z

Yeah, I agree an object would be better. IIRC, I added this one mimicking childOldestFourAccountIDs, just using GROUP_CONCAT in cpp, and didn't consider making it an array or object at the time.

We should probably update both.

melvin-bot · 2023-10-13T22:03:09Z

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:

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

melvin-bot · 2023-10-13T22:55:01Z

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

Handle latest receipts as object #28225

If no regressions arise, payment will be issued on 2023-10-20. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

External issue reporter
Contributor that fixed the issue
Contributor+ that helped on the issue and/or PR

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

@ArekChr does not require payment (Contractor)

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

Merged PR within 3 business days of assignment - 50% bonus
Merged PR more than 9 business days after assignment - 50% penalty

melvin-bot · 2023-10-13T22:55:03Z

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:

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

ArekChr · 2023-10-16T11:44:26Z

Link to the PR: No bug identified.
Link to comment: n/a
Link to discussion: n/a
Determine if we should create a regression test for this bug: I don’t think we need to add a regression test here.

melvin-bot · 2023-10-16T21:17:37Z

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

Handle latest receipts as object #28225

If no regressions arise, payment will be issued on 2023-10-23. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

External issue reporter
Contributor that fixed the issue
Contributor+ that helped on the issue and/or PR

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

@ArekChr does not require payment (Contractor)

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

Merged PR within 3 business days of assignment - 50% bonus
Merged PR more than 9 business days after assignment - 50% penalty

melvin-bot · 2023-10-16T21:17:39Z

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:

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

melvin-bot · 2023-10-16T21:43:18Z

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

Handle latest receipts as object #28225

If no regressions arise, payment will be issued on 2023-10-23. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

External issue reporter
Contributor that fixed the issue
Contributor+ that helped on the issue and/or PR

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

@ArekChr does not require payment (Contractor)

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

Merged PR within 3 business days of assignment - 50% bonus
Merged PR more than 9 business days after assignment - 50% penalty

melvin-bot · 2023-10-16T21:43:20Z

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:

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

alexpensify · 2023-10-20T23:13:01Z

@ArekChr - to prepare for the payment date, can you please complete the checklist? Thanks!

ArekChr · 2023-10-23T12:41:38Z

@alexpensify I completed checklist here

alexpensify · 2023-10-23T23:18:33Z

Whoops I missed that note, thanks!

alexpensify · 2023-10-23T23:32:11Z

Here is the payment summary:

External issue reporter - @Ahmed-Abdella $50
Contributor that fixed the issue - Internal
Contributor+ that helped on the issue and/or PR - @ArekChr does not require payment (Contractor)

Upwork Job: https://www.upwork.com/jobs/~0141c4e4c10fcd3f72

*If applicable, the bonuses will be applied on the final payment

Extra Notes regarding payment: The job is closed, so I had to create a new one.

alexpensify · 2023-10-23T23:33:02Z

@Ahmed-Abdella - please accept the job in Upwork and I can complete the process. Thanks!

Ahmed-Abdella · 2023-10-23T23:42:43Z

@alexpensify I accepted the offer. Thanks!

alexpensify · 2023-10-23T23:54:31Z

Perfect, all set here! I'm going to close now.

kbecciv added External Added to denote the issue can be worked on by a contributor Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Sep 15, 2023

melvin-bot bot assigned alexpensify Sep 15, 2023

melvin-bot bot changed the title ~~Web - Only one map is displayed for two distance requests if they are identical~~ [$500] Web - Only one map is displayed for two distance requests if they are identical Sep 15, 2023

melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Sep 15, 2023

melvin-bot bot assigned MitchExpensify and ArekChr Sep 15, 2023

alexpensify unassigned MitchExpensify Sep 15, 2023

melvin-bot bot added the Overdue label Sep 17, 2023

tgolen self-assigned this Sep 17, 2023

melvin-bot bot removed the Overdue label Sep 17, 2023

melvin-bot bot added Weekly KSv2 and removed Weekly KSv2 labels Oct 13, 2023

melvin-bot bot added Weekly KSv2 and removed Weekly KSv2 labels Oct 16, 2023

melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Oct 20, 2023

melvin-bot bot added the Overdue label Oct 23, 2023

melvin-bot bot removed the Overdue label Oct 23, 2023

alexpensify closed this as completed Oct 23, 2023

github-project-automation bot moved this from YELLOW to Done in [#whatsnext] Wave 05 - Deprecate Free Oct 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[HOLD for payment 2023-10-23] [HOLD for payment 2023-10-23] [HOLD for payment 2023-10-20] [HOLD for payment 2023-10-20] [$500] MEDIUM: Web - Only one map is displayed for two distance requests if they are identical #27567

[HOLD for payment 2023-10-23] [HOLD for payment 2023-10-23] [HOLD for payment 2023-10-20] [HOLD for payment 2023-10-20] [$500] MEDIUM: Web - Only one map is displayed for two distance requests if they are identical #27567

kbecciv commented Sep 15, 2023 •

edited

Loading

melvin-bot bot commented Sep 15, 2023

melvin-bot bot commented Sep 15, 2023

melvin-bot bot commented Sep 15, 2023

melvin-bot bot commented Sep 15, 2023

melvin-bot bot commented Sep 15, 2023

akinwale commented Sep 15, 2023

Ahmed-Abdella commented Sep 15, 2023

akinwale commented Sep 15, 2023

zukilover commented Sep 16, 2023 •

edited

Loading

zukilover commented Sep 16, 2023

hoangzinh commented Sep 16, 2023

sakluger commented Sep 17, 2023

tgolen commented Sep 17, 2023

tgolen commented Sep 17, 2023

ArekChr commented Sep 18, 2023

paultsimura commented Sep 18, 2023 •

edited

Loading

tgolen commented Sep 19, 2023

Gonals commented Sep 19, 2023

melvin-bot bot commented Oct 13, 2023

melvin-bot bot commented Oct 13, 2023

melvin-bot bot commented Oct 13, 2023

ArekChr commented Oct 16, 2023

melvin-bot bot commented Oct 16, 2023

melvin-bot bot commented Oct 16, 2023

melvin-bot bot commented Oct 16, 2023

melvin-bot bot commented Oct 16, 2023

alexpensify commented Oct 20, 2023

ArekChr commented Oct 23, 2023

alexpensify commented Oct 23, 2023

alexpensify commented Oct 23, 2023 •

edited

Loading

alexpensify commented Oct 23, 2023

Ahmed-Abdella commented Oct 23, 2023

alexpensify commented Oct 23, 2023

[HOLD for payment 2023-10-23] [HOLD for payment 2023-10-23] [HOLD for payment 2023-10-20] [HOLD for payment 2023-10-20] [$500] MEDIUM: Web - Only one map is displayed for two distance requests if they are identical #27567

[HOLD for payment 2023-10-23] [HOLD for payment 2023-10-23] [HOLD for payment 2023-10-20] [HOLD for payment 2023-10-20] [$500] MEDIUM: Web - Only one map is displayed for two distance requests if they are identical #27567

Comments

kbecciv commented Sep 15, 2023 • edited Loading

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

melvin-bot bot commented Sep 15, 2023

melvin-bot bot commented Sep 15, 2023

melvin-bot bot commented Sep 15, 2023

Bug0 Triage Checklist (Main S/O)

melvin-bot bot commented Sep 15, 2023

melvin-bot bot commented Sep 15, 2023

akinwale commented Sep 15, 2023

Ahmed-Abdella commented Sep 15, 2023

akinwale commented Sep 15, 2023

zukilover commented Sep 16, 2023 • edited Loading

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

zukilover commented Sep 16, 2023

Proposal

hoangzinh commented Sep 16, 2023

sakluger commented Sep 17, 2023

tgolen commented Sep 17, 2023

tgolen commented Sep 17, 2023

ArekChr commented Sep 18, 2023

paultsimura commented Sep 18, 2023 • edited Loading

tgolen commented Sep 19, 2023

Gonals commented Sep 19, 2023

melvin-bot bot commented Oct 13, 2023

melvin-bot bot commented Oct 13, 2023

melvin-bot bot commented Oct 13, 2023

ArekChr commented Oct 16, 2023

melvin-bot bot commented Oct 16, 2023

melvin-bot bot commented Oct 16, 2023

melvin-bot bot commented Oct 16, 2023

melvin-bot bot commented Oct 16, 2023

alexpensify commented Oct 20, 2023

ArekChr commented Oct 23, 2023

alexpensify commented Oct 23, 2023

alexpensify commented Oct 23, 2023 • edited Loading

alexpensify commented Oct 23, 2023

Ahmed-Abdella commented Oct 23, 2023

alexpensify commented Oct 23, 2023

kbecciv commented Sep 15, 2023 •

edited

Loading

zukilover commented Sep 16, 2023 •

edited

Loading

paultsimura commented Sep 18, 2023 •

edited

Loading

alexpensify commented Oct 23, 2023 •

edited

Loading