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 2024-09-03] [$250] Distance - Error displayed when creating distance expense in workspace that not opened yet #45856

Closed
2 of 6 tasks
lanitochka17 opened this issue Jul 20, 2024 · 34 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2

Comments

@lanitochka17
Copy link

lanitochka17 commented Jul 20, 2024

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!


Version Number: 9.0.10-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: N/A
Issue reported by: Applause - Internal Team

Issue found when executing PR #45000

Action Performed:

  1. Go to https://staging.new.expensify.com/
    and log in with an existing account than has a workspace (make sure to open any chat other that workspace chat when logging out)
  2. Click on FAB > Distance expense
  3. Enter the start and finish waypoints
  4. On the next screen select the workspace you have not opened yet after logging in
  5. Finish creating a distance expense

Expected Result:

The distance expense is created

Actual Result:

The error "The selected rate has been deletedUnexpected error submitting this expense. Please try again later." is displayed

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

Bug6548285_1721499025434.Recording__622.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~010cab8213f1b00e98
  • Upwork Job ID: 1816211306344908300
  • Last Price Increase: 2024-08-14
  • Automatic offers:
    • allgandalf | Reviewer | 103543103
    • Krishna2323 | Contributor | 103543106
Issue OwnerCurrent Issue Owner: @allgandalf
@lanitochka17 lanitochka17 added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Jul 20, 2024
Copy link

melvin-bot bot commented Jul 20, 2024

