-
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
[HOLD for payment 2024-06-28] [HOLD for payment 2024-06-24] [$250] Subscription size value not trim able to add spaces before the value #43069
Comments
Triggered auto assignment to @Christinadobrzyn ( |
👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:
|
@Christinadobrzyn FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors |
Triggered auto assignment to @tylerkaraszewski ( |
Job added to Upwork: https://www.upwork.com/jobs/~0110dce96b3114bcdc |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @thesahindia ( |
ProposalPlease re-state the problem that we are trying to solve in this issue.Subscription size value not trim able to add spaces before the value What is the root cause of that problem?We display the original data when showing the entered subscription size here:
We should trim the data before we display it. What changes do you think we should make in order to solve the problem?Change this line
to title={translate('subscription.subscriptionSize.activeMembers', {size: subscriptionSizeFormDraft ? subscriptionSizeFormDraft[INPUT_IDS.SUBSCRIPTION_SIZE].trim() : 0})} AlternativelyWe may save the size value in a variable to make code simpler. |
This doesn't need to block deploy since it's a new feature and cannot be accessed yet except via direct link. @tylerkaraszewski I'm happy to take it over if you want, just lmk! |
ProposalPlease re-state the problem that we are trying to solve in this issue.Subscription size value not trim able to add spaces before the value What is the root cause of that problem?We are not trimming the value before moving to next step.
What changes do you think we should make in order to solve the problem?We can introduce a new prop (shouldTrimEmptySpaces) inside We will trim the value if App/src/components/Form/FormProvider.tsx Line 105 in 9b179ce
App/src/components/Form/FormProvider.tsx Line 173 in 9b179ce
App/src/components/Form/FormProvider.tsx Line 198 in 9b179ce
And also update the code below: App/src/components/Form/FormProvider.tsx Lines 353 to 355 in 9b179ce
To: if (inputProps.shouldSaveDraft && !formID.includes('Draft')) {
let trimmedValue = value;
if (typeof value === 'string' && shouldTrimEmptySpaces) {
trimmedValue = value.trim();
}
FormActions.setDraftValues(formID as OnyxFormKey, {[inputKey]: trimmedValue});
} What alternative solutions did you explore? (Optional)We can trim spaces by default and use the same prop If we need, we can only trim the draft value instead of all values. |
Proposal Updated
|
ProposalPlease re-state the problem that we are trying to solve in this issue.Subscription size value not trim What is the root cause of that problem?We lack the logic to trim the subscription size value What changes do you think we should make in order to solve the problem?In the FormProvider, we've incorporated a property called shouldTrimValues. If shouldTrimValues is set to true, we'll invoke this function. App/src/libs/ValidationUtils.ts Line 453 in 3c04b02
However, currently, this function does not include logic to trim values. I propose that we integrate logic to trim values within this function, if the input value is a string. code change:
Additionally, before saving the draft, we should also trim the value. We need to verify whether the value is a string; if it is, we should apply a trim function. App/src/components/Form/FormProvider.tsx Line 354 in 3c04b02
code change:
What alternative solutions did you explore? (Optional)Reminder: Please use plain English, be brief and avoid jargon. Feel free to use images, charts or pseudo-code if necessary. Do not post large multi-line diffs or write walls of text. Do not create PRs unless you have been hired for this job. |
PR is ready cc: @thesahindia |
Just a heads up - I'm going to be ooo until June 24th. I'm not going to assign anyone new but if you need a new BZ teammate for any reason please feel free to ask for one to be assigned here. |
|
The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.84-3 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-06-24. 🎊 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:
|
The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.85-7 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-06-28. 🎊 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:
|
Payouts due:
Upwork job is here. @thesahindia let us know about a regression test! |
still have a few days till payment! |
This bug was present since the implementation of this page. It was a minor issue that got missed in #42683. I don't think we need a test case for this. |
Sounds good - no regression test for this. Thanks @thesahindia! |
Awesome! Payment summary is here - #43069 (comment) This is good to close. Feel free to reach out if I missed anything. |
$250 approved for @thesahindia |
If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!
Issue found when validating the PR #42683
Version Number: 1.4.79-2
Reproducible in staging?: y
Reproducible in production?: New feature
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: Applause internal team
Slack conversation:
Action Performed:
Expected Result:
Spaces should not displayed with value
Actual Result:
Subscription size value not trim able to add spaces before the value
Workaround:
unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Screenshots/Videos
Bug6501964_1717527964130.2024-06-04_23-59-06.mp4
View all open jobs on GitHub
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @Issue Owner
Current Issue Owner: @ChristinadobrzynThe text was updated successfully, but these errors were encountered: