Adds Client-side Violations when creating money requests #32528

cdanwards · 2023-12-05T21:09:34Z

Fixed Issues

$ #31092
PROPOSAL:

This is another addition to bringing the Violations functionality to New Expensify. This PR addresses building the required client side onyx data for violations when creating a money request.

Tests

General Tests

Verify that no errors appear in the JS console

Setup:

Follow the instructions here to set up a policy with a workspace in New Dot.
Within the policy in Old Dot, navigate Settings > Categories and verify that People must categorize expenses is enabled, and make sure there's at least one category on the policy
Within the policy in Old Dot, navigate Settings > Tags and verify that People must tag expenses is enabled, and make sure there's at least one tag enabled on the policy

Testing the code worked correctly

Navigate to the ONYXDB -> keyvaluepairs within your browser storage tab
Identify the transaction that was created with the dollar amount you specified, should look like transactions_someID. If you open chrome dev tools before making the money request, you can inspect the response which should have the transactionID
Verify there's a transaction violations object with the corresponding ID like transactionViolations_SameIDasTransaction, with the missingCategory and missingTag violations in the array (unless the test says otherwise)

What to test

Create a manual money request and request money from your workspace with the policy you set up earlier. You should see the missingCategory and missingTag violations. Add a category and a tag, and confirm the violations are gone
Create a distance money request and request money from your workspace with the policy you set up earlier. You should see the missingCategory and missingTag violations. Add a category and a tag, and confirm the violations are gone
Create a scan request and request money from your test workspace. You should see the missingCategory and missingTag violations. Add a category and a tag, and confirm the violations are gone
Create a manual money request and request money from another person instead of a workspace. You should NOT see a corresponding transactionViolations_ object.
Create a distance money request and request money from another person instead of a workspace. You should NOT see a corresponding transactionViolations_ object.
Create a scan money request and request money from another person instead of a workspace. You should NOT see a corresponding transactionViolations_ object.

Offline tests

For each test that generates a transactionViolations_ object, test it offline as well. The results should be the same.

QA Steps

Same as tests, both online and offline

PR Author Checklist

Screenshots/Videos

No visible changes are expected at this point; violations are not yet displayed in the app.

Android: Native

Android: mWeb Chrome

iOS: Native

iOS: mWeb Safari

MacOS: Chrome / Safari

MacOS: Desktop

… functions

…m:infinitered/ExpensifyApp into cdanwards/violations/money-request-violations

…ards/violations/money-request-violations

in IOU.js

…ards/violations/money-request-violations

melvin-bot · 2023-12-08T17:01:29Z

@mollfpr 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]

cdanwards · 2023-12-08T17:01:58Z

@cead22 This is ready for review!

cead22

@mollfpr please review this and complete the reviewer checklist when you get a chance

src/pages/workspace/withPolicy.tsx

src/pages/iou/steps/MoneyRequestConfirmPage.js

cead22 · 2023-12-08T20:53:18Z

src/pages/iou/steps/MoneyRequestConfirmPage.js

