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

[$500] Split - Billable toggle is disabled in split details view when it is enabled during creation #33763

Closed
6 tasks done
kbecciv opened this issue Dec 29, 2023 · 30 comments
Closed
6 tasks done
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review

Comments

@kbecciv
Copy link

kbecciv commented Dec 29, 2023

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.19-1
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:

Precondition:

  • User is an employee of a Collect workspace.
  • The Collect workspace has "Re-bill expenses to clients" set to "Default to billable".
    1 Go to workspace chat as employee.
  1. Click + > Split bill > Manual.
  2. Enter amount > Proceed to confirmation page.
  3. Split the bill with "Default to billable" toggled on.
  4. Click on the split details view.
  5. Click on "Default to billable" toggle.
  6. Go to expense details page for the split.

Expected Result:

In Step 5 and 7, "Default to billable" toggle is enabled.

Actual Result:

In Step 5 and 7, "Default to billable" toggle is disabled when it is toggled on during creation. It cannot be toggled on in split details page.

Workaround:

Unknown

Platforms:

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

  • Android: Native
  • Android: mWeb Chrome
  • iOS: Native
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6328518_1703867572502.20231229_205310.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01ac2c6e9de8acf9c4
  • Upwork Job ID: 1740787546852888576
  • Last Price Increase: 2024-01-05
  • Automatic offers:
    • dukenv0307 | Contributor | 28120009
Issue OwnerCurrent Issue Owner: @sobitneupane
@kbecciv kbecciv added External Added to denote the issue can be worked on by a contributor Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Dec 29, 2023
Copy link

melvin-bot bot commented Dec 29, 2023

Job added to Upwork: https://www.upwork.com/jobs/~01ac2c6e9de8acf9c4

@melvin-bot melvin-bot bot changed the title Split - Billable toggle is disabled in split details view when it is enabled during creation [$500] Split - Billable toggle is disabled in split details view when it is enabled during creation Dec 29, 2023
@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Dec 29, 2023
Copy link

melvin-bot bot commented Dec 29, 2023

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

Copy link

melvin-bot bot commented Dec 29, 2023

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

Copy link

melvin-bot bot commented Dec 29, 2023

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

@unidev727
Copy link
Contributor

unidev727 commented Dec 29, 2023

Proposal

from: @unicorndev-727

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

Split - Billable toggle is disabled in split details view when it is enabled during creation

What is the root cause of that problem?

The root cause is that we don't send billable value to buildOptimisticTransaction method so that default value(false) is set when creating splitTransaction.

App/src/libs/actions/IOU.js

Lines 1151 to 1165 in 595bf40

const splitTransaction = TransactionUtils.buildOptimisticTransaction(
amount,
currency,
CONST.REPORT.SPLIT_REPORTID,
comment,
'',
'',
'',
merchant,
undefined,
undefined,
undefined,
category,
tag,
);

