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] Search - Expense report has transaction thread & expense previews when submitted from Search #49533

Closed
3 of 6 tasks
IuliiaHerets opened this issue Sep 20, 2024 · 33 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Needs Reproduction Reproducible steps needed retest-weekly Apply this label if you want this issue tested on a Weekly basis by Applause Weekly KSv2

Comments

@IuliiaHerets
Copy link

IuliiaHerets commented Sep 20, 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.0.39-0
Reproducible in staging?: Y
Reproducible in production?: Y
Email or phone of affected tester (no customers): [email protected]
Issue reported by: Applause Internal Team

Action Performed:

  1. Go to staging.new.expensify.com
  2. Submit an expense to user that has no unsettled expense.
  3. Go to Search > Expenses.
  4. Click on the expense submitted in Step 2.
  5. From the report RHP, click + > Submit expense.
  6. Submit another expense.

Expected Result:

After submitting another expense from Search, app will open expense report with two expense previews.

Actual Result:

After submitting another expense from Search, app shows expense report that has transaction thread and also two expense previews.

Workaround:

Unknown

Platforms:

  • Android: Native
  • Android: mWeb Chrome
  • iOS: Native
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Bug6609873_1726832563061.20240920_193701.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021837129942878375275
  • Upwork Job ID: 1837129942878375275
  • Last Price Increase: 2024-09-27
  • Automatic offers:
    • situchan | Reviewer | 104200352
    • FitseTLT | Contributor | 104200357
Issue OwnerCurrent Issue Owner: @sonialiap
@IuliiaHerets IuliiaHerets added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Sep 20, 2024
Copy link

melvin-bot bot commented Sep 20, 2024

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

@IuliiaHerets
Copy link
Author

We think that this bug might be related to #wave-control

@IuliiaHerets
Copy link
Author

@sonialiap FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

@sonialiap sonialiap added the External Added to denote the issue can be worked on by a contributor label Sep 20, 2024
@melvin-bot melvin-bot bot changed the title Search - Expense report has transaction thread & expense previews when submitted from Search [$250] Search - Expense report has transaction thread & expense previews when submitted from Search Sep 20, 2024
Copy link

melvin-bot bot commented Sep 20, 2024

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Sep 20, 2024
Copy link

melvin-bot bot commented Sep 20, 2024

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

@FitseTLT
Copy link
Contributor

Proposal

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

Search - Expense report has transaction thread & expense previews when submitted from Search

What is the root cause of that problem?

We are getting transactionThreadReport from prop.transactionThreadReportID here

transactionThreadReport: {
key: ({transactionThreadReportID}) => `${ONYXKEYS.COLLECTION.REPORT}${transactionThreadReportID ?? -1}`,
initialValue: {} as OnyxTypes.Report,
},

although transactionThreadReportID is properly updating to undefined on creating another expense internal bug in withOnyx doesn't allow transactionThreadReport to update so it holds its previous value and money request view will be shown along with the two request previews.

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

withOnyx is already deprecated and migrating to useOnyx solves the issue

