Replace policy.submitsTo with PolicyUtils.getSubmitToAccountID

[bernhardoj]

Details

We want to create a new util function PolicyUtils.getSubmitToAccountID and replace policy.submitsTo usage with the new function.

Fixed Issues

$ #40356
PROPOSAL: #40356 (comment)

Tests

Same as QA Steps

• Verify that no errors appear in the JS console

Offline tests

Same as QA Steps

QA Steps

A. Prevent self-approval

1. Go to collect workspace setting in OldDot
2. Enable prevent self-approval settings
3. In Workflow tab, either disable Add approval or enable it and set it to yourself
4. Go to collect workspace chat in NewDot
5. Request money
6. Verify the Submit button is disabled

Note: Remember to disable prevent self-approval back

B. Approval optional

1. Go to collect workspace setting
2. Open the Workflow tab
3. Disable the Add approvals
4. Go to collect workspace chat
5. Request money
6. Submit the request
7. Verify the submit request is successful and the pay button is shown

C. Self-approval

1. Go to collect workspace setting
2. Open the Workflow tab
3. Enable Add approval and set the approver as yourself
4. Go to collect workspace chat
5. Request money
6. Open the expense report
7. Submit the request
8. Verify the next step below the header is "Waiting for you to review these expenses"
9. Verify there is an Approve button

D. Other approver

1. Go to collect workspace setting
2. Open the Workflow tab
3. Enable Add approval and set the approver to another user
4. Go to collect workspace chat
5. Request money
6. Open the expense report
7. Submit the request
8. Verify the next step below the header is "Waiting for {another user name} to approve these expenses"
9. Verify the approve button doesn't shows
10. As the approver, go to the workspace chat and approve the request
11. Verify the approve successful

• Verify that no errors appear in the JS console

PR Author Checklist

• I linked the correct issue in the ### Fixed Issues section above
• I wrote clear testing steps that cover the changes made in this PR
  • I added steps for local testing in the Tests section
  • I added steps for the expected offline behavior in the Offline steps section
  • I added steps for Staging and/or Production testing in the QA steps section
  • I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
  • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
  • I tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
• I included screenshots or videos for tests on all platforms
• I ran the tests on all platforms & verified they passed on:
  • Android: Native
  • Android: mWeb Chrome
  • iOS: Native
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop
• I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
• I followed proper code patterns (see Reviewing the code)
  • I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick)
  • I verified that the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
  • I verified that comments were added to code that is not self explanatory
  • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
  • I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
    • If any non-english text was added/modified, I verified the translation was requested/reviewed in #expensify-open-source and it was approved by an internal Expensify engineer. Link to Slack message:
  • I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
  • I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is either coming verbatim from figma or has been approved by marketing (in order to get marketing approval, ask the Bug Zero team member to add the Waiting for copy label to the issue)
  • I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
  • I verified the JSDocs style guidelines (in STYLE.md) were followed
• If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
• I followed the guidelines as stated in the Review Guidelines
• I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
• I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
• I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
• I verified that if a function's arguments changed that all usages have also been updated correctly
• If any new file was added I verified that:
  • The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
• If a new CSS style is added I verified that:
  • A similar style doesn't already exist
  • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))
• If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
• If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
• If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
• If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
• If the PR modifies the UI (e.g. new buttons, new UI components, changing the padding/spacing/sizing, moving components, etc) or modifies the form input styles:
  • I verified that all the inputs inside a form are aligned with each other.
  • I added Design label and/or tagged @Expensify/design so the design team can review the changes.
• If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
• If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.

Screenshots/Videos

Android: Native

A. Prevent self-approval

B. Approval optional

Screen.Recording.2024-04-20.at.13.06.00.mov

C. Self-approval

Screen.Recording.2024-04-20.at.13.10.44.mov

D. Other approver

Screen.Recording.2024-04-20.at.13.15.10.mov

Android: mWeb Chrome

A. Prevent self-approval

B. Approval optional

Screen.Recording.2024-04-20.at.13.06.56.mov

C. Self-approval

Screen.Recording.2024-04-20.at.13.09.23.mov

D. Other approver

Screen.Recording.2024-04-20.at.13.20.18.mov

iOS: Native

A. Prevent self-approval

B. Approval optional

Screen.Recording.2024-04-20.at.13.04.15.mov

C. Self-approval

Screen.Recording.2024-04-20.at.13.12.21.mov

