[Backwards Compatibility] [HIGH] Push all policy updates to all policy members forever

[mountiny]

Problem

When the approver on Collect policy is changed, we do not update all the policy members with this information.

The submitsTo property is used to determine who to submit the report to or to compute correct NextStep information on the expense report. Hence if its not up to date after admin's changes, we can show incorrect data to the user.

Solution

When the approver is changed, push onyx update of the new accountID in the submitsTo field to all the members of a policy.

Left to do:

• Undo https://github.com/Expensify/Web-Expensify/commit/da6e05d51c05cd81e1bb786761cf09458fcc3f65 to always count 3
• Fix https://github.com/Expensify/Expensify/issues/378289
• Fix https://github.com/Expensify/Expensify/issues/381552
• Check performance
• Disabling multiple categories at the same time with the toggle create separate onyxUpdates for each of them when it could have been just one: - https://github.com/Expensify/Auth/pull/9773#issuecomment-1981549368 -- same problem exists for tags, see test testDeleteAllTags
• Send updates in Policy::updateAttribute
• Remove old Onyx Updates: https://github.com/Expensify/Web-Expensify/pull/40872
• Remove PHP param 'shouldSendOnyxUpdates'
• Refactor tests to use AuthTestHelper::isOnyxUpdateQueued
• Figure out why verifyOnyxUpdates is continuing execution after an assertion in the loop: https://expensify.slack.com/archives/C03TQ48KC/p1710791698732929?thread_ts=1710789927.884809&cid=C03TQ48KC
• Improve verifyOnyxUpdates so we know where it failed in the calling method https://expensify.slack.com/archives/C03TQ48KC/p1710791531318939?thread_ts=1710789927.884809&cid=C03TQ48KC
• Re-enable & figure out testDeleteAllTags
• Create an external issue to implement Policy::getSubmitsTo in app [HOLD for payment 2024-05-03] [Backwards Compatibility] [$250] Remove usages of policy.submitsTo in favor of using a new method PolicyUtils.getSubmitTo #40356
  • HOLD [hold for payment 2024-04-17] [$500] Improve performance of getPersonalDetailByEmail #38960
  • HOLD https://github.com/Expensify/Expensify/issues/382927
  • HOLD [Awaiting Payment 23rd April] [$250] Deprecate policy.role in favor of employeeList #39025
  • HOLD At some point somewhere in App, we need to update the code that gets an employee from the employeeList to use an email instead of an accountID ?
• TODO: Ensure this is updated to handle tag list order properly.. See [HOLD for payment 2024-03-14] [MEDIUM] Ensure that we preserve order of tags for multiple levels of tag lists #37054
• https://github.com/Expensify/Expensify/issues/382227

[melvin-bot]

Triggered auto assignment to @puneetlath (NewFeature), see https://stackoverflowteams.com/c/expensify/questions/14418#:~:text=BugZero%20process%20steps%20for%20feature%20requests for more details.

[flodnv]

I spent around 2 hours discussing what seems like a design problem in our code: https://expensify.slack.com/archives/C02MW39LT9N/p1706111344262919?thread_ts=1706104155.034209&cid=C02MW39LT9N

[flodnv]

This is where that discussion landed:

1. Leave the policy change logs out of this entirely. This means not touching them as part of this, and not coupling them with onyx updates.
2. For onyx updates, I will write some code in SavePolicy that gets the policy diff and sends out all the updates to all members all the time for any change that may occur.
3. Once that's done and deployed and working, we kill all the other code written that's coupled with policy change logs
4. We profit, because thanks to (2) we don't ever need to think about sending out policy changes again.

[mountiny]

Thanks for making this plan, it is better approach 🙌

[melvin-bot]

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

[flodnv]

I've been trying to wrap my ahead around how to accomplish this without introducing performance problems https://expensify.slack.com/archives/C02MW39LT9N/p1706557806120739?thread_ts=1706104155.034209&cid=C02MW39LT9N

[flodnv]

I've been swamped with reviews this week, and working 2 days at 50% did not help. This is my top priority after https://github.com/Expensify/Expensify/issues/358326

[melvin-bot]

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

[flodnv]

No update, I hope to have some time this week to pick this back up

…

On Mon, Feb 5, 2024 at 11:24 AM melvin-bot[bot] ***@***.***> wrote: @flodnv <https://github.com/flodnv> Uh oh! This issue is overdue by 2 days. Don't forget to update your issues! — Reply to this email directly, view it on GitHub <#34834 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA7Y2OKIEJUKO733X3FBMR3YSD2UHAVCNFSM6AAAAABCCNGF6CVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMRXGI2TCMRQGY> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[flodnv]

Continued today, I have a POC where it works with the top level 'name' attribute. Tomorrow I'll work on finding a way to make it work with sub levels. I have a feeling this is going to be really challenging.

[flodnv]

BTW, I updated the title of this issue to be more clear: Push all policy updates to all policy members forever.

More progress today:

Make it work with outputCurrency changes, which makes it work for reportFields and customUnits

More to come 🙏

[flodnv]

• Sent https://github.com/Expensify/Auth/pull/10282 yesterday
• Fixed a crash today: https://github.com/Expensify/Auth/pull/10298
• Looking at more items today: https://expensify.slack.com/archives/C03TQ48KC/p1711047236645789

[flodnv]

2 months later, we finally get to Implement submitsTo @mountiny

From looking at it, it seems to me like we want to send submitsTo to everyone in the policy anytime approvalMode or approver changes, right??

[flodnv]

Latest discussion on submitsTo: https://expensify.slack.com/archives/C03TQ48KC/p1711391658212239

[flodnv]

Next discussion about role so I can send updates in SharePolicy: https://expensify.slack.com/archives/C03TQ48KC/p1711481616732939

[flodnv]

Create an external issue to implement Policy::getSubmitsTo in app

This is the only important issue remaining for the Spring release. As you can see in the OP, it's being held by at least 5 other different tasks, in order to implement it the correct way. I'll be pushing these tasks so they keep going forward.

[melvin-bot]

@flodnv Eep! 4 days overdue now. Issues have feelings too...

[flodnv]

Send updates in Policy::updateAttribute

Started brainstorming this here: https://expensify.slack.com/archives/C036QM0SLJK/p1712606285676069

[flodnv]

Calling Check performance done: https://github.com/Expensify/Auth/pull/9773#issuecomment-2048407381

[flodnv]

Create an external issue to implement Policy::getSubmitsTo in app

Issues for which we are HOLDing this last item are being worked on daily.

[flodnv]

Created #40356

[flodnv]

Once #40356 is closed, we can close this.

[flodnv]

Cleanup task for later here: https://github.com/Expensify/Expensify/issues/389638

[flodnv]

• Replace policy.submitsTo with PolicyUtils.getSubmitToAccountID #40532 will be deployed to prod soon
• A couple of cleanup tasks will remain (somewhat related but not blockers):
  • https://github.com/Expensify/Expensify/issues/389638
  • https://github.com/Expensify/Expensify/issues/382227
• Disabling multiple categories at the same time with the toggle create separate onyxUpdates for each of them when it could have been just one was the last remaining thing here, but it may be a time sink and there doesn't seem to be an active problem about it, so I'm just going to ignore it for now

In summary, let's close this once #40532 is in prod.

[trjExpensify]

Sounds good to me! Deployed to prod, closing. Solid work here!