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 2023-10-10] [$125] Task - When the receiver clicks on a task, "Hmm... it's not here" is displayed briefly #28508

Closed
6 tasks done
kbecciv opened this issue Sep 29, 2023 · 40 comments
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 External Added to denote the issue can be worked on by a contributor

Comments

@kbecciv
Copy link

kbecciv commented Sep 29, 2023

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:

  1. Log in to User A's account.
  2. Open User B's chat.
  3. Navigate to the "Assign task" section.
  4. Create a task by entering a title.
  5. Open another browser and log in to User B's account.
  6. Open User A's chat and open the task.

Expected Result:

When the receiver clicks on the task, "Hmm... it's not here" should not be displayed

Actual Result:

When the receiver clicks on the task, "Hmm... it's not here" is displayed briefly

Workaround:

Unknown

Platforms:

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

  • Android / native
  • Android / Chrome
  • iOS / native
  • iOS / Safari
  • MacOS / Chrome / Safari
  • MacOS / Desktop

Version Number: 1.3.75.2
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
Notes/Photos/Videos: Any additional supporting documentation

screen-recording-2023-09-28-at-94712-pm_Ajx72u09.1.mp4
Screen.Recording.2023-09-28.at.9.49.08.PM.mov
Screen.Recording.2023-09-28.at.9.50.03.PM.mov
Screen.Recording.2023-09-28.at.9.52.22.PM.mov
20230929211406.mp4
Screen_Recording_20230929_121540_New.Expensify.mp4

Expensify/Expensify Issue URL:
Issue reported by: @ayazhussain79
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1695920292264359

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01f2fa932f5d6f4f9c
  • Upwork Job ID: 1708867633313923072
  • Last Price Increase: 2023-10-02
  • Automatic offers:
    • fedirjh | Reviewer | 26981978
    • dukenv0307 | Contributor | 26981980
@kbecciv kbecciv added the DeployBlockerCash This issue or pull request should block deployment label Sep 29, 2023
@OSBotify
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.

@melvin-bot
Copy link

melvin-bot bot commented Sep 29, 2023

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

@astrohunter62
Copy link
Contributor

Proposal

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

When the receiver clicks on a task, "Hmm... it's not here" is displayed briefly.

What is the root cause of that problem?

In the ReportScreen, the report prop does not immediately update when the navigation changes. Consequently, when the recipient clicks on a task, report.reportID becomes undefined, causing shouldShowNotFoundPage to be true and displaying the 'Hmm... it's not here.

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

We can use the currentReportID props.

For example:

const shouldShowNotFoundPage = useMemo(
  () =>
      (!firstRenderRef.current &&
          !_.isEmpty(report) &&
          !isDefaultReport &&
          !currentReportID &&    // report.reportID -> currentReportID 
          !isOptimisticDelete &&
          !report.isLoadingReportActions &&
          !isLoading &&
          !userLeavingStatus) ||
      shouldHideReport,
  [report, isDefaultReport, currentReportID, isOptimisticDelete, isLoading, userLeavingStatus, shouldHideReport],
);

Result:

28508.Chrome.webm
28508.Safari.webm

@dukenv0307
Copy link
Contributor

dukenv0307 commented Sep 30, 2023

Proposal

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

Task - When the receiver clicks on a task, "Hmm... it's not here" is displayed briefly

What is the root cause of that problem?

We move isLoadingReportActions to reportMetaData and in openReport API, we set it as true in reportMetaData

