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-08-26] Accessing one expense reports from search opening transaction view instead of report view #45408

Closed
1 of 6 tasks
m-natarajan opened this issue Jul 15, 2024 · 31 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Internal Requires API changes or must be handled by Expensify staff Reviewing Has a PR in review Weekly KSv2

Comments

@m-natarajan
Copy link

m-natarajan commented Jul 15, 2024

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


Version Number: 9.06-4
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: @shawnborton
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1721024057246389

Action Performed:

  1. As a submitter submit a report with one expense to the workspace
  2. Go to search and click on the expense created in step 1

Expected Result:

clicking on any expense in the Search page where the expense is the only expense on a report opens up the report view instead of the transaction view

  • Here is thinking on why we should show the report view when viewing a single expense on a one-expense report:
  • we get a much more consistent experience with workspace chat rooms - in the attached video, see the difference when you view an expense from the workspace chat vs when you view it from Search
  • we limit user exposure to our multiple levels of navigation hierarchy when it comes to the Workspace chat > Report > Expense relationship. This way if someone views the expense from Search and leaves a comment, we are only showing the report thread in their Inbox LHN
  • we expose the actual action button a user might want to take on the expense. For example, it's weird if the Search page is going to eventually show a "Submit" button on the expense row, yet you wouldn't be able to see that same button on the expense once you open it - you'd have to navigate one level up to the report (which most users will have no idea why that's even a thing) to see those report action buttons
  • still think it makes sense to show the transaction view when you are trying to view an individual transaction from a grouped batch. But I do think the above change would make a much better experience for the majority of cases where it's a one-expense report.

Actual Result:

Expenses on one-expense reports accessed from the Search > Expenses page open up the transaction view instead of the report view

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

Recording.326.mp4

View all open jobs on GitHub

Issue OwnerCurrent Issue Owner: @muttmuure
@m-natarajan m-natarajan added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Jul 15, 2024
Copy link

melvin-bot bot commented Jul 15, 2024

