Tags - Violation for unselected dependent tags displays tag name briefly then changes to "tag" #40741

pac-guerreiro · 2024-04-23T02:34:44Z

Details

Fixed Issues

$ #38095
PROPOSAL:

Determine tag violations for dependent multilevel tags on the client side, based one the data coming from the server. This is because the server data doesn't include the name of the tags that are missing, so we have to add them on the client side.

Tests

Create a policy in old dot with multi level dependent tags. You can use this tags file dependent_tags.csv
Click on Submit expense in a workspace you just created
Select manual and enter any valid amount
Fill the required fields while leaving the tag fields empty and create the expense
Click on the preview of the expense you just created
The tag fields that you didn't fill should display an error message - missing {tagName}, where tagName is the name of the tag field
Wait a bit (1 min or so) until the app is refreshed with updated data from the server
Confirm that the same error messages are showing, and not missing tag on every tag field
Then, pick an option for a tag field that is missing and confirm that missing {tagName} is replaced by all tags required on every tag fields

Offline tests

Scenario A - Violations are shown correctly after synching with the server

Do steps from 1 to 5
Become online again, then do step 6
Do the remaining steps

Scenario B - Violations are shown correctly while offline

Do steps from 1 to 5
Do step 8

QA Steps

Same as tests

PR Author Checklist

Screenshots/Videos

Android: Native

Android.-.Native.mp4

Android: mWeb Chrome

Android.-.Chrome.mp4

iOS: Native

iOS.-.Native.mp4

iOS: mWeb Safari

iOS.-.Safari.mp4

MacOS: Chrome / Safari

MacOS.-.Chrome.mp4

MacOS: Desktop

MacOS.-.Desktop.mp4

…ion-for-multi-level-dependencies

cead22 · 2024-04-23T03:34:20Z

I think we should update and move a few things, let me know what you think

