[HOLD for payment 2024-04-03] [Report Fields] [Onyx] Move policyReportFields_ key back into the policy_ key

[flodnv]

Problem & Solution

Sister issue: #36171

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 @MonilBhavsar

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~010e62ab2c798627fa
• Upwork Job ID: 1755672881949298688
• Last Price Increase: 2024-02-08

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~010e62ab2c798627fa

[melvin-bot]

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

[thienlnam]

Sibtain will be handling the FE changes and I will take the BE changes

[marcaaron]

I am not aware of any documented problem with storing everything related to a policy in the policy_ key.

Some hypothetical problems to watch out for could be:

• Large policy JSON is inefficient to modify in Onyx because we do a deep merge (maybe some copying still happens in some places as well).
• Optimizations exist to deep compare to see if we should update subscribers. Larger the JSON the slower these comparisons would be.
• Subscribing to a subset of data should result in faster updates than subscribing to a large JSON object. Even with the use of a selector. I think it would still be faster.

That's all I have to say on it though. I have not instrumented any of that and it's all just theoretical. This proposal looks fine.

[flodnv]

Large policy JSON is inefficient to modify in Onyx

How large? Tags is the most bloating data and it's already in its own key (and for now we'll leave it there)

because we do a deep merge

Can you expand on "we do a deep merge"? Where and why?

---

Thanks for having a look and commenting 🙇

[marcaaron]

How large? Tags is the most bloating data and it's already in its own key (and for now we'll leave it there)

I haven't instrumented anything so I can't answer that question.

Can you expand on "we do a deep merge"? Where and why?

There is a memory cache layer. Before we save things to the Onyx dbs we merge them into the existing objects in the cache. The smaller those objects are the faster they would be to merge (hypothetically). I am not aware that we've hit any kind of limit there yet - but I do know that it was slow enough in the past that we optimized it to make it faster. We could have someone instrument to see at what point the merges get slow - maybe it's a lot or not.

[melvin-bot]

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

[allroundexperts]

Will have a PR ready by tomorrow Melv.

[allroundexperts]

Update: Still working on a PR. Taking me longer than I expected. Will provide an update tomorrow.

[thienlnam]

No worries, I'll still need to fold the reportField back into the policy_ key on the back-end. I'm currently working on those pusher updates for modifying an existing report field / new endpoint for removing a field

[allroundexperts]

Created draft PR #36553. Please let me know once the backend changes are done and I'll then record the videos.

[melvin-bot]

@allroundexperts, @thienlnam Huh... This is 4 days overdue. Who can take care of this?

[thienlnam]

Still haven't had the chance to add the BE changes as I'm focused on Track, likely will get to it later this week

[thienlnam]

Back-end PR is in review, as part of simplifying the changes I've made a few additional changes that will need to be reflected in the FE PR

• Report fields are now listed under the 'fieldList' key in the policy and the report
• We're now going to return all the keys on the field list item, that being said we'll need to now have a way on the FE to get the valid options (Removing keys from the values, where disabledOptions are true), and the types upgraded
• The report fields will be returned keyed with expensify_ so we can remove the changes I added here

        {
            "type": "formula",
            "fieldID": "text_title",
            "defaultValue": "{report:type} {report:startdate}",
            "value": null,
            "name": "title",
            "target": "expense",
            "values": [],
            "keys": [],
            "externalIDs": [],
            "disabledOptions": [],
            "externalID": null,
            "orderWeight": 0,
            "deletable": true,
            "origin": null,
            "isTax": false,
            "defaultExternalID": null
        },

   {
            "type": "dropdown",
            "fieldID": "field_id_NUGGET_SHOPZ",
            "defaultValue": "",
            "value": null,
            "name": "Nugget Shopz",
            "target": "expense",
            "values": [
                "Booger King",
                "Chick Fill Hay",
                "Kay Eff See",
                "Mickey Dee's",
                "Wendeez"
            ],
            "keys": [],
            "externalIDs": [],
            "disabledOptions": [
                false,
                false,
                false,
                false,
                false
            ],
            "externalID": null,
            "orderWeight": 1,
            "deletable": false,
            "origin": null,
            "isTax": false,
            "defaultExternalID": ""
        }

[thienlnam]

The last Auth PR was merged and should be getting deployed on Monday - once that is done, we can merge @allroundexperts's App PR here #36553

[JmillsExpensify]

Auth PR was deployed today, so looks like we need to get @allroundexperts PR into review. When do we expect that to happen?

[melvin-bot]

@allroundexperts, @thienlnam Whoops! This issue is 2 days overdue. Let's get this updated quick!

[allroundexperts]

@thienlnam @JmillsExpensify PR ready for review!

[JmillsExpensify]

Thank you!

[jjcoffee]

@JmillsExpensify Could you assign me here as I reviewed the PR? Thanks! 🙇

[JmillsExpensify]

Done. I'll go ahead and assign myself for the payment later.

[melvin-bot]

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

[melvin-bot]

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

• feat: add policyReportFields to the policy object directly #36553

If no regressions arise, payment will be issued on 2024-04-03. 🎊

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

• @jjcoffee requires payment (Needs manual offer from BZ)
• @allroundexperts requires payment through NewDot Manual Requests

[JmillsExpensify]

Payment summary:

• Contributor: @allroundexperts $500
• Contributor+: @jjcoffee $500

@jjcoffee Do we need any regression tests for this issue?

[JmillsExpensify]

Upwork offer sent to C+

[jjcoffee]

@JmillsExpensify Accepted, thanks! There should already be regression tests that cover the report fields functionality, so I think we're good here.

[JmillsExpensify]

Awesome! Contract paid, so closing this issue.

[laurenreidexpensify]

Payment summary:

• Contributor: @allroundexperts $500 newdot
• Contributor+: @jjcoffee $500 upwork

[JmillsExpensify]

$500 approved for @allroundexperts