-
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
[HOLD for payment 2024-08-14] [$250] Tax-In tax name field, saving with code does not throw error #45887
Comments
Triggered auto assignment to @RachCHopkins ( |
We think this issue might be related to the #vip-vsb |
ProposalPlease re-state the problem that we are trying to solve in this issue.Tax-In tax name field, saving with code does not throw error What is the root cause of that problem?
App/src/pages/workspace/taxes/WorkspaceCreateTaxPage.tsx Lines 79 to 88 in e3ebeff
App/src/components/Form/FormProvider.tsx Lines 65 to 66 in e3ebeff
What changes do you think we should make in order to solve the problem?
What alternative solutions did you explore? (Optional)We can validate the value inside Code changes:
validate={(value?: string) => {
if (!policy) {
return;
}
if (validateTaxName(policy, {[INPUT_IDS.NAME]: value ?? ''})?.[INPUT_IDS.NAME]) {
return validateTaxName(policy, {[INPUT_IDS.NAME]: value ?? ''})?.[INPUT_IDS.NAME];
}
// This can also be directly used in `TextSelectorModal`, that way we don't need to pass this in validation callback
if (ValidationUtils.validateInvalidCharacter(value)) {
return ValidationUtils.validateInvalidCharacter(value);
}
return '';
}}
<FormAlertWithSubmitButton
isAlertVisible={!!error}
onSubmit={() => {
if (validate && validate(currentValue)) {
setError(validate(currentValue) ?? '');
setIsTouched(true);
return;
}
Keyboard.dismiss();
onValueSelected?.(currentValue ?? '');
}}
onFixTheErrorsLinkPressed={() => {
inputRef.current?.focus();
}}
buttonText={translate('common.save')}
/> |
Job added to Upwork: https://www.upwork.com/jobs/~01e31e4849306c8612 |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @hungvu193 ( |
ProposalPlease re-state the problem that we are trying to solve in this issue.In tax name field, saving with code throws error " invalid character " in android but error not shown in mweb. What is the root cause of that problem?We don't set This is odd because the user comes all the way from clicking into the Name field, inputting the invalid name and come back, and do not see any error until they click What changes do you think we should make in order to solve the problem?Update
Then the user will see the error immediately after they come back from the Name What alternative solutions did you explore? (Optional)I think the best UX would be to validate right inside the Name input, so if the name is invalid, it the user will see it immediately after clicking We don't really need to duplicate the validation logic from General steps:
Because we don't want the Enter key press to trigger submission in the create tax form, if the name text picker is opened, because Enter should trigger submission in the text picker there instead because it's focused.
With those minimal changes, it works well now: Some further enhancements that could be done:
All those follow-ups can be done in the PR phase. |
Thanks for proposal everyone. @dominictb Screen.Recording.2024-07-24.at.11.15.43.mov |
@hungvu193 That's the correct behavior when the user focuses on a required field but then leave it empty, it's good to signify to the user that the value is required and they must fill it. It's the same UX for any other required field, please take a look at the example video below in the Manual bank account connection feature (all required fields in every bank account step behave the same): Screen.Recording.2024-07-24.at.1.33.33.PM.movAnother example when adding category: Screen.Recording.2024-07-24.at.1.36.03.PM.movLet me know if you think that makes sense 👍 |
That's different cases, these cases that you mentioned we validated on blur directly on that field, in this case we opened a new screen to do that so I'm still aligned with my opinion above. |
We have two options here:
onInputChange: (value, key) => {
const inputKey = key ?? inputID;
setInputValues((prevState) => {
const newState = {
...prevState,
[inputKey]: value,
};
if (setInputTouchedOnChange) {
setTouchedInput(inputID);
}
if (shouldValidateOnChange) {
onValidate(newState);
}
return newState as Form;
});
if (inputProps.shouldSaveDraft && !formID.includes('Draft')) {
FormActions.setDraftValues(formID, {[inputKey]: value});
}
inputProps.onValueChange?.(value, inputKey);
}, form_validation_add_rate.mp4 |
@hungvu193 Curious how you think it is different? Isn't blurring the TextPicker in the new screen the same as blurring the text input itself?
@hungvu193 Could you give more details on why you think that's not a good way in terms of UX perspective, as far as I can see it's the same UX we've been using everywhere else, and it's better than not showing any error after the user blurs the required inputs. |
I think that's why we added
It's not the same, if you're talking about validating on blur, it should show error on TextPicker screen when we blurred TextInput as well. There will be inconsistency here, we click back from TextPicker => no error, we blur the input from TextPicker and click back => error showed. cc @shawnborton for more thoughts here. |
Thanks for your explanation, I'll review after we have confirmation from design team 🙏 |
I'm a little lost - can you explain in more detail? Based on what I see from the bug report though, it seems like we shouldn't even allow you to save the tax rate name with the forbidden character. We should throw the error on that same "edit name" page and make the user fix the error before it saves. |
I agree with this one.
We're talking about the way we should show error on
ExampleScreen.Recording.2024-07-24.at.11.15.43.mov
|
Oh interesting... part of me thinks we wouldn't show those errors until we hit the Save button to submit the form for the Add rate page. |
Also cc @Expensify/design for their input too. |
@shawnborton I think this would be the best UX for the user, if there's invalid character, they'd know right away and not having to go back and forth to change it (because now they only see the error at the last step where they submit). We're doing that for all other forms too. |
I agree with Shawn. I don't think we would show those errors on the But on the individual Does that make sense / follow our other validation patterns? |
Agreed.
I agree here too, but I would also say if they blur the filed we should show the error like other forms, right? |
@hungvu193 Looks like everyone agrees with this UX, that's also what I suggested and proposed a solution to in the alternative part here Could you take a look to see if we can go with it? Thanks. |
Please make sure that you will post a comment to let everyone know you updated your proposal.
I think it's looking good, but can you give it a little bit more details? Also an evidence video would be appreciated. Thank you 😄 |
Triggered auto assignment to @yuwenmemon, see https://stackoverflow.com/c/expensify/questions/7972 for more details. |
📣 @dominictb 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app! Offer link |
@hungvu193 Assigned. You may need to solicit another Engineer for merge as I'll be OOO until the 13th starting tomorrow. |
PR will be ready by tomorrow. |
Requested internal engineer to take over this issue on Slack |
|
The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.17-2 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue: If no regressions arise, payment will be issued on 2024-08-14. 🎊 For reference, here are some details about the assignees on this issue:
|
BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:
|
Regression test:
Do we 👍 or 👎 ? |
little bump @RachCHopkins for the payment 😄 |
Sorry @hungvu193 I've been a little tied up! I'll get this organised tomorrow! |
Payment Summary: Contributor: @dominictb paid $250 via Upwork Upwork job here |
Contributor has been paid, the contract has been completed, and the Upwork post has been closed. @hungvu193 please go ahead and raise your NewDot Request! |
$250 approved for @hungvu193 |
If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!
Version Number: 9.0.10
Reproducible in staging?: Y
Reproducible in production?: Y
Logs: https://stackoverflow.com/c/expensify/questions/4856
Issue reported by: Applause-Internal team
Action Performed:
Pre-condition:Have a workspace with tax enabled
4.Paste and save it
Expected Result:
In tax name field, saving with code must throw error " invalid character " across site consistently in same way.
Actual Result:
In tax name field, saving with code throws error " invalid character " in android but error not shown in mweb.
Across the site in all input field on saving html code throws error except in tax & Report field name field.
Workaround:
Unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Screenshots/Videos
Bug6548631_1721565991865.Screenrecorder-2024-07-21-18-05-27-361_compress_1.mp4
View all open jobs on GitHub
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @RachCHopkinsThe text was updated successfully, but these errors were encountered: