Skip to content
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.

Sign up for GitHub

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

fix: Subscription size - Decimal point can be inserted and not removed on the confirmation page. #44507

Merged
merged 4 commits into from
Jul 22, 2024
Merged

fix: Subscription size - Decimal point can be inserted and not removed on the confirmation page. #44507

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7b25554
fix: Subscription size - Decimal point can be inserted and not remove…
Krishna2323 Jun 26, 2024
518d99c
minor updated.
Krishna2323 Jun 26, 2024
518f36d
Merge branch 'Expensify:main' into krishnna2323/issue/43796
Krishna2323 Jul 15, 2024
08c4a55
add suggested changes.
Krishna2323 Jul 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/pages/settings/Subscription/SubscriptionSize/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import type {StackScreenProps} from '@react-navigation/stack';
import React from 'react';
import {InteractionManager} from 'react-native';
import {useOnyx} from 'react-native-onyx';
import HeaderWithBackButton from '@components/HeaderWithBackButton';
import ScreenWrapper from '@components/ScreenWrapper';
Expand All @@ -8,6 +9,7 @@ import useSubStep from '@hooks/useSubStep';
import type {SubStepProps} from '@hooks/useSubStep/types';
import Navigation from '@libs/Navigation/Navigation';
import type {SettingsNavigatorParamList} from '@navigation/types';
import * as FormActions from '@userActions/FormActions';
import * as Subscription from '@userActions/Subscription';
import ONYXKEYS from '@src/ONYXKEYS';
import type SCREENS from '@src/SCREENS';
Expand All @@ -29,6 +31,9 @@ function SubscriptionSizePage({route}: SubscriptionSizePageProps) {
const onFinished = () => {
Subscription.updateSubscriptionSize(subscriptionSizeFormDraft ? Number(subscriptionSizeFormDraft[INPUT_IDS.SUBSCRIPTION_SIZE]) : 0, privateSubscription?.userCount ?? 0);
Navigation.goBack();
InteractionManager.runAfterInteractions(() => {
FormActions.clearDraftValues(ONYXKEYS.FORMS.SUBSCRIPTION_SIZE_FORM);
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Krishna2323 This is not needed; we have already a code for cleaning the draft values when the form is closed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

calling FormActions.clearDraftValues(ONYXKEYS.FORMS.SUBSCRIPTION_SIZE_FORM); without interaction manager will show 0 in the input before animation starts.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we have to clear the drafvalues here?

Copy link
Contributor Author

@Krishna2323 Krishna2323 Jun 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

} else if (inputProps.shouldSaveDraft && draftValues?.[inputID] !== undefined && inputValues[inputID] === undefined) {
inputValues[inputID] = draftValues[inputID];

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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),
+                },
+            },
         ],
     };

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Friendly bump @Krishna2323.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
InteractionManager.runAfterInteractions(() => {
FormActions.clearDraftValues(ONYXKEYS.FORMS.SUBSCRIPTION_SIZE_FORM);
});
FormActions.clearDraftValues(ONYXKEYS.FORMS.SUBSCRIPTION_SIZE_FORM);

};

const {componentToRender: SubStep, screenIndex, nextScreen, prevScreen, moveTo} = useSubStep({bodyContent, startFrom, onFinished});
Expand Down
5 changes: 3 additions & 2 deletions src/pages/settings/Subscription/SubscriptionSize/substeps/Confirmation.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,9 @@ function Confirmation({onNext, isEditing}: ConfirmationProps) {
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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);


const isTryingToIncreaseSubscriptionSize = (subscriptionSizeFormDraft ? Number(subscriptionSizeFormDraft[INPUT_IDS.SUBSCRIPTION_SIZE]) : 0) > (privateSubscription?.userCount ?? 0);
const isTryingToIncreaseSubscriptionSize = subscriptionSizeDraft > (privateSubscription?.userCount ?? 0);
const canChangeSubscriptionSize = (account?.canDowngrade ?? false) || (isTryingToIncreaseSubscriptionSize && isEditing);
const formattedSubscriptionEndDate = formatSubscriptionEndDate(privateSubscription?.endDate);

Expand All @@ -41,7 +42,7 @@ function Confirmation({onNext, isEditing}: ConfirmationProps) {
<MenuItemWithTopDescription
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})}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
title={translate('subscription.subscriptionSize.activeMembers', {size: subscriptionSizeDraft})}
title={translate('subscription.subscriptionSize.activeMembers', {size: subscriptionSize})}

/>
<MenuItemWithTopDescription
interactive={false}
Expand Down
Loading