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

[$250] Track Expense - Hmm..not here page on selecting category with no existing workspace #43073

Closed
3 of 6 tasks
kbecciv opened this issue Jun 4, 2024 · 31 comments
Closed
3 of 6 tasks
Assignees
Labels
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 Jun 4, 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-79-3
Reproducible in staging?: y
Reproducible in production?: n
If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/4594444&group_by=cases:section_id&group_order=asc&group_id=309130
Issue reported by: Applause - Internal Team

Action Performed:

  1. Navigate to staging.new.expensify.com
  2. Create new account
  3. Submit a track expense in the self DM
  4. Click on Categorize it on the actionable whisper options of a track expense
  5. Select a category shown in the list to categorize the expense

Expected Result:

As per step TC 6 - After selecting a category, the "To" field should be filled with a "new" workspace

Actual Result:

Hmm..not here page on selecting category with no existing workspace

Workaround:

n/a

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

image

image

Bug6502071_1717534334395.RPReplay_Final1717534228.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01a18541e1775fab5a
  • Upwork Job ID: 1798437262634635540
  • Last Price Increase: 2024-06-05
Issue OwnerCurrent Issue Owner: @mananjadhav
@kbecciv kbecciv added DeployBlockerCash This issue or pull request should block deployment DeployBlocker Indicates it should block deploying the API labels Jun 4, 2024
Copy link

melvin-bot bot commented Jun 4, 2024

Triggered auto assignment to @blimpich (DeployBlockerCash), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

Copy link
Contributor

github-actions bot commented Jun 4, 2024

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

@kbecciv
Copy link
Author

kbecciv commented Jun 4, 2024

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

@blimpich
Copy link
Contributor

blimpich commented Jun 4, 2024

Looking into this.

@blimpich
Copy link
Contributor

blimpich commented Jun 4, 2024

Reproduced it locally. I think this PR is really suspicious given the timing and the area of the codebase. Reverting that PR locally doesn't fix it though, so that doesn't seem like its the root cause.

Not 100% sure if this is backend or frontend, seems like frontend because I can't find any wonky network requests or logs that are giving me any leads. Though maybe it has to do with bad handling of onyx updates in the backend. Not sure.

Reached out in #expensify-opensource (link)

@blimpich
Copy link
Contributor

blimpich commented Jun 4, 2024

I think this might be backend related, since I went back about a week and a half in App history and its still an issue. Stuck on how to debug this. Can't seem to figure out where it is going wrong.

@blimpich
Copy link
Contributor

blimpich commented Jun 4, 2024

Huh, well when I actually try to reproduce this on staging it doesn't reproduce, so maybe someone put out a fix that is related to this.

@blimpich
Copy link
Contributor

blimpich commented Jun 4, 2024

Weird, yup, pulled everything and now its working locally on main.

@blimpich
Copy link
Contributor

blimpich commented Jun 4, 2024

Asked QA to retest

In the meantime I think we can demote as this is no longer reproducing for me on staging or locally. Should probably figure out what the exact cause was, but seems to be fixed.

@blimpich blimpich added Daily KSv2 and removed DeployBlocker Indicates it should block deploying the API DeployBlockerCash This issue or pull request should block deployment Hourly KSv2 labels Jun 4, 2024
@kavimuru
Copy link

kavimuru commented Jun 5, 2024

Tester can reproduce the bug with new gmail account too.

RPReplay_Final1717548936.MP4
RPReplay_Final1717549357.MP4

@cretadn22
Copy link
Contributor

cretadn22 commented Jun 5, 2024

Proposal

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

Hmm..not here page on selecting category with no existing workspace

What is the root cause of that problem?

When navigating to the confirmation page, the reportID parameter is undefined, causing this bug

