fix: Subscription size - Decimal point can be inserted and not removed on the confirmation page. #44507
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…d on the confirmation page. Signed-off-by: Krishna Gupta <belivethatkg@gmail.com>
fedirjh
suggested changes
Jun 26, 2024
Comment on lines
34
to
36
| InteractionManager.runAfterInteractions(() => { | ||
| FormActions.clearDraftValues(ONYXKEYS.FORMS.SUBSCRIPTION_SIZE_FORM); | ||
| }); |
Contributor
There was a problem hiding this comment.
@Krishna2323 This is not needed; we have already a code for cleaning the draft values when the form is closed.
Contributor
Author
There was a problem hiding this comment.
calling FormActions.clearDraftValues(ONYXKEYS.FORMS.SUBSCRIPTION_SIZE_FORM); without interaction manager will show 0 in the input before animation starts.
Contributor
There was a problem hiding this comment.
why do we have to clear the drafvalues here?
Contributor
Author
There was a problem hiding this comment.
If not cleared, we will see value with decimals when we visit the form again.
App/src/components/Form/FormProvider.tsx
Lines 243 to 244 in e24cf4d
Contributor
There was a problem hiding this comment.
If not cleared, we will see value with decimals when we visit the form again.
@Krishna2323 could you please share steps to replicate this bug?
Contributor
There was a problem hiding this comment.
@Krishna2323 Here is the diff code
diff --git a/src/libs/actions/Subscription.ts b/src/libs/actions/Subscription.ts
index beed2b1b29..4bd52bc8e8 100644
--- a/src/libs/actions/Subscription.ts
+++ b/src/libs/actions/Subscription.ts
@@ -7,6 +7,7 @@ import CONST from '@src/CONST';
import type {FeedbackSurveyOptionID, SubscriptionType} from '@src/CONST';
import ONYXKEYS from '@src/ONYXKEYS';
import type {OnyxData} from '@src/types/onyx/Request';
+import INPUT_IDS from '@src/types/form/SubscriptionSizeForm';
/**
* Fetches data when the user opens the SubscriptionSettingsPage
@@ -190,6 +191,11 @@ function updateSubscriptionSize(newSubscriptionSize: number, currentSubscription
},
},
},
+ {
+ onyxMethod: Onyx.METHOD.MERGE,
+ key: ONYXKEYS.FORMS.SUBSCRIPTION_SIZE_FORM_DRAFT,
+ value: null,
+ },
],
successData: [
{
@@ -217,6 +223,13 @@ function updateSubscriptionSize(newSubscriptionSize: number, currentSubscription
},
},
},
+ {
+ onyxMethod: Onyx.METHOD.MERGE,
+ key: ONYXKEYS.FORMS.SUBSCRIPTION_SIZE_FORM_DRAFT,
+ value: {
+ [INPUT_IDS.SUBSCRIPTION_SIZE]: String(newSubscriptionSize),
+ },
+ },
],
};
Contributor
Author
There was a problem hiding this comment.
@fedirjh, I will update in few moments, I still think this won't work correctly, in offline mode the input won't be cleared and if we clear it in optimistic data, the transition issue will occur but I will test and let you know.
Contributor
Author
There was a problem hiding this comment.
@fedirjh, the subscription size is cleared before the animation happens, its hard to catch on web but I think it can cause regression on slower mWeb/native devices.
web_chrome.mp4
Contributor
There was a problem hiding this comment.
@Krishna2323 I see your point, it's not really that noticeable. I am still against using the InteractionManager.runAfterInteractions for web because it just uses setTimout under the hood and it looks hacky. One final try is to fall back to privateSubscription.userCount when the form draft is cleared; I will suggest the changes shortly.
src/pages/settings/Subscription/SubscriptionSize/substeps/Confirmation.tsx
Outdated
Show resolved
Hide resolved
Signed-off-by: Krishna Gupta <belivethatkg@gmail.com>
Contributor
Reviewer Checklist
Screenshots/VideosAndroid: Native
Android: mWeb ChromeCleanShot.2024-07-01.at.01.41.30.mp4iOS: Native
iOS: mWeb SafariCleanShot.2024-07-01.at.01.23.36.mp4MacOS: Chrome / SafariCleanShot.2024-07-01.at.01.17.25.mp4MacOS: DesktopCleanShot.2024-07-01.at.01.43.45.mp4 |
fedirjh
suggested changes
Jul 11, 2024
Contributor
fedirjh
left a comment
fedirjh
left a comment
There was a problem hiding this comment.
Let's avoid the using InteractionManager.runAfterInteractions per https://github.com/Expensify/App/pull/44507/files#r1674425940
Comment on lines
34
to
36
| InteractionManager.runAfterInteractions(() => { | ||
| FormActions.clearDraftValues(ONYXKEYS.FORMS.SUBSCRIPTION_SIZE_FORM); | ||
| }); |
Contributor
There was a problem hiding this comment.
Suggested change
| InteractionManager.runAfterInteractions(() => { | |
| FormActions.clearDraftValues(ONYXKEYS.FORMS.SUBSCRIPTION_SIZE_FORM); | |
| }); | |
| FormActions.clearDraftValues(ONYXKEYS.FORMS.SUBSCRIPTION_SIZE_FORM); |
| const [privateSubscription] = useOnyx(ONYXKEYS.NVP_PRIVATE_SUBSCRIPTION); | ||
| const [subscriptionSizeFormDraft] = useOnyx(ONYXKEYS.FORMS.SUBSCRIPTION_SIZE_FORM_DRAFT); | ||
| const subscriptionRenewalDate = getNewSubscriptionRenewalDate(); | ||
| const subscriptionSizeDraft = subscriptionSizeFormDraft ? Number(subscriptionSizeFormDraft[INPUT_IDS.SUBSCRIPTION_SIZE]) : 0; |
Contributor
There was a problem hiding this comment.
Suggested change
| const subscriptionSizeDraft = subscriptionSizeFormDraft ? Number(subscriptionSizeFormDraft[INPUT_IDS.SUBSCRIPTION_SIZE]) : 0; | |
| const subscriptionSizeDraft = subscriptionSizeFormDraft ? Number(subscriptionSizeFormDraft[INPUT_IDS.SUBSCRIPTION_SIZE]) : 0; | |
| const subscriptionSize = subscriptionSizeDraft || (privateSubscription?.userCount ?? 0); |
| interactive={false} | ||
| description={translate('subscription.subscriptionSize.subscriptionSize')} | ||
| title={translate('subscription.subscriptionSize.activeMembers', {size: subscriptionSizeFormDraft ? subscriptionSizeFormDraft[INPUT_IDS.SUBSCRIPTION_SIZE] : 0})} | ||
| title={translate('subscription.subscriptionSize.activeMembers', {size: subscriptionSizeDraft})} |
Contributor
There was a problem hiding this comment.
Suggested change
| title={translate('subscription.subscriptionSize.activeMembers', {size: subscriptionSizeDraft})} | |
| title={translate('subscription.subscriptionSize.activeMembers', {size: subscriptionSize})} |
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Contributor
Author
|
@fedirjh, changes works well, thanks for the help🙏🏻, code updated. |
fedirjh
approved these changes
Jul 15, 2024
Contributor
fedirjh
left a comment
fedirjh
left a comment
There was a problem hiding this comment.
Looks good and tests well 
Contributor
Author
|
@AndrewGable bump for a review. |
AndrewGable
approved these changes
Jul 22, 2024
Contributor
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
95 tasks
Contributor
|
🚀 Deployed to staging by https://github.com/AndrewGable in version: 9.0.11-0 🚀
|
Contributor
|
🚀 Cherry-picked to staging by https://github.com/francoisl in version: 9.0.11-2 🚀
@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes. |
6 tasks
Contributor
|
🚀 Deployed to production by https://github.com/francoisl in version: 9.0.11-5 🚀
|
Contributor
|
🚀 Deployed to production by https://github.com/francoisl in version: 9.0.12-0 🚀
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Details
Fixed Issues
$ #43796
PROPOSAL: #43796 (comment)
Tests
Note: Subscription Size page is only available on web/mWeb and desktop app
/settings/subscription/subscription-sizeOffline tests
Note: Subscription Size page is only available on web/mWeb and desktop app
/settings/subscription/subscription-sizeQA Steps
Note: Subscription Size page is only available on web/mWeb and desktop app
/settings/subscription/subscription-sizePR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
android_native.mp4
Android: mWeb Chrome
android_chrome.mp4
iOS: Native
ios_native.mp4
iOS: mWeb Safari
ios_safari.mp4
MacOS: Chrome / Safari
web_chrome.mp4
MacOS: Desktop
desktop_app.mp4