[HOLD for payment 2024-10-16] [$500] Distance rates - Enabled distance rate changes to "Disabled" after deleting it #48290

lanitochka17 · 2024-08-29T19:09:25Z

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.26-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: https://expensify.testrail.io/index.php?/tests/view/4902934
Email or phone of affected tester (no customers): applausetester+kh050806@applause.expensifail.com
Issue reported by: Applause - Internal Team

Action Performed:

Go to staging.new.expensify.com
Go offline
Go to Categories
Disable an active (enabled) category
Note that the disabled category that was previously enabled shows "Enabled" in strikethrough style
Go to Distance rates
Disable an active (enabled) distance rate

Expected Result:

The distance rate status should remain "Enabled" in strikethrough style, which is the behavior for categories and tags

Actual Result:

The distance rate status changes to "Disabled" after deleting the rate offline, which is inconsistent with category and tag behavior

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Add any screenshot/video evidence

Bug6586441_1724930004395.20240829_190821.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~0103dd03b250392922
Upwork Job ID: 1829237965961527437
Last Price Increase: 2024-09-10
Automatic offers:

paultsimura | Contributor | 103803832
Krishna2323 | Contributor | 103803839

Issue Owner

Current Issue Owner: @puneetlath

melvin-bot · 2024-08-29T19:09:34Z

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.

mkzie2 · 2024-08-29T19:13:02Z

I think this is expected as per #45146.

melvin-bot · 2024-08-29T19:21:32Z

Job added to Upwork: https://www.upwork.com/jobs/~0103dd03b250392922

melvin-bot · 2024-08-29T19:21:37Z

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

FitseTLT · 2024-08-29T19:22:32Z

This is intentional #45146 Tagging @paultsimura

Krishna2323 · 2024-08-29T19:22:38Z

Edited by proposal-police: This proposal was edited at 2024-08-29 19:23:47 UTC.

Proposal

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

Distance rates - Enabled distance rate changes to "Disabled" after deleting it

What is the root cause of that problem?

The rate is disabled when deleted.

App/src/libs/actions/Policy/DistanceRate.ts

Line 492 in e054a71

enabled: false,

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

We need to remove the enabled: false,
In getMileageRates, we also need to include the pending action.

App/src/libs/DistanceRequestUtils.ts

Line 35 in e054a71

function getMileageRates(policy: OnyxInputOrEntry<Policy>, includeDisabledRates = false): Record<string, MileageRate> {

In IOURequestStepDistanceRate, we need to filter out the rates with pending action delete. Or we can use the isDisabled property.

App/src/pages/iou/request/step/IOURequestStepDistanceRate.tsx

Lines 72 to 82 in e054a71

    
           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: currentRateID ? currentRateID === rate.customUnitRateID : rate.name === CONST.CUSTOM_UNITS.DEFAULT_RATE, 
        
               }; 
        
           });

We should also look for other uses of the distance rates in other component and filter out the deleted rates if needed.

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.

App/src/pages/workspace/distanceRates/PolicyDistanceRatesPage.tsx

Lines 113 to 118 in 68374a5

    
           pendingAction: 
        
               value.pendingAction ?? 
        
               value.pendingFields?.rate ?? 
        
               value.pendingFields?.enabled ?? 
        
               value.pendingFields?.currency ?? 
        
               (policy?.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD ? policy?.pendingAction : undefined),

    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: currentRateID ? currentRateID === rate.customUnitRateID : rate.name === CONST.CUSTOM_UNITS.DEFAULT_RATE, 
        pendingAction:
        rate.pendingAction ??
        rate.pendingFields?.rate ??
        rate.pendingFields?.enabled ??
        rate.pendingFields?.currency ??
        rate.pendingFields?.taxRateExternalID ??
        rate.pendingFields?.taxClaimablePercentage ??
        (policy?.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD ? policy?.pendingAction : undefined),
        };
    });

What alternative solutions did you explore? (Optional 2) 

If we want to disable category, tag, taxes when they are deleted then we can:

Update enabled property in all the delete util functions for category, tag, taxes.

App/src/libs/actions/Policy/Tag.ts

Line 301 in 9275b86

acc[tagName] = {pendingAction: CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE};

App/src/libs/actions/TaxRate.ts

Line 307 in 9275b86