{
onyxMethod: Onyx.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.REPORT_METADATA}${reportID}`,
value: {
isLoadingReportActions: true,

But here we use report.isLoadingReportActions to check whether the report action is loading or not. So the check is wrong and not found page is displayed briefly

!report.isLoadingReportActions &&

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

We should replace report.isLoadingReportActions with reportMetaData.isLoadingReportActions to check whether the report action is loading or not

!report.isLoadingReportActions &&

We should set the default for isLoadingReportActions here as true because when we open the new report, reportMedata doesn't exist before it's merged in Onyx.

reportMetadata: {
isLoadingReportActions: false,
isLoadingMoreReportActions: false,

isLoadingReportActions: false,

And here, we should check the report exists in Onyx but reportActions is empty we will call openReport API to get the reportActions and update isLoadingReportActions to false

if (report.reportID && report.reportID === getReportID(route) && !_.isEmpty(reportActions)) {
    return;
}

if (report.reportID && report.reportID === getReportID(route)) {

What alternative solutions did you explore? (Optional)

NA

Result

Screencast.from.30-09-2023.14.23.16.webm

@mountiny
Copy link
Contributor

mountiny commented Oct 1, 2023

the proposal from @dukenv0307 sounds good to me! @ospfranco @fedirjh what do you think?

@fedirjh
Copy link
Contributor

fedirjh commented Oct 1, 2023

And here, we should check the report exists in Onyx but reportActions is empty we will call openReport API to get the reportActions and update isLoadingReportActions to false

Why this change is needed? is it possible that we have an existing report without report actions?

Edit: It's possible as openApp command doesn’t include all reports actions collection.

@mountiny
Copy link
Contributor

mountiny commented Oct 1, 2023

We only return the report actions for parent threads with the parent report actions

@dukenv0307
Copy link
Contributor

dukenv0307 commented Oct 1, 2023

Why this change is needed? is it possible that we have an existing report without report actions?

@fedirjh Let's imagine this case that is after signing out and signing again, we open a report in LHN, the report data exists but doesn't have any report actions, and openReport API isn't called with this condition and then the page will load infinitely

if (report.reportID && report.reportID === getReportID(route)) {
return;

It was not necessary before because in the past ReportActionView was rendered briefly and in this component, we always call openReport API when it's mounted. But with my proposal, the skeleton will be displayed until the openReport API is called in ReportScreen. So this check above is necessary.

const openReportIfNecessary = () => {
// If the report is optimistic (AKA not yet created) we don't need to call openReport again
if (props.report.isOptimisticReport) {
return;
}
Report.openReport(reportID);
};
useEffect(() => {
openReportIfNecessary();
// eslint-disable-next-line react-hooks/exhaustive-deps
}, []);

@fedirjh
Copy link
Contributor

fedirjh commented Oct 1, 2023

Thanks @dukenv0307 that makes sense to me.

cc @mountiny the proposal looks good to me.

@ospfranco
Copy link
Contributor

@dukenv0307 LGTM

@mountiny
Copy link
Contributor

mountiny commented Oct 2, 2023

This will be $125 to @dukenv0307 for the solution proposal

@mountiny mountiny added the Bug Something is broken. Auto assigns a BugZero manager. label Oct 2, 2023
@melvin-bot
Copy link

melvin-bot bot commented Oct 2, 2023

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

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 Daily KSv2 labels Oct 9, 2023
@kevinksullivan
Copy link
Contributor

@mountiny can you take a look at this?

@mountiny
Copy link
Contributor

mountiny commented Oct 9, 2023

@fedirjh @dukenv0307 can you please complete the checklist above? Thanks!

@fedirjh
Copy link
Contributor

fedirjh commented Oct 10, 2023

BugZero Checklist:


Regression Test Proposal

  1. Log in to User A's account.
  2. Open User B's chat.
  3. Navigate to the "Assign task" section.
  4. Create a task by entering a title.
  5. Open another browser and log in to User B's account.
  6. Open User A's chat and open the task.
  7. Verify Skeleton is displayed briefly and "Hmm... it's not here" should not be displayed
  • Do we agree 👍 or 👎

@mountiny
Copy link
Contributor

Thanks created the regression test here https://github.com/Expensify/Expensify/issues/324653

@melvin-bot melvin-bot bot added the Overdue label Oct 13, 2023
@melvin-bot
Copy link

melvin-bot bot commented Oct 16, 2023

@ospfranco, @kevinksullivan, @mountiny, @fedirjh, @dukenv0307 Huh... This is 4 days overdue. Who can take care of this?

@mountiny
Copy link
Contributor

@kevinksullivan can we please pay $50 to @ayazhussain79 and $125 to @dukenv0307 for the proposal

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Oct 16, 2023
@mountiny mountiny changed the title [HOLD for payment 2023-10-10] [HOLD for payment 2023-10-09] [$125] Task - When the receiver clicks on a task, "Hmm... it's not here" is displayed briefly [HOLD for payment 2023-10-10] [$125] Task - When the receiver clicks on a task, "Hmm... it's not here" is displayed briefly Oct 20, 2023
@melvin-bot
Copy link

melvin-bot bot commented Oct 20, 2023

@ospfranco, @kevinksullivan, @mountiny, @fedirjh, @dukenv0307 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Oct 23, 2023
@mountiny mountiny added Bug Something is broken. Auto assigns a BugZero manager. and removed Bug Something is broken. Auto assigns a BugZero manager. labels Oct 23, 2023
@melvin-bot
Copy link

melvin-bot bot commented Oct 23, 2023

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

@melvin-bot melvin-bot bot removed the Overdue label Oct 23, 2023
@melvin-bot
Copy link

melvin-bot bot commented Oct 23, 2023

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

@mountiny
Copy link
Contributor

@trjExpensify could you please help me process the payments here #28508 (comment) thanks!

@trjExpensify
Copy link
Contributor

as per here, confirming payments as follows:

$50 to @ayazhussain79 for the bug report and $125 to @dukenv0307 for the proposal.

@dukenv0307 has been paid. @ayazhussain79 i've sent you an offer.

@ayazhussain79
Copy link
Contributor

@trjExpensify offer accepted, Thank you

@trjExpensify
Copy link
Contributor

Paid.

@kevinksullivan
Copy link
Contributor

kevinksullivan commented Oct 25, 2023

Sorry for missing this one guys. And thanks @trjExpensify !

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 External Added to denote the issue can be worked on by a contributor
Projects
None yet
Development

No branches or pull requests