Policy.ts
- Add hasDependentTags = () => hasMultiLevelTagList && _.some(tagLists, (tagList) => _.some(tagList.tagListData.tags, (tag) => boolean(tag.rules?.parentTagsFilter? || tag.parentTagsFilter?)
  - rules is an object and parentTagsFilter is a string, both optional
MoneyRequestView
- Update getErrorForField('tag', {tagListIndex: index, tagListName: name}) to getErrorForField('tag', policy.hasDependentTags ? {tagListIndex: index, tagListName: name, policyHasDependentTagLists: true} : {tagListIndex: index, tagListName: name})
useViolations
- Shouldn't need new params
- Similar to someTagLevelsRequired and tagOutOfPolicy, only one of the three -- including missingTag can happen at a time
- Given that, we can add another check like currentViolations[0]?.data?.policyHasDependentTagLists? && currentViolations[0]?.name === 'missingTag' && currentViolations[0]?.data?.tagName == data?.tagListName with an early return like the ones we have for someTagLevelsRequired and tagOutOfPolicy
ViolationsUtils
- We shouldn't need to export getTagViolationsForSingleLevelTags/getTagViolationsForMultiLevelTags
- Update getTagViolationsForMultiLevelTags
  - We can pass a new bool param hasDependentTags -- which we'd need to pass to getViolationsOnyxData
  - if hasDependentTags is false, we run the current code in getTagViolationsForMultiLevelTags
    - Create getViolationsForDependentTags/getViolationsForIndependentTags if you think it's a good idea to split this functionality
  - Otherwise
    - If (!updatedTransaction.tag)
      - Iterate over the array of tagLists
        
        Push {name: CONST.VIOLATIONS.MISSING_TAG, type: CONST.VIOLATION_TYPES.VIOLATION, data: {tagName: tagList.name}} to the newTransactionViolations
      - return newTransactionViolations

pac-guerreiro · 2024-04-23T18:49:09Z

@cead22

I applied your suggestions locally and the correct data is being stored optimistically, but the API overwrites it 😞

Optimistic data:

API data:

This is why I used the same logic that we use to generate the optimistic violations after the data is fetched from the API

cead22 · 2024-04-23T21:49:33Z

Yeah, that also happens today with the existing violations, but so long as we display the correct violations, that's fine. If we want to refactor this, I think we should do it in a separate issue.

I think we'll need a separate issue to handle the allTagLevelsRequired violation case for dependent tags in getViolationsOnyxData, cause I didn't want to include it in my previous comment to avoid adding scope to this PR that isn't related to the linked issue

…ion-for-multi-level-dependencies

pac-guerreiro · 2024-04-24T21:01:37Z

@cead22

They are displayed correctly until the API data overwrites the local data, thus causing the bug again 😅

I think this needs to be handled from the backend side!

…ion-for-multi-level-dependencies

pac-guerreiro · 2024-04-25T21:19:36Z

@cead22

Just pushed the fix, let me know if there's anything else that needs polishing 😄

cead22

Code looks good. Let's add some tests too!

src/hooks/useViolations.ts

src/libs/PolicyUtils.ts

src/libs/Violations/ViolationsUtils.ts

src/libs/actions/IOU.ts

src/types/onyx/PolicyTag.ts

src/types/onyx/TransactionViolation.ts

…ion-for-multi-level-dependencies # Conflicts: # src/libs/PolicyUtils.ts

pac-guerreiro · 2024-04-29T21:26:24Z

@cead22

All feedback addressed and test case added! 😄

cead22

Code is looking good! Let's add manual tests

src/hooks/useViolations.ts

src/libs/Violations/ViolationsUtils.ts

cead22 · 2024-05-03T00:33:50Z

@pac-guerreiro do you have an ETA for this PR?

…ion-for-multi-level-dependencies # Conflicts: # src/libs/PolicyUtils.ts

pac-guerreiro · 2024-05-03T18:23:39Z

@cead22

Feedback applied, added manual tests and filled PR author checklist.

Only things missing now are screen recordings 😄

…ion-for-multi-level-dependencies

cead22 · 2024-05-06T22:26:01Z

Ping me when ready for review :)

pac-guerreiro · 2024-05-07T08:07:17Z

@cead22

Sorry for the delay, I was having problems building the app on android but after an intensive cleaning process I was able to 😅

All screen recordings added! Feel free to review and let me know when I can set this PR as ready for review 😄

hoangzinh · 2024-06-04T16:00:21Z

The tag fields that you didn't fill should display an error message - missing {tagName}, where tagName is the name of the tag field

@pac-guerreiro This step failed on my end when in offline mode.

src/components/ReportActionItem/MoneyRequestView.tsx

src/hooks/useViolations.ts

src/libs/Violations/ViolationsUtils.ts

pac-guerreiro · 2024-06-04T22:29:26Z

@hoangzinh Strange, can you provide a screen recording while testing that step?

Here is a screen recording I took while doing the steps on my side:

Screen.Recording.2024-06-04.at.23.25.27.mp4

hoangzinh · 2024-06-04T23:41:39Z

@pac-guerreiro sorry I couldn't reproduce that bug anymore.

hoangzinh

LGTM. all yours @cead22

src/types/onyx/TransactionViolation.ts

src/components/ReportActionItem/MoneyRequestView.tsx

src/libs/Violations/ViolationsUtils.ts

cead22 · 2024-06-06T19:49:04Z

src/libs/Violations/ViolationsUtils.ts

    let newTransactionViolations = [...transactionViolations];
-    newTransactionViolations = newTransactionViolations.filter(


Why was this removed? I think we should keep it so we're not adding SOME_TAG_LEVELS_REQUIRED to an array of violations that already has it

@cead22 I removed it because we're already filtering those in the function that calls this one 😅

src/libs/Violations/ViolationsUtils.ts

…ion-for-multi-level-dependencies # Conflicts: # src/libs/Violations/ViolationsUtils.ts

…ion-for-multi-level-dependencies

rezkiy37 · 2024-06-07T16:07:17Z

src/hooks/useViolations.ts

@@ -79,8 +79,28 @@ function useViolations(violations: TransactionViolation[]) {
                    }));
            }

