Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[HOLD for payment 2023-10-12] [$500] HIGH: Add ability to request money from within IOU and `expense reports #27075

Closed
JmillsExpensify opened this issue Sep 9, 2023 · 31 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Daily KSv2 External Added to denote the issue can be worked on by a contributor NewFeature Something to build that is a new item.

Comments

@JmillsExpensify
Copy link

JmillsExpensify commented Sep 9, 2023

At the moment we allow the ability to request money via the big green plus button, aka "Global Create". We also allow you to create requests via a policyExpenseChat and even DM. Curiously, we don't allow you to request money via either an IOU or expense report, and we should, especially given that you can create a task in any report. Here's an example of the issue at hand. Notice there is no request money option.

Screenshot 2023-09-04 at 11 22 09

Accordingly, let's add request money to IOU and expense reports as well, just like the policyExpenseChat.

Note: One special consideration is that we shouldn't show the Request money button when a report reaches the approved or reimbursed state, as at that point reports become "locked" and no new requests can be added to them.

cc @mountiny You might decide to take this one, so I'm keeping it internal until you decide one way or another.

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~0166385e94742cacb0
  • Upwork Job ID: 1700387490099986432
  • Last Price Increase: 2023-09-09
@JmillsExpensify JmillsExpensify added Daily KSv2 NewFeature Something to build that is a new item. labels Sep 9, 2023
@JmillsExpensify JmillsExpensify self-assigned this Sep 9, 2023
@melvin-bot
Copy link

melvin-bot bot commented Sep 9, 2023

Current assignee @JmillsExpensify is eligible for the NewFeature assigner, not assigning anyone new.

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Daily KSv2 labels Sep 9, 2023
@dukenv0307
Copy link
Contributor

dukenv0307 commented Sep 9, 2023

Proposal

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

Add ability to request money from within IOU and `expense reports

What is the root cause of that problem?

We are not allowing money request option for iou and expense

if (isMoneyRequestReport(report)) {

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

  1. We should only block money request option for this report if the report reaches approved or reimbursed
if (isMoneyRequestReport(report) && (isReportApproved(report) || isSettled(report.reportID)))

if (isMoneyRequestReport(report)) {

  1. And in navigateToNextPage function, if the report is iou or expense report, we will set the participant is the participant of the parent report of this.
const chatReport = ReportUtils.isMoneyRequestReport(report.reportID) ? ReportUtils.getReport(report.chatReportID) : report;
// Reinitialize the participants when the money request ID in Onyx does not match the ID from params
if (_.isEmpty(iou.participants) || shouldReset) {
    const currentUserAccountID = currentUserPersonalDetails.accountID;
    const participants = ReportUtils.isPolicyExpenseChat(chatReport)
        ? [{reportID: chatReport.reportID, isPolicyExpenseChat: true, selected: true}]
        : _.chain(chatReport.participantAccountIDs)
              .filter((accountID) => currentUserAccountID !== accountID)
              .map((accountID) => ({accountID, selected: true}))
              .value();
    setMoneyRequestParticipants(participants);
}

if (report.reportID) {

  1. And in requestMoney function, we will get the chatReport to pass to getMoneyRequestInformation function
const currentReport = ReportUtils.isMoneyRequestReport(report.reportID) ? ReportUtils.getReport(report.chatReportID) : report;
const {payerEmail, iouReport, chatReport, transaction, iouAction, createdChatReportActionID, createdIOUReportActionID, reportPreviewAction, onyxData} = getMoneyRequestInformation(
    currentReport,
Navigation.dismissModal(ReportUtils.isMoneyRequestReport(report.reportID) ? report.reportID : chatReport.reportID);

function requestMoney(report, amount, currency, created, merchant, payeeEmail, payeeAccountID, participant, comment, receipt = undefined) {

What alternative solutions did you explore? (Optional)

NA

Resut

Screen.Recording.2023-09-09.at.13.58.21.mov

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Sep 9, 2023
@mountiny mountiny added the External Added to denote the issue can be worked on by a contributor label Sep 9, 2023
@melvin-bot melvin-bot bot changed the title Add ability to request money from within IOU and `expense reports [$500] Add ability to request money from within IOU and `expense reports Sep 9, 2023
@mountiny mountiny self-assigned this Sep 9, 2023
@melvin-bot
Copy link

melvin-bot bot commented Sep 9, 2023

Job added to Upwork: https://www.upwork.com/jobs/~0166385e94742cacb0

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Sep 9, 2023
@melvin-bot
Copy link

melvin-bot bot commented Sep 9, 2023

Current assignee @JmillsExpensify is eligible for the External assigner, not assigning anyone new.

@melvin-bot
Copy link

melvin-bot bot commented Sep 9, 2023

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

@mountiny
Copy link
Contributor

mountiny commented Sep 9, 2023

Looking for detailed proposals

There is a chance we need to update the request money actions too to ensure the correct binding to the DM or workspace chats are made.

@dukenv0307
Copy link
Contributor

@mountiny I have a question here If we create a request in iou report or expense report, the request will be added to the report preview of the chat report or it will be a new request?

@dukenv0307
Copy link
Contributor

@mountiny Updated proposal for more details.

@ghost
Copy link

ghost commented Sep 9, 2023

Do we need to submit proposals for $500 issues?

@JmillsExpensify
Copy link
Author

All issues require proposals, yes.

@narefyev91
Copy link
Contributor

I think @dukenv0307 add a good proposal and steps what should be implemented. Any additional code changes can be discussed during PR review
🎀 👀 🎀 C+ reviewed

@melvin-bot
Copy link

melvin-bot bot commented Sep 11, 2023

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

@mountiny
Copy link
Contributor

the request will be added to the report preview of the chat report or it will be a new request?

There will be a new request/ transaction on the IOU/expense report and then we need to update the report preview (which at this point should always exist). This will be the number of the requests also meaking sure the correct number of receipts/ images shows there

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Sep 11, 2023
@dukenv0307
Copy link
Contributor

dukenv0307 commented Sep 11, 2023

@mountiny The final question is, Will expense report and iou report be displayed as an option when we choose participants in participant page?

@melvin-bot melvin-bot bot added the Weekly KSv2 label Sep 11, 2023
@dukenv0307
Copy link
Contributor

@narefyev91 The PR is ready to review.

@mountiny
Copy link
Contributor

MoneyRequestParticipantsPage if you are requesting from IOU or expense report, then there are no participants to choose, they are the same as in the existing report

@JmillsExpensify
Copy link
Author

JmillsExpensify commented Sep 13, 2023

Agreed, though we'd still show the workspace on the confirmation screen, I think. Maybe eventually that confirmation screen clarifies the report name but that's TBD and out of scope for this PR.

@dylanexpensify dylanexpensify changed the title [$500] Add ability to request money from within IOU and `expense reports [$500] HIGH: Add ability to request money from within IOU and `expense reports Sep 14, 2023
@melvin-bot melvin-bot bot added Weekly KSv2 and removed Weekly KSv2 labels Sep 30, 2023
@mountiny
Copy link
Contributor

mountiny commented Sep 30, 2023

Creating two new PRs to cover couple other cases for this flow:

@mountiny
Copy link
Contributor

mountiny commented Oct 1, 2023

@dukenv0307 I believe the payment automation failed here, you should get paid $500 for this, right?

@dukenv0307
Copy link
Contributor

@mountiny I think this issue should have bonus

Screenshot 2023-10-01 at 12 57 58 Screenshot 2023-10-01 at 12 58 08

@mountiny
Copy link
Contributor

mountiny commented Oct 1, 2023

@dukenv0307 I am not sure if we havent missed the case of the payer though which would be a regression. With your PR, I dont think the payer would be able to request money at the IOU report. I did not see it tested in the PR.

@dukenv0307
Copy link
Contributor

@mountiny Thanks for clarifying got it now.

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Oct 5, 2023
@melvin-bot melvin-bot bot changed the title [$500] HIGH: Add ability to request money from within IOU and `expense reports [HOLD for payment 2023-10-12] [$500] HIGH: Add ability to request money from within IOU and `expense reports Oct 5, 2023
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Oct 5, 2023
@melvin-bot
Copy link

melvin-bot bot commented Oct 5, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Oct 5, 2023

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

If no regressions arise, payment will be issued on 2023-10-12. 🎊

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.

  • External issue reporter
  • Contributor that fixed the issue
  • Contributor+ that helped on the issue and/or PR

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

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

  • Merged PR within 3 business days of assignment - 50% bonus
  • Merged PR more than 9 business days after assignment - 50% penalty

@JmillsExpensify
Copy link
Author

Payment summary: $500 to @dukenv0307 for PR.

@narefyev91 @mountiny Have we added regression test yet? If not, @narefyev91 can you suggest the right ones we should add?

@narefyev91
Copy link
Contributor

Regression test:

  1. Navigate to a DM with an existing User B
  2. Click the composer + button
  3. Verify you can see the Request Money option and no split
    4.Request money from User B
  4. Navigate to the IOU report which got created
  5. Click the composer + button in the IOU report
  6. Verify you can see the Request money option
  7. Request more money from User B
  8. Log in as User B in incognito window
  9. Go to the DM with User A
  10. Verify you can see the request money option in the composer + actions menu
  11. Navigate to the IOU report where User A requested money from the User B
  12. Verify you can request money from User A in the IOU report composer + actions menu too
  13. As User B, in the IOU report, request more money from the User A than you owe at the moment
  14. Verify the request was successful and the User A now owes money to User B
  15. Verify you can still see the Request Money option as User B
  16. Go back to the User A window and verify you can also still see an option to request money from User B
  17. As User A, pay the money report elsewhere to settle the report
  18. Verify that as User A, you cannot request money in the paid IOU report
  19. Similarly, verify that as User B, you cannot request money in the paid IOU report

@mountiny
Copy link
Contributor

mountiny commented Oct 9, 2023

Something tells me i have added one but happy to pass this to QA to double check too

@JmillsExpensify
Copy link
Author

Cool, agreed. Let's have applause confirm. Everything is ready in Upwork for payment when we hit 10/12, will circle back.

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Oct 11, 2023
@JmillsExpensify
Copy link
Author

Contributor paid out. Closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Daily KSv2 External Added to denote the issue can be worked on by a contributor NewFeature Something to build that is a new item.
Projects
No open projects
Development

No branches or pull requests

4 participants