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.56k stars 2.9k forks source link

[HOLD for payment 2023-12-07] [$500] IOU - Inconsistency in Scan split bill IOU preview for creator&Participant #30680

Closed kbecciv closed 11 months ago

kbecciv commented 1 year 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: 1.3.94.0 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: Applause - Internal Team Slack conversation:

Action Performed:

  1. Go to https://staging.new.expensify.com/
  2. Tap fab-Request Money
  3. Tap Scan
  4. Upload a picture by taking photo
  5. Split with any 2 users
  6. Tap add to Split
  7. Tap split
  8. Tap scan request created
  9. Enter amount, description and merchant field
  10. Tap split Note: The description in OIU is displayed
  11. Log in with any user with whom you've split the bill
  12. Open IOU Note: The Merchant in OIU is displayed

Expected Result:

There should not be inconsistency in IOU preview displayed for scan request creator and participant.

Actual Result:

For user who made scan request and split bill with 2 users, description details entered is shown in IOU preview. But for participant of split bill, merchant details entered is shown in IOU preview. There is inconsistency in IOU preview displayed for scan request creator and participant.

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Android: Native
Android: mWeb Chrome https://github.com/Expensify/App/assets/93399543/19a2f4e6-9cd6-4a1b-806b-099a54c05b5a ![Vasinashi](https://github.com/Expensify/App/assets/93399543/7ab66f9b-87c1-4bec-9268-ef59d71d3888)
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01890520fc54e620e1
  • Upwork Job ID: 1719686303480401920
  • Last Price Increase: 2023-11-01
  • Automatic offers:
    • cubuspl42 | Reviewer | 27750967
    • dukenv0307 | Contributor | 27750969
melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

Bug0 Triage Checklist (Main S/O)

melvin-bot[bot] commented 1 year ago

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

dukenv0307 commented 1 year ago

Proposal

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

Split bill preview display description while IOU Preview display merchant

What is the root cause of that problem?

We only show merchant if the request preview is not split bill

https://github.com/Expensify/App/blob/c8e7ee5718730aef7eaeec052ee5c97e4284ff53/src/components/ReportActionItem/MoneyRequestPreview.js#L172-L175

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

I think we should also show merchant for split bill by removing the condition !props.isBillSplit and and change the marigin style of merchant text if it's split bill

https://github.com/Expensify/App/blob/c8e7ee5718730aef7eaeec052ee5c97e4284ff53/src/components/ReportActionItem/MoneyRequestPreview.js#L173

Or Update the condition here to

{shouldShowDescription || (shouldShowMerchant && props.isBillSplit) && <Text style={[styles.colorMuted]}>{description || requestMerchant}</Text>}

And here to

shouldShowMerchant && !props.isBillSplit

https://github.com/Expensify/App/blob/c8e7ee5718730aef7eaeec052ee5c97e4284ff53/src/components/ReportActionItem/MoneyRequestPreview.js#L318

https://github.com/Expensify/App/blob/c8e7ee5718730aef7eaeec052ee5c97e4284ff53/src/components/ReportActionItem/MoneyRequestPreview.js#L328

What alternative solutions did you explore? (Optional)

We can prioritize description over merchant

miljakljajic commented 1 year ago

@cubuspl42 any thoughts on the above proposal?

cubuspl42 commented 1 year ago

@dukenv0307 I will not review any of your proposals as long as the "Please re-state the problem that we are trying to solve in this issue" section is a copy-pasted issue name.

dukenv0307 commented 1 year ago

@cubuspl42 Thanks for your review, will improve this in the further. I updated.

cubuspl42 commented 1 year ago

@kbecciv I re-watched the provided video multiple times and still cannot understand the issue's essence. Maybe it has something to do with the second attachment not loading correctly.

@dukenv0307 While your new updated issue re-statement is now acceptable in the formal sense, it didn't help me understand the actual problem here.

dukenv0307 commented 1 year ago

Seem like the image in the screenshot of this issue is unavailable now. The problem here is when we create a split bill with description and merchant. The Split bill preview displays description field while the iou preview in iou report of each participant displays merchant field. That is the current bug which this issue mentioned.

cubuspl42 commented 1 year ago

