kbecciv commented 12 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!

Action Performed:

Open the app
Click on plus and click request money->scan
Select any file
Observe now it displays 'Manual' in header, select any user
Click on back and observe that now it displays 'Split' in header even though there is no option to add other user to split

Expected Result:

App should not display 'Split' in header when coming back from scan request as we don't allow adding user for split in scan request

Actual Result:

App displays 'Split' in header when coming back from scan request even though we don't allow adding user for split in scan request

Workaround:

Unknown

Platforms:

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

[x] Android / native
[x] Android / Chrome
[x] iOS / native
[x] iOS / Safari
[x] MacOS / Chrome / Safari
[x] MacOS / Desktop

Version Number: 1.3.77.2 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 Notes/Photos/Videos: Any additional supporting documentation

https://github.com/Expensify/App/assets/93399543/ba11ac46-fd7d-4fff-b140-61705ec3b51f

https://github.com/Expensify/App/assets/93399543/2a3bcc2f-7528-46e7-8d86-a723b5313369

https://github.com/Expensify/App/assets/93399543/00a95b85-4f98-4e9b-99b1-2a2246dd0ba5

https://github.com/Expensify/App/assets/93399543/0b9a48dc-886d-473f-b30b-2eb1a257ff0d

https://github.com/Expensify/App/assets/93399543/7232ee71-e30c-41d3-920a-e912dc9f143e

https://github.com/Expensify/App/assets/93399543/9669b6b8-ab02-48d4-a411-7d6fd3c116a6

https://github.com/Expensify/App/assets/93399543/5f9e8d4f-c20a-4521-9d08-fa72e32f1ec5

Expensify/Expensify Issue URL: Issue reported by: @dhanashree-sawant Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1696334987842689

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~011c86077fd238d8d9
Upwork Job ID: 1709312814973755392
Last Price Increase: 2023-10-10
Automatic offers:

fedirjh | Reviewer | 27263280
DylanDylann | Contributor | 27263283
dhanashree-sawant | Reporter | 27263284

Issue Owner

Current Issue Owner: @trjExpensify

melvin-bot[bot] commented 12 months ago

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

melvin-bot[bot] commented 12 months ago

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

melvin-bot[bot] commented 12 months ago

Bug0 Triage Checklist (Main S/O)

[ ] This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
[ ] This bug is not a duplicate report (check E/App issues and #expensify-bugs)
- If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
[ ] This bug is reproducible using the reproduction steps in the OP. S/O
- If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
- If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
[ ] This issue is filled out as thoroughly and clearly as possible
- Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
[ ] I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

melvin-bot[bot] commented 12 months ago

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

neonbhai commented 12 months ago

Proposal

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

Header incorrectly shows Split for receipt requests

What is the root cause of that problem?

We don't check if the request is receipt while setting the header here: https://github.com/Expensify/App/blob/81a6a529756c1492ca9d0f5fcc67b6dd2196c56e/src/pages/iou/steps/MoneyRequstParticipantsPage/MoneyRequestParticipantsPage.js#L63

We are missing check for Scan Request while setting Header.

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

Add condition to keep the Header title as Manual if isScanRequest is true.

And only change the title to Split when isSplitRequest is true.

Header Manual.webm

What alternative solutions did you explore? (Optional)

Alternatively, we can change the Scan flow to reflect that this is a Scan request.

To achieve this, in this useEffect: https://github.com/Expensify/App/blob/81a6a529756c1492ca9d0f5fcc67b6dd2196c56e/src/pages/iou/steps/MoneyRequstParticipantsPage/MoneyRequestParticipantsPage.js#L57

We will set the header title to 'Scan' if the isScanRequest is true

This will achieve the original goal of reflecting the request type (Tab Name) on the header:

Scan Request Header.webm

ZhenjaHorbach commented 12 months ago

Proposal

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

App displays 'Split' in title on back from scan request money request to request money participants page

What is the root cause of that problem?

The text depends on the selected participant or not When selected we have split When non selected we have manual

In this case, when we select a participant, we update the condition that indicates that we need to use split and here it does not take into account what kind of request Scan or not scan

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

We can add the new condition for title

https://github.com/Expensify/App/blob/81a6a529756c1492ca9d0f5fcc67b6dd2196c56e/src/pages/iou/steps/MoneyRequstParticipantsPage/MoneyRequestParticipantsPage.js#L57-L64

Like

        if (isScanRequest) {
            setHeaderTitle(translate('tabSelector.manual'));
            return;
        }

https://github.com/Expensify/App/assets/68128028/fd0517cc-7247-46fa-9686-ff80ee0505b1

What alternative solutions did you explore? (Optional)

NA

vadymbokatov commented 12 months ago

Proposal

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

title of page changes from manual to split on back from scan request money

What is the root cause of that problem?

the root cause of the problem is this condition: https://github.com/Expensify/App/blob/81a6a529756c1492ca9d0f5fcc67b6dd2196c56e/src/pages/iou/steps/MoneyRequstParticipantsPage/MoneyRequestParticipantsPage.js#L57-L64 on back from scan request money page the iou.participants. array is no longer empty so the condition is false and the title changes.

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

the setHeader condition should change to setHeaderTitle(!_.isEmpty(iou.participants) && isSplitRequest ?translate('iou.split') :translate('tabSelector.manual') );

DylanDylann commented 12 months ago

Proposal

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

App displays 'Split' in title on back from scan request money
App displays 'Split' in title on back from manual request money

What is the root cause of that problem?

Let's see the logic of header message https://github.com/Expensify/App/blob/7f063c7b6487e037804e1d97c08a739071e67679/src/pages/iou/steps/MoneyRequstParticipantsPage/MoneyRequestParticipantsPage.js#L57-L63 In manual request and scan request, we are using _.isEmpty(iou.participants) as a condition for header message. It is incorrect. Because in manual request and Scan request, when clicking any option iou.participants also be updated

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

In Scan Request: There are no options to split the bill, the header title should be fixed

if (isScanRequest) {
        setHeaderTitle(translate('tabSelector.scan'));     
        return;
}

We also do the same thing on Confirmation Page (MoneyRequestConfirmPage)

In Manual request We need to display 'Split' in 2 cases as confirmed here

Because in both manual request and split request we both update iou.participants, we should not use iou.participants as condition to display split or manual

I suggest we should create a new state called selectedParticipant. We only update this state if the user adds new option to the selections list (not update if the user selects the option). Then we will use this state instead of iou.participants

Result

https://github.com/Expensify/App/assets/141406735/5d83da94-2458-4296-bfc2-4e1bfb5fa636

DylanDylann commented 12 months ago

@shawnborton We are displaying "Manual" in the header message of Scan Request. Is it expected? Note that: The header message is Distance in Distance Request The header message is Manual in Manual Request The header message is Manual in Scan Request --> my question here

https://github.com/Expensify/App/assets/141406735/4337be17-3826-4612-acd8-13f96f70b78a

shawnborton commented 12 months ago

Hmmm @JmillsExpensify @trjExpensify thoughts on that one?

trjExpensify commented 12 months ago

The header message is Manual in Scan Request --> my question here

I think this should be Scan, not Manual.

trjExpensify commented 12 months ago

Then wrt to when Split is in the header:

on the participants selector screen when you click Split on any of the rows to enter that multi-select flow
on the split confirmation page thereafter

DylanDylann commented 12 months ago

UPdated proposal

trjExpensify commented 12 months ago

Hey @DylanDylann in your vid can you make sure to go through to the confirmation page in the Scan flow to ensure it's correct there as well?

Also on this:

In Scan Request: There are no options to split the bill, the header title should be fixed

There will be an option to split the bill in scan receipts real soon, so let's not assume that isn't on the way. CC: @youssef-lr

DylanDylann commented 12 months ago

@trjExpensify

Hey @DylanDylann in your vid can you make sure to go through to the confirmation page in the Scan flow to ensure it's correct there as well?

Yeah, updated video

https://github.com/Expensify/App/assets/141406735/a107fc74-4398-4644-8eac-d91fb87e5a61

trjExpensify commented 12 months ago

Huh, it might be because it's late, but why is it now blank in that vid? It should read Scan?

trjExpensify commented 12 months ago

@fedirjh can you review these proposals please? Thanks!

fedirjh commented 11 months ago

@trjExpensify Should we cover both the manual and scan in this issue?

For the manual request, not only does the header change to Split, but also the confirm button is updated:

https://github.com/Expensify/App/assets/36869046/6d3543e0-cd53-431e-8f0a-e77bf10a47ac

neonbhai commented 11 months ago

This behavior is a flaw in the request flow.

Reason:

The Split header on Participants page is set with this line: https://github.com/Expensify/App/blob/c0fe5051041d29b585bd01d9fd9455f5eb2b3a5f/src/pages/iou/steps/MoneyRequstParticipantsPage/MoneyRequestParticipantsPage.js#L63

This means when Participants are selected and we are still on the Participants page, the page assumes we are going to split.

The way this is designed, if the user comes back, looks like they want to add participants, which triggers split flow.

To enter manual flow, user has to directly click on the participant.
To enter split flow, user has to stay on the page with a participant selected (by choosing the Split button (optionSelectorButton))

Image

![Screenshot from 2023-10-07 05-27-37](https://github.com/Expensify/App/assets/64629613/6f72f136-7fd2-4359-912d-e527f170778b)

The Participant options selector only shows confirmButton if there is at least 1 participant selected. As the component is now assuming we are going to the split flow. (The confirm button only allows to split)

The manual and split will have to be redesigned, since this assumption will get in the way.

Split showing on Confirmation page is a symptom of this problem, we should probably not hack it with another participants list.

The problem is we cannot start a manual request from the participants page in a 1 participant selected state.

How to solve this?

Redesign the flow, to allow user to request with 1 participant + split with 1 participant when we come back. I suggest a dropdown button menu allowing to choose between split and manual request
Another solution is when going back from Confirmation with 1 participant, we can remove the participant. This will allow the user to again directly click on the target recipient, without adding to split. This lets them go to the Manual Confirm page, with the right heading (aligning the app with the user's intention)

trjExpensify commented 11 months ago

Hm, interesting. So basically, in the instance of one person being tapped for a 1:1 request, when coming back to the participants page we remember the selection, and because a participant is selected, it's recognised as being in the split flow because that's the flow where you select participants. I also agree the Add to split green button is super confusing here when navigating back.

https://github.com/Expensify/App/assets/16232057/ca5d5175-5331-48a2-8488-a556dda6b0de

when going back from Confirmation with 1 participant, we can remove the participant.

I think I agree we should remove the participant in this case. I can understand not in the split case, because you might be going back to add someone else you forgot to the split.

CC: @JmillsExpensify @shawnborton @dannymcclain here's another weird byproduct of this combined request/split participant selector thang. Curious for your take!

shawnborton commented 11 months ago

Whoa yeah, that is weird. I don't think we should say split until more than one person is selected.

dannymcclain commented 11 months ago

Agree - this seems super wacky to me. I think we're trying to do much behind the scenes when we should just let things be simple and explicit.

trjExpensify commented 11 months ago

Yeah, I mean.. I think the real solution is not combining these flows really. You can hypothetically split with just one person, so I can see why the page header was transformed to "Split" after tapping the split button alongside one row. It just causes this weird thing when you don't do that but go back from the confirmation page and the participant is selected when you didn't go through the split flow.

DylanDylann commented 11 months ago

@trjExpensify Could you help to update the expected in the OP?

trjExpensify commented 11 months ago

Yeah, we're kinda' still discussing that.

melvin-bot[bot] commented 11 months ago

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

trjExpensify commented 11 months ago

@shawnborton @dannymcclain @JmillsExpensify anyone against this interim solution until we decide on separating these flows?

I think I agree we should remove the participant in this case. I can understand not in the split case, because you might be going back to add someone else you forgot to the split.

By not faux selecting the single participant when navigating back in the request from one person flow, we won't falsely show Split in the header and the Add to group button also wouldn't show.

shawnborton commented 11 months ago

Ah, that makes sense to me

dannymcclain commented 11 months ago

Yeah I think that makes sense to me too as an interim solution. It doesn't feel like the ideal end state for this but agree that it's probably the best path forward for now.

neonbhai commented 11 months ago

Proposal

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

We have to hide the Merchant and Date Fields in Send Money Flow according to the Slack Conversation.

What is the root cause of that problem?

Logic missing to hide the Merchant and Date Fields in MoneyRequestConfirmationList

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

We should add a variable isTypeSend and use it to hide the Merchant and Date fields.

const isTypeSend = props.iouType === CONST.IOU.MONEY_REQUEST_TYPE.SEND;

Only show the Merchant and Date fields if isTypeSend is not true:

{!isTypeSend &&
    ...
}

fedirjh commented 11 months ago

@neonbhai I think you should post this proposal in https://github.com/Expensify/App/issues/29330

neonbhai commented 11 months ago

ah sorry :bow:

DylanDylann commented 11 months ago

@trjExpensify @shawnborton @dannymcclain This is the result after applying my proposal

https://github.com/Expensify/App/assets/141406735/a137e31c-2f38-45f5-8a62-5e50f0daa867

Could you guys help to confirm? Thanks

cc @fedirjh

shawnborton commented 11 months ago

That looks pretty good to me.

dannymcclain commented 11 months ago

Yup I think this looks good too.

trjExpensify commented 11 months ago

Same, looks good. Thanks! For good measure in testing the PR, can you go through the Scan tab flow as well to make sure that has been equally fixed?

DylanDylann commented 11 months ago

@trjExpensify Oh yeah, in the previous discussion I forgot to update the title on the confirmation page. I updated

https://github.com/Expensify/App/assets/141406735/4336f2c7-b7e2-4b88-8cc0-7af8d2400f05

trjExpensify commented 11 months ago

Perfect, thanks!

melvin-bot[bot] commented 11 months ago

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

trjExpensify commented 11 months ago

@fedirjh can you review, please?

fedirjh commented 11 months ago

@trjExpensify Is the expected result for manual requests that we remove the selected user when we go back to the participants' selector?

I think @DylanDylann has a different approach that looks better to me, and I think it will not remove the selected user when we navigate back, right?

trjExpensify commented 11 months ago

Yeah, if the participant wasn't selected in the first place (i.e by tapping "Split") then on navigating back the participant shouldn't be selected. Looks like @DylanDylann's proposal is aligned with that though: https://github.com/Expensify/App/issues/28751#issuecomment-1758947446

fedirjh commented 11 months ago

I think the participant is still selected but it is not displayed as selected when we go back from a manual request (not split). I want to confirm this because if we remove the participation when navigating back, we can still navigate forward using the browser buttons and I am not sure how the App behaves in that case (I think it may crash?)

https://github.com/Expensify/App/assets/36869046/238bed0e-d73d-4c0b-bf9f-12aa30e55693

trjExpensify commented 11 months ago

Okay, well @DylanDylann can maybe confirm, because this video shows the member not being selected when navigating back: https://github.com/Expensify/App/issues/28751#issuecomment-1759945190

DylanDylann commented 11 months ago

@fedirjh @trjExpensify

I think the participant is still selected but it is not displayed as selected when we go back from a manual request

It is correct. If we use the browser back/forward button, the old value will appear on the confirmation page. Let's see the video

https://github.com/Expensify/App/assets/141406735/450f74f2-a182-4333-a7f1-ee752c115f28

IMO, It is fine. Could you guys help to confirm the expected for this case?

DylanDylann commented 11 months ago

@fedirjh In case you want to test more. This is draft change https://github.com/Expensify/App/pull/29737

fedirjh commented 11 months ago

It is correct. If we use the browser back/forward button, the old value will appear on the confirmation page.

Thanks for confirming, that looks good to me.

Let's proceed with @DylanDylann's proposal.

🎀 👀 🎀 C+ reviewed

melvin-bot[bot] commented 11 months ago

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

melvin-bot[bot] commented 11 months ago

@amyevans @trjExpensify @fedirjh 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[bot] commented 11 months ago

📣 @fedirjh 🎉 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

Expensify / App

[HOLD for payment 2023-11-07] [$500] Scan - App displays 'Split' in title on back from scan request money request #28751

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

Bug0 Triage Checklist (Main S/O)

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

Proposal

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

What is the root cause of that problem?

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

Proposal

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

What is the root cause of that problem?

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

Result

Reason:

Proposal

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

What is the root cause of that problem?

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