D. Other approver

Screen.Recording.2024-04-20.at.13.21.32.mov

iOS: mWeb Safari

A. Prevent self-approval

B. Approval optional

Screen.Recording.2024-04-20.at.13.05.06.mov

C. Self-approval

Screen.Recording.2024-04-20.at.13.11.31.mov

D. Other approver

Screen.Recording.2024-04-20.at.13.22.19.mov

MacOS: Chrome / Safari

A. Prevent self-approval

B. Approval optional

Screen.Recording.2024-04-19.at.15.03.41.mov

C. Self-approval

Screen.Recording.2024-04-19.at.15.04.57.mov

D. Other approver

Screen.Recording.2024-04-19.at.15.13.13.mov

Screen.Recording.2024-04-19.at.15.27.50.mov

MacOS: Desktop

A. Prevent self-approval

B. Approval optional

Screen.Recording.2024-04-20.at.13.03.09.mov

C. Self-approval

Screen.Recording.2024-04-20.at.13.08.43.mov

D. Other approver

Screen.Recording.2024-04-20.at.13.23.08.mov

[melvin-bot]

@getusha Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

[bernhardoj]

@flodnv

1. I decided to accept accountID instead of email in PolicyUtils.getSubmitToAccountID param since accountID is easier to get. Inside the function, getLoginsByAccountIDs to get the employee email.
2. So far I tested 4 cases, prevent self-approval, approval optional, self-approval, and other approver.
3. I pass ownerAccountID instead of the currentUserAccountID for all usages, please check whether it's correct or not.
4. I only record for the web since the code doesn't depend on the platform

cc: @getusha

[flodnv]

@bernhardoj looking good, thanks! 👍

[flodnv]

I only record for the web since the code doesn't depend on the platform

Please always test on all platforms 🙇

[bernhardoj]

Updated the code, will complete the rest of the recording tomorrow.

[bernhardoj]

Recording/screenshot complete.

[trjExpensify]

👋 @getusha this is a deliverable for our May release, can you review the PR today please? Thanks!

[getusha]

👋 @getusha this is a deliverable for our May release, can you review the PR today please? Thanks!

I will, thanks!

[ikevin127]

I'm taking over the review here based on #40356 (comment).
♻️ Review in progress, ETA ~1h, max 1h30m.

[trjExpensify]

Assigned you, @ikevin127!

[ikevin127]

Reviewer Checklist

• I have verified the author checklist is complete (all boxes are checked off).
• I verified the correct issue is linked in the ### Fixed Issues section above
• I verified testing steps are clear and they cover the changes made in this PR
  • I verified the steps for local testing are in the Tests section
  • I verified the steps for Staging and/or Production testing are in the QA steps section
  • I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
  • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
• I checked that screenshots or videos are included for tests on all platforms
• I included screenshots or videos for tests on all platforms
• I verified tests pass on all platforms & I tested again on:
  • Android: Native
  • Android: mWeb Chrome
  • iOS: Native
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop
• If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
• I verified proper code patterns were followed (see Reviewing the code)
  • I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
  • I verified that the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
  • I verified that comments were added to code that is not self explanatory
  • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
  • I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
  • I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
  • I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is either coming verbatim from figma or has been approved by marketing (in order to get marketing approval, ask the Bug Zero team member to add the Waiting for copy label to the issue)
  • I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
  • I verified the JSDocs style guidelines (in STYLE.md) were followed
• If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
• I verified that this PR follows the guidelines as stated in the Review Guidelines
• I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
• I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
• I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
• If a new component is created I verified that:
  • A similar component doesn't exist in the codebase
  • All props are defined accurately and each prop has a /** comment above it */
  • The file is named correctly
  • The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
  • The only data being stored in the state is data necessary for rendering and nothing else
  • For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
  • Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
  • All JSX used for rendering exists in the render method
  • The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
• If any new file was added I verified that:
  • The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
• If a new CSS style is added I verified that:
  • A similar style doesn't already exist
  • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
• If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
• If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
• If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
• If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
• If the PR modifies the UI (e.g. new buttons, new UI components, changing the padding/spacing/sizing, moving components, etc) or modifies the form input styles:
  • I verified that all the inputs inside a form are aligned with each other.
  • I added Design label and/or tagged @Expensify/design so the design team can review the changes.
• If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
• If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
• I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Android: Native

