-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Feature: Update merchant to be empty and with validation #32486
Feature: Update merchant to be empty and with validation #32486
Conversation
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.
Left comments.
@eVoloshchak 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] |
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.
@waterim I think there was one condition not clear and that is when you are using SmartScan so submitting a picture receipt we still do not require a merchant and it should work the same as it does now and set the (none)
partial merchant optimistically
@mountiny Okay, pushed changes, removed required for isScanRequest expense report, now it works as before my changes. |
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.
@waterim thanks, getting closer, is this the cleanest way to add the merchant empty check to this form? also wont the reset of the money request remove the Request
from merchant too?
…chant-field-required-for-requests-on-group-policies
I ma making a test build for easier testing |
This comment has been minimized.
This comment has been minimized.
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.
BUG: Requesting manual money from group policy is possible without merchant, the field should be required and it should not be hidden behind the Show More
There is also some weird bug here, which is also repro in staging, however, from the video you can see the merchant is set to Request
for a second which seems to be a bug in the optimistic data as the merchant should be (none)
in this case and hence it should show as empty
Screen.Recording.2023-12-14.at.17.37.16.mp4
@mountiny Damn, thats super weird, this happens after I merged main into this branch.. |
…chant-field-required-for-requests-on-group-policies
I cant even see console.logs in MoneyRequestConfirmationList.js component.. |
Okay, I found, there was some refactor and all my code is unused now.. :D Now the component I need to update is: And it means that I need to update now all new components, right @mountiny ? |
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪
|
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.
Code is looking good to me
Reviewer Checklist
Screenshots/VideosVideo included in another comment Android: NativeAndroid: mWeb ChromeiOS: NativeiOS: mWeb SafariMacOS: Chrome / SafariMacOS: Desktop |
Been testing around and noticed two issues:
Screen.Recording.2023-12-27.at.16.34.28.mp4 |
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.
Couple notes in comment above
…requests-on-group-policies
@mountiny I think I fixed the first issue, will push the changes in a minute. I just wanted to ask - if we fill in the merchant field it should disappear (I mean move to the hidden fields under Show more button)? Screen.Recording.2023-12-28.at.10.18.56.mp4It's a current behaviour, I changed it in my commit and now it's visible for workspace request. Let me know if I should restore hiding of this field. For the second issue - I don't really understand, could you explain what is the expected behavior here? And is it on recording? In which second it starts? |
@mountiny so in my last commit, I applied this change for this field to be always visible
yes, now it's fixed and validates the field after clicking submit. Thanks for explaining the issue with the Scan request, I'll take care of it 🙂 |
@mountiny for scans - it should be fixed now |
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪 |
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.
Tested new changes and it works well, thank you very much!
✋ 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/mountiny in version: 1.4.20-0 🚀
|
@@ -618,6 +618,7 @@ export default { | |||
genericSmartscanFailureMessage: 'Transaction is missing fields', | |||
atLeastTwoDifferentWaypoints: 'Please enter at least two different addresses', | |||
splitBillMultipleParticipantsErrorMessage: 'Split bill is only allowed between a single workspace or individual users. Please update your selection.', | |||
invalidMerchant: 'Please enter a corrent merchant.', |
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.
typo - #33781
🚀 Deployed to production by https://github.com/marcaaron in version: 1.4.20-3 🚀
|
Coming from #33786: |
@@ -500,7 +503,7 @@ function MoneyRequestConfirmationList(props) { | |||
} | |||
|
|||
const shouldShowSettlementButton = props.iouType === CONST.IOU.TYPE.SEND; | |||
const shouldDisableButton = selectedParticipants.length === 0; | |||
const shouldDisableButton = selectedParticipants.length === 0 || shouldDisplayMerchantError; |
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.
Coming from #38130:
Form Submit buttons should not be disabled or blocked from being pressed
Details
This PR is about to add an error message when merchant is empty on expense reports in request money and split bill
Fixed Issues
$ #32222
Tests
For IOU request:
For Expense request:
———-
IOU
Offline tests
N/A automated tests
QA Steps
Same as tests
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)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)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
Web