+            // missingTag has special logic for policies with dependent tags, because only one violation is returned for all tags
+            // when no tags are present, so the tag name isn't set in the violation data. That's why we add it here
+            if (policyHasDependentTags && currentViolations[0]?.name === CONST.VIOLATIONS.MISSING_TAG && data?.tagListName) {


Just a suggestion: currentViolations[0] is used several times so we can create a variable for this like firstViolation.

src/types/onyx/PolicyTag.ts

src/components/ReportActionItem/MoneyRequestView.tsx

src/libs/Violations/ViolationsUtils.ts

cead22

@hoangzinh can you give it another quick review and test please? If everything looks good, I'll merge it

hoangzinh · 2024-06-10T06:58:09Z

Looks good, tested again on Web & IOS

Screenshots/Videos

iOS: Native

Screen.Recording.2024-06-10.at.13.50.47.ios.mov

MacOS: Chrome / Safari

Screen.Recording.2024-06-10.at.13.29.30.web.mp4

OSBotify · 2024-06-11T22:27:41Z

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

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

OSBotify · 2024-06-13T15:30:17Z

🚀 Deployed to production by https://github.com/mountiny in version: 1.4.82-4 🚀

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

pac-guerreiro added 2 commits April 23, 2024 03:32

fix(money request): missing tags in violations translations

43e030b

Merge branch 'main' into pac-guerreiro/issue/38095-missing-tag-violat…

b9a8d90

…ion-for-multi-level-dependencies

pac-guerreiro mentioned this pull request Apr 23, 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

refactor: remove unexpected return

b9d8b24

Merge branch 'main' into pac-guerreiro/issue/38095-missing-tag-violat…

fcdc252

…ion-for-multi-level-dependencies

pac-guerreiro added 3 commits April 24, 2024 22:04

chore: apply PR suggestions

0a0cac1

Merge branch 'main' into pac-guerreiro/issue/38095-missing-tag-violat…

7a3db00

…ion-for-multi-level-dependencies

fix: tag name missing in error UI

a174c60

cead22 reviewed Apr 25, 2024

View reviewed changes

pac-guerreiro added 4 commits April 29, 2024 18:27

Merge branch 'main' into pac-guerreiro/issue/38095-missing-tag-violat…

47bf6f2

…ion-for-multi-level-dependencies # Conflicts: # src/libs/PolicyUtils.ts

refactor: apply pull request suggestions

72f8575

chore(typescript): add missing argument

5c7e16c

chore(tests): add violation case for missing dependent tags

3d6478f

pac-guerreiro requested a review from cead22 April 29, 2024 21:25

cead22 requested changes Apr 29, 2024

View reviewed changes

src/hooks/useViolations.ts Outdated Show resolved Hide resolved

src/hooks/useViolations.ts Outdated Show resolved Hide resolved

src/libs/Violations/ViolationsUtils.ts Outdated Show resolved Hide resolved

src/libs/Violations/ViolationsUtils.ts Outdated Show resolved Hide resolved

pac-guerreiro added 3 commits May 3, 2024 09:17

Merge branch 'main' into pac-guerreiro/issue/38095-missing-tag-violat…

0ab716d

…ion-for-multi-level-dependencies # Conflicts: # src/libs/PolicyUtils.ts

refactor: apply pull request suggestions

e297686

chore(typescript): add missing argument

a8ef1c3

pac-guerreiro requested a review from cead22 May 3, 2024 18:22

Merge branch 'main' into pac-guerreiro/issue/38095-missing-tag-violat…

5e36ed7

…ion-for-multi-level-dependencies

cead22 requested changes Jun 4, 2024

View reviewed changes

refactor: apply pull request suggestions

30189ca

pac-guerreiro requested a review from cead22 June 5, 2024 12:35

hoangzinh approved these changes Jun 5, 2024

View reviewed changes

cead22 requested changes Jun 6, 2024

View reviewed changes

pac-guerreiro added 3 commits June 7, 2024 15:02

Merge branch 'main' into pac-guerreiro/issue/38095-missing-tag-violat…

4cd7802

…ion-for-multi-level-dependencies # Conflicts: # src/libs/Violations/ViolationsUtils.ts

refactor: apply suggestions

492a672

refactor: remove unused argument

df3326a

pac-guerreiro requested a review from cead22 June 7, 2024 15:49

Merge branch 'main' into pac-guerreiro/issue/38095-missing-tag-violat…

e54d678

…ion-for-multi-level-dependencies

rezkiy37 reviewed Jun 7, 2024

View reviewed changes

src/types/onyx/PolicyTag.ts Show resolved Hide resolved

rezkiy37 reviewed Jun 7, 2024

View reviewed changes

src/components/ReportActionItem/MoneyRequestView.tsx Outdated Show resolved Hide resolved

refactor: apply suggestions

e7f175f

cead22 requested changes Jun 7, 2024

View reviewed changes

src/components/ReportActionItem/MoneyRequestView.tsx Outdated Show resolved Hide resolved

src/libs/Violations/ViolationsUtils.ts Outdated Show resolved Hide resolved

src/libs/Violations/ViolationsUtils.ts Outdated Show resolved Hide resolved

refactor: apply suggestions

df5e19f

pac-guerreiro requested a review from cead22 June 7, 2024 17:14

cead22 approved these changes Jun 7, 2024

View reviewed changes

cead22 merged commit 19f9424 into Expensify:main Jun 10, 2024
15 checks passed

kavimuru mentioned this pull request Jun 12, 2024

[$250] Tag - Dependent tag violation message isn't changing to "all tags required" #43574

Closed

6 tasks

lanitochka17 mentioned this pull request Jun 12, 2024

[$250] Tag - Sometimes the "missing {tagName}" error message returns on a chosen tag #43590

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tags - Violation for unselected dependent tags displays tag name briefly then changes to "tag" #40741

Tags - Violation for unselected dependent tags displays tag name briefly then changes to "tag" #40741

pac-guerreiro commented Apr 23, 2024 •

edited

Loading

cead22 commented Apr 23, 2024

pac-guerreiro commented Apr 23, 2024

cead22 commented Apr 23, 2024

pac-guerreiro commented Apr 24, 2024

pac-guerreiro commented Apr 25, 2024

cead22 left a comment

pac-guerreiro commented Apr 29, 2024

cead22 left a comment

cead22 commented May 3, 2024

pac-guerreiro commented May 3, 2024

cead22 commented May 6, 2024

pac-guerreiro commented May 7, 2024

hoangzinh commented Jun 4, 2024

pac-guerreiro commented Jun 4, 2024

hoangzinh commented Jun 4, 2024

hoangzinh left a comment

cead22 Jun 6, 2024

pac-guerreiro Jun 7, 2024

rezkiy37 Jun 7, 2024

cead22 left a comment

hoangzinh commented Jun 10, 2024

OSBotify commented Jun 11, 2024

OSBotify commented Jun 13, 2024

		let newTransactionViolations = [...transactionViolations];
		newTransactionViolations = newTransactionViolations.filter(

Tags - Violation for unselected dependent tags displays tag name briefly then changes to "tag" #40741

Tags - Violation for unselected dependent tags displays tag name briefly then changes to "tag" #40741

Conversation

pac-guerreiro commented Apr 23, 2024 • edited Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

cead22 commented Apr 23, 2024

pac-guerreiro commented Apr 23, 2024

cead22 commented Apr 23, 2024

pac-guerreiro commented Apr 24, 2024

pac-guerreiro commented Apr 25, 2024

cead22 left a comment

Choose a reason for hiding this comment

pac-guerreiro commented Apr 29, 2024

cead22 left a comment

Choose a reason for hiding this comment

cead22 commented May 3, 2024

pac-guerreiro commented May 3, 2024

cead22 commented May 6, 2024

pac-guerreiro commented May 7, 2024

hoangzinh commented Jun 4, 2024

pac-guerreiro commented Jun 4, 2024

hoangzinh commented Jun 4, 2024

hoangzinh left a comment

Choose a reason for hiding this comment

cead22 Jun 6, 2024

Choose a reason for hiding this comment

pac-guerreiro Jun 7, 2024

Choose a reason for hiding this comment

rezkiy37 Jun 7, 2024

Choose a reason for hiding this comment

cead22 left a comment

Choose a reason for hiding this comment

hoangzinh commented Jun 10, 2024

Screenshots/Videos

OSBotify commented Jun 11, 2024

OSBotify commented Jun 13, 2024

pac-guerreiro commented Apr 23, 2024 •

edited

Loading