acc[taxID] = {pendingAction: CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE, errors: null};

App/src/libs/actions/Policy/Category.ts

Line 544 in 9275b86

acc[categoryName] = {pendingAction: CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE};

Result

Krishna2323 · 2024-08-29T19:27:22Z

@brunovjk, I think this is not intentional because the PR tried to solve a different issue but introduced this bug, the PR was focused on not showing the deleted distance rates in the IOU request flow.

Krishna2323 · 2024-08-29T19:29:39Z

Proposal Updated

Added more details in the main solution.

Krishna2323 · 2024-08-29T19:36:35Z

Proposal Updated

Added alternative

paultsimura · 2024-08-29T19:39:17Z

Well, my take is we should align Categories, Rates, and Tags.
But we might consider updating the Tags & Categories behavior to disable the value on removal, similar to how I've done it with the rates in that PR – as there is no need to keep it enabled while removed.

@brunovjk could you please raise a discussion about it in Slack? 🙏

Krishna2323 · 2024-08-29T19:49:24Z

Proposal Updated

But we might consider updating the Tags & Categories behavior to disable the value on removal

Added alternative 2 if decide to go with the option above.

brunovjk · 2024-08-29T20:39:46Z

Thank you all for your time. I've shared a Slack message about this issue here. @paultsimura, do you think it sufficiently addresses our concern? Is there anyone else we should tag in the Slack discussion? Thanks!

Gajendra-Gupta · 2024-09-02T11:37:19Z

Proposal

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

Distance rates - Enabled distance rate changes to "Disabled" after deleting it

What is the root cause of that problem?

The rate is disabled when deleted.
App/src/libs/actions/Policy/DistanceRate.ts

enabled: false, Line 492 in e054a71

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

We need to replace this line
"enabled: false,"
By
enabled: currentRates[rateID].enabled,

melvin-bot · 2024-09-02T18:41:44Z

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

paultsimura · 2024-09-02T19:02:37Z

I'll take this over as a C+, thanks @brunovjk 😌

paultsimura · 2024-09-02T20:51:23Z

@puneetlath could you please assign me here?

paultsimura · 2024-09-03T09:15:40Z

In the Slack discussion, we decided not to add complications with downstream checking pendingAction: delete in multiple places for the removal of rates.
Instead, we'll adjust the Tags, Categories, Tax, and Report Fields (list values) logic to disable values on removal optimistically.

Therefore, the proposal by @Krishna2323 looks good to me (the 2nd alternative solution).

🎀👀🎀 C+ reviewed

As a bonus note: I've noticed another bug with the optimistic List Value removal on Report Fields, which we may want to fix in this PR.

melvin-bot · 2024-09-03T09:15:49Z

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

Krishna2323 · 2024-09-10T05:23:52Z

As a bonus note: I've noticed another bug with the optimistic List Value removal on Report Fields, which we may want to fix in this PR.

@paultsimura, do we also need to fix this? I think I saw a proposal for fixing that recently but couldn't find it. I believe it was proposed by @bernhardoj. @bernhardoj, could you please confirm this for us?

bernhardoj · 2024-09-10T11:16:06Z

Hmm, I don't remember about that. Can you explain more what the bug is?

paultsimura · 2024-09-10T11:29:53Z

Never mind – the report fields behavior has gotten even worse, I truly hope there is an open issue for it somewhere.
For example:

2024-09-10.-.13.29.-.Screen.Recording.2024-09-10.at.13.28.56.mp4

paultsimura · 2024-09-10T12:30:33Z

However @Krishna2323, if you'd be interested in fixing this bug and @puneetlath agrees to bump the bounty, we could deliver the fix in our PR since it also blocks us from testing the optimistic removal of report field values.

puneetlath · 2024-09-10T16:29:31Z

That seems fine to me.

Krishna2323 · 2024-09-10T16:31:02Z

Thanks, I will add the changes for that also.

melvin-bot · 2024-09-10T16:49:01Z

Upwork job price has been updated to $500

melvin-bot · 2024-10-03T18:16:45Z

This issue has not been updated in over 15 days. @puneetlath, @paultsimura, @Krishna2323 eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

Krishna2323 · 2024-10-03T18:22:44Z

