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.78k forks source link

[ON HOLD] [MEDIUM] [Splits] [$250] Split bill - Split amount is lost when adding receipt and splitting bill #37491

Open izarutskaya opened 7 months ago

izarutskaya commented 7 months 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: v1.4.45-0 Reproducible in staging?: Y Reproducible in production?: N Email or phone of affected tester (no customers): natnael.expensify+3@gmail.com Logs: https://stackoverflow.com/c/expensify/questions/4856 Issue reported by: Applause-Internal Team

Action Performed:

  1. Click on 'FAB' > Request money > enter amount
  2. Select a user > Add receipt > return to where you can select a user
  3. Click on 'Split' button
  4. Click on 'Add to split' > click on 'Split'

Expected Result:

Entered amount shouldn't be lost, and request shouldn't change to scan request

Actual Result:

Entered amount is lost and request is changed to a scan request

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

https://github.com/Expensify/App/assets/115492554/316ac613-a1bf-4035-972d-c2ae29b23716

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01e554328af9becb46
  • Upwork Job ID: 1763525709189742592
  • Last Price Increase: 2024-08-08
  • Automatic offers:
    • shubham1206agra | Reviewer | 0
    • rojiphil | Contributor | 0
JmillsExpensify commented 3 months ago

Let's just do it then? So we'd use this pattern on splits/IOUs and keep the empty state for expense reports, right?

shawnborton commented 3 months ago

I can get down with that, yeah!

dannymcclain commented 3 months ago

Yeah that's what I was thinking too. Something like this?

image

Is this the right placement for the Receipt field?

JmillsExpensify commented 3 months ago

Hmm, doesn't that change the location of the receipt for IOUs vs expenses? I kind of think they need to be in the same place, because IOUs can be converted into expenses.

dannymcclain commented 3 months ago

Yeah that's fair, I just wasn't sure. So if we put it in the same place, something like this?

image

And here's the submit expense flow with receipt empty state for reference: image

JmillsExpensify commented 3 months ago

My preference, to keep this simple, would be to not show the receipt in the creation flow. Instead, we just add an option row to the details page post-creation. Basically, I feel like putting the receipt more front-end center in creation flow feels like a bigger change, and we still expect most split/iou users to never need a receipt in the manual flow.

dannymcclain commented 3 months ago

Ahhh ok I was misunderstanding that.

shawnborton commented 3 months ago

Or we could tuck it under "Show more" for those flows?

JmillsExpensify commented 3 months ago

We could do that too, though if you end up adding a receipt, then I think we need to show the receipt in the same place as expenses.

dubielzyk-expensify commented 3 months ago

I don't mind where it's just a push row, but in the "Show more" works too

JmillsExpensify commented 3 months ago

Alright, so it sounds like we're largely decided here? Can we get an updated set of mocks for clarity?

dannymcclain commented 3 months ago

Alright, so it sounds like we're largely decided here? Can we get an updated set of mocks for clarity?

I'm sorry, can you summarize what we decided? I actually can't figure it out from the thread haha.

JmillsExpensify commented 3 months ago

I think we decided:

shawnborton commented 3 months ago

For IOUs, we always put the receipt under the To: field. So for split, does it make sense to put it under all of the people rows, like so? CleanShot 2024-06-11 at 09 50 43@2x

(I started exploring this in this Figma file here btw, wasn't sure where to put it!)

dubielzyk-expensify commented 3 months ago

I like that 👍

dannymcclain commented 3 months ago

What I don't really love is how the filled receipt is in a different spot than the empty push row. I find it to be pretty weird that the order of the information changes based on whether things are filled out or not.

Also, wouldn't we label that push row Receipt instead of Add receipt? And then show the little Receipt label above the image?

shawnborton commented 3 months ago

Good point that it should just say "Receipt" and that we'd maintain a small label above the thumbnail. Looks like you beat me to that but those updates to my mocks look like this: CleanShot 2024-06-11 at 17 18 09@2x

I think that's a fair point about the receipt position though, curious what @JmillsExpensify thinks as he mentioned this:

If added, the receipt will always appear in the same position. So above the detailed expense data like description, etc.

isabelastisser commented 3 months ago

Not overdue, it looks like we're still discussing this @shawnborton @dannymcclain

JmillsExpensify commented 3 months ago

Yes, I still think it's weird that we're switching the receipt position around. I do think it's a fair point that the read-only split details/preview is different than than the expense details/preview, but I still find is weird that we're changing the location of the receipt around between types.

shawnborton commented 3 months ago

Hmm so then I think the options are:

Does that sound right?

dannymcclain commented 3 months ago

Yeah that sounds right to me Shawn. Personally I'm in favor of your first option because I don't think the push rows are super in the way. If you don't want to add a receipt, just skip over the input. No big deal. IMO, a single empty push input is not so obtrusive that we desperately need to hide it.

shawnborton commented 3 months ago

Okay so that would essentially be your mockup you made here: image

Which personally I don't mind. But as Jason says:

Basically, I feel like putting the receipt more front-end center in creation flow feels like a bigger change, and we still expect most split/iou users to never need a receipt in the manual flow.

So then maybe we just do nothing in terms of where we place the empty state or the option to add the receipt, since it already exists in the overflow menu for non-workspace flows: CleanShot 2024-06-19 at 16 10 29@2x

So back to the original question:

Should add an Add receipt option in the details page for manual splits?

I would think we'd just reuse the overflow menu for it then.

dannymcclain commented 3 months ago

I would think we'd just reuse the overflow menu for it then.

I'm totally fine with this approach. Basically do nothing for IOUs and make Splits work the same way, right?

Though because I can't help myself 😅, my 2 cents is that adding an empty push input is not really making adding a receipt front and center. But I don't feel SO strongly as to keep pushing for it if we're happy to just copy how IOUs currently work for this.

shawnborton commented 3 months ago

Basically do nothing for IOUs and make Splits work the same way, right?

Exactly. Let's see what Jason thinks though :)

JmillsExpensify commented 3 months ago

Haha I'm also in agreement. Let's bring splits in line with IOU/expense and add the option to the overflow menu.

dannymcclain commented 3 months ago

Sounds good. Once they're the same, we can always decide later to update both to something different if we want. But yeah, just getting them in line will be a great first step.

JmillsExpensify commented 3 months ago

Nice, big agree on that point!

JmillsExpensify commented 3 months ago

What are the next steps on this PR?

youssef-lr commented 3 months ago

@rojiphil are you able to work on this soon?

rojiphil commented 3 months ago

are you able to work on this soon?

Lot to unpack here but, yes, will pick this up today in some time.

isabelastisser commented 2 months ago

@rojiphil can you please provide an update? Thanks!

rojiphil commented 2 months ago

can you please provide an update? Thanks!

The current status is that @youssef-lr is taking a look (mentioned here) at any needed BE changes for this feature based on the FE changes made thus far in a draft PR

youssef-lr commented 2 months ago

I haven't had time to prioritize this yet, I'm on it next week.

isabelastisser commented 1 month ago

Not overdue. Any updates, @youssef-lr? Thanks!

youssef-lr commented 1 month ago

No update yet @isabelastisser :/, I'm focusing on Aug 15th issues

melvin-bot[bot] commented 1 month ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

shubham1206agra commented 1 month ago

@rojiphil @youssef-lr Bump on the status here.

isabelastisser commented 1 month ago

Any updates here @youssef-lr ? thanks!

youssef-lr commented 1 month ago

I think we can put this on monthly for as long as vip-split is paused

isabelastisser commented 5 days ago

vip-split is paused; issue is on hold.