Add violation logic for multi level tags #37461

cead22 · 2024-02-29T01:53:06Z

Details

Fixed Issues

$ #37117
PROPOSAL:

Tests

In old dot, in the workspace editor > tags tab, switch on the Use multiple levels of tags toggle
Click import from spreadsheet, and upload the file in this issue description
On new dot, tap big plus sign > request money
Select Manual tab (from Manual | Scan | Distance)
Enter $15 as the amount amount and tap next
Tap the workspace from the pre-testing steps (tap the row, not the split button in the row)
Tap the merchant row and enter a merchant
Tap the Request $15 button
Confirm the report preview has RBR
Tap the report preview
Confirm the money request preview has RBR and shows “Review Required”
Tap the money request preview
- Verify you see Missing Region, Missing Project, and Missing Department violations
Tap the Region menu item, and select a region
- Verify that the Missing Region violation disappears right away
Repeat for Department and Project
Tap the Region menu item, and unselect the selected region
- Verify that the Missing Region violation appears right away
🛜 Switch to new dot and go offline
Repeat steps 1-16. The report preview, money request preview should be grayed out
Go online, and confirm everything looks the same, except nothing is grayed out
Click on the link in the transaction thread header to go back to the expense report
- Verify the money request preview says Missing Region next to the RBR

Verify that no errors appear in the JS console

Offline tests

Covered in tests

QA Steps

Same as tests

Verify that no errors appear in the JS console

PR Author Checklist

Note

I posted about console warnings here https://expensify.slack.com/archives/C01GTK53T8Q/p1709336500920559

Screenshots/Videos

Android: Native

nativeandroid.mp4

Android: mWeb Chrome

I'm having issues running the app on android web, so will rely on reviewer testing this platform

iOS: Native

nativeios.mp4

iOS: mWeb Safari

MacOS: Chrome / Safari

Online

desktopweboffline.mp4

Offline

https://github.com/Expensify/App/assets/165133/363e8b52-b5c9-4b36-b8b0-1ad9f3cb02b3 x

MacOS: Desktop

desktop.mp4

melvin-bot · 2024-03-05T00:57:42Z

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

cead22 · 2024-03-05T00:59:06Z

cc @yuwenmemon @allroundexperts since you reviewed #34983

cead22 · 2024-03-05T01:13:15Z

Sorry for the back and forth @parasharrajat , I thought I had linked to this PR in this issue #36441, which you had asked to be a reviewer on, but I had linked a different PR

DylanDylann · 2024-03-05T09:48:18Z

@cead22 I was assigned to this PR by Melvin. Should I review this PR?

allroundexperts · 2024-03-05T09:50:12Z

I think it would be better if I review this since I have good context on this.

cead22 · 2024-03-07T01:17:09Z

Updated

allroundexperts · 2024-03-07T11:45:14Z

src/libs/Violations/ViolationsUtils.ts

+                        newTransactionViolations = reject(newTransactionViolations, {
+                            name: CONST.VIOLATIONS.TAG_OUT_OF_POLICY,
+                        });


filter can be used here as well.

There's no difference here since we're only calling reject once, so it should go over the array only once, right?

Yes, but aren't we preferring native JS methods for filtering after TS migration?

allroundexperts

Looks good other than a NAB change that is upon the engineers discretion to fix.

melvin-bot · 2024-03-08T13:55:24Z

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

blimpich

NAB comments, otherwise looks good 👍

blimpich · 2024-03-08T17:20:02Z

src/libs/Violations/ViolationsUtils.ts

+                        errorIndexes.push(i);
+                    }
+                }
+                if (errorIndexes.length !== 0) {


NAB: since 0 is falsy in JS this can be simplified.

Suggested change

if (errorIndexes.length !== 0) {

if (errorIndexes.length) {

blimpich · 2024-03-08T17:25:43Z

tests/unit/ViolationUtilsTest.js

+                    required: true,
+                },
+            };
+        });


NAB: can we add a line break in between tests?

blimpich · 2024-03-08T17:25:49Z

tests/unit/ViolationUtilsTest.js

+            result = ViolationsUtils.getViolationsOnyxData(transaction, transactionViolations, policyRequiresTags, policyTags, policyRequiresCategories, policyCategories);
+            expect(result.value).toEqual([]);
+        });
+        it('should return tagOutOfPolicy when a tag is not enabled in the policy but is set in the transaction', () => {


NAB: can we add a line break in between tests?

blimpich · 2024-03-08T17:26:16Z

tests/unit/ViolationUtilsTest.js

@@ -201,4 +196,77 @@ describe('getViolationsOnyxData', () => {
            expect(result.value).not.toContainEqual([missingTagViolation]);
        });
    });