function buildOptimisticTransaction(
amount: number,
currency: string,
reportID: string,
comment = '',
created = '',
source = '',
originalTransactionID = '',
merchant = '',
receipt: Receipt = {},
filename = '',
existingTransactionID: string | null = null,
category = '',
tag = '',
billable = false,

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

We need to send billable value down to buildOptimisticTransaction when creating IOU request.

function createSplitsAndOnyxData(participants, currentUserLogin, currentUserAccountID, amount, comment, currency, category, tag, existingSplitChatReportID = '', merchant) {

to

function createSplitsAndOnyxData(participants, currentUserLogin, currentUserAccountID, amount, comment, currency, merchant, category, tag, existingSplitChatReportID = '', billable = false) {
    const currentUserEmailForIOUSplit = OptionsListUtils.addSMSDomainIfPhoneNumber(currentUserLogin);
    const participantAccountIDs = _.map(participants, (participant) => Number(participant.accountID));
    const existingSplitChatReport =
        existingSplitChatReportID || participants[0].reportID
            ? allReports[`${ONYXKEYS.COLLECTION.REPORT}${existingSplitChatReportID || participants[0].reportID}`]
            : ReportUtils.getChatByParticipants(participantAccountIDs);
    const splitChatReport = existingSplitChatReport || ReportUtils.buildOptimisticChatReport(participantAccountIDs);
    const isOwnPolicyExpenseChat = splitChatReport.isOwnPolicyExpenseChat;

    const splitTransaction = TransactionUtils.buildOptimisticTransaction(
        amount,
        currency,
        CONST.REPORT.SPLIT_REPORTID,
        comment,
        '',
        '',
        '',
        merchant || Localize.translateLocal('iou.request'),
        undefined,
        undefined,
        undefined,
        category,
        tag,
        billable
    );

function splitBill(participants, currentUserLogin, currentUserAccountID, amount, comment, currency, category, tag, existingSplitChatReportID = '', merchant) {

to

function splitBill(participants, currentUserLogin, currentUserAccountID, amount, comment, currency, merchant, category, tag, existingSplitChatReportID = '', billable = false) {
    const {splitData, splits, onyxData} = createSplitsAndOnyxData(
        participants,
        currentUserLogin,
        currentUserAccountID,
        amount,
        comment,
        currency,
        merchant,
        category,
        tag,
        existingSplitChatReportID,
        billable 
    );

transaction.tag,
report.reportID,
transaction.merchant,
);
return;
}
// If the request is created from the global create menu, we also navigate the user to the group report
if (iouType === CONST.IOU.TYPE.SPLIT) {
IOU.splitBillAndOpenReport(
selectedParticipants,
currentUserPersonalDetails.login,
currentUserPersonalDetails.accountID,
transaction.amount,
trimmedComment,
transaction.currency,

if (iouType === CONST.IOU.TYPE.SPLIT && !transaction.isFromGlobalCreate) {
                IOU.splitBill(
                    selectedParticipants,
                    currentUserPersonalDetails.login,
                    currentUserPersonalDetails.accountID,
                    transaction.amount,
                    trimmedComment,
                    transaction.currency,
                    transaction.merchant,
                    transaction.category,
                    transaction.tag,
                    report.reportID,
                    transaction.billable
                );
                return;
            }

Above solution is only for split money request in workspace and this error is happened because we didn't consider billable state when creating transaction so we would need to check transaction creation workflow in money request and would add billable values here for other edge cases.

const createTransaction = useCallback(

What alternative solutions did you explore? (Optional)

N/A

@melvin-bot melvin-bot bot added the Overdue label Jan 1, 2024
Copy link

melvin-bot bot commented Jan 1, 2024

@sobitneupane, @laurenreidexpensify Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@laurenreidexpensify
Copy link
Contributor

@sobitneupane bump for review of the proposal above thanks

@melvin-bot melvin-bot bot removed the Overdue label Jan 2, 2024
@dukenv0307
Copy link
Contributor

dukenv0307 commented Jan 3, 2024

Proposal

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

"Default to billable" toggle is disabled when it is toggled on during creation. It cannot be toggled on in split details page.

What is the root cause of that problem?

  1. In optimistic data, we don't pass billable to buildOptimisticTransaction which makes it's false by default

  1. When creating the split bill, we don't pass billable to the payload of the API

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

  1. In each function of split bill request like splitBill, splitBillAndOpenReport, createSplitsAndOnyxData and startSplitBill, add an extra param billable with default value is false. And pass this param to buildOptimisticTransaction.

  2. On confirm page, we will pass transaction.billable to the function when we create the split bill.

  3. In SplitBillDetailsPage, pass iouIsBillable into MoneyRequestConfirmList.

  4. I tested adding billable as true to the payload of splitBill API but BE still returns billable as false. So if we allow billable for split bill we should also fix from BE to return billable as true if the billable in the payload is true.

What alternative solutions did you explore? (Optional)

If the expected behavior is not enabled billable for split bill we should hide billable option if the request is split bill or disable billable section if the request is split bill.

const shouldShowBillable = !lodashGet(policy, 'disabledFields.defaultBillable', true);

@melvin-bot melvin-bot bot added the Overdue label Jan 4, 2024
@laurenreidexpensify
Copy link
Contributor

@sobitneupane please review the proposals above thank you

@melvin-bot melvin-bot bot removed the Overdue label Jan 5, 2024
Copy link

melvin-bot bot commented Jan 5, 2024

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

@sobitneupane
Copy link
Contributor

I tested to add billable as true to the payload of splitBill API but BE still returns billable as false. So if we allow billable for split bill we should also fix from BE to return billable as true if the billable in the payload is true.

As pointed out by @dukenv0307 in his proposal, this should probably be handled in BE as well. @laurenreidexpensify Can we assign internal engineer to investigate issue on backend.

@melvin-bot melvin-bot bot added the Overdue label Jan 8, 2024
@laurenreidexpensify laurenreidexpensify added the Internal Requires API changes or must be handled by Expensify staff label Jan 8, 2024
Copy link

melvin-bot bot commented Jan 8, 2024

Current assignee @sobitneupane is eligible for the Internal assigner, not assigning anyone new.

Copy link

melvin-bot bot commented Jan 8, 2024

Triggered auto assignment to @francoisl (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

@melvin-bot melvin-bot bot removed the Overdue label Jan 8, 2024
@laurenreidexpensify
Copy link
Contributor

@francoisl assigning internal as this is BE fix

@melvin-bot melvin-bot bot added the Overdue label Jan 10, 2024
@francoisl francoisl added the Reviewing Has a PR in review label Jan 11, 2024
@melvin-bot melvin-bot bot removed the Overdue label Jan 11, 2024
@laurenreidexpensify laurenreidexpensify added Weekly KSv2 and removed Daily KSv2 Help Wanted Apply this label when an issue is open to proposals by contributors labels Jan 11, 2024
@francoisl
Copy link
Contributor

Backend fixes are in review, I'll update here when they're deployed to staging.

@francoisl
Copy link
Contributor

The backend fixes are on staging now, feel free to test your proposals again by passing billable to the API. Let me know if something's not working as expected.

@sobitneupane
Copy link
Contributor

@dukenv0307 Do you want to update your proposal? Is backend responding as expected?

@dukenv0307
Copy link
Contributor

@sobitneupane I tested and BE returns billable correctly now. And my proposal #33763 (comment) already has the fix in front-end now.

@sobitneupane
Copy link
Contributor

Proposal from @dukenv0307 looks good to me.

🎀👀🎀 C+ reviewed

Copy link

melvin-bot bot commented Jan 24, 2024

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

Copy link

melvin-bot bot commented Jan 24, 2024

📣 @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 📖

@francoisl francoisl removed the Internal Requires API changes or must be handled by Expensify staff label Jan 24, 2024
@melvin-bot melvin-bot bot added Weekly KSv2 and removed Weekly KSv2 labels Jan 25, 2024
@dukenv0307
Copy link
Contributor

@sobitneupane The PR is ready for review.

@laurenreidexpensify
Copy link
Contributor

Deployed to prod #35125 (comment) 3 days ago, so should be paid on 12 feb

@laurenreidexpensify
Copy link
Contributor

Ah yikes the automation failed here!! Let's get this paid!!

@laurenreidexpensify
Copy link
Contributor

Payment Summary:

@laurenreidexpensify
Copy link
Contributor

@sobitneupane please complete a checklist and confirm if we need regression steps thanks

@laurenreidexpensify laurenreidexpensify added Daily KSv2 and removed Weekly KSv2 labels Feb 20, 2024
@laurenreidexpensify
Copy link
Contributor

@sobitneupane bump for checklist and regression steps thanks

@laurenreidexpensify
Copy link
Contributor

@sobitneupane bump thanks

@sobitneupane
Copy link
Contributor

@laurenreidexpensify As the issue was reported by Applause Team, I believe the regression tests are already included. In case it is requried:

Precondition:

  • User is an employee of a Collect workspace.
  • The Collect workspace has "Re-bill expenses to clients" set to "Default to billable".

Steps:

  1. Go to workspace chat or announce room as employee.
  2. Create a split bill with default billable is toggled on
  3. Open split bill detail page and verify that billable is toggled on
  4. Open expense detail page of the split bill and verify that billable is also toggled on

@JmillsExpensify
Copy link

$500 approved for @sobitneupane based on this summary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review
Projects
None yet
Development

No branches or pull requests

7 participants