Sorry for the delay, we had some confusion about the changes, which caused the PR to be delayed. The PR will be ready to merge today—just need to complete the checklist.

paultsimura · 2024-10-03T18:26:25Z

No need for "monthly", melvin.

melvin-bot · 2024-10-09T20:28:28Z

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

melvin-bot · 2024-10-09T20:28:35Z

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

fix: Distance rates - Enabled distance rate changes to Disabled after deleting it. #48859

If no regressions arise, payment will be issued on 2024-10-16. 🎊

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

@paultsimura requires payment automatic offer (Contributor)
@Krishna2323 requires payment automatic offer (Contributor)

melvin-bot · 2024-10-09T20:28:36Z

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

[@paultsimura] The PR that introduced the bug has been identified. Link to the PR:
[@paultsimura] 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:
[@paultsimura] 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:
[@paultsimura] Determine if we should create a regression test for this bug.
[@paultsimura] 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.
[@puneetlath] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

paultsimura · 2024-10-12T09:58:52Z

Note

This is not a bug fix, but rather an agreed change implementation – more context here. Therefore, no offending PR.

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
Determine if we should create a regression test for this bug: Yes
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

Pre-condition:

Own a workspace that has enabled tags, categories, tax, and distance rates.

Test:

Go offline
Go to the workspace categories page
Delete an enabled category
Verify the category becomes disabled after being deleted offline
Go to the workspace taxes page
Delete an enabled tax rate
Verify the tax rate becomes disabled after being deleted offline
Go to the workspace tags page
Delete an enabled tag
Verify the tag becomes disabled after being deleted offline
Go to the workspace distance rates page
Delete an enabled distance rate
Verify the rate becomes disabled after being deleted offline

Do we agree 👍 or 👎

puneetlath · 2024-10-18T16:39:35Z

Regression test issue: https://github.com/Expensify/Expensify/issues/437246

All paid. Sorry for the delay. Thanks y'all!

lanitochka17 added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Aug 29, 2024

melvin-bot bot assigned puneetlath Aug 29, 2024

This comment was marked as outdated.

Sign in to view

puneetlath added the External Added to denote the issue can be worked on by a contributor label Aug 29, 2024

melvin-bot bot changed the title ~~Distance rates - Enabled distance rate changes to "Disabled" after deleting it~~ [$250] Distance rates - Enabled distance rate changes to "Disabled" after deleting it Aug 29, 2024

melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Aug 29, 2024

melvin-bot bot assigned brunovjk Aug 29, 2024

melvin-bot bot added the Overdue label Sep 2, 2024

brunovjk removed their assignment Sep 2, 2024

melvin-bot bot removed the Overdue label Sep 2, 2024

puneetlath assigned paultsimura Sep 3, 2024

melvin-bot bot added Weekly KSv2 and removed Daily KSv2 labels Sep 10, 2024

puneetlath changed the title ~~[$250] Distance rates - Enabled distance rate changes to "Disabled" after deleting it~~ [$500] Distance rates - Enabled distance rate changes to "Disabled" after deleting it Sep 10, 2024

melvin-bot bot removed the Weekly KSv2 label Oct 3, 2024

melvin-bot bot added the Monthly KSv2 label Oct 3, 2024

melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Monthly KSv2 Weekly KSv2 labels Oct 4, 2024

melvin-bot bot changed the title ~~[$500] Distance rates - Enabled distance rate changes to "Disabled" after deleting it~~ [HOLD for payment 2024-10-16] [$500] Distance rates - Enabled distance rate changes to "Disabled" after deleting it Oct 9, 2024

melvin-bot bot removed the Reviewing Has a PR in review label Oct 9, 2024

melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Oct 16, 2024

puneetlath closed this as completed Oct 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[HOLD for payment 2024-10-16] [$500] Distance rates - Enabled distance rate changes to "Disabled" after deleting it #48290

[HOLD for payment 2024-10-16] [$500] Distance rates - Enabled distance rate changes to "Disabled" after deleting it #48290

lanitochka17 commented Aug 29, 2024 •

edited by melvin-bot bot

Loading

melvin-bot bot commented Aug 29, 2024

mkzie2 commented Aug 29, 2024

This comment was marked as outdated.

melvin-bot bot commented Aug 29, 2024

melvin-bot bot commented Aug 29, 2024