@kbecciv Would you re-record the videos?

Two reasons:

kbecciv commented 1 year ago

@cubuspl42 I apologize for any inconvenience. I've reviewed the issue again, corrected the reproduction steps, and added a new video and screenshot. Please let me know if the reported issue is still difficult to understand, and I'll be happy to assist further.

image

cubuspl42 commented 1 year ago

@kbecciv Thank you! <3

cubuspl42 commented 1 year ago

The proposal by @dukenv0307 makes sense. I personally would prefer the "alternative solution", i.e. preferring the description over the merchant. In my opinion, the description is the most personal element of the form filled by the user and is the one that deserves to be presented with the highest priority.

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

melvin-bot[bot] commented 1 year ago

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

cubuspl42 commented 1 year ago

@nkuoch Would you share your opinion about which field should be consistently presented in the Split/IOU preview: should it be the "merchant" or "description"?

nkuoch commented 1 year ago

cc @vitHoracek as you originally did https://github.com/Expensify/App/pull/26603/files#diff-d8e6b69ad4461bebf91e940a67ddc07adf36a3f7c156d6066363c3bde06a0f8aR162

nkuoch commented 1 year ago

friendly bump @vitHoracek

dukenv0307 commented 1 year ago

@mountiny Friendly bump.

miljakljajic commented 1 year ago

heads up @nkuoch that Vit's handle is @mountiny not @vitHoracek - shared a link to this issue in e chat to him

mountiny commented 1 year ago

I personally think this should be consistent so Merchant whenever available and fallback to description anytime Merchant is not defined. however with Scanned receipt, there will be merchant most of the time.

trjExpensify commented 1 year ago

I personally think this should be consistent so Merchant whenever available and fallback to description anytime Merchant is not defined

Yes, that's the logic for the preview. Merchant. If no merchant, then description. If no merchant or description, then blank. πŸ‘

dukenv0307 commented 1 year ago

I personally think this should be consistent so Merchant whenever available and fallback to description anytime Merchant is not defined. however with Scanned receipt, there will be merchant most of the time.

@mountiny My main proposal already had this expected.

melvin-bot[bot] commented 1 year ago

@nkuoch @cubuspl42 @miljakljajic 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!

miljakljajic commented 1 year ago

@cubuspl42 - with the above in mind, do you have a preference amongst the proposals?

cubuspl42 commented 1 year ago

@cubuspl42 - with the above in mind, do you have a preference amongst the proposals?

There is one proposal and I approved it.

@nkuoch @mountiny

Thanks for the clarification of the requirements! Are we in agreement that this is the expected order...

Merchant. If no merchant, then description. If no merchant or description, then blank.

...and the code should be adjusted so it's consistent for all previews?

mountiny commented 1 year ago

Yes!

cubuspl42 commented 12 months ago

@nkuoch In such a case, I think we're ready to assign @dukenv0307 and start working on a PR πŸ™‚

melvin-bot[bot] commented 12 months ago

πŸ“£ @cubuspl42 πŸŽ‰ 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 12 months ago

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

aimane-chnaif commented 12 months ago

Let's hold this for #30721 cc: @youssef-lr

dukenv0307 commented 12 months ago

@cubuspl42 The PR is ready for review.

melvin-bot[bot] commented 11 months ago

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

melvin-bot[bot] commented 11 months ago

Reviewing label has been removed, please complete the "BugZero Checklist".

melvin-bot[bot] commented 11 months ago

The solution for this issue has been :rocket: deployed to production :rocket: in version 1.4.5-7 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2023-12-07. :confetti_ball:

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

For reference, here are some details about the assignees on this issue:

melvin-bot[bot] commented 11 months ago

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

miljakljajic commented 11 months ago

Both contracts paid and ended. Thank you for your work!

pecanoro commented 11 months ago

@miljakljajic Did we reduce the pay? Since it caused a deploy blocker and PR had to be reverted.

miljakljajic commented 11 months ago

Ah, I had no idea we needed to reduce the pay. I'll request a partial refund from @dukenv0307 and @cubuspl42

cubuspl42 commented 11 months ago

@miljakljajic Is it possible on Upwork?