[Onyx] Move policyTaxRates_ key back into the policy_ key

[flodnv]

Problem & Solution

Sister issue: #36170

Discussed internally, summarised here and concluded there

• We added the onyx keys policyTaxRates_ and policyReportFields_ for no good reason. There's no size to optimize for in these collections.
• This makes my life harder in trying to fix an architectural problem of sending policy updates in https://github.com/Expensify/Auth/pull/9773 (see https://expensify.slack.com/archives/C03TQ48KC/p1706285244529629 & [Backwards Compatibility] [HIGH] Push all policy updates to all policy members forever #34834)
• This makes code more complex than it needs to be (lots of existing data massaging for really no gain)
• As such, we should kill and fix this unnecessarily complex code that was recently introduced. This means, at a minimum (I might be missing things):
  • Moving this data back into the policy_ key
  • Removing any data massaging from the backend
  • Have the frontend accept the same data as what's in the database
  • Where we should not display disabled fields, then let's use the disabled property to hide them. We will need to display them to admins soon enough
• No need to worry about sending updates to policy members when the policy changes, that will be done as part of https://github.com/Expensify/Auth/pull/9773, which includes doing this for all current and future policy attributes

cc @iwiznia @puneetlath @tgolen @marcaaron @francoisl @dangrous @johnmlee101 @stitesExpensify @thienlnam @nkuoch @AndrewGable @luacmartins

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~019f2faa357c786564
• Upwork Job ID: 1755673931208187904
• Last Price Increase: 2024-02-08

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~019f2faa357c786564

[melvin-bot]

Triggered auto assignment to Contributor Plus for review of internal employee PR - @getusha (Internal)

[flodnv]

I just realized we also need to fix the location of isTaxTrackingEnabled like we did in https://github.com/Expensify/Web-Expensify/pull/40728 for harvesting...

cc @mountiny

[mountiny]

@flodnv Made an issue for this here #36176 and will address it

Also noted on this PR https://github.com/Expensify/Auth/pull/9677/files

[iwiznia]

Removing any data massaging from the backend

The data in onyx is an object keyed by fieldID but in the DB is an array, so I think we can't do this

[melvin-bot]

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

[MonilBhavsar]

The plan is to

1. Have Web-E send tax details in policy_ onyx along with policyTaxRates_ key
2. Update App to use policy_ key for tax rates
3. After some time, completely deprecate policyTaxRates_ from server

[teneeto]

Hi, I'm Eto from Callstack - expert contributor group - and I would like to take care of this issue.

[tgolen]

Is the plan to use an Onyx migration to ensure the data exists in the proper key?

…

On Tue, Feb 13, 2024 at 12:15 AM Etotaziba Olei ***@***.***> wrote: Hi, I'm Eto from Callstack - expert contributor group - and I would like to take care of this issue. — Reply to this email directly, view it on GitHub <#36171 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAJMAB75VH2QGML36N5FJATYTMHJFAVCNFSM6AAAAABDAJRNCSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNBQGU2DONRSHA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[flodnv]

Also, is the plan to store tax details in policy_ in the same place they are in the DB, aka in the expenseUDFs key?

[marcaaron]

After some time, completely deprecate policyTaxRates_ from server

Oh also, anytime we force users to upgrade to a new app version would be a good opportunity to remove this code. Then we don't have to consider how much time is "enough" before removing things. Not sure if I'd force an upgrade for this though and we haven't used this yet.

[MonilBhavsar]

From this SO https://stackoverflowteams.com/c/expensify/questions/14164 I think we don't require Onyx migration
This case is very similar to

The name of a key has changed (because it should get the new data from the server stored on the new key when they open the app)

Where we're moving data from one Onyx key to another and currently client is only reading that data

[MonilBhavsar]

Also, is the plan to store tax details in policy_ in the same place they are in the DB, aka in the expenseUDFs key?

No, under the taxRates key and in the form of object. Object only contains required data and in the format that can simply be rendered

[MonilBhavsar]

Oh also, anytime we force users to upgrade to a new app version would be a good opportunity to remove this code. Then we don't have to consider how much time is "enough" before removing things. Not sure if I'd force an upgrade for this though and we haven't used this yet.

Yes correct! We can note an app version where the App PR is deployed to production and once MINIMUM_APP_VERSION is updated to greater than the version we can safely remove the code. Will bump priority to Monthly to cleanup the code.

[teneeto]

@MonilBhavsar I raised a draft_PR where taxRates are been applied from policy_ rather than policyTaxRates_.

This is done for the main branch for now. I'm currently updating on edit fields PR as well.

[MonilBhavsar]

App PR is merged and deployed to production in version 1.4.44-13.
Once Minimum App version is set to 1.4.44-13, we'll deprecate policyTaxRates_ from Web-E completely

[luacmartins]

cc @kosmydel @filip-solecki since you're working on the Simplified Collect: Taxes doc too

[MonilBhavsar]

As per this comment https://github.com/Expensify/Auth/pull/9773#discussion_r1516014889
Need to update Onyx keys in auth once that PR is shipped

[MonilBhavsar]

All PR's are deployed and policyTaxRates_ collection is removed. So closing