and then get transactionThreadReport in ReportActionsView via useOnyx instead of withOnyx

        const [transactionThreadReport] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT}${transactionThreadReportID ?? -1}`, {initialValue: {}});


We need to replace this with prop.transactionThreadReportID as we are using useOnyx

    if (oldProps.transactionThreadReportID !== newProps.transactionThreadReportID) {
        return false;
    }

What alternative solutions did you explore? (Optional)

Copy link

melvin-bot bot commented Sep 23, 2024

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

@melvin-bot melvin-bot bot added the Overdue label Sep 23, 2024
@situchan
Copy link
Contributor

@FitseTLT the root cause is not correct. This is not internal bug in Onyx.
ReportActionsView is not re-rendered when transactionThreadReportID prop changes because arePropsEqual doesn't check transactionThreadReportID change and thus this function returns true

@melvin-bot melvin-bot bot removed the Overdue label Sep 23, 2024
@FitseTLT
Copy link
Contributor

@FitseTLT the root cause is not correct. This is not internal bug in Onyx. ReportActionsView is not re-rendered when transactionThreadReportID prop changes because arePropsEqual doesn't check transactionThreadReportID change and thus this function returns true

I thought so in the beginning but the problem will not be fixed even if you add transactionThreadReportID change in memoization because what we are memoizing is ReportActionsView which is the component passed to withOnyx and transactionThreadReport is provided by the withOnyx which correctly gets the new value of the transactionThreadReportID as I have debugged it.

@situchan
Copy link
Contributor

@FitseTLT have you tested with useOnyx? Please share branch.
The problem is in this file so we're sure to migrate useOnyx.

@FitseTLT
Copy link
Contributor

@FitseTLT have you tested with useOnyx? Please share branch. The problem is in this file so we're sure to migrate useOnyx.

Yes, I have tested it will fix it. Here is the branch

2024-09-24.00-35-29.mp4

@situchan
Copy link
Contributor

@FitseTLT's proposal looks good to me.
🎀👀🎀 C+ reviewed

Copy link

melvin-bot bot commented Sep 24, 2024

Triggered auto assignment to @rafecolton, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

@raza-ak
Copy link
Contributor

raza-ak commented Sep 24, 2024

Proposal

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

Search - Expense report has transaction thread & expense previews when submitted from Search

What is the root cause of that problem?

The issue originates in the "ReportActionItemContentCreated" component. Here, the logic for displaying the transaction thread section relies only on the report object's length and type, without adequately checking if there is an associated transaction thread.

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

We need to update the component logic to check for the existence of the transactionThreadReportID. If this ID is present, the thread section is displayed; otherwise, only the expense previews are shown. This requires passing the transactionThreadReportID from the parent component to the child component via prop drilling.

Following changes will fix the issue.
image
image
image
image
image
image

Video:

370242178-45d8e101-5689-4770-91b4-99a40a92bb0e.mp4

@FitseTLT
Copy link
Contributor

@rafecolton bump for an assignment

@rafecolton
Copy link
Member

Going to take this to Slack for a little discussion before reviewing the proposal.

@rafecolton
Copy link
Member

@IuliiaHerets where did this bug come from? I don't see a link to a PR or discussion

@melvin-bot melvin-bot bot added the Overdue label Sep 30, 2024
@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Sep 30, 2024
Copy link

melvin-bot bot commented Sep 30, 2024

📣 @situchan 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job

Copy link

melvin-bot bot commented Sep 30, 2024

📣 @FitseTLT 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

@melvin-bot melvin-bot bot removed the Overdue label Sep 30, 2024
@rafecolton
Copy link
Member

Thanks for your patience, assigned!

@FitseTLT
Copy link
Contributor

The intended change is already made in #49626. We can close this.

@FitseTLT FitseTLT removed their assignment Sep 30, 2024
@rafecolton
Copy link
Member

@mallenexpensify can you look into why we had duplicated efforts here? Also please advise on how we should close this out to ensure proper handling of upwork job.

@mallenexpensify mallenexpensify added retest-weekly Apply this label if you want this issue tested on a Weekly basis by Applause Weekly KSv2 and removed Daily KSv2 labels Oct 1, 2024
@mallenexpensify
Copy link
Contributor

mallenexpensify commented Oct 1, 2024

Unsure how they're related. The PR for that hit production yesterday so I threw retest-weekly on here. @FitseTLT can you provide more details? The steps in the OPs of both didn't look like they were dupes.

@FitseTLT
Copy link
Contributor

FitseTLT commented Oct 1, 2024

Unsure how they're related. The PR for that hit production yesterday so I threw retest-weekly on here. @FitseTLT can you provide more details? The steps in the OPs of both didn't look like they were dupes.

They are not dupes @mallenexpensify Just the other PR changes fixed this bug 👍

@mallenexpensify mallenexpensify added the Needs Reproduction Reproducible steps needed label Oct 1, 2024
@MelvinBot
Copy link

This has been labelled "Needs Reproduction". Follow the steps here: https://stackoverflowteams.com/c/expensify/questions/16989

@rafecolton rafecolton removed their assignment Oct 2, 2024
@rafecolton
Copy link
Member

Seems there's no action required from me so unassigning myself for now. @sonialiap please re-assign me directly if this issue can be reproduced

Copy link

melvin-bot bot commented Oct 4, 2024

@sonialiap @situchan 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!

@melvin-bot melvin-bot bot added the Overdue label Oct 10, 2024
@sonialiap
Copy link
Contributor

Looks like this was resolved with #49626 in a different job. Closing as duplicate

@mallenexpensify
Copy link
Contributor

Since we hired/assigned @FitseTLT because they had an accepted proposal and @situchan reviewed the proposal, per our internal process, 50% is due each.

@situchan @FitseTLT can you please accept the job and reply here once you have?
https://www.upwork.com/jobs/~021837129942878375275

@FitseTLT
Copy link
Contributor

@mallenexpensify Offer accepted

@mallenexpensify
Copy link
Contributor

Contributor: @FitseTLT paid $125 via Upwork
Contributor+: @situchan paid $125 via Upwork.

Thanks!

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. External Added to denote the issue can be worked on by a contributor Needs Reproduction Reproducible steps needed retest-weekly Apply this label if you want this issue tested on a Weekly basis by Applause Weekly KSv2
Projects
Archived in project
Development

No branches or pull requests

8 participants