[Hold] Distance rates- Deleted distance rate is not disabled when submitting distance expense offline

lanitochka17 commented 1 week 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!

Version Number: 9.0.0-0 Reproducible in staging?: Y Reproducible in production?: Y If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/4652159 Issue reported by: Applause - Internal Team

Action Performed:

Precondition:

Workspace has a few distance rates
1. Go to staging.new.expensify.com
2. Go to workspace settings > Distance rates
3. Go offline
4. Delete one of the distance rates
5. Go to workspace chat
6. Submit a distance expense with the deleted distance rate
7. Go online

Expected Result:

In Step 6, app should prevent the deleted distance rate from being selected when submitting a distance distance

Actual Result:

In Step 6, user can submit a distance expense with the deleted distance rate, which results in error when returning online

Workaround:

Unknown

Platforms:

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

[x] Android: Native
[x] Android: mWeb Chrome
[x] iOS: Native
[x] iOS: mWeb Safari
[x] MacOS: Chrome / Safari
[x] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/78819774/18e08046-57e2-466d-86c2-7f0b9ead18c5

View all open jobs on GitHub

melvin-bot[bot] commented 1 week ago

Triggered auto assignment to @OfstadC (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 commented 1 week ago

@OfstadC 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 commented 1 week ago

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

bernhardoj commented 1 week ago

Proposal

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

We can still select pending-delete distance rate and will result in an error when submitted.

What is the root cause of that problem?

We are not filtering any distance rate that is pending-delete when showing the rate list. https://github.com/Expensify/App/blob/68374a552f1af8c866753d46df2b0a0349615d2f/src/pages/iou/request/step/IOURequestStepDistanceRate.tsx#L62-L72

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

Filter out any pending-delete rate. To do that:

Return the pendingAction from getMileageRates https://github.com/Expensify/App/blob/68374a552f1af8c866753d46df2b0a0349615d2f/src/libs/DistanceRequestUtils.ts#L59-L65

pendingAction: rate.pendingAction,

Filter out rate with pending action of delete. https://github.com/Expensify/App/blob/68374a552f1af8c866753d46df2b0a0349615d2f/src/pages/iou/request/step/IOURequestStepDistanceRate.tsx#L62 .filter(rate => rate.pendingAction !== 'delete')

Krishna2323 commented 1 week ago

Proposal

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

Distance rates- Deleted distance rate is not disabled when submitting distance expense offline

What is the root cause of that problem?

We don't have any check for disabled rates on IOURequestStepDistanceRate page.

https://github.com/Expensify/App/blob/68374a552f1af8c866753d46df2b0a0349615d2f/src/pages/iou/request/step/IOURequestStepDistanceRate.tsx#L62-L72

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

We need to first return pendingAction field from getMileageRates.

        mileageRates[rateID] = {
            rate: rate.rate,
            currency: rate.currency,
            unit: distanceUnit.attributes.unit,
            name: rate.name,
            customUnitRateID: rate.customUnitRateID,
            pendingAction: rate.pendingAction,
        };

Then add isDisabled and pendingAction.

        return {
            text: rate.name ?? rateForDisplay,
            alternateText: rate.name ? rateForDisplay : '',
            keyForList: rate.customUnitRateID,
            value: rate.customUnitRateID,
            isSelected: lastSelectedRateID ? lastSelectedRateID === rate.customUnitRateID : !!(rate.name === CONST.CUSTOM_UNITS.DEFAULT_RATE),
            isDisabled: rate.pendingAction === 'delete',
            pendingAction: rate.pendingAction,
        };

What alternative solutions did you explore? (Optional)

If we also need to grey out the option when updated offline, we nee to also return pendingFields from getMileageRates and update the pendingAction property like we do in PolicyDistanceRatesPage.

https://github.com/Expensify/App/blob/68374a552f1af8c866753d46df2b0a0349615d2f/src/pages/workspace/distanceRates/PolicyDistanceRatesPage.tsx#L113-L118

    const sections = Object.values(rates).map((rate) => {
        const rateForDisplay = DistanceRequestUtils.getRateForDisplay(rate.unit, rate.rate, rate.currency, translate, toLocaleDigit);

        return {
            text: rate.name ?? rateForDisplay,
            alternateText: rate.name ? rateForDisplay : '',
            keyForList: rate.customUnitRateID,
            value: rate.customUnitRateID,
            isSelected: lastSelectedRateID ? lastSelectedRateID === rate.customUnitRateID : !!(rate.name === CONST.CUSTOM_UNITS.DEFAULT_RATE),
            isDisabled: rate.pendingAction === 'delete',
            pendingAction:
                rate.pendingAction ??
                rate.pendingFields?.rate ??
                rate.pendingFields?.enabled ??
                rate.pendingFields?.currency ??
                (policy?.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD ? policy?.pendingAction : undefined),
        };
    });

Krishna2323 commented 1 week ago

Proposal Updated

Removed redundant code.

Krishna2323 commented 1 week ago

Proposal Updated

We also need to add pendingAction: rate.pendingAction for correct styling.

Krishna2323 commented 1 week ago

Proposal Updated

Added alternative

OfstadC commented 1 week ago

Following this Slack thread before proceeding

OfstadC commented 1 week ago

@mallenexpensify I wonder if the Offline Distance request issues should be consolidated since they might have the same/similar solution? 🤔

mallenexpensify commented 1 week ago

Asking about consolidating, should have an update tomorrow 🤞

OfstadC commented 1 week ago

Thanks Matt!

OfstadC commented 1 week ago

Any update @mallenexpensify ? 😃

mallenexpensify commented 1 week ago

@paultsimura can you please comment so I can assign to you? Created a tracking issue for 'offline distance' issues.

https://github.com/Expensify/App/issues/44582

paultsimura commented 6 days ago

👋🏼

OfstadC commented 3 days ago

@mallenexpensify Is there any action I need to take here?

mallenexpensify commented 2 days ago

No @OfstadC , @paultsimura is going to to plan to holistically address offline distance bugs. He'll comment here with an update once he's reviewed all issues (and either raise a PR or recommend making the issue external)

melvin-bot[bot] commented 9 hours ago

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

Expensify / App