Triggered auto assignment to @puneetlath (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

@lanitochka17
Copy link
Author

@puneetlath FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

@lanitochka17
Copy link
Author

We think that this bug might be related to #wave-collect - Release 1

@Krishna2323
Copy link
Contributor

Krishna2323 commented Jul 21, 2024

Edited by proposal-police: This proposal was edited at 2024-08-07 16:44:06 UTC.

Proposal

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

Distance - Error displayed when creating distance expense in workspace that not opened yet

What is the root cause of that problem?

  • Initially when we login or clear cache the policy properties like customUnits are not available on the frontend.
  • When we select the policy from the participants page, we try to get the custom unit rate using DistanceRequestUtils.getCustomUnitRateID and update the draft transaction with it, but since the policy is still missing some data, the customUnitRateID is set to -1.
    const rateID = DistanceRequestUtils.getCustomUnitRateID(firstParticipantReportID);
    const isInvoice = iouType === CONST.IOU.TYPE.INVOICE && ReportUtils.isInvoiceRoomWithID(firstParticipantReportID);
    numberOfParticipants.current = val.length;
    IOU.setMoneyRequestParticipants(transactionID, val);
    IOU.setCustomUnitRateID(transactionID, rateID);

    function getCustomUnitRateID(reportID: string) {
    const allReports = ReportConnection.getAllReports();
    const report = allReports?.[`${ONYXKEYS.COLLECTION.REPORT}${reportID}`];
    const parentReport = allReports?.[`${ONYXKEYS.COLLECTION.REPORT}${report?.parentReportID}`];
    const policy = PolicyUtils.getPolicy(report?.policyID ?? parentReport?.policyID ?? '-1');
    let customUnitRateID: string = CONST.CUSTOM_UNITS.FAKE_P2P_ID;
    if (ReportUtils.isPolicyExpenseChat(report) || ReportUtils.isPolicyExpenseChat(parentReport)) {
    const distanceUnit = Object.values(policy?.customUnits ?? {}).find((unit) => unit.name === CONST.CUSTOM_UNITS.NAME_DISTANCE);
    const lastSelectedDistanceRateID = lastSelectedDistanceRates?.[policy?.id ?? '-1'] ?? '-1';
    const lastSelectedDistanceRate = distanceUnit?.rates[lastSelectedDistanceRateID] ?? {};
    if (lastSelectedDistanceRate.enabled && lastSelectedDistanceRateID) {
    customUnitRateID = lastSelectedDistanceRateID;
    } else {
    customUnitRateID = getDefaultMileageRate(policy)?.customUnitRateID ?? '-1';
    }
    }
    return customUnitRateID;
    }
  • In IOURequestStepConfirmation we call openDraftWorkspaceRequest which gets all the data of the policy and then in MoneyRequestConfirmationList we get the defaultMileageRate and try to update the customUnitRateID for the draft transaction. But the condition in the useEffect isn't correct. It checks for canUseP2PDistanceRequests which isn't required since we always have a customUnitRateID even if canUseP2PDistanceRequests is false and it also checks for customUnitRateID which will return true because the default value we added as customUnitRateID is "-1".
    useEffect(() => {
    if (customUnitRateID || !canUseP2PDistanceRequests) {
    return;
    }
    if (!customUnitRateID) {
    const rateID = lastSelectedDistanceRates?.[policy?.id ?? ''] ?? defaultMileageRate?.customUnitRateID ?? '';
    IOU.setCustomUnitRateID(transactionID, rateID);
    }
    }, [defaultMileageRate, customUnitRateID, lastSelectedDistanceRates, policy?.id, canUseP2PDistanceRequests, transactionID]);

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

We should update the condition to remove canUseP2PDistanceRequests and only return if customUnitRateID is present and is not equal to -1.

    useEffect(() => {
        if (customUnitRateID && customUnitRateID !== '-1') {
            return;
        }
        const rateID = lastSelectedDistanceRates?.[policy?.id ?? ''] ?? defaultMileageRate?.customUnitRateID ?? '';
        IOU.setCustomUnitRateID(transactionID, rateID);
    }, [defaultMileageRate, customUnitRateID, lastSelectedDistanceRates, policy?.id, canUseP2PDistanceRequests, transactionID]);

We can also consider not updating the customUnitRateID to -1 and instead we can set it to 0 or empty string. For this we can update the util functions which returns the rateID (e.g. getCustomUnitRateID). We should also look for places where we use setCustomUnitRateID.

What alternative solutions did you explore? (Optional)

  • If we want to select only default rate when canUseP2PDistanceRequests is false, we can modify the condition as below:
    useEffect(() => {
        if (customUnitRateID && customUnitRateID !== '-1') {
            return;
        }
        const defaultRate = defaultMileageRate?.customUnitRateID ?? '';
        const rateID = canUseP2PDistanceRequests ? lastSelectedDistanceRates?.[policy?.id ?? ''] ?? defaultRate : defaultRate;
        IOU.setCustomUnitRateID(transactionID, rateID );
    }, [defaultMileageRate, customUnitRateID, lastSelectedDistanceRates, policy?.id, canUseP2PDistanceRequests, transactionID]);
  • Same should be done in IOURequestStepParticipants,
    const rateID = DistanceRequestUtils.getCustomUnitRateID(firstParticipantReportID);
    const isInvoice = iouType === CONST.IOU.TYPE.INVOICE && ReportUtils.isInvoiceRoomWithID(firstParticipantReportID);
    numberOfParticipants.current = val.length;
    IOU.setMoneyRequestParticipants(transactionID, val);
    IOU.setCustomUnitRateID(transactionID, rateID);
  • We can modify getCustomUnitRateID to accept a second parameter shouldUseDefault and if it is true we will only use the default value.
    function getCustomUnitRateID(reportID: string) {
    const allReports = ReportConnection.getAllReports();
    const report = allReports?.[`${ONYXKEYS.COLLECTION.REPORT}${reportID}`];
    const parentReport = allReports?.[`${ONYXKEYS.COLLECTION.REPORT}${report?.parentReportID}`];
    const policy = PolicyUtils.getPolicy(report?.policyID ?? parentReport?.policyID ?? '-1');
    let customUnitRateID: string = CONST.CUSTOM_UNITS.FAKE_P2P_ID;
    if (ReportUtils.isPolicyExpenseChat(report) || ReportUtils.isPolicyExpenseChat(parentReport)) {
    const distanceUnit = Object.values(policy?.customUnits ?? {}).find((unit) => unit.name === CONST.CUSTOM_UNITS.NAME_DISTANCE);
    const lastSelectedDistanceRateID = lastSelectedDistanceRates?.[policy?.id ?? '-1'] ?? '-1';
    const lastSelectedDistanceRate = distanceUnit?.rates[lastSelectedDistanceRateID] ?? {};
    if (lastSelectedDistanceRate.enabled && lastSelectedDistanceRateID) {
    customUnitRateID = lastSelectedDistanceRateID;
    } else {
    customUnitRateID = getDefaultMileageRate(policy)?.customUnitRateID ?? '-1';
    }
    }
    return customUnitRateID;
    }
/**
 * Returns custom unit rate ID for the distance transaction
 */
function getCustomUnitRateID(reportID: string, shouldUseDefault?: boolean) {
    const allReports = ReportConnection.getAllReports();
    const report = allReports?.[`${ONYXKEYS.COLLECTION.REPORT}${reportID}`];
    const parentReport = allReports?.[`${ONYXKEYS.COLLECTION.REPORT}${report?.parentReportID}`];
    const policy = PolicyUtils.getPolicy(report?.policyID ?? parentReport?.policyID ?? '-1');
    let customUnitRateID: string = CONST.CUSTOM_UNITS.FAKE_P2P_ID;

    if (ReportUtils.isPolicyExpenseChat(report) || ReportUtils.isPolicyExpenseChat(parentReport)) {
        const distanceUnit = Object.values(policy?.customUnits ?? {}).find((unit) => unit.name === CONST.CUSTOM_UNITS.NAME_DISTANCE);
        const lastSelectedDistanceRateID = lastSelectedDistanceRates?.[policy?.id ?? '-1'] ?? '-1';
        const lastSelectedDistanceRate = distanceUnit?.rates[lastSelectedDistanceRateID] ?? {};
        if (lastSelectedDistanceRate.enabled && lastSelectedDistanceRateID && !shouldUseDefault) {
            customUnitRateID = lastSelectedDistanceRateID;
        } else {
            customUnitRateID = getDefaultMileageRate(policy)?.customUnitRateID ?? '-1';
        }
    }

    return customUnitRateID;
}
const rateID = DistanceRequestUtils.getCustomUnitRateID(firstParticipantReportID, !canUseP2PDistanceRequests);

@melvin-bot melvin-bot bot added the Overdue label Jul 23, 2024
Copy link

melvin-bot bot commented Jul 24, 2024

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

@puneetlath puneetlath added the External Added to denote the issue can be worked on by a contributor label Jul 24, 2024
@melvin-bot melvin-bot bot changed the title Distance - Error displayed when creating distance expense in workspace that not opened yet [$250] Distance - Error displayed when creating distance expense in workspace that not opened yet Jul 24, 2024
Copy link

melvin-bot bot commented Jul 24, 2024

Job added to Upwork: https://www.upwork.com/jobs/~010cab8213f1b00e98

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Jul 24, 2024
Copy link

melvin-bot bot commented Jul 24, 2024

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

@melvin-bot melvin-bot bot removed the Overdue label Jul 24, 2024
@daledah
Copy link
Contributor

daledah commented Jul 25, 2024

Proposal

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

The error "The selected rate has been deletedUnexpected error submitting this expense. Please try again later." is displayed

What is the root cause of that problem?

We have logic to update the customUnitRateID if it is not set:

useEffect(() => {
if (customUnitRateID || !canUseP2PDistanceRequests) {
return;
}
if (!customUnitRateID) {
const rateID = lastSelectedDistanceRates?.[policy?.id ?? ''] ?? defaultMileageRate?.customUnitRateID ?? '';
IOU.setCustomUnitRateID(transactionID, rateID);
}
}, [defaultMileageRate, customUnitRateID, lastSelectedDistanceRates, policy?.id, canUseP2PDistanceRequests, transactionID]);

but that useEffect is working incorrectly.

In case canUseP2PDistanceRequests is false, the useEffect is always early returned. It should be isDistanceRequest instead.

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

We should update the useEffect to:

    useEffect(() => {
        const firstParticipantReportID = selectedParticipants[0]?.reportID ?? '';
        if (customUnitRateID !== '-1' || !isDistanceRequest) {
            return;
        }
        const rateID = DistanceRequestUtils.getCustomUnitRateID(firstParticipantReportID);
        if (rateID === '-1') {
            return;
        }
        IOU.setCustomUnitRateID(transactionID, rateID);
    }, [customUnitRateID, transactionID, selectedParticipants, isDistanceRequest, policy]);

In above:

  • The useEffect will early return if customUnitRateID has already been set or it is not distance request.
  • And we use DistanceRequestUtils.getCustomUnitRateID to calculate the rateID because this function will cover all case that the previous logic:
lastSelectedDistanceRates?.[policy?.id ?? ''] ?? defaultMileageRate?.customUnitRateID ?? '';

does not cover. For example, the P2P case, lastSelectedDistanceRate is disabled, ... and it is used in other position:

const rateID = DistanceRequestUtils.getCustomUnitRateID(firstParticipantReportID);

@allgandalf
Copy link
Contributor

I am waiting to review the proposals based on this thread #38543 (comment), interesting discussion going on. Will tell of the next steps once that discussion leads somewhere

Copy link

melvin-bot bot commented Jul 29, 2024

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

@melvin-bot melvin-bot bot added the Overdue label Jul 29, 2024
@puneetlath puneetlath added Weekly KSv2 and removed Daily KSv2 labels Jul 29, 2024
@melvin-bot melvin-bot bot removed the Overdue label Jul 29, 2024
Copy link

melvin-bot bot commented Jul 31, 2024

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

Copy link

melvin-bot bot commented Aug 3, 2024

@puneetlath @allgandalf 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!

@allgandalf
Copy link
Contributor

Still no update on that PR discussion, i will bump neil on open source channel on monday

@allgandalf
Copy link
Contributor

bumped them on open source channel

Copy link

melvin-bot bot commented Aug 7, 2024

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

@Krishna2323
Copy link
Contributor

Proposal Updated

  • Added alternative to address the suggested changes here.

@allgandalf
Copy link
Contributor

Sorry, was stuck with something super urgent internally 🔥 (slack for reference), I will review the proposals today

Copy link

melvin-bot bot commented Aug 14, 2024

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

@puneetlath
Copy link
Contributor

@allgandalf have you had a chance to review?

@allgandalf
Copy link
Contributor

sorry, this was weekly so didn't see it in K2 😅 , I will review proposals today EOD

@puneetlath puneetlath added Daily KSv2 and removed Weekly KSv2 labels Aug 14, 2024
@allgandalf
Copy link
Contributor

From the conversation we had, and from what @neil-marcellini suggested here, we should go with @Krishna2323's alternative solution here, their RCA is correct and alternate solution would work based on the results we expect for this bug.

🎀👀🎀 C+ reviewed

Copy link

melvin-bot bot commented Aug 15, 2024

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

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Aug 15, 2024
Copy link

melvin-bot bot commented Aug 15, 2024

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

Copy link

melvin-bot bot commented Aug 15, 2024

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

@Krishna2323
Copy link
Contributor

@allgandalf, PR ready for review ^

@allgandalf
Copy link
Contributor

I approve the PR, all yours @puneetlath 🙇

@allgandalf
Copy link
Contributor

PR got into production today

@puneetlath
Copy link
Contributor

@allgandalf can you complete the checklist?

@allgandalf
Copy link
Contributor

Sure thing !

@allgandalf
Copy link
Contributor

  • The PR that introduced the bug has been identified. Link to the PR: Feat/36985 create new rate field #38543
  • The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment: https://github.com/Expensify/App/pull/38543/files#r1738107409
  • A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion: N/A
  • If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.

Regression Test Proposal

  1. Go to Settings > Troubleshoot > Clear cache and restart > Reset and refresh
  2. Click on FAB > Distance expense
  3. Enter the start and finish waypoints
  4. On the next screen select the workspace you have not opened yet after logging in
  5. Finish creating a distance expense
  • Verify the distance expense is created without an error

Do we agree 👍 or 👎

@allgandalf
Copy link
Contributor

This should be ready for payment on 3rd

@puneetlath puneetlath changed the title [$250] Distance - Error displayed when creating distance expense in workspace that not opened yet [HOLD for payment 2024-09-03] [$250] Distance - Error displayed when creating distance expense in workspace that not opened yet Aug 30, 2024
@allgandalf
Copy link
Contributor

This is ready for payment @puneetlath 🙇

@puneetlath
Copy link
Contributor

Regression test: https://github.com/Expensify/Expensify/issues/425941

All paid. Thanks everyone!

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. External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2
Projects
No open projects
Status: Done
Development

No branches or pull requests

6 participants