UK fuel-band rates in the App's types, optimistic copy and lightning bolt check #104135

Activity

1. neil-marcellini

   self-assigned this

   on Oct 15, 2026
2. neil-marcellini

   added

   DailyKSv2KSv2

   InternalRequires API changes or must be handled by Expensify staffRequires API changes or must be handled by Expensify staff

   on Oct 15, 2026
3. thelullabyy

   self-assigned this

   on Oct 16, 2026
4. thelullabyy commented on Oct 16, 2026

   

   thelullabyy

   on Oct 16, 2026

   Contributor

   More actions

   @MelvinBot could you please post a proposal?

   👀React with 👀1Reacted by melvin-bot
5. MelvinBot commented on Oct 16, 2026

   

   MelvinBot

   on Oct 16, 2026

   Contributor

   More actions

   Proposal

   What is the root cause of that problem?

   The App assumes each country publishes one rate series with no VAT data.

   • The optimistic copy loop only filters by currency, country and already-copied sourceRateID. A GBP workspace would get AMAP plus all 8 fuel bands. It never reads tax tracking or distance tax, and it builds attributes with only governmentRate.
   • GovernmentMileageRate and GovernmentRateSnapshot have no taxClaimablePercentage, so the value is dropped.
   • isGovernmentRateUnmodified checks only amount and dates. Editing a band's reclaimable amount keeps the lightning bolt in WorkspaceDistanceRatesTableRow.

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

   1. Types. Add optional taxClaimablePercentage to GovernmentMileageRate and GovernmentRateSnapshot.
   2. Series helper. In PolicyDistanceRatesUtils, add getGovernmentRateSeries(sourceRateID). It returns the sourceRateID without the trailing _<startDate>, for example GB for AMAP or GB_<band> for a fuel band. A rate is a fuel-band rate when its series differs from its country prefix.
   3. Pick the right rates. Add an isTaxTrackingEnabled argument to buildOnyxDataForGovernmentRateAutoUpdate. The workspace is eligible when that flag and customUnit.attributes.taxEnabled are both true. In the loop, skip fuel-band rates when not eligible and AMAP rates when eligible. Other countries have only one series, so they behave as before.
   4. Copy the new fields onto each optimistic rate:
      • taxClaimablePercentage on attributes and on the governmentRate snapshot, when present.
      • enabled from the workspace rate with the same series and the latest earlier startDate. Fall back to governmentMileageRate.enabled ?? true.
      • Leave taxRateExternalID unset; the server response fills it in.
   5. Pass eligibility from callers. Pass !!policy?.tax?.trackingEnabled from setWorkspaceDistanceAutoUpdate and updateGeneralSettings. Its three page callers (settings, country, upgrade) already have policy.
   6. Lightning bolt check. In isGovernmentRateUnmodified, also require |rate.rate × rate pct − governmentRateAmount × snapshot pct| < GOVERNMENT_RATE_MATCH_TOLERANCE. Treat a missing percentage as 0. governmentRateAmount is already unit-converted, and the percentage is a ratio, so the unit conversion still holds. Rates without VAT data compare 0 to 0 and behave as before.
   7. Tests (Given/When/Then):
      • PolicyDistanceRatesUtilsTest: an edited reclaimable amount is modified; a matching one is unmodified; an amount within tolerance is unmodified; no percentage on either side is unmodified.
      • DistanceRateTest: eligible GBP workspace gets only bands, with taxClaimablePercentage on the rate and the snapshot. Non-eligible gets only AMAP. A disabled previous band stays disabled. US and EUR are unchanged.

   What alternative solutions did you explore? (Optional)

   Detecting fuel bands by whether taxClaimablePercentage is set. That is simpler, but the series ID matches how the server finds successors, and it still works if a band's percentage is ever 0.

   Open question

   The fuel rate list starts empty until launch. If an eligible GBP workspace turns on auto-update before then, this proposal copies nothing optimistically, matching the rule "bands when eligible, otherwise AMAP." If the server falls back to AMAP when no bands exist, step 3 should do the same. Please confirm the intended server behavior.

   ---

   Next Steps for Contributor+ team:
   To accept: @MelvinBot implement [this](https://github.com/Expensify/App/issues/104135) to create a draft PR.
   To refine: @MelvinBot <your feedback>
   To reject: Explain why you are rejecting Melvin's proposal.
6. thelullabyy commented on Oct 18, 2026

   

   thelullabyy

   on Oct 18, 2026

   Contributor

   More actions

   @MelvinBot thanks, the root cause and overall approach look good. Please update the proposal with the following:

   1. Lightning bolt tolerance. PolicyDistanceRateTaxReclaimableEditPage displays the reclaimable amount with toFixed(CONST.MAX_TAX_RATE_DECIMAL_PLACES) (4 dp) and saves pct = value * 100 / rate, so restoring the displayed value can be off by up to 0.005 cents, more than GOVERNMENT_RATE_MATCH_TOLERANCE (0.001). Example: a 14p band with 1/6 VAT gives 2.3333p, displayed as 0.0233, so re-entering it gives 2.33p and the bolt never comes back. Please compare at display precision (or use a suitable separate tolerance) and add a test for this restore case.
   2. Missing percentage treated as 0. This changes behavior for non-UK government rates: a US/EUR rate where the admin set a reclaimable amount would lose its bolt even though the snapshot has no percentage. Please either compare only when the snapshot has a percentage, or explain why this matches how the server decides a rate is unmodified.
   3. Reuse the existing eligibility helper. isTaxTrackingEnabled in src/libs/PolicyUtils/tax.ts already combines policy.tax.trackingEnabled with the distance custom unit's taxEnabled. Please reuse it instead of re-deriving the rule, and avoid naming the new parameter isTaxTrackingEnabled (e.g. shouldUseFuelBandRates).
   4. Function signature. buildOnyxDataForGovernmentRateAutoUpdate / setWorkspaceDistanceAutoUpdate already take 8 positional params. Rather than appending a 9th boolean, consider passing policy (all callers have it) or an options object.
   5. Series ID and enabled inheritance. Please confirm the band sourceRateID format against Expensify/Expensify#692142, and strip the date with lastIndexOf('_') so a band name containing _ still works. Also clarify how enabled should be inherited when one request copies several rates of the same series (e.g. two years of a band on first enable): from the workspace only, or also from an earlier rate in the same batch?
   6. Tests. Also cover a band rate whose snapshot has no percentage, and the updateGeneralSettings path (changing currency to GBP on an eligible workspace picks bands only).

   👀React with 👀1Reacted by melvin-bot
7. MelvinBot commented on Oct 18, 2026

   

   MelvinBot

   on Oct 18, 2026

   Contributor

   More actions

   Proposal (updated)

   Updated for thelullabyy's review. Changes by point:

   1. Tolerance: the lightning bolt now compares the reclaimable amount at the edit page's display precision (4 dp), so restoring the shown value brings the bolt back.
   2. Missing percentage: the reclaimable amount is compared only when the snapshot has a percentage. US/EUR rates behave exactly as today.
   3. Eligibility: uses isTaxTrackingEnabled(true, policy, true). The local is named shouldUseFuelBandRates.
   4. Signature: both functions take policy instead of policyID + customUnit. That's 7 params, down from 8.
   5. Series and enabled: band IDs are confirmed as GB_<fuelBand>_<startDate>. The series strips the date with lastIndexOf('_'). enabled chains through earlier rates in the same batch.
   6. Tests: added the cases you asked for. A GBP currency change doesn't run the optimistic copy today; details are in step 7.

   What is the root cause of that problem?

   The App assumes each country publishes one rate series with no VAT data.

   • The optimistic copy loop filters only by currency, country and already-copied sourceRateID. A GBP workspace would get AMAP plus all 8 fuel bands. The loop never checks tax eligibility, and its attributes contain only governmentRate.
   • GovernmentMileageRate and GovernmentRateSnapshot have no taxClaimablePercentage, so the value is dropped.
   • isGovernmentRateUnmodified checks only the amount and dates. An edited band reclaimable amount keeps the bolt in WorkspaceDistanceRatesTableRow.

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

   1. Types. Add optional taxClaimablePercentage to GovernmentMileageRate and GovernmentRateSnapshot.
   2. Series helper. Add getGovernmentRateSeries(sourceRateID) to PolicyDistanceRatesUtils. It returns sourceRateID.slice(0, sourceRateID.lastIndexOf('_')), or the whole ID if there is no _.
      • Band IDs are GB_<fuelBand>_<startDate>, for example GB_PETROL_UP_TO_1400_2026-12-01 → GB_PETROL_UP_TO_1400. The band name itself contains _, so only the last _ marks the date.
      • AMAP stays GB_<startDate> → GB.
      • A rate is a fuel-band rate when its series differs from its country (split('_').at(0), which still returns GB).
   3. Signature. In buildOnyxDataForGovernmentRateAutoUpdate and setWorkspaceDistanceAutoUpdate, replace policyID and customUnit with policy. Derive policy.id and getDistanceRateCustomUnit(policy) inside. Every caller already gets its unit that way (settings, country, upgrade), and updateGeneralSettings has policy.
   4. Pick the right rates. In the builder, set const shouldUseFuelBandRates = isTaxTrackingEnabled(true, policy, true) (tax.ts). Skip band rates when it is false and AMAP rates when it is true. Other countries have one series, so nothing changes for them. While the production fuel list is empty, an eligible GBP workspace gets no optimistic rates. This matches the server rule (bands when eligible, otherwise AMAP).
   5. Copy the new fields onto each optimistic rate:
      • taxClaimablePercentage goes on attributes and on the governmentRate snapshot, only when the reference rate has it.
      • enabled: sort the candidates by startDate. Seed a per-series map with the enabled of the workspace's latest rate in each series. Each copied rate takes the map value, falling back to governmentMileageRate.enabled ?? true, then writes its own value back. If one request copies two years of a band, the second year inherits from the first. A batch rate always copies its predecessor, so this gives the same result as a workspace-only lookup. It just follows "previous rate in the series" directly.
      • taxRateExternalID stays unset; the server response fills it in.
   6. Lightning bolt. Add getTaxClaimableDisplayAmount(taxClaimablePercentage, rate), which returns ((taxClaimablePercentage * rate) / 100).toFixed(CONST.MAX_TAX_RATE_DECIMAL_PLACES). Use it on the edit page as well. In isGovernmentRateUnmodified, only when governmentRate.taxClaimablePercentage is defined, also require that the display amount of the rate's own percentage (missing = 0) matches the display amount of governmentRateAmount × the snapshot percentage.
      • The edit page saves value * 100 / rate. Re-entering the shown value therefore rounds back to the same string, and the bolt returns.
      • A snapshot with no percentage skips the check. A US/EUR rate where an admin added a reclaimable amount keeps its bolt, as today.
   7. Currency change to GBP. This path never runs the optimistic copy. WorkspaceOverviewCurrencyPage passes no country for single-country currencies. updateGeneralSettings builds rates only when governmentRateCountry is set, which only the EUR country picker does. For GBP, the server copies the bands and the response brings them in. Making GBP optimistic would be a separate behavior change, so this proposal leaves it as is.
   8. Tests (Given/When/Then):
      • PolicyDistanceRatesUtilsTest:
        • getGovernmentRateSeries handles GB_2026-04-06, GB_PETROL_UP_TO_1400_2026-12-01, and an ID with no _.
        • isGovernmentRateUnmodified treats an edited reclaimable amount as modified and a matching one as unmodified.
        • Restore case: a 14p rate with a 1/6 snapshot shows 0.0233. Saving 0.0233 is unmodified.
        • A band snapshot with no percentage is unmodified, even when the rate has one.
        • A US snapshot with no percentage plus an admin-set reclaimable amount is unmodified.
      • DistanceRateTest (setWorkspaceDistanceAutoUpdate):
        • An eligible GBP workspace gets only bands, with taxClaimablePercentage on the rate and the snapshot.
        • Tax tracking on but distance tax off gives only AMAP.
        • A band reference rate with no percentage is copied without the field.
        • A disabled previous band stays disabled. Two years of one band in one request both inherit it.
        • US and EUR are unchanged.
      • PolicyTest (updateGeneralSettings):
        • On an eligible workspace, changing the currency to GBP adds no optimistic rates and sends no optimisticRateIDs.
        • A call with governmentRateCountry: 'GB' on an eligible workspace copies bands only. This covers passing policy through that caller.

   What alternative solutions did you explore? (Optional)

   • Detect fuel bands by whether taxClaimablePercentage is set. That is simpler, but the series ID matches how the server finds successors, and it still works if a band's percentage is ever 0.
   • Use a separate tolerance of half a display unit (0.005 cents). That works too, but display-precision comparison means "unmodified" whenever the edit page shows the government value. It also needs no new constant.

   Notes for the reviewer

   • Server parity. The server's planned unmodified check compares the reclaimable amount within GOVERNMENT_RATE_MATCH_TOLERANCE. With step 6, a restored value can show the bolt while the server still treats the rate as edited and stops back-filling it. The server check should round the same way. That needs an internal decision outside this issue.
   • The 1/6 VAT example. Under the server design, a band's rate is the AMAP pence, and taxClaimablePercentage is the band's fuel pence ÷ the AMAP pence. The shown reclaimable amount is therefore the whole-pence fuel rate (for example, 14p shows 0.1400), and VAT comes from the linked tax rate. Reference values should restore exactly. Display-precision comparison still matters for any value an admin types.

   ---

   Next Steps for Contributor+ team:
   To accept: @MelvinBot implement [this](https://github.com/Expensify/App/issues/104135) to create a draft PR.
   To refine: @MelvinBot <your feedback>
   To reject: Explain why you are rejecting Melvin's proposal.

   ---

   view run
8. thelullabyy commented on Oct 18, 2026

   

   thelullabyy

   on Oct 18, 2026

   Contributor

   More actions

   @MelvinBot Implement

   👀React with 👀1Reacted by melvin-bot
9. MelvinBot commented on Oct 18, 2026

   

   MelvinBot

   on Oct 18, 2026

   Contributor

   More actions

   🤖 Draft PR created: Copy UK fuel-band rates with their reclaimable fraction and check it for the lightning bolt
   Please mark it as "Ready for review" or ask me to do that for you when it is ready for review.

   It implements the updated proposal. Two details differ from the proposal:

   • The bands-or-standard-rate filter applies only to GB rates, using a new CONST.CUSTOM_UNITS.GOVERNMENT_RATE_FUEL_BAND_COUNTRY. Without that limit, a US workspace that tracks distance tax would get no rates.
   • To keep React Compiler working after the edit page started using the shared getTaxClaimableDisplayAmount helper, I removed its useCallback.

   Typecheck, lint, spelling, the React Compiler check, and 798 related Jest tests pass. The full Jest suite and the Storybook smoke test weren't run locally.

   ---

   view run
10. melvin-bot

    added

    ReviewingHas a PR in reviewHas a PR in review

    WeeklyKSv2KSv2

    and removed

    DailyKSv2KSv2

    on Oct 19, 2026