FitseTLT commented Aug 29, 2024

Krishna2323 commented Aug 29, 2024 •

edited

Loading

Krishna2323 commented Aug 29, 2024

Krishna2323 commented Aug 29, 2024

Krishna2323 commented Aug 29, 2024

paultsimura commented Aug 29, 2024 •

edited

Loading

Krishna2323 commented Aug 29, 2024

brunovjk commented Aug 29, 2024

Gajendra-Gupta commented Sep 2, 2024 •

edited

Loading

melvin-bot bot commented Sep 2, 2024

paultsimura commented Sep 2, 2024 •

edited

Loading

paultsimura commented Sep 2, 2024

paultsimura commented Sep 3, 2024

melvin-bot bot commented Sep 3, 2024

Krishna2323 commented Sep 10, 2024

bernhardoj commented Sep 10, 2024

paultsimura commented Sep 10, 2024

paultsimura commented Sep 10, 2024

puneetlath commented Sep 10, 2024

Krishna2323 commented Sep 10, 2024

melvin-bot bot commented Sep 10, 2024

melvin-bot bot commented Oct 3, 2024

Krishna2323 commented Oct 3, 2024

paultsimura commented Oct 3, 2024

melvin-bot bot commented Oct 9, 2024

melvin-bot bot commented Oct 9, 2024

melvin-bot bot commented Oct 9, 2024

paultsimura commented Oct 12, 2024 •

edited

Loading

puneetlath commented Oct 18, 2024

[HOLD for payment 2024-10-16] [$500] Distance rates - Enabled distance rate changes to "Disabled" after deleting it #48290

[HOLD for payment 2024-10-16] [$500] Distance rates - Enabled distance rate changes to "Disabled" after deleting it #48290

Comments

lanitochka17 commented Aug 29, 2024 • edited by melvin-bot bot Loading

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

Screenshots/Videos

melvin-bot bot commented Aug 29, 2024

mkzie2 commented Aug 29, 2024

This comment was marked as outdated.

melvin-bot bot commented Aug 29, 2024

melvin-bot bot commented Aug 29, 2024

FitseTLT commented Aug 29, 2024

Krishna2323 commented Aug 29, 2024 • edited Loading

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

What alternative solutions did you explore? (Optional 2)

Result

Krishna2323 commented Aug 29, 2024

Krishna2323 commented Aug 29, 2024

Proposal Updated

Krishna2323 commented Aug 29, 2024

Proposal Updated

paultsimura commented Aug 29, 2024 • edited Loading

Krishna2323 commented Aug 29, 2024

Proposal Updated

brunovjk commented Aug 29, 2024

Gajendra-Gupta commented Sep 2, 2024 • edited Loading

Proposal

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

What is the root cause of that problem?

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

melvin-bot bot commented Sep 2, 2024

paultsimura commented Sep 2, 2024 • edited Loading

paultsimura commented Sep 2, 2024

paultsimura commented Sep 3, 2024

melvin-bot bot commented Sep 3, 2024

Krishna2323 commented Sep 10, 2024

bernhardoj commented Sep 10, 2024

paultsimura commented Sep 10, 2024

paultsimura commented Sep 10, 2024

puneetlath commented Sep 10, 2024

Krishna2323 commented Sep 10, 2024

melvin-bot bot commented Sep 10, 2024

melvin-bot bot commented Oct 3, 2024

Krishna2323 commented Oct 3, 2024

paultsimura commented Oct 3, 2024

melvin-bot bot commented Oct 9, 2024

melvin-bot bot commented Oct 9, 2024

melvin-bot bot commented Oct 9, 2024

paultsimura commented Oct 12, 2024 • edited Loading

Regression Test Proposal

puneetlath commented Oct 18, 2024

lanitochka17 commented Aug 29, 2024 •

edited by melvin-bot bot

Loading

Krishna2323 commented Aug 29, 2024 •

edited

Loading

What alternative solutions did you explore? (Optional)  

What alternative solutions did you explore? (Optional 2) 

paultsimura commented Aug 29, 2024 •

edited

Loading

Gajendra-Gupta commented Sep 2, 2024 •

edited

Loading

paultsimura commented Sep 2, 2024 •

edited

Loading

paultsimura commented Oct 12, 2024 •

edited

Loading