Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.36k stars 2.79k forks source link

[Awaiting payment] [$125] [P2P Distance] Dupe detection - Distance rate is editable on the confirmation page #46954

Closed IuliiaHerets closed 3 weeks ago

IuliiaHerets commented 1 month ago

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: v9.0.17-1 Reproducible in staging?: Y Reproducible in production?: N Email or phone of affected tester (no customers): applausetester+kh050806@applause.expensifail.com Issue reported by: Applause Internal Team

Action Performed:

  1. Go to staging.new.expensify.com
  2. Go to workspace chat.
  3. Submit two same distance expenses.
  4. Go to transaction thread of any submitted expense.
  5. Click Review duplicates.
  6. Click Keep this one (any).
  7. Proceed to confirmation page.
  8. Click Rate.

Expected Result:

Distance rate should not be editable on the confirmation page (like other fields).

Actual Result:

Distance rate is editable on the confirmation page.

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

https://github.com/user-attachments/assets/20efdbcb-969a-495e-a47c-8d2eed7028f0

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01aa94659fe7b3028a
  • Upwork Job ID: 1821151696845785168
  • Last Price Increase: 2024-08-07
  • Automatic offers:
    • ishpaul777 | Reviewer | 103588098
    • dominictb | Contributor | 103588104
melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

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

github-actions[bot] commented 1 month ago

:wave: 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.
IuliiaHerets commented 1 month ago

We think that this bug might be related to #wave-collect - Release 2

etCoderDysto commented 1 month ago

Proposal

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

Distance rate is editable on the confirmation page

What is the root cause of that problem?

In MoneyRequestview we are only checking canEditDistanceRate prop before we are making the button interactive and displaying right icon https://github.com/Expensify/App/blob/2c60fc937881a77062f0e07a405fa0380cc847b3/src/components/ReportActionItem/MoneyRequestView.tsx#L349-L353

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

We should check for readonly prop too

interactive={canEditDistanceRate && !readonly}
shouldShowRightIcon={canEditDistanceRate && !readonly}

Note: We can check other menuItems on MoneyRequestView and apply the change to prevent this issue happening on other menu items

What alternative solutions did you explore? (Optional)

etCoderDysto commented 1 month ago

I can raise a quick PR if needed.

trjExpensify commented 1 month ago

@pecanoro dupe detection is behind a beta, this isn't a deploy blocker right?

pecanoro commented 1 month ago

Yes, removing the blocker

pecanoro commented 1 month ago

This one is also pretty easy so I am going to change the initial price to $125

trjExpensify commented 1 month ago

Perf, thanks!

trjExpensify commented 1 month ago

Adding External to get a C+ assigned to review https://github.com/Expensify/App/issues/46954#issuecomment-2273158439.

melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

Upwork job price has been updated to $125

dominictb commented 1 month ago

Proposal

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

Distance rate is editable on the confirmation page.

What is the root cause of that problem?

We are using canEditDistanceRate to check if the rate field is editable or not: https://github.com/Expensify/App/blob/2c60fc937881a77062f0e07a405fa0380cc847b3/src/components/ReportActionItem/MoneyRequestView.tsx#L352-L353 with: https://github.com/Expensify/App/blob/2c60fc937881a77062f0e07a405fa0380cc847b3/src/components/ReportActionItem/MoneyRequestView.tsx#L194

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

We should update: https://github.com/Expensify/App/blob/2c60fc937881a77062f0e07a405fa0380cc847b3/src/components/ReportActionItem/MoneyRequestView.tsx#L182 to:

    const canUserPerformWriteAction = !!ReportUtils.canUserPerformWriteAction(report) && !readonly;

The variable name canUserPerformWriteAction suggests it should determine whether the report is editable. However, the condition canUserPerformWriteAction && !readonly is applied to each field, resulting in redundant logic that can lead to oversights, as seen in this issue. This redundancy suggests that canUserPerformWriteAction isn't fully serving its intended purpose, as it should be sufficient to control editability without requiring additional checks.

With my above solution, we can remove all the redundant check !readonly in entire the file.

ishpaul777 commented 1 month ago

While RCA and solution from @etCoderDysto is correct, I prefer going with @dominictb solution as it suggests a hollistic and more future error proof approach.

πŸŽ€ πŸ‘€ πŸŽ€ C+ reviewed

melvin-bot[bot] commented 1 month ago

Current assignee @pecanoro is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

ishpaul777 commented 1 month ago

not overdue, Pending a final proposal review

sakluger commented 1 month ago

@pecanoro this one is waiting for your review. Thanks!

sakluger commented 1 month ago

Rocio is on vacation until next week - she'll review once she's back.

pecanoro commented 1 month ago

Just came back today, thank you for waiting! Assigning @dominictb to the issue

melvin-bot[bot] commented 1 month ago

πŸ“£ @ishpaul777 πŸŽ‰ 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

melvin-bot[bot] commented 1 month ago

πŸ“£ @dominictb πŸŽ‰ 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 πŸ“–

ishpaul777 commented 4 weeks ago

Ready for payment πŸŽ‰

sakluger commented 3 weeks ago

Summarizing payment on this issue:

Contributor: @dominictb $125, paid via Upwork Contributor+: @ishpaul777 $125, paid via Upwork

@ishpaul777 do we need regression steps or are we good as is?

melvin-bot[bot] commented 3 weeks ago

@sakluger @pecanoro @ishpaul777 @dominictb this issue is now 4 weeks old, please consider:

Thanks!

sakluger commented 3 weeks ago

Friendly bump @ishpaul777 I think we may need a regression test for this one.

ishpaul777 commented 3 weeks ago

Regression Test proposal

  1. Go to newdot
  2. Go to workspace chat.
  3. Submit two same distance expenses.
  4. Go to transaction thread of any submitted expense.
  5. Click Review duplicates.
  6. Click Keep this one (any).
  7. Proceed to confirmation page.

Expected Result: There should not be any field that is editable. All fields should be read only

Do we agree πŸ‘ or πŸ‘Ž

sakluger commented 3 weeks ago

Looks great, thanks!