-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Update MoneyRequestView.js to show Violations #32594
Conversation
…n string instead of array.
…onNameForField for clarity
…`useViolations()`
…will trigger a re-render
…-fields # Conflicts: # src/ONYXKEYS.ts # src/components/ReportActionItem/MoneyRequestView.js # src/languages/en.ts
…module from `violation-utils` branch
…w, and move translation to the display component.
…equest-view-fields # Conflicts: # package-lock.json # src/ONYXKEYS.ts # src/languages/en.ts # src/libs/Violations/ViolationsUtils.ts # src/libs/Violations/useViolations.ts # src/types/onyx/TransactionViolation.ts
…ations/money-request-view-fields
…equest-view-fields
Resolved conflicts. |
@trevor-coleman please complete missing checklist |
Main merged, conflicts resolved, translations updated, checkboxes checked. I think this is good to go. |
# Conflicts: # src/libs/ViolationsUtils.ts # src/types/onyx/index.ts
✋ 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.22-0 🚀
|
🚀 Deployed to staging by https://github.com/cead22 in version: 1.4.22-0 🚀
|
🚀 Deployed to production by https://github.com/thienlnam in version: 1.4.22-6 🚀
|
@@ -349,5 +403,15 @@ export default compose( | |||
return `${ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`; | |||
}, | |||
}, | |||
transactionViolation: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there's a typo here which I'm fixing here if you can review please @trevor-coleman . I was able to see the violations once I fixed it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! Let me review
}, | ||
smartscanFailed: 'Receipt scanning failed. Enter details manually.', | ||
someTagLevelsRequired: 'Missing tag', | ||
tagOutOfPolicy: ({tagName}: ViolationsTagOutOfPolicyParams) => `${tagName ?? ''} no longer valid`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If tagName
is not available we should have fallen back to Tag
instead of an empty string for consistency with categoryOutOfPolicy
and also because no longer valid
message on its own is not clear
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same for tax
@@ -349,5 +403,15 @@ export default compose( | |||
return `${ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`; | |||
}, | |||
}, | |||
transactionViolation: { | |||
key: ({report}) => { | |||
const parentReportAction = ReportActionsUtils.getParentReportAction(report); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@trevor-coleman ReportActionsUtils.getParentReportAction
is a deprecated method. I'm trying to remove these in #27262. Can you please create a PR to clean this up and remove the usage of the deprecated method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, I was a paid contractor and I'm no longer on this project.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tgolen can you do this as part of one of your PRs removing the method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll add it to the list.
Thanks for responding @trevor-coleman and I hope you come back at some point! 👋
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Details
This adds Violations warnings and RBR indicators to fields in the
MoneyRequestView
.Notes on Implementation:
Created FieldViolationMessages component -- The violation messages display is repeated 8 times on
the
MoneyRequestView
page, which is enough that it should be encapsulated in a component. It takes a single prop --the list of violations for that field -- which it then maps to a list of
<Text>
componentsCONST.VIOLATIONS
-- The list of violation names is defined in the CONST file, and other types are derived fromthere. This way there is a single place to add/remove violations and the type system would provide helpful errors if
other changes to be made.
Onyx Keys -- added
ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS
with the valuetransactionViolation_
-- thisseemed to follow the pattern
Prop Types -- created
propTypes
objects for atransactionViolation
and array oftransactionViolations
--put them in
@libs/Violations
in case some other component needs to use them later.Clean-up -- cleaned up some imports I wrote in the ViolationUtils PR to match the repo style. Also added some
JSDoc comments for things
Fixed Issues
$ #31084
Tests
Unfortunately the Violations API is not yet working on the back end, so testing this requires changing the code to add
some test data.
Adding Test Violations
In
@src/hooks/useViolations.ts
add the following snippet anywhere below the definition ofviolationFields
:Then, define a
testViolations
variable and use it in place ofviolations
in theviolationsByField
factoryfunction. For instance this will add all violations on the
amount
field:This will allow you to select which violations are shown by redefining
testViolations
in the test.As no native code has changed you will not need to rebuild the app between tests. Simply adjust the value of
testViolations and it should hot-reload.
Billable, Category, and Tags Violations
To test the violations on the Billable, Category and Tag violations, we need to be in a report with a policy -- make
sure the policy has Billable Expenses, Categories, and Tags enabled.
Alternatively, if you have access to the code, you can edit the
MoneyRequestView
component and set the valuesof
shouldShowBillable
,shouldShowCategory
, andshouldShowTags
in the totrue
to test these violations.Tax Violations
The MoneyRequestView component does not currently support Tax violations, so there is no way to test them.
Testing violations are displayed correctly for each field:
Set up the test data to include all violations on the
amount
Field:Create a new Money Request without a Receipt
a. Open the app
b. Click the
+
buttonc. Touch
Create New Money Request
d. Select the "Manual" tab
e. Enter a random amount
f. Touch the
Next
buttong. Select a policy to request money from
h. Enter a description
i. Touch the
Request ${amount}
buttonj. When you are taken to the chat, touch the Report Preview to open the report
k. Touch the money request you just created to view it
For the
amount
field, check that:a. the correct violations are displayed in red text under the associated field
b. that the RBR indicator is displayed next to the chevron (when applicable)
c. that the field can be edited as normal
d. that the screen matches the screenshot below for the
amount
fieldRepeat for each additional field in the table below. You can do this
dummy.violations.missingTag
dummy.violations.someTagLevelsRequired
dummy.violations.tagOutOfPolicy
dummy.violations.customUnitOutOfPolicy
dummy.violations.duplicatedTransaction
dummy.violations.fieldRequired
dummy.violations.nonExpensiworksExpense
dummy.violations.rter
dummy.violations.receiptNotSmartScanned
dummy.violations.receiptRequired
dummy.violations.smartscanFailed
dummy.violations.missingCategory
dummy.violations.invoiceMarkup
dummy.violations.modifiedAmount
dummy.violations.overAutoApprovalLimit
dummy.violations.overCategoryLimit
dummy.violations.overLimit
dummy.violations.overLimitAttendee
dummy.violations.perDayLimit
dummy.violations.maxAge
dummy.violations.modifiedDate
Test that NO violations are displayed when there are no violations
a. Open the app
b. Click the
+
buttonc. Touch
Create New Money Request
d. Scan or select a receipt image
e. Select a policy to request money from
f. Complete each field on the request with any valid input
g. Touch the
Request
buttonh. Touch the Money Request to navigate to the Money Request View
testViolations
to an empty array:Offline tests
This PR does not have any offline behaviour, as it only specifies where/how error messages should appear in the display.
QA Steps
a. Open the app
b. Click the
+
buttonc. Touch
Create New Money Request
d. Scan or select a receipt image
e. Select a policy to request money from
f. Complete each field on the request with any valid input
g. Touch the
Request
buttonh. Touch the Money Request to navigate to the Money Request View
end is not implemented yet.
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
Amount
BIllable
Category
Date
Merchant
Receipt
Tag
Android: mWeb Chrome
Amount ![web-android-amount](https://github.com/Expensify/App/assets/22041394/e1faeb20-7493-47e0-a606-1718b502c966)Billable
Comment
Date
Merchant
Receipt
Tag
iOS: Native
Amount
Billable
Category
Comment
Date
Merchant
Receipt
Tag
iOS: mWeb Safari
Amount ![ios-web-amount](https://github.com/Expensify/App/assets/22041394/8139d956-c980-49a1-918b-81d5a689d450)Billable
Category
Comment
Date
Merchant
Receipt
Tag
MacOS: Chrome / Safari
**Chrome**Amount
Billable
Category
Comment
Date
Merchant
![web-desktop-merchant]
(https://github.com/Expensify/App/assets/22041394/ed300c94-4ba4-4a51-a974-ad068df7ee9d)
Receipt
Tag
MacOS: Desktop
MacOS Desktop is currently crashing on
launch due to an unrelated issue, so I have been
unable to test it. Given the scope of the changes I am highly confident that there will not be any errors specific to
that platform.