if (action === CONST.IOU.ACTION.CATEGORIZE) {
Navigation.navigate(ROUTES.MONEY_REQUEST_STEP_CONFIRMATION.getRoute(action, iouType, transactionID, report?.reportID ?? ''));
return;

The report.reportID is undefined because we're utilizing the wrong report field. In this scenario, we should be using reportDraft instead of realReport.

const report = reportReal ?? reportDraft;

The realReport screenshot lacks crucial information.

0

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

const report = reportReal ?? reportDraft;

we should update to use draftReport if realReport.reportID is null

const report = (reportReal && reportReal?.reportID) ? reportReal : reportDraft;

After implementing this change, the application crashes due to another bug in the getDefaultMileageRate function. In this function, we need to incorporate optional chaining.

return {
        customUnitRateID: distanceRate?.customUnitRateID,
        rate: distanceRate?.rate,
        currency: distanceRate?.currency,
        unit: distanceUnit?.attributes?.unit,
        name: distanceRate?.name,
    };

After making those adjustments, everything seems to be functioning well, but there's an issue with the "to" field on the confirmation page, which displays as "undefined workspace."

8

This problem is likely caused by the same bug we encountered earlier in here

We should also apply the same fix to resolve the issue with the "to" field on the confirmation page.

My suggestion: we should consider creating a utility function to address this bug in other parts of the codebase as well.

@blimpich
Copy link
Contributor

blimpich commented Jun 5, 2024

@cretadn22 thank you for your proposal! Question: wouldn't the root cause of this problem be that the reportID is undefined when it should be defined?

@cretadn22
Copy link
Contributor

@blimpich No, reportID is valuable when it is defined. But it is defined in reportDraft and we utilize reportReal. More detail in my proposal

@blimpich
Copy link
Contributor

blimpich commented Jun 5, 2024

This to me seems like the issue, the backend isn't finding the report when we open the list of categories:

Screenshot 2024-06-05 at 12 00 24 PM

thank you @ZhenjaHorbach for pointing out this error to me here.

@ZhenjaHorbach
Copy link
Contributor

Plus
I think this issue came from this PR
#31529

@blimpich
Copy link
Contributor

blimpich commented Jun 5, 2024

Yeah we can see here that we really shouldn't be calling openReport since (A) why would we need to when we're just opening the category list and (B) we don't call openReport when we do the same flow but with a workspace.

Screen.Recording.2024-06-05.at.12.20.07.PM.mov

@blimpich blimpich added the External Added to denote the issue can be worked on by a contributor label Jun 5, 2024
Copy link

melvin-bot bot commented Jun 5, 2024

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

@melvin-bot melvin-bot bot changed the title Track Expense - Hmm..not here page on selecting category with no existing workspace [$250] Track Expense - Hmm..not here page on selecting category with no existing workspace Jun 5, 2024
@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Jun 5, 2024
Copy link

melvin-bot bot commented Jun 5, 2024

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

@blimpich
Copy link
Contributor

blimpich commented Jun 5, 2024

Opening this up to contributors since this discussion makes me believe this is a frontend issue, and I can't find a PR that, when reverted, totally fixes the problem.

@blimpich blimpich added the Bug Something is broken. Auto assigns a BugZero manager. label Jun 5, 2024
Copy link

melvin-bot bot commented Jun 5, 2024

Triggered auto assignment to @CortneyOfstad (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

@dominictb
Copy link
Contributor

dominictb commented Jun 6, 2024

Proposal

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

Hmm..not here page on selecting category with no existing workspace

What is the root cause of that problem?

When there is no existing workspace, we create a draft policy and draft report to categorize the track expense.

Let see the withWritableReportOrNotFound HOC here, we always call OpenReport one time in this HOC if the real report doesn't exist which makes the real report becomes an object after OpenReport API is called

And then in IOURequestStepCategory we get the wrong report here

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

  1. Since for the create flow, the report can be not found and we call this API here to fix the issue when opening the edit money request page by deep link, we should prevent call OpenReport here if we're not in the editing flow
if (Boolean(report?.reportID) || !route.params.reportID || !isEditing) {
    return;
}
  1. After changing with point 1, I found another bug that is the APP crashes after we select the category and go to the confirm page. The problem is here, we only get the rate that is enabled meanwhile when we create the draft policy the rate is not enabled. So we should add enabled: true here when we build the optimistic custom unit.

What alternative solutions did you explore? (Optional)

NA

@CortneyOfstad
Copy link
Contributor

@getusha any feedback on the proposal above?

@melvin-bot melvin-bot bot added the Overdue label Jun 10, 2024
@blimpich blimpich assigned mananjadhav and unassigned getusha Jun 10, 2024
@melvin-bot melvin-bot bot removed Help Wanted Apply this label when an issue is open to proposals by contributors Overdue labels Jun 10, 2024
@mananjadhav
Copy link
Collaborator

Reviewing proposals.

@mananjadhav
Copy link
Collaborator

I am testing the proposals to see if it works and doesn't break anything. I'll have an update before EOD today.

@CortneyOfstad
Copy link
Contributor

@mananjadhav any update on the testing?

@melvin-bot melvin-bot bot added the Overdue label Jun 17, 2024
Copy link

melvin-bot bot commented Jun 17, 2024

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

Copy link

melvin-bot bot commented Jun 18, 2024

@mananjadhav @CortneyOfstad @blimpich this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

@mananjadhav
Copy link
Collaborator

I was out sick past 3-4 days. I'll post an update today/tomorrow.

@melvin-bot melvin-bot bot removed the Overdue label Jun 19, 2024
@CortneyOfstad
Copy link
Contributor

Thanks @mananjadhav and hope you're feeling better!

@mananjadhav
Copy link
Collaborator

mananjadhav commented Jun 23, 2024

Apologies for the delay here. I've been trying to reproduce this on staging and the latest main. But I haven't been able to reproduce the bug. Earlier when it was reported I was able to, but not recently. @kbecciv @CortneyOfstad @blimpich Are you able to consistently reproduce this?

Here's the video from Web

web-not-found-on-categorize.mov

iPhone Safari

mweb-safari-not-found-on-categorize.mp4

and I've tried this on Native, Mobile Web, Mac Safari. Tried it with 3 different new accounts.

@CortneyOfstad @blimpich I do see Not found when we click on the Workspace name. I am assuming that is expected as the workspace is not created yet. May be that shouldn't be clickable? But definitely a separate issue.

@blimpich
Copy link
Contributor

I can no longer reproduce this either, seems to have been fixed elsewhere. Closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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

9 participants