-
Notifications
You must be signed in to change notification settings - Fork 3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Violations] Update tagOutOfPolicy and taxOutOfPolicy #35048
[Violations] Update tagOutOfPolicy and taxOutOfPolicy #35048
Conversation
We update the copy to use 'Tag' when tagName is not available
We update the copy to use 'Tax' when taxName is not available
Please update tests to include both cases: the single level tags, and the multi level tags. To setup multi level tags you can follow these instructions https://community.expensify.com/discussion/5756/how-to-set-up-and-manage-multi-level-tagging?utm_source=community-search&utm_medium=organic-search&utm_term=independent+tag, and use this file That file will give the tag lists a name, which should produce violations like "Department no longer valid" Also note that we're currently not sending the data array |
Code looks good but I'm unable to test as I'm not getting any violations errors. I asked for help on Slack |
Reviewer Checklist
Screenshots/Videos |
@neonbhai Please tag me once the PR is updated (added new testing steps and missing screenshots, complete the checklist) |
@neonbhai Any updates here? |
Bump @neonbhai! I also pinged you on #expensify-open-source |
Hi, sorry for delay, I have updated the PR! Noting a few things:
|
No, it's just that multiple tags isn't currently supported in new dot, so what you're seeing is what's expected. We also have to update some API commands so when you update a request, violations clear automatically (which is something I'm working on)
I think I have an issue for this as well -- but I'll look into it, but let's not block this PR on that |
@neonbhai Can you raise the translation question in Slack? |
The translations are good, let's not block on this |
@neonbhai Please apply the same fix for the Spanish translation and we should be good to merge |
@cead22 looks like the backend was updated to show So currently it shows as: Is this good? |
Depends, can you share the policy setup, to see if it has multiple levels of tags? |
Here's the policy setup: It does not have multiple levels of tag. VideoThe Policy ID is `484377E4416A2AFE` if that's helpfulScreen.Recording.2024-02-08.at.7.57.23.AM.movFor Multiple TagsThe violations are now shown as Screen.Recording.2024-02-08.at.8.00.47.AM.movThis is the Line 2168 in 9f37d3e
|
Yeah that looks good, and I submitted a back end fix so when the tagName is |
@neonbhai Please clarify the expected results in the testing steps |
Tests Updated! @s77rt |
@neonbhai In step 12 you are referring to the wrong step (should be 8 not 10) |
ah right, fixed! |
I don't think that's the case yet, still getting |
Right, BE change doesn't seem to be live yet. Updated tests! |
@cead22 all yours! |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/cead22 in version: 1.4.40-0 🚀
|
🚀 Deployed to production by https://github.com/mountiny in version: 1.4.40-5 🚀
|
Details
We update the violation messages for
tagOutOfPolicy
andtaxOutOfPolicy
Fixed Issues
$ #34539
PROPOSAL: #34539 (comment)
Tests
Prerequisite:
For Tag Out Of Policy:
8
Tag no longer valid
For Tax Out Of Policy:
We cannot test the
taxOutOfPolicy
violation as we do not yet render it in MoneyRequestView.Offline tests
Violations for a new iou will not be received when offline as they are sent from the backend.
QA Steps
Same as Tests step.
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop