You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
[$250] Send money - User can send money to group by modifying the URL to ../send/new/.. #29367
👋 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:
Identify the pull request that introduced this issue and revert it.
Please re-state the problem that we are trying to solve in this issue.
Send money - User can send money to group by modifying the URL to ../send/new/..
What is the root cause of that problem?
This problem exists for all kind of iou requests. We have not implemented any conditions to prevent such cases.
I found this comment describing a function which is used to show available iou actions in report + button. Based on this comment we can see which combinations are not allowed, like send request in non 1:1 chats, or split in 1:1 chats
This will ensure that combination of current report and iou type is allowed one, and prevent from unwanted cases.
Note that, we only apply this check if route has report params, not creating new without selecting report yet.
What alternative solutions did you explore? (Optional)
If this usage is confusing or overloads a function usage, we can create similar function which will check for above cases based on conditions similar to getMoneyRequestOptions conditions
changed the title [-][$100] Send money - User can send money to group by modifying the URL to ../send/new/..[/-][+][$250] Send money - User can send money to group by modifying the URL to ../send/new/..[/+]on Oct 12, 2023
@hannojg Updated it to 250, but I think these are minor fixes with no edge case(hopefully) and shouldn't take long other than just verifying it across platforms.
can you please review the proposal posted by Ali, thanks.
same thought as @esh-g. The issue #26523 is requesting money, this issue is sending money. But because they share the same component. So looks like we can fix them together. cc @techievivek
@techievivek Looks like proposal(#26523 (comment)) provided in that issue is very similar to mine and solve the issue. So I think there is no need for my proposal
Activity
melvin-bot commentedon Oct 11, 2023
Triggered auto assignment to @muttmuure (
Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.melvin-bot commentedon Oct 11, 2023
Bug0 Triage Checklist (Main S/O)
Platformsin OP are ✅)OSBotify commentedon Oct 11, 2023
👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open
StagingDeployCashdeploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:melvin-bot commentedon Oct 11, 2023
Triggered auto assignment to @neil-marcellini (
Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.alitoshmatov commentedon Oct 11, 2023
Proposal
Please re-state the problem that we are trying to solve in this issue.
Send money - User can send money to group by modifying the URL to ../send/new/..
What is the root cause of that problem?
This problem exists for all kind of iou requests. We have not implemented any conditions to prevent such cases.
I found this comment describing a function which is used to show available
iouactions in report+button. Based on this comment we can see which combinations are not allowed, like send request in non 1:1 chats, or split in 1:1 chatsApp/src/libs/ReportUtils.js
Lines 3353 to 3376 in 8c73c2b
What changes do you think we should make in order to solve the problem?
We can leverage this
getMoneyRequestOptionsfunction to check if iou request is valid or not and showNotFoundPagein corresponding placeshttps://github.com/Expensify/App/blob/main/src/pages/iou/MoneyRequestSelectorPage.js#L92
like this:
This will ensure that combination of current report and iou type is allowed one, and prevent from unwanted cases.
Note that, we only apply this check if route has report params, not creating new without selecting report yet.
What alternative solutions did you explore? (Optional)
If this usage is confusing or overloads a function usage, we can create similar function which will check for above cases based on conditions similar to
getMoneyRequestOptionsconditionsalitoshmatov commentedon Oct 11, 2023
I don't think this is Deploy blocker since we can reproduce similar issue using
.../split/new/...in 1:1 chats.jasperhuangg commentedon Oct 11, 2023
cc @techievivek since this seems related to your recent Send Money PR
19 remaining items
[-][$100] Send money - User can send money to group by modifying the URL to ../send/new/..[/-][+][$250] Send money - User can send money to group by modifying the URL to ../send/new/..[/+]melvin-bot commentedon Oct 12, 2023
Upwork job price has been updated to $250
techievivek commentedon Oct 12, 2023
@hannojg Updated it to 250, but I think these are minor fixes with no edge case(hopefully) and shouldn't take long other than just verifying it across platforms.
can you please review the proposal posted by Ali, thanks.
esh-g commentedon Oct 12, 2023
@techievivek @muttmuure This issue is a dupe and will get fixed here: #26523
hannojg commentedon Oct 12, 2023
@techievivek did you tag me by accident? I don't see why I was tagged
hoangzinh commentedon Oct 12, 2023
same thought as @esh-g. The issue #26523 is requesting money, this issue is sending money. But because they share the same component. So looks like we can fix them together. cc @techievivek
techievivek commentedon Oct 13, 2023
@hannojg
Sorry, I tagged you by accident 😄
techievivek commentedon Oct 13, 2023
Yes, that will be great.
techievivek commentedon Oct 13, 2023
@alitoshmatov Do you mind posting your proposal to the other issue so we can tackle them all together.
alitoshmatov commentedon Oct 13, 2023
@techievivek Looks like proposal(#26523 (comment)) provided in that issue is very similar to mine and solve the issue. So I think there is no need for my proposal
techievivek commentedon Oct 16, 2023
This is a duplicate of #26523 so I will close this one.