A	B	C	D
android.mp4	android.mp4	android.mp4	android.mp4

Android: mWeb Chrome

A	B	C	D
android-mweb.mp4	android-mweb.mp4	android-mweb.mp4	android-mweb.mp4

iOS: Native

A	B	C	D
ios.mov	ios.mov	ios.mov	ios.mp4

iOS: mWeb Safari

A	B	C	D
ios-mweb.mov	ios-mweb.mov	ios-mweb.mov	ios-mweb.mp4

MacOS: Chrome / Safari

A	B	C	D
web.mov	web.mov	web.mov	web.mov

MacOS: Desktop

A	B	C	D
desktop.mov	desktop.mov	desktop.mov	desktop.mp4

[ikevin127]

Test check:
✅ A. Prevent self-approval
✅ B. Approval optional
⚠️ C. Self-approval
⚠️ D. Other approver

Warning

@bernhardoj I found issues with test C. Self-approval and test D. Other approver , hence why I left the following checkbox unchecked from the reviewer checklist:

[ ] I verified tests pass on all platforms & I tested again on:

The issues are the following for Test C:

1. After step 7. Submit the request -> the next step below the header is "Waiting for you to review these expenses." but if we wait a bit it changes to "These expenses are scheduled to automatically submit on Sunday! No further action required!" which is what we usually see when we still have the Submit button showing, but it's showing that header while we have the Approve button displayed.

2. If we press the Approve button -> the next step below the header is "Waiting for you to pay these expenses." but if we wait a bit it changes to "No further action required!".

To see the behaviour more clearly here's the video from web:

C-web-issue.mov

Important

Same as for test C, test D has similar behaviour in the fact that if we wait a bit after we press Submit and Approve buttons, the header changes instead of displaying the correct message consistently.

Note: for test D the message also changes after the approver opens the report if we wait a bit from "These expenses are scheduled to automatically submit on Sunday! No further action required!" to "kevin.bader96+3@gmail.com is waiting for you to review these expenses." -> then once Approve is pressed, the same thing as for test C happens if we wait a bit.

If this is expected behaviour let me know and I'll check the last checkbox and approve!

cc @flodnv @trjExpensify

[trjExpensify]

After step 7. Submit the request -> the next step below the header is "Waiting for you to review these expenses." but if we wait a bit it changes to "These expenses are scheduled to automatically submit on Sunday! No further action required!" which is what we usually see when we still have the Submit button showing, but it's showing that header while we have the Approve button displayed.

Same as for test C, test D has similar behaviour in the fact that if we wait a bit after we press Submit and Approve buttons, the header changes instead of displaying the correct message consistently.

That doesn't sound right. It shouldn't be reverting back to the previous next step. Is there a mismatch of what the OldDot next steps banner shows when this happens if you look at the corresponding expense report?

CC: @mountiny as perhaps being related to "optimistic next steps"

[ikevin127]

Is there a mismatch of what the OldDot next steps banner shows when this happens if you look at the corresponding expense report?

Just tested and there's no mismatch on OD side, only thing is that on OD we have to refresh the page to see the changes but the next step status is correct on OD compared to issues reported from testing on this branch.

I performed a npm ci again to make sure there's no cached code or anything like that.
I did a re-test and now test C. Self-approval seems to pass, meaning it matches OD behaviour:

C.mov

Now only test D. Other approver seems to behave weirdly, after the other approver navigates to the report:

D.mov

Note

If this issues are not reproducible on anybody else's side, neither author or CME, then I can approve as there might be something going on specifically on my side which causes the weird behaviour. Let me know!

[bernhardoj]

So, for test D, if I understand correctly, there are 2 issues:

1. When the other approver opens the submitted report, the next step still shows "These expenses are scheduled to automatically submit on Sunday! No further action required!" for a while. This is a BE problem where the pusher doesn't send the updated reportNextStep data.

The outdated next step in storage

The updated next step received from OpenReport

2. When the other approver approves the request, the next step shows "Waiting for you to pay these expenses" even though they are not the ones who pay the expense, and then the message changes to "No further action required!" which is the correct one.

The optimistic next step for approved action doesn't have a case when to show the "No further action required" case

App/src/libs/NextStepUtils.ts

Lines 273 to 322 in 64695c8

	case CONST.REPORT.STATUS_NUM.APPROVED:
	// Self review
	optimisticNextStep = {
	type,
	title: 'Next Steps:',
	message: [
	{
	text: 'Waiting for ',
	},
	{
	text: 'you',
	type: 'strong',
	},
	{
	text: ' to ',
	},
	{
	text: 'pay',
	type: 'strong',
	},
	{
	text: ' %expenses.',
	},
	],
	};
	// Another owner
	if (!isOwner) {
	optimisticNextStep.message = [
	{
	text: 'Waiting for ',
	},
	{
	text: managerDisplayName,
	type: 'strong',
	},
	{
	text: ' to ',
	},
	{
	text: 'pay',
	type: 'strong',
	},
	{
	text: ' %expenses.',
	},
	];
	}
	break;

This issue happens on the staging too.

Screen.Recording.2024-04-23.at.10.46.36.mov

[bernhardoj]

For the test C, I can reproduce what you reproduced earlier @ikevin127. Here is the root cause of each issue:

1. After submitting the request, we optimistically set the next step to "Waiting for you to review these expenses.", but the OpenReport request will return the next step as "These expenses are scheduled to automatically submit on Sunday! No further action required!".

How can this happen? If you open the report and quickly submit it before the OpenReport request completes, the outdated response from OpenReport will overwrite our optimistic next step.

2. The ApproveMoneyRequests response sets the next step to "No further action required!". I think this is a BE issue because we still need to pay the expense.

Both happens on main too.

[ikevin127]

@bernhardoj Thanks for checking!

Indeed for Test C it's on and off for me regarding the issue, sometimes it works as expected, sometimes next steps text jumps back.

As for Test D, if that happens on staging too and CME is fine with moving on in this state then that's fine with me.

@flodnv @trjExpensify Let me know and I'll complete the checklist and approve!

[mountiny]

The optimistic next step for approved action doesn't have a case when to show the "No further action required" case

This is probably expected, I believe that we did not have the dedicated reimburser logic yet when the next steps were built optimistically

Eitherway, what is coming from the BE should be correct after taking some action so we should make sure the optimistic steps match it. They did match it when it was build first time but we are adding more policy data/ conditions to the app throughout simplified collect so it could be that some conditions should have been updated along the way too

Dont forget there are unit tests for the next steps so please cover everything that is changing there

[trjExpensify]

What are the next steps here?

[bernhardoj]

We are not changing the next step in this PR, but just to make sure the next step still works as expected, but @ikevin127 found some issues with it that also exist on main. I think we need @flodnv confirmation on what to do with all of those issues. The root cause is explained here and here

[flodnv]

If both issues happen exactly the same way on main, then:

1. This PR did not cause it and they are just a disctraction
2. Let's report them as bugs

Right?

[bernhardoj]

I agree with that

cc: @ikevin127

[ikevin127]

Thanks everybody for chiming-in on this!

Based on the latest discussions it looks like we're fine moving on with this PR even though there were some issues with two of the test scenarios during testing, issues related to the next steps logic / incoming BE data.

These will be handled as separete issues and therefore won't be reported as regressions of this PR post-merge, that's all I wanted to confirm!

[flodnv]

Thanks @ikevin127 -- can you please report these in #expensify-bugs ?

[OSBotify]

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

[OSBotify]

🚀 Deployed to staging by https://github.com/flodnv in version: 1.4.66-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	failure ❌
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

[OSBotify]

🚀 Deployed to staging by https://github.com/flodnv in version: 1.4.66-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

[ikevin127]

Thanks @ikevin127 -- can you please report these in #expensify-bugs ?

@flodnv Reported in Slack 🧵, feel free to add more info in 🧵 if you think I missed anything.

[OSBotify]

🚀 Deployed to production by https://github.com/mountiny in version: 1.4.66-5 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

[eh2077]

@bernhardoj @ikevin127 Seems like this PR caused #41081, is it a regression of this PR or we should handle it separately?

Cc @flodnv

[bernhardoj]

It's an existing issue, so should be handled separately

[flodnv]

@bernhardoj in this PR we added another usage of policy.submitsTo here: https://github.com/Expensify/App/pull/39321/files#diff-577fcc5f3a5e4930916ff310e61c38edfa093792fe3ab55b9a30d2aecd8811dbR3487, 2 days before we merged this PR 😞 Can you please update that code to the newer version?

[bernhardoj]

@flodnv Sure, here is the PR

[flodnv]

Thank you!! 🙇