Skip to content

Remove isReset from SMSDeliveryFailurePage #59163

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

Merged
carlosmiceli merged 7 commits into from
Apr 9, 2025
Merged

Remove isReset from SMSDeliveryFailurePage #59163

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
cc0cefd
rm isReset from sms delivery failure
rushatgabhane Mar 26, 2025
3d1299c
rm isreset
rushatgabhane Mar 26, 2025
a9d8eb3
Merge branch 'Expensify:main' into sms-fail
rushatgabhane Apr 1, 2025
2df4ff0
Merge branch 'main' into sms-fail
rushatgabhane Apr 2, 2025
98484d7
Merge branch 'sms-fail' of github.com:rushatgabhane/exfy into sms-fail
rushatgabhane Apr 2, 2025
f3844bb
add state for button click
rushatgabhane Apr 2, 2025
5a4a7ff
add comment
rushatgabhane Apr 2, 2025
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
1 change: 0 additions & 1 deletion src/libs/actions/Session/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1304,7 +1304,6 @@ function resetSMSDeliveryFailureStatus(login: string) {
value: {
smsDeliveryFailureStatus: {
isLoading: false,
isReset: true,
},
},
},
Expand Down
14 changes: 9 additions & 5 deletions src/pages/signin/SMSDeliveryFailurePage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import {Str} from 'expensify-common';
import React, {useEffect, useMemo} from 'react';
import React, {useEffect, useMemo, useState} from 'react';
import {Keyboard, View} from 'react-native';
import {useOnyx} from 'react-native-onyx';
import Button from '@components/Button';
Expand Down Expand Up @@ -45,7 +45,8 @@ function SMSDeliveryFailurePage() {

const hasSMSDeliveryFailure = account?.smsDeliveryFailureStatus?.hasSMSDeliveryFailure;

const isReset = account?.smsDeliveryFailureStatus?.isReset;
// We need to show two different messages after clicking validate button, based on API response for hasSMSDeliveryFailure.
const [hasClickedValidate, setHasClickedValidate] = useState(false);

const errorText = useMemo(() => (account ? getLatestErrorMessage(account) : ''), [account]);
const shouldShowError = !!errorText;
Expand All @@ -57,7 +58,7 @@ function SMSDeliveryFailurePage() {
Keyboard.dismiss();
}, [isKeyboardShown]);

if (hasSMSDeliveryFailure && isReset) {
if (hasSMSDeliveryFailure && hasClickedValidate) {
return (
<>
<View style={[styles.mv3, styles.flexRow]}>
Expand Down Expand Up @@ -87,7 +88,7 @@ function SMSDeliveryFailurePage() {
);
}

if (!hasSMSDeliveryFailure && isReset) {
if (!hasSMSDeliveryFailure && hasClickedValidate) {

@suneox suneox Jul 3, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Coming from #60329 checklist — while the API is still loading, the final form briefly flashes. We can address this by checking the loading status at this condition

@carlosmiceli carlosmiceli Jul 3, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Are you saying that it's not fixed yet and it wasn't in the latest PR for #60329? Can we get a PR up to clean that up? cc @hoangzinh

@hoangzinh hoangzinh Jul 3, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nope, @carlosmiceli. In the latest PR for #60329, we already fixed the flash issue. All goods now.

@carlosmiceli carlosmiceli Jul 3, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@suneox not suree what you meant by your comment then, could you explain please? thank you!

@suneox suneox Jul 4, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

My earlier comment was actually part of the review checklist for issue #60329. At the time of this PR, the flash issue hadn’t been checked yet — that’s why I mentioned it. But no worries, it has already been handled in the latest PR latest PR — so it’s all good now. 🙌

return (
<>
<View style={[styles.mv3, styles.flexRow]}>
Expand Down Expand Up @@ -126,7 +127,10 @@ function SMSDeliveryFailurePage() {
<FormAlertWithSubmitButton
buttonText={translate('common.validate')}
isLoading={account?.smsDeliveryFailureStatus?.isLoading}
onSubmit={() => resetSMSDeliveryFailureStatus(login)}
onSubmit={() => {
resetSMSDeliveryFailureStatus(login);
setHasClickedValidate(true);
}}
message={errorText}
isAlertVisible={shouldShowError}
containerStyles={[styles.w100, styles.mh0]}
Expand Down
3 changes: 0 additions & 3 deletions src/types/onyx/Account.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,9 +65,6 @@ type SMSDeliveryFailureStatus = {
/** The message associated with the SMS delivery failure */
message: string;

/** Indicates whether the SMS delivery failure status has been reset by an API call */
isReset?: boolean;

/** Whether a sign is loading */
isLoading?: boolean;
};
Expand Down