+    describe('policy has multi level tags', () => {


NAB: can we add a line break in between describe statements?

blimpich · 2024-03-08T17:31:10Z

src/libs/Violations/ViolationsUtils.ts

+                            hasInvalidTag = true;
+                            break;
+                        }
+                    }


NAB Can we add some line breaks to the new code for readability?

blimpich · 2024-03-08T17:51:31Z

src/libs/Violations/ViolationsUtils.ts

@@ -78,6 +79,53 @@ const ViolationsUtils = {
                if (!hasMissingTagViolation && !updatedTransaction.tag && policyRequiresTags) {
                    newTransactionViolations.push({name: CONST.VIOLATIONS.MISSING_TAG, type: 'violation'});
                }
+            } else {


NAB: can we consider extracting some part of this new code into a helper function? I find the logic hard to follow with all the if/else/for statements nested together. Abstracting out some of the logic would make this a lot more readable IMO.

cead22 · 2024-03-08T19:39:08Z

Updated! Please re-review. @blimpich I'm taking the NAB that I didn't reply to as non-blockers

I kinda like the explicitness of errorIndexes.length !== 0
Our style guide says not to add blank lines to separate code, and only add blank lines that are going to be followed by comments. Is searched usages of it and describe and it looks like we have a combination of having blank lines and no blank lines

blimpich · 2024-03-08T20:02:02Z

Sounds good! @cead22 can you help me find that style guide? I can't find one that mentions that we prefer to not use newlines to separate code.

OSBotify · 2024-03-08T20:09:06Z

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

cead22 · 2024-03-09T00:01:09Z

Yes, but aren't we preferring native JS methods for filtering after TS migration?

I meant to address this but forgot 😬

cead22 · 2024-03-09T00:04:09Z

@blimpich I thought it was in our general style guides, but it looks like it's on our php style guide https://github.com/Expensify/Style-Guides/blob/4a2750183223b1f84398e0c1c633fa95a7512841/php.md#blank-lines

Unnecessary blank lines without comments should be avoided

OSBotify · 2024-03-12T18:38:09Z

🚀 Deployed to production by https://github.com/luacmartins in version: 1.4.50-5 🚀

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

hoangzinh · 2024-06-20T22:58:01Z

src/libs/Violations/ViolationsUtils.ts

+/**
+ * Calculates some tag levels required and missing tag violations for the given transaction
+ */
+function getTagViolationsForMultiLevelTags(


Hi team, come from #38095, this function was missing to handle multilevel dependent tags cases.

Thanks! This wasn't really missing, as much as it just hadn't been implemented yet. When we added support for multi level tags, we broke the violations for tags, and this PR was meant to fix that.

We then added support for dependent tags, and then implemented getTagViolationsForDependentTags and getTagViolationForIndependentTags in https://github.com/Expensify/App/pull/40741/files

allroundexperts · 2024-10-20T22:08:34Z

src/libs/Violations/ViolationsUtils.ts

+    policyTagList: PolicyTagList,
+): TransactionViolation[] {
+    const policyTagKeys = Object.keys(policyTagList);
+    const selectedTags = updatedTransaction.tag?.split(CONST.COLON) ?? [];


We should have split the tags using getTagArrayFromName method instead. The split function fails if a tag contains more than one :. This caused #50105

cead22 added 5 commits February 28, 2024 15:38

Add violation logic for multi level tags

01281ed

Lint + TS

f0093d6

Check tagListIndex only for someTagLevelsRequired

7520f36

Add someTagLevelsRequired violation on the client

b17a7c9

Multi tag violations on the client

bf609e7

cead22 self-assigned this Feb 29, 2024

cead22 added 9 commits February 29, 2024 12:18

Merge branch 'main' into carlos-multitags

2dd37c1

Merge branch 'main' into carlos-multitags

1b2f767

Cleanup

5c1a77e

Merge branch 'main' into carlos-multitags

33f437a

Remove bad test data

3a36c34

Add tests for someTagLevelsRequired

38d0051

Add tagOutOfPolicyViolation testfor multi tags

2402537

Lint + Style

983e245

Remove unnecessary tag in test

6b4de30

roryabraham mentioned this pull request Mar 4, 2024

[$500] Tag - "Missing tag" error appears & disappears when change Category on created request #37644

Closed

6 tasks

Merge branch 'main' into carlos-multitags

954c3a7

cead22 marked this pull request as ready for review March 5, 2024 00:57

cead22 requested a review from a team as a code owner March 5, 2024 00:57

melvin-bot bot removed the request for review from a team March 5, 2024 00:57

melvin-bot bot requested a review from DylanDylann March 5, 2024 00:57

cead22 requested review from parasharrajat and removed request for parasharrajat March 5, 2024 01:09

cead22 requested review from allroundexperts and removed request for DylanDylann March 5, 2024 16:48

allroundexperts reviewed Mar 7, 2024

View reviewed changes

allroundexperts approved these changes Mar 8, 2024

View reviewed changes

melvin-bot bot requested a review from blimpich March 8, 2024 13:55

blimpich previously approved these changes Mar 8, 2024

View reviewed changes

cead22 added 4 commits March 8, 2024 10:05

Merge branch 'main' into carlos-multitags

f167b3a

Remove outdated comment

a47a77c

New getTagViolationsForSingleLevelTags

ff3bebb

New getTagViolationsForMultiLevelTags

32e20ed

cead22 dismissed blimpich’s stale review via c5e7087 March 8, 2024 18:58

Clean up

e77b01a

cead22 force-pushed the carlos-multitags branch from c5e7087 to e77b01a Compare March 8, 2024 19:25

blimpich approved these changes Mar 8, 2024

View reviewed changes

blimpich merged commit 4ada067 into main Mar 8, 2024
15 checks passed

blimpich deleted the carlos-multitags branch March 8, 2024 20:03

github-actions bot mentioned this pull request Mar 11, 2024

Deploy Checklist: New Expensify 2024-03-11 #38037

Closed

49 tasks

grgia mentioned this pull request Mar 11, 2024

[HOLD for payment 2024-04-15] [Violations] [$500] Multilevel tags - "no longer valid" violation is shown after selecting all the tags #38044

Closed

6 tasks

kavimuru mentioned this pull request Mar 12, 2024

Tag - Violation in expense preview does not show specific tag next to RBR #38124

Closed

6 tasks

melvin-bot bot mentioned this pull request Mar 12, 2024

[HOLD for payment 2024-03-19] [Violations] Implement violations for mutiple tag lists #37117

Closed

hoangzinh reviewed Jun 20, 2024

View reviewed changes

hoangzinh mentioned this pull request Jun 20, 2024

[HOLD for payment 2024-06-20] [$250] [Violations] Tags - Violation for unselected dependent tags displays tag name briefly then changes to "tag" #38095

Closed

6 tasks

allroundexperts reviewed Oct 20, 2024

View reviewed changes

allroundexperts mentioned this pull request Oct 20, 2024

[$250] [HOLD for payment 2024-10-24] QBO - Some Customer/projects values are trigger an error that disappears #50105

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add violation logic for multi level tags #37461

Add violation logic for multi level tags #37461

cead22 commented Feb 29, 2024 •

edited

Loading

melvin-bot bot commented Mar 5, 2024

cead22 commented Mar 5, 2024

cead22 commented Mar 5, 2024

DylanDylann commented Mar 5, 2024

allroundexperts commented Mar 5, 2024

cead22 commented Mar 7, 2024

allroundexperts Mar 7, 2024

cead22 Mar 8, 2024

allroundexperts Mar 8, 2024

allroundexperts left a comment

melvin-bot bot commented Mar 8, 2024

blimpich left a comment

blimpich Mar 8, 2024

blimpich Mar 8, 2024

blimpich Mar 8, 2024

blimpich Mar 8, 2024

blimpich Mar 8, 2024

blimpich Mar 8, 2024

cead22 commented Mar 8, 2024

blimpich commented Mar 8, 2024 •

edited

Loading

OSBotify commented Mar 8, 2024

cead22 commented Mar 9, 2024

cead22 commented Mar 9, 2024 •

edited

Loading

OSBotify commented Mar 12, 2024

hoangzinh Jun 20, 2024

cead22 Jun 21, 2024

allroundexperts Oct 20, 2024

Add violation logic for multi level tags #37461

Add violation logic for multi level tags #37461

Conversation

cead22 commented Feb 29, 2024 • edited Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

melvin-bot bot commented Mar 5, 2024

cead22 commented Mar 5, 2024

cead22 commented Mar 5, 2024

DylanDylann commented Mar 5, 2024

allroundexperts commented Mar 5, 2024

cead22 commented Mar 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allroundexperts left a comment

Choose a reason for hiding this comment

melvin-bot bot commented Mar 8, 2024

blimpich left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cead22 commented Mar 8, 2024

blimpich commented Mar 8, 2024 • edited Loading

OSBotify commented Mar 8, 2024

cead22 commented Mar 9, 2024

cead22 commented Mar 9, 2024 • edited Loading

OSBotify commented Mar 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cead22 commented Feb 29, 2024 •

edited

Loading

blimpich commented Mar 8, 2024 •

edited

Loading

cead22 commented Mar 9, 2024 •

edited

Loading