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

Jump to bottom

26773-bug-user_is_able_to_delete_default_contact_method #27788

Merged
merged 3 commits into from
Sep 29, 2023
Merged

26773-bug-user_is_able_to_delete_default_contact_method #27788

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6a327f1
added delete account error handling and delete modal visiblity fixx
lukemorawski Sep 19, 2023
ddd63b5
bool coercion by Boolean casting
lukemorawski Sep 20, 2023
d0cb155
cleaner way of presenting default contact delete error
lukemorawski Sep 20, 2023
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
30 changes: 21 additions & 9 deletions src/pages/settings/Profile/Contacts/ContactMethodDetailsPage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,6 +230,7 @@ class ContactMethodDetailsPage extends Component {
const isDefaultContactMethod = this.props.session.email === loginData.partnerUserID;
const hasMagicCodeBeenSent = lodashGet(this.props.loginList, [contactMethod, 'validateCodeSent'], false);
const isFailedAddContactMethod = Boolean(lodashGet(loginData, 'errorFields.addedLogin'));
const isFailedRemovedContactMethod = Boolean(lodashGet(loginData, 'errorFields.deletedLogin'));

return (
<ScreenWrapper onEntryTransitionEnd={() => this.validateCodeFormRef.current && this.validateCodeFormRef.current.focus()}>
Expand All @@ -245,7 +246,7 @@ class ContactMethodDetailsPage extends Component {
prompt={this.props.translate('contacts.removeAreYouSure')}
confirmText={this.props.translate('common.yesContinue')}
cancelText={this.props.translate('common.cancel')}
isVisible={this.state.isDeleteModalOpen}
isVisible={this.state.isDeleteModalOpen && !isDefaultContactMethod}
Copy link
Contributor

@Ollyws Ollyws Sep 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

An issue with using isDefaultContactMethod is that this is only updated when the server response is recieved, is there any way we can hide the modal using optimisticData so it responds immediately when the contact method is deleted? I was in some cases able to successfully delete the second contact method by clicking the confirm button before it was hidden.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll see what I can do :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like it's the only thing we can do right now. The app cannot use any optimistic approach here, as the contact method is being set to default on a different device. There is no way to know if the method status was changed, other then to wait for the update from the backend. That's why this PR has the additional error handling added (using optimistic update approach). If you delete the contact before themodal was hidden, but on the BE it was already flagged as default, you will receive and error (the red brick road indicators) so you'll know what nothing has changed.
Sorry, but that's how this works :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah ok no worries, I just meant when the user was using two tabs in the same browser but it's a pretty edge case scenario. Current behaviour is acceptable, thanks for looking anyway!

danger
/>
{isFailedAddContactMethod && (
Expand Down Expand Up @@ -284,14 +285,25 @@ class ContactMethodDetailsPage extends Component {
</OfflineWithFeedback>
) : null}
{isDefaultContactMethod ? (
<OfflineWithFeedback
pendingAction={lodashGet(loginData, 'pendingFields.defaultLogin', null)}
errors={ErrorUtils.getLatestErrorField(loginData, 'defaultLogin')}
errorRowStyles={[styles.ml8, styles.mr5]}
onClose={() => User.clearContactMethodErrors(contactMethod, 'defaultLogin')}
>
<Text style={[styles.ph5, styles.mv3]}>{this.props.translate('contacts.yourDefaultContactMethod')}</Text>
</OfflineWithFeedback>
<>
<OfflineWithFeedback
pendingAction={lodashGet(loginData, 'pendingFields.defaultLogin', null)}
errors={ErrorUtils.getLatestErrorField(loginData, 'defaultLogin')}
errorRowStyles={[styles.ml8, styles.mr5]}
onClose={() => User.clearContactMethodErrors(contactMethod, 'defaultLogin')}
>
<Text style={[styles.ph5, styles.mv3]}>{this.props.translate('contacts.yourDefaultContactMethod')}</Text>
</OfflineWithFeedback>
{isFailedRemovedContactMethod && (
Copy link
Contributor

Choose a reason for hiding this comment

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

I would remove this conditional rendering and wrap the OfflineWithFeedback above with this. Because the error will only be shown if ErrorUtils.getLatestErrorField(loginData, 'deletedLogin') returns an error, and it's the same as doing lodashGet(loginData, 'errorFields.deletedLogin')

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure if I follow your logic, but I think I have found a slightly better way of dealing with this conundrum.

{isDefaultContactMethod ? (
    <OfflineWithFeedback
	pendingAction={lodashGet(loginData, 'pendingFields.defaultLogin', null)}
	errors={ErrorUtils.getLatestErrorField(loginData, isFailedRemovedContactMethod ? 'deletedLogin' : 'defaultLogin')}
	errorRowStyles={[styles.ml8, styles.mr5]}
	onClose={() => User.clearContactMethodErrors(contactMethod, isFailedRemovedContactMethod ? 'deletedLogin' : 'defaultLogin')}
    >
	<Text style={[styles.ph5, styles.mv3]}>{this.props.translate('contacts.yourDefaultContactMethod')}</Text>
    </OfflineWithFeedback>
) : (
	...
)}

<OfflineWithFeedback
errors={ErrorUtils.getLatestErrorField(loginData, 'deletedLogin')}
errorRowStyles={[styles.ml8, styles.mr5]}
onClose={() => User.clearContactMethodErrors(contactMethod, 'deletedLogin')}
>
<></>
lukemorawski marked this conversation as resolved.
Show resolved Hide resolved
</OfflineWithFeedback>
)}
</>
) : (
<OfflineWithFeedback
pendingAction={lodashGet(loginData, 'pendingFields.deletedLogin', null)}
Expand Down
Loading