Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[HOLD for payment 2024-01-25] IOU - Page flashes when adding or replacing receipt in details page #34379

Closed
3 of 6 tasks
lanitochka17 opened this issue Jan 11, 2024 · 24 comments
Closed
3 of 6 tasks
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering

Comments

@lanitochka17
Copy link

lanitochka17 commented Jan 11, 2024

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.24-4
Reproducible in staging?: Y
Reproducible in production?: N
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 1:1 DM
  2. Create a manual request
  3. Go to request details page
  4. Add a receipt
  5. Replace the receipt

Expected Result:

When adding or replacing receipt, the thread view will not flash

Actual Result:

When adding or replacing receipt, the thread view flashes briefly

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

Bug6339083_1704989674828.20240111_211846__1_.mp4

View all open jobs on GitHub

@lanitochka17 lanitochka17 added the DeployBlockerCash This issue or pull request should block deployment label Jan 11, 2024
Copy link
Contributor

👋 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.

Copy link

melvin-bot bot commented Jan 11, 2024

Triggered auto assignment to @rlinoz (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

@situchan
Copy link
Contributor

This happens on production for me

@bernhardoj
Copy link
Contributor

bernhardoj commented Jan 11, 2024

Proposal

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

Report screen flashes after replacing a receipt.

What is the root cause of that problem?

This happens after #32764. When we press replace receipt, it will go to the receipt page and pass a backTo param of the current report screen path (/r/123).

After we select a new receipt, it will call goBack with the backTo params.

const updateScanAndNavigate = useCallback(
(file, source) => {
IOU.replaceReceipt(transactionID, file, source);
if (backTo) {
Navigation.goBack(backTo);
return;
}
Navigation.dismissModal();

Because the receipt selector is the only RHP page, it will REPLACE the current screen with the report screen, thus the flash.

if (shouldEnforceFallback || (isFirstRouteInNavigator && fallbackRoute)) {
navigate(fallbackRoute, CONST.NAVIGATION.TYPE.UP);
return;
}

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

I think we don't need the backTo params for the edit case, so we can just dismiss the RHP modal instead of calling the goBack function.

What alternative solutions did you explore? (Optional)

If the reason backTo is added is to handle the case where the user deep-linked to the edit receipt page from a different report and we want to open the report after editing it, then we can just pass the reportID to dismissModal function.

(we will still remove the goBack function)

Navigation.dismissModal(reportID);

This backTo to report flash/blink issue is actually easily reproducible if we pass the backTo param manually to any other page that supports it. To further fix this, we can handle this inside the Navigation.goBack function itself. The idea is, if the fallback route is a report screen route, then we get the report ID from the route and call Navigation.dismissModal.

@situchan
Copy link
Contributor

I am not sure what the bug is here.
@bernhardoj can you share 2 videos? one for staging (bug), one for production (not bug)?

@situchan
Copy link
Contributor

cc: @tgolen

@bernhardoj
Copy link
Contributor

Here is on prod, the report screen doesn't flash.

Screen.Recording.2024-01-12.at.00.36.18.mov

You can see the issue between 0:15-0:17

@situchan
Copy link
Contributor

situchan commented Jan 11, 2024

Gotcha. I see 00:23-00:24 is the bug from the video from @lanitochka17 .

@rlinoz
Copy link
Contributor

rlinoz commented Jan 11, 2024

I am still not sure if I get it, but not a blocker, removing the label.

@rlinoz rlinoz removed the DeployBlockerCash This issue or pull request should block deployment label Jan 11, 2024
@tgolen tgolen added Daily KSv2 and removed Hourly KSv2 labels Jan 11, 2024
@tgolen
Copy link
Contributor

tgolen commented Jan 11, 2024

Skipping the goBack() and just calling dismissModal() for the edit case makes sense 👍

@rlinoz
Copy link
Contributor

rlinoz commented Jan 11, 2024

@situchan do you agree with the proposal here?

@situchan
Copy link
Contributor

@situchan do you agree with the proposal here?

yes agree

@rlinoz
Copy link
Contributor

rlinoz commented Jan 11, 2024

Ok, assigning @bernhardoj then, thanks!

@situchan
Copy link
Contributor

Please assign me here too

@bernhardoj
Copy link
Contributor

PR is ready

cc: @situchan

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Jan 18, 2024
@melvin-bot melvin-bot bot changed the title IOU - Page flashes when adding or replacing receipt in details page [HOLD for payment 2024-01-25] IOU - Page flashes when adding or replacing receipt in details page Jan 18, 2024
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Jan 18, 2024
Copy link

melvin-bot bot commented Jan 18, 2024

Reviewing label has been removed, please complete the "BugZero Checklist".

Copy link

melvin-bot bot commented Jan 18, 2024

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

If no regressions arise, payment will be issued on 2024-01-25. 🎊

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

  • @bernhardoj requires payment (Needs manual offer from BZ)
  • @situchan requires payment (Needs manual offer from BZ)

@melvin-bot melvin-bot bot added the Overdue label Jan 29, 2024
@rlinoz rlinoz added the Bug Something is broken. Auto assigns a BugZero manager. label Jan 29, 2024
Copy link

melvin-bot bot commented Jan 29, 2024

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

@melvin-bot melvin-bot bot added Daily KSv2 and removed Overdue Weekly KSv2 labels Jan 29, 2024
@rlinoz
Copy link
Contributor

rlinoz commented Jan 29, 2024

I forgot to add the bug label before.

@trjExpensify could you please handle the payment?

@trjExpensify
Copy link
Contributor

Sure thing! Confirming payments as follows:

$500 - @situchan for C+
$500 - @bernhardoj for the fix

Let me know if that's correct and I'll ship the offers.

@rlinoz
Copy link
Contributor

rlinoz commented Jan 30, 2024

That does seem correct!

@trjExpensify
Copy link
Contributor

Cool, offers sent!

@trjExpensify
Copy link
Contributor

@bernhardoj - paid!

@trjExpensify
Copy link
Contributor

@situchan - paid. Closing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering
Projects
None yet
Development

No branches or pull requests

6 participants