[$250][P2P Distance] - Disabled rate is not listed in the Rate list when it is still selected #46884

m-natarajan · 2024-08-06T13:04:30Z

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: v9.0.17-0
Reproducible in staging?: y
Reproducible in production?: no, new feature
If this was caught during regression testing, add the test name, ID and link from TestRail: n/a
Email or phone of affected tester (no customers): applausetester+kh050801@applause.expensifail.com
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: Applause internal team
Slack conversation:

Action Performed:

Precondition:

Workspace has some distance rates.

Go to staging.new.expensify.com
Go to workspace chat.
Submit a distance expense with Distance rate A.
Go to workspace settings > Distance rates.
Disable Distance rate A used in Step 3.
Go back to transaction thread of the distance expense.
Click Rate.

Expected Result:

The disabled rate will appear selected in the Rate list but grayed out (like Category and Tag when selected and disabled afterward).

Actual Result:

The disabled rate is not listed in the Rate list when it is still selected.

Workaround:

unknown

Platforms:

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

Screenshots/Videos

Bug6563344_1722947953071.20240806_203405.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~01113b3b52e50f7f4e
Upwork Job ID: 1821117740687763365
Last Price Increase: 2024-08-14
Automatic offers:

dominictb | Contributor | 103608790

Issue Owner

Current Issue Owner: @mananjadhav

melvin-bot · 2024-08-06T13:04:40Z

Triggered auto assignment to @iwiznia (DeployBlockerCash), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

melvin-bot · 2024-08-06T13:04:41Z

Triggered auto assignment to @dylanexpensify (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.

github-actions · 2024-08-06T13:04:47Z

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

Identify the pull request that introduced this issue and revert it.
Find someone who can quickly fix the issue.
Fix the issue yourself.

BhuvaneshPatil · 2024-08-06T13:52:50Z

Edited by proposal-police: This proposal was edited at 2024-08-06 14:00:06 UTC.

PR - #40021

I can raise PR if needed

Proposal

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

Distance rate which is selected but after disabling, not shown.

What is the root cause of that problem?

We are filtering the disabled rates when showing the rates list in selector.

App/src/libs/DistanceRequestUtils.ts

Lines 47 to 50 in 627ef1a

    
           Object.entries(distanceUnit.rates).forEach(([rateID, rate]) => { 
        
               if (!includeDisabledRates && rate.enabled === false) { 
        
                   return; 
        
               }

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

Lines 141 to 144 in 627ef1a

    
           rates: { 
        
               key: ({report}) => `${ONYXKEYS.COLLECTION.POLICY}${report?.policyID ?? '-1'}`, 
        
               selector: (policy: OnyxEntry<OnyxTypes.Policy>) => DistanceRequestUtils.getMileageRates(policy), 
        
           },

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

We shall pass true as second argument that will give us disabled rates as well and we will also pass enabled value.

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

Lines 141 to 144 in 627ef1a

    
           rates: { 
        
               key: ({report}) => `${ONYXKEYS.COLLECTION.POLICY}${report?.policyID ?? '-1'}`, 
        
               selector: (policy: OnyxEntry<OnyxTypes.Policy>) => DistanceRequestUtils.getMileageRates(policy), 
        
           },

Once that is done -
we can filter the disabled rate in IOURequestStepDistanceRate as following. and pass isDisabled value so it will show selected but style will be different for selected but disabled value.

    const sections = Object.values(rates).filter(rate => {
        const isSelected = currentRateID ? currentRateID === rate.customUnitRateID : rate.name === CONST.CUSTOM_UNITS.DEFAULT_RATE
        return isSelected || rate?.enabled
    }).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,
            isDisabled: !rate.enabled
        };
    });

What alternative solutions did you explore? (Optional)

demo -

distance.rates.disabled.mp4

neil-marcellini · 2024-08-06T17:50:44Z

Not a blocker, the rate field is still behind the beta.

BhuvaneshPatil · 2024-08-06T18:21:51Z

@neil-marcellini Are we planning to make it external?

github-actions · 2024-08-06T19:55:37Z

@dominictb Your proposal will be dismissed because you did not follow the proposal template.

dominictb · 2024-08-06T19:57:55Z

Proposal

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

The disabled rate is not listed in the Rate list when it is still selected.

What is the root cause of that problem?

This logic:

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

Lines 71 to 81 in 627ef1a

    
           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, 
        
               }; 
        
           });

does not return the disable rate.

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

In general, we need to make sure, when we disable rate A:

If A is selected, display A in list. And A will have a gray color.
Otherwise, do not display it.

Details:

We need to refactor the getMileageRates function, which will receive an additional param, named selectedRateID, and for each rate returned by that function, we add an additional prop, enabled: rate.enabled:

+function getMileageRates(policy: OnyxInputOrEntry<Policy>, includeDisabledRates = false, selectedRateID: string): Record<string, MileageRate> {
    Object.entries(distanceUnit.rates).forEach(([rateID, rate]) => {
+      if (!includeDisabledRates && rate.enabled === false && selectedRateID !== rateID) {
            return;
        }

        mileageRates[rateID] = {
            ...
+          enabled: rate.enabled,
        };
    });
}

Remove this:

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

Lines 141 to 144 in 627ef1a

    
           rates: { 
        
               key: ({report}) => `${ONYXKEYS.COLLECTION.POLICY}${report?.policyID ?? '-1'}`, 
        
               selector: (policy: OnyxEntry<OnyxTypes.Policy>) => DistanceRequestUtils.getMileageRates(policy), 
        
           },

because it is redundant and use:

    const rates = DistanceRequestUtils.getMileageRates(policy, false, currentRateID)

in here instead.

Finally, return additional prop isDisabled: !rate.enabled in:

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

Line 74 in 627ef1a

return {

to get the grey-out color.

Note: In this page, we not only use the rates data to display the list, but also use it in:

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

Line 83 in 627ef1a

    
           const unit = (Object.values(rates)[0]?.unit === CONST.CUSTOM_UNITS.DISTANCE_UNIT_MILES ? translate('common.mile') : translate('common.kilometer')) as Unit;

and

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

Line 93 in 627ef1a

    
           const taxAmount = CurrencyUtils.convertToBackendAmount(TransactionUtils.calculateTaxAmount(taxPercentage, taxableAmount, rates[customUnitRateID].currency ?? CONST.CURRENCY.USD));

so rates data should be consistent whole the file.

What alternative solutions did you explore? (Optional)

melvin-bot · 2024-08-07T09:34:41Z

Job added to Upwork: https://www.upwork.com/jobs/~01113b3b52e50f7f4e

melvin-bot · 2024-08-07T09:34:46Z

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

dylanexpensify · 2024-08-07T09:35:27Z

made external

melvin-bot · 2024-08-12T18:32:52Z

@iwiznia, @mananjadhav, @dylanexpensify Eep! 4 days overdue now. Issues have feelings too...

iwiznia · 2024-08-20T18:34:49Z

@dominictb a couple questions:

Remove this: ... because it is redundant and use:

Don't we need that there? Otherwise this component will not update if onyx data changes, no?

Finally, return additional prop isDisabled: !rate.enabled in:

Why are we transforming enabled to isDisabled? Why not keep enabled?

dominictb · 2024-08-20T18:44:30Z

Don't we need that there? Otherwise this component will not update if onyx data changes, no?

I think we already had the policy data is always up-to-date here:

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

Line 132 in 627ef1a

policy: {

so we don't need:

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

Line 141 in 627ef1a

rates: {

Why are we transforming enabled to isDisabled? Why not keep enabled?

We only have a logic to gray out the option if it has isDisabled is true. So if need to convert enabled to isDisabled.

BhuvaneshPatil · 2024-08-20T18:45:25Z

Yes, I agree with @iwiznia points here.

I am not sure if we should modify getMileageRates() method, because it's being used at multiple places.
For unit, we use what is being used in workspace not individual rate. ~~Can you please confirm that~~. Checked we use what unit is set for workspace to all distance rates

App/src/libs/DistanceRequestUtils.ts

Lines 42 to 61 in 72c8cf7

    
           const distanceUnit = PolicyUtils.getCustomUnit(policy); 
        
           if (!distanceUnit?.rates) { 
        
               return mileageRates; 
        
           } 
        
           Object.entries(distanceUnit.rates).forEach(([rateID, rate]) => { 
        
               if (!includeDisabledRates && rate.enabled === false) { 
        
                   return; 
        
               } 
        
               mileageRates[rateID] = { 
        
                   rate: rate.rate, 
        
                   currency: rate.currency, 
        
                   unit: distanceUnit.attributes.unit, 
        
                   name: rate.name, 
        
                   customUnitRateID: rate.customUnitRateID, 
        
               }; 
        
           }); 
        
           return mileageRates;

And for calculateTaxAmount() we use particular tax rate. Meaning when we click on option then only that's triggered.

So I am not getting what we are achieving.

cc @mananjadhav

BhuvaneshPatil · 2024-08-20T19:06:40Z

Also in selected proposal, root cause is not correct.

We are not passing disabled values to component and the RCA mentions about the logic for creating list items.

iwiznia · 2024-08-20T19:11:56Z

I think we already had the policy data is always up-to-date here:

Ohhhhh, ok yeah, looking at those now I agree it is not needed

We only have a logic to gray out the option if it has isDisabled is true. So if need to convert enabled to isDisabled.

Got it, makes sense

iwiznia · 2024-08-20T19:13:51Z

I am not sure if we should modify getMileageRates() method, because it's being used at multiple places.

Like where? I'd think most places it is being used needs this logic and the ones that do not, can just not pass a selected value and the behavior would stay the same as now, no?

or unit, we use what is being used in workspace not individual rate
And for calculateTaxAmount() we use particular tax rate. Meaning when we click on option then only that's triggered.

Not sure what you mean here

mananjadhav · 2024-08-20T19:19:32Z

Checking recent comments.

mananjadhav · 2024-08-20T19:28:15Z

I don't think any of the existing calls should break. We can even set that as optional param. I don't think we're modifying anything for calculateTaxAmount or unit. We're only ensuring it works as before. Still feel we should go with the shortlisted proposal.

BhuvaneshPatil · 2024-08-20T19:28:37Z

about RCA?

Also didn't get this point.

It involves cleaning up the existing code as well.

melvin-bot · 2024-08-20T19:46:50Z

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

dominictb · 2024-08-22T08:00:01Z

Working on this now!

dylanexpensify · 2024-09-10T19:59:20Z

pending regression period

melvin-bot · 2024-09-16T18:39:34Z

This issue has not been updated in over 15 days. @iwiznia, @mananjadhav, @dylanexpensify, @dominictb 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!

dominictb · 2024-09-18T04:04:59Z

Should be ready for payment shortly.

mananjadhav · 2024-09-25T17:36:01Z

@iwiznia Can you check if this was deployed to production? I don't see any automation message. This is the last comment.

iwiznia · 2024-09-25T17:55:13Z

FYI you can check yourself by looking at the production branch. I think it was since I see your changes here

mananjadhav · 2024-09-25T18:17:05Z

Okay I should've clarified. I did see the changes in the production branch, was wondering about the payout date. @dylanexpensify can you please help with the payout info? I'll work on the checklist meanwhile.

iwiznia · 2024-09-25T18:24:31Z

hmmm yeah, I am not sure. Maybe you can find the merge commit in the production branch?

mananjadhav · 2024-09-26T16:09:03Z

Correct me if I am wrong but we can use this release link to confirm this was deployed 2 weeks ago - https://github.com/Expensify/App/releases/tag/9.0.31-26

@dylanexpensify as for the checklist I think this was more of an edge case and hence I don't have a PR that I can pinpoint to.

But I do feel we should add a regression test for this one.

Regression Test Proposal

Precondition: Workspace has some distance rates.
Go to NewDot app
Go to workspace chat.
Submit a distance expense with Distance rate A.
Go to workspace settings > Distance rates.
Disable Distance rate A used in Step 3.
Go back to transaction thread of the distance expense.
Click Rate.
Verify that: The disabled rate will appear selected in the rate list but grayed out.

dylanexpensify · 2024-09-26T17:34:41Z

Payment summary:

Contributor: @dominictb $250 via Upwork
Contributor+: @mananjadhav $250 via NewDot

Please apply/request!

dylanexpensify · 2024-09-26T17:37:16Z

payment sent in upwork @dominictb!

m-natarajan added DeployBlockerCash This issue or pull request should block deployment Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. DeployBlocker Indicates it should block deploying the API labels Aug 6, 2024

melvin-bot bot assigned iwiznia Aug 6, 2024

melvin-bot bot assigned dylanexpensify Aug 6, 2024

github-actions bot added Engineering Hourly KSv2 and removed Daily KSv2 labels Aug 6, 2024

github-actions bot mentioned this issue Aug 6, 2024

Deploy Checklist: New Expensify 2024-08-06 #46849

Closed

83 tasks

neil-marcellini added Daily KSv2 and removed DeployBlockerCash This issue or pull request should block deployment DeployBlocker Indicates it should block deploying the API Hourly KSv2 labels Aug 6, 2024

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

melvin-bot bot changed the title ~~Distance - Disabled rate is not listed in the Rate list when it is still selected~~ [$250] Distance - Disabled rate is not listed in the Rate list when it is still selected Aug 7, 2024

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

melvin-bot bot assigned mananjadhav Aug 7, 2024

iwiznia assigned dominictb Aug 20, 2024

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

dominictb mentioned this issue Aug 22, 2024

fix: show disabled distance rate if it is selected in IOU request #47850

Merged

50 tasks

melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Aug 22, 2024

melvin-bot bot added Monthly KSv2 and removed Weekly KSv2 labels Sep 16, 2024

dylanexpensify closed this as completed Sep 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[$250][P2P Distance] - Disabled rate is not listed in the Rate list when it is still selected #46884

[$250][P2P Distance] - Disabled rate is not listed in the Rate list when it is still selected #46884

m-natarajan commented Aug 6, 2024 •

edited by melvin-bot bot

Loading

melvin-bot bot commented Aug 6, 2024

melvin-bot bot commented Aug 6, 2024

github-actions bot commented Aug 6, 2024

BhuvaneshPatil commented Aug 6, 2024 •

edited by github-actions bot

Loading

neil-marcellini commented Aug 6, 2024

BhuvaneshPatil commented Aug 6, 2024

github-actions bot commented Aug 6, 2024

dominictb commented Aug 6, 2024

melvin-bot bot commented Aug 7, 2024

melvin-bot bot commented Aug 7, 2024

dylanexpensify commented Aug 7, 2024

melvin-bot bot commented Aug 12, 2024

iwiznia commented Aug 20, 2024

dominictb commented Aug 20, 2024

BhuvaneshPatil commented Aug 20, 2024 •

edited

Loading

BhuvaneshPatil commented Aug 20, 2024

iwiznia commented Aug 20, 2024

iwiznia commented Aug 20, 2024

mananjadhav commented Aug 20, 2024

mananjadhav commented Aug 20, 2024

BhuvaneshPatil commented Aug 20, 2024 •

edited

Loading

melvin-bot bot commented Aug 20, 2024

dominictb commented Aug 22, 2024

dylanexpensify commented Sep 10, 2024

melvin-bot bot commented Sep 16, 2024

dominictb commented Sep 18, 2024

mananjadhav commented Sep 25, 2024

iwiznia commented Sep 25, 2024

mananjadhav commented Sep 25, 2024 •

edited

Loading

iwiznia commented Sep 25, 2024

mananjadhav commented Sep 26, 2024

dylanexpensify commented Sep 26, 2024

dylanexpensify commented Sep 26, 2024

[$250][P2P Distance] - Disabled rate is not listed in the Rate list when it is still selected #46884

[$250][P2P Distance] - Disabled rate is not listed in the Rate list when it is still selected #46884

Comments

m-natarajan commented Aug 6, 2024 • edited by melvin-bot bot Loading

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

Screenshots/Videos

melvin-bot bot commented Aug 6, 2024

melvin-bot bot commented Aug 6, 2024

github-actions bot commented Aug 6, 2024

BhuvaneshPatil commented Aug 6, 2024 • edited by github-actions bot 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)

neil-marcellini commented Aug 6, 2024

BhuvaneshPatil commented Aug 6, 2024

github-actions bot commented Aug 6, 2024

dominictb commented Aug 6, 2024

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)

melvin-bot bot commented Aug 7, 2024

melvin-bot bot commented Aug 7, 2024

dylanexpensify commented Aug 7, 2024

melvin-bot bot commented Aug 12, 2024

iwiznia commented Aug 20, 2024

dominictb commented Aug 20, 2024

BhuvaneshPatil commented Aug 20, 2024 • edited Loading

BhuvaneshPatil commented Aug 20, 2024

iwiznia commented Aug 20, 2024

iwiznia commented Aug 20, 2024

mananjadhav commented Aug 20, 2024

mananjadhav commented Aug 20, 2024

BhuvaneshPatil commented Aug 20, 2024 • edited Loading

melvin-bot bot commented Aug 20, 2024

dominictb commented Aug 22, 2024

dylanexpensify commented Sep 10, 2024

melvin-bot bot commented Sep 16, 2024

dominictb commented Sep 18, 2024

mananjadhav commented Sep 25, 2024

iwiznia commented Sep 25, 2024

mananjadhav commented Sep 25, 2024 • edited Loading

iwiznia commented Sep 25, 2024

mananjadhav commented Sep 26, 2024

dylanexpensify commented Sep 26, 2024

dylanexpensify commented Sep 26, 2024

m-natarajan commented Aug 6, 2024 •

edited by melvin-bot bot

Loading

BhuvaneshPatil commented Aug 6, 2024 •

edited by github-actions bot

Loading

BhuvaneshPatil commented Aug 20, 2024 •

edited

Loading

BhuvaneshPatil commented Aug 20, 2024 •

edited

Loading

mananjadhav commented Sep 25, 2024 •

edited

Loading