Triggered auto assignment to @muttmuure (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.

@tienifr
Copy link
Contributor

tienifr commented Jul 15, 2024

Proposal

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

  • Transaction report opens instead of expense report.

What is the root cause of that problem?

  • When accessing any transaction, in here, we don't have logic to check if it is only transaction in report.

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

  • We need to update it to:
        if (SearchUtils.isTransactionListItemType(item)) {
            const transactionReport = getAllReports?.()?.[`${ONYXKEYS.COLLECTION.REPORT}${reportID}`];
            const parentReportAction = ReportActionsUtils.getReportAction(transactionReport?.parentReportID ?? '-1', transactionReport?.parentReportActionID ?? '-1');
            if (ReportUtils.isOneTransactionThread(reportID, transactionReport?.parentReportID, parentReportAction) && transactionReport?.parentReportID) {
                reportID = transactionReport?.parentReportID;
            }
        }
  • In above, we check if the accessed transaction is the one transaction thread or not. If true, access its expense report instead of transaction report.

  • A similar logic should be updated to it:

        let oneTransactionReportID;
        const transactionThreadReportID = transactionItem.transactionThreadReportID;
        const transactionReport = getAllReports?.()?.[`${ONYXKEYS.COLLECTION.REPORT}${transactionThreadReportID}`];
        const parentReportAction = ReportActionsUtils.getReportAction(transactionReport?.parentReportID ?? '-1', transactionReport?.parentReportActionID ?? '-1');
        if (ReportUtils.isOneTransactionThread(transactionThreadReportID, transactionReport?.parentReportID, parentReportAction) && transactionReport?.parentReportID) {
            oneTransactionReportID = transactionReport?.parentReportID;
        }
        Navigation.navigate(ROUTES.SEARCH_REPORT.getRoute(currentQuery, oneTransactionReportID ?? transactionItem.transactionThreadReportID));

Alternative solution

  • We can check if the transaction is one transaction thread on BE side, so the transaction data return in Search API will contain an additional props, named isOneTransactionThreadReport.
  • We need to update it to:
        if (item.isOneTransactionThreadReport) {
                reportID = item.reportID;
        }
  • A similar logic should be updated to it:
        let oneTransactionReportID;
        if (transactionItem.isOneTransactionThreadReport) {
            oneTransactionReportID = transactionReport?.reportID;
        }
        Navigation.navigate(ROUTES.SEARCH_REPORT.getRoute(currentQuery, oneTransactionReportID ?? transactionItem.transactionThreadReportID));

@tienifr
Copy link
Contributor

tienifr commented Jul 15, 2024

Proposal updated

@luacmartins
Copy link
Contributor

@tienifr I'm not sure that your solution will work because we might not have the report or reportActions available in Onyx given that the Search API only returns data in the snapshot_ key. I think we'll need to work on a solution on the backend.

@luacmartins luacmartins added the Internal Requires API changes or must be handled by Expensify staff label Jul 15, 2024
@tienifr
Copy link
Contributor

tienifr commented Jul 15, 2024

@luacmartins I tested the app with my solution and it works well. Can you help me reproduce the case "we might not have the report or reportActions available in Onyx"? Thanks

@tienifr
Copy link
Contributor

tienifr commented Jul 15, 2024

Updated alternative solution in case we want to fix from BE side as well.

@luacmartins
Copy link
Contributor

@tienifr I just tried this in my account:

  1. Go to search page
  2. Sort transactions by ascending date
  3. Get the transactionID from an old transaction (in my case it was from 2021)
  4. Verified that the transaction, report, reportActions key for that transaction didn't exist in Onyx

@luacmartins
Copy link
Contributor

@tienifr thanks for the updated proposal. Can't we just use the reportID if isOneTransactionThreadReport = true?

@tienifr
Copy link
Contributor

tienifr commented Jul 15, 2024

@luacmartins

Can't we just use the reportID if isOneTransactionThreadReport = true

Yeah, it makes sense. I tested and it works well, we can use transaction.reportID instead of introducing a new parentReportID props. I updated alternative proposal.

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

melvin-bot bot commented Jul 18, 2024

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

@luacmartins
Copy link
Contributor

No updates yet. Focused on other Search issues atm.

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Jul 18, 2024
Copy link

melvin-bot bot commented Jul 22, 2024

@luacmartins, @muttmuure Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@luacmartins
Copy link
Contributor

No updates yet. Focused on other Search issues atm.

@luacmartins
Copy link
Contributor

Got a couple of PRs up

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Jul 26, 2024
@muttmuure
Copy link
Contributor

Not overdue

@melvin-bot melvin-bot bot removed the Overdue label Jul 29, 2024
@rayane-djouah
Copy link
Contributor

rayane-djouah commented Aug 20, 2024

This ^ was a false positive (The linked issue is a backend bug and not an App bug)

@rayane-djouah
Copy link
Contributor

Note

The production deploy automation failed: This should be on [HOLD for Payment 2024-08-26] according to #47512 prod deploy checklist, confirmed in #46324 (comment).

cc @muttmuure

@luacmartins luacmartins changed the title Accessing one expense reports from search opening transaction view instead of report view [HOLD for Payment 2024-08-26] Accessing one expense reports from search opening transaction view instead of report view Aug 27, 2024
@luacmartins luacmartins added Daily KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Reviewing Has a PR in review Weekly KSv2 labels Aug 27, 2024
@muttmuure
Copy link
Contributor

I've invited you to apply to this job: https://www.upwork.com/jobs/~01b7e6d6058445ffef

@melvin-bot melvin-bot bot removed the Overdue label Aug 28, 2024
@rayane-djouah
Copy link
Contributor

@muttmuure - I've applied, thanks

@muttmuure
Copy link
Contributor

Hired

@rayane-djouah
Copy link
Contributor

@muttmuure - Offer accepted

@rayane-djouah
Copy link
Contributor

@muttmuure Friendly bump

@muttmuure
Copy link
Contributor

Paid

@luacmartins
Copy link
Contributor

Reopening this issue since it seems like the problem is back

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Sep 16, 2024
@luacmartins
Copy link
Contributor

PR in review

This comment has been minimized.

@rayane-djouah
Copy link
Contributor

^ false positive

@rayane-djouah
Copy link
Contributor

This is fixed on staging, we can close this one

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. Internal Requires API changes or must be handled by Expensify staff Reviewing Has a PR in review Weekly KSv2
Projects
No open projects
Status: Done
Development

No branches or pull requests

6 participants