-    }),
-    // eslint-disable-next-line rulesdir/no-multiple-onyx-in-file
-    withOnyx({


Why remove this? I think this is wrong, because having a separate withOnyx makes the keys in it "dependent" on the values of the previous withOnyx, right?

I wasn't aware of this! I'll ask in the open-source channel, but I'm happy to change this back.

cead22 · 2023-12-08T20:53:38Z

src/pages/iou/steps/MoneyRequestConfirmPage.js

        policy: {
            key: ({report}) => `${ONYXKEYS.COLLECTION.POLICY}${report ? report.policyID : '0'}`,
        },
+        policyCategories: {
+            key: ({policy}) => `${ONYXKEYS.COLLECTION.POLICY_CATEGORIES}${policy ? policy.id : '0'}`,


When/why can this be called without a policy?

So it looks that when this file is converted to TS that the policy will be of the OnyxEntry type which could return null so this was just to go ahead and account for that. Once it is converted it can easily just be replaced with optional chaining like policy?.id.

I asked about the multiple withOnyx thing from the comment above on slack

So it looks that when this file is converted to TS that the policy will be of the OnyxEntry type which could return null so this was just to go ahead and account for that

But is it possible that this is because these are no longer defined in a separate withOnyx?

src/libs/actions/IOU.js

Co-authored-by: Carlos Alvarez <[email protected]>

mollfpr

Leave some suggestions based on the policy keys.

@cdanwards I can't access the Slack link to see the instruction, could you put it here? Thanks!

src/pages/workspace/withPolicy.tsx

src/libs/actions/IOU.js

cdanwards · 2023-12-11T15:52:29Z

@mollfpr no worries! Let me grab that slack conversation.

Co-authored-by: Luthfi <[email protected]>

cead22 · 2023-12-21T22:09:25Z

@mollfpr can you please re-review, re-test this when you get a chance?

mollfpr · 2023-12-27T14:41:03Z

Add a category and a tag, and confirm the violations are gone

@cead22 @lindboe Regarding the above step, is this on editing the request or creating a new request? I don't see it removing the transactionViolations_SameIDasTransaction from the previous request.

…s/money-request-violations

lindboe · 2024-01-03T20:14:39Z

@mollfpr This should be reviewable now!

mollfpr · 2024-01-04T15:38:29Z

Reviewer Checklist

Screenshots/Videos

Android: Native

Android: mWeb Chrome

32528.mWeb-Chrome.-.1.mp4

32528.mWeb-Chrome.-.2.mp4

32528.mWeb-Chrome.-.3.mp4

32528.mWeb-Chrome.-.4.-.6.mp4

iOS: Native

32528.iOS.-.4.-.6.mp4

iOS: mWeb Safari

32528.mWeb-Safari.-.1.mp4

32528.mWeb-Safari.-.2.mp4

32528.mWeb-Safari.-.3.mp4

32528.mWeb-Safari.-.4.-.6.mp4

MacOS: Chrome / Safari

32528.Web.-.1.mp4

32528.Web.-.2.mp4

32528.Web.-.3.mp4

32528.Web.-.4.mp4

32528.Web.-.1.Offline.mp4

32528.Desktop.-.6.mp4

MacOS: Desktop

32528.Desktop.-.1.mp4

32528.Desktop.-.2.mp4

32528.Desktop.-.3.mp4

32528.Desktop.-.4.mp4

32528.Desktop.-.6.mp4

mollfpr

All yours @cead22

…s/money-request-violations

cead22 · 2024-01-05T00:37:42Z

Seeing Invalid prop reportActionsof typeobjectsupplied toReportScreen, expected an array which was brought up here https://expensify.slack.com/archives/C02NK2DQWUX/p1700167185955379?thread_ts=1700167075.099769&cid=C02NK2DQWUX, and it exists in main along with several others errors

cead22 · 2024-01-05T00:40:17Z

The Android videos were outdated, and I tried running the app on android but I was getting an error. @mollfpr ran this on native iOS (sim) after merging main into this branch (here) and things worked. I also tested on web and things worked, so I'm merging to avoid this getting more conflicts tomorrow

OSBotify · 2024-01-05T00:44:43Z

✋ 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 · 2024-01-09T05:35:45Z

🚀 Deployed to staging by https://github.com/cead22 in version: 1.4.23-0 🚀

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

kavimuru · 2024-01-09T07:14:58Z

@cdanwards Could you help us with the steps 4,5 and 6 in "What to test" section?

mollfpr · 2024-01-09T07:19:03Z

@kavimuru May I help you with the problem?

kavimuru · 2024-01-09T13:43:18Z

@mollfpr where do we have to check "You should NOT see a corresponding transactionViolations_ object.

mollfpr · 2024-01-09T13:55:53Z

@kavimuru The object is on the browser storage, so we can only verify it on the Web. You can see the below video on how to get there.

Screen.Recording.2024-01-09.at.20.52.27.mp4

OSBotify · 2024-01-10T06:03:00Z

🚀 Deployed to production by https://github.com/thienlnam in version: 1.4.23-4 🚀

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

cdanwards · 2024-01-10T14:20:48Z

@kavimuru @mollfpr Let me know if you require any more assistance! I've been out on parental leave but am now back to work.

eh2077 · 2024-04-09T06:27:59Z

src/libs/actions/IOU.js

+        return [optimisticData, successData, failureData];
+    }
+
+    const violationsOnyxData = ViolationsUtils.getViolationsOnyxData(transaction, [], policy.requiresTag, policyTags, policy.requiresCategory, policyCategories);


This PR caused a regression #38131 as we don't want to show those violations when a transaction is a "partial transaction", see also #38131 (comment)

So, we need to add a check for partial transactions when adding violations data.

cdanwards added 12 commits November 21, 2023 11:23

Possible solution to not passing down tags and categories

e4135d1

Updated withPolicy with new properties and also passed through to IOU…

8fe667d

… functions

Added some logs for testing -- remove these

1cd0e08

Fixed merge conflicts

22ee011

Merge branch 'trevor-coleman/violations/violation-utils' of github.co…

441788c

…m:infinitered/ExpensifyApp into cdanwards/violations/money-request-violations

Fixed merge conflicts

0cf233c

remove unused export

6f84a87

Merge branch 'main' of github.com:infinitered/ExpensifyApp into cdanw…

57f2aa8

…ards/violations/money-request-violations

Remove console.log statements and fix import path

f6f9cac

in IOU.js

Remove unused function getPolicyTags

9a041cc

Fixed withOnyx call

150487e

Merge branch 'main' of github.com:infinitered/ExpensifyApp into cdanw…

5b8bcc5

…ards/violations/money-request-violations

cdanwards mentioned this pull request Dec 6, 2023

Adds Client-side Violations when creating money requests infinitered/ExpensifyApp#81

Closed

5 tasks

Merge branch 'main' of github.com:infinitered/ExpensifyApp into cdanw…

b2128e5

…ards/violations/money-request-violations

cdanwards marked this pull request as ready for review December 8, 2023 17:01

cdanwards requested a review from a team as a code owner December 8, 2023 17:01

melvin-bot bot requested review from mollfpr and removed request for a team December 8, 2023 17:01

cead22 requested changes Dec 8, 2023

View reviewed changes

cdanwards and others added 3 commits December 11, 2023 10:39

Update src/libs/actions/IOU.js

6c30158

Co-authored-by: Carlos Alvarez <[email protected]>

Update src/pages/workspace/withPolicy.tsx

705329c

Co-authored-by: Carlos Alvarez <[email protected]>

Update src/pages/iou/steps/MoneyRequestConfirmPage.js

c93a59d

Co-authored-by: Carlos Alvarez <[email protected]>

mollfpr suggested changes Dec 11, 2023

View reviewed changes

src/pages/workspace/withPolicy.tsx Outdated Show resolved Hide resolved

src/pages/workspace/withPolicy.tsx Outdated Show resolved Hide resolved

src/pages/workspace/withPolicy.tsx Outdated Show resolved Hide resolved

src/libs/actions/IOU.js Outdated Show resolved Hide resolved

cdanwards and others added 4 commits December 11, 2023 10:53

Update src/pages/workspace/withPolicy.tsx

b6e959f

Co-authored-by: Luthfi <[email protected]>

Update src/pages/workspace/withPolicy.tsx

f0c609b

Co-authored-by: Luthfi <[email protected]>

Update src/pages/workspace/withPolicy.tsx

dbf3e5a

Co-authored-by: Luthfi <[email protected]>

Update src/libs/actions/IOU.js

9783c47

Co-authored-by: Luthfi <[email protected]>

This comment was marked as resolved.

Sign in to view

lindboe added 3 commits January 2, 2024 14:14

Merge remote-tracking branch 'upstream/main' into cdanwards/violation…

34c080e

…s/money-request-violations

Use policy arg for fetching paid policy info

ab06fbe

Merge remote-tracking branch 'upstream/main' into cdanwards/violation…

da3469e

…s/money-request-violations

This comment was marked as duplicate.

Sign in to view

mollfpr approved these changes Jan 4, 2024

View reviewed changes

melvin-bot bot requested a review from cead22 January 4, 2024 15:40

mollfpr approved these changes Jan 4, 2024

View reviewed changes

Merge remote-tracking branch 'upstream/main' into cdanwards/violation…

9b2fb39

…s/money-request-violations

cead22 approved these changes Jan 5, 2024

View reviewed changes

cead22 merged commit 095161a into Expensify:main Jan 5, 2024
15 checks passed

github-actions bot mentioned this pull request Jan 9, 2024

Deploy Checklist: New Expensify 2024-01-09 #34121

Closed

87 tasks

melvin-bot bot mentioned this pull request Jan 10, 2024

[HOLD for payment 2024-01-17] Implement Client-side Violations when creating money requests #31092

Closed

lindboe mentioned this pull request Jan 17, 2024

Client-side violations for money request updates #34402

Merged

50 tasks

eh2077 reviewed Apr 9, 2024

View reviewed changes

eh2077 mentioned this pull request Apr 9, 2024

[HOLD for payment 2024-04-15] [Violations][$500] Scan - Category row shows violation briefly after the scan request is created #38131

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds Client-side Violations when creating money requests #32528

Adds Client-side Violations when creating money requests #32528

cdanwards commented Dec 5, 2023 •

edited by cead22

Loading

melvin-bot bot commented Dec 8, 2023

cdanwards commented Dec 8, 2023

cead22 left a comment

cead22 Dec 8, 2023

cdanwards Dec 11, 2023

cead22 Dec 8, 2023

cdanwards Dec 11, 2023

cead22 Dec 11, 2023

mollfpr left a comment •

edited

Loading

cdanwards commented Dec 11, 2023

cead22 commented Dec 21, 2023

This comment was marked as resolved.

mollfpr commented Dec 27, 2023

lindboe commented Jan 3, 2024

mollfpr commented Jan 4, 2024 •

edited by cead22

Loading

This comment was marked as duplicate.

mollfpr left a comment

cead22 commented Jan 5, 2024

cead22 commented Jan 5, 2024

OSBotify commented Jan 5, 2024

OSBotify commented Jan 9, 2024

kavimuru commented Jan 9, 2024

mollfpr commented Jan 9, 2024

kavimuru commented Jan 9, 2024

mollfpr commented Jan 9, 2024

OSBotify commented Jan 10, 2024

cdanwards commented Jan 10, 2024

eh2077 Apr 9, 2024 •

edited

Loading

Adds Client-side Violations when creating money requests #32528

Adds Client-side Violations when creating money requests #32528

Conversation

cdanwards commented Dec 5, 2023 • edited by cead22 Loading

Fixed Issues

Tests

General Tests

Setup:

Testing the code worked correctly

What to test

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

melvin-bot bot commented Dec 8, 2023

cdanwards commented Dec 8, 2023

cead22 left a comment

Choose a reason for hiding this comment

cead22 Dec 8, 2023

Choose a reason for hiding this comment

cdanwards Dec 11, 2023

Choose a reason for hiding this comment

cead22 Dec 8, 2023

Choose a reason for hiding this comment

cdanwards Dec 11, 2023

Choose a reason for hiding this comment

cead22 Dec 11, 2023

Choose a reason for hiding this comment

mollfpr left a comment • edited Loading

Choose a reason for hiding this comment

cdanwards commented Dec 11, 2023

cead22 commented Dec 21, 2023

This comment was marked as resolved.

mollfpr commented Dec 27, 2023

lindboe commented Jan 3, 2024

mollfpr commented Jan 4, 2024 • edited by cead22 Loading

Reviewer Checklist

Screenshots/Videos

This comment was marked as duplicate.

mollfpr left a comment

Choose a reason for hiding this comment

cead22 commented Jan 5, 2024

cead22 commented Jan 5, 2024

OSBotify commented Jan 5, 2024

OSBotify commented Jan 9, 2024

kavimuru commented Jan 9, 2024

mollfpr commented Jan 9, 2024

kavimuru commented Jan 9, 2024

mollfpr commented Jan 9, 2024

OSBotify commented Jan 10, 2024

cdanwards commented Jan 10, 2024

eh2077 Apr 9, 2024 • edited Loading

Choose a reason for hiding this comment

cdanwards commented Dec 5, 2023 •

edited by cead22

Loading

mollfpr left a comment •

edited

Loading

mollfpr commented Jan 4, 2024 •

edited by cead22

Loading

eh2077 Apr 9, 2024 •

edited

Loading