26773-bug-user_is_able_to_delete_default_contact_method #27788
Conversation
lukemorawski
changed the title
lukemorawski
changed the title
src/pages/settings/Profile/Contacts/ContactMethodDetailsPage.js
Outdated
Show resolved
Hide resolved
src/pages/settings/Profile/Contacts/ContactMethodDetailsPage.js
Outdated
Show resolved
Hide resolved
Co-authored-by: Michael (Mykhailo) Kravchenko <rezkiy37@gmail.com>
| > | ||
| <Text style={[styles.ph5, styles.mv3]}>{this.props.translate('contacts.yourDefaultContactMethod')}</Text> | ||
| </OfflineWithFeedback> | ||
| {isFailedRemovedContactMethod && ( |
There was a problem hiding this comment.
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')
There was a problem hiding this comment.
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>
) : (
...
)}
|
@Ollyws Hey! Any update on this? |
|
testing/reviewing at the moment. |
| @@ -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} | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I'll see what I can do :)
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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!
Reviewer Checklist
Screenshots/VideosWebMacOS_Chrome.mp4Mobile Web - ChromeAndroid_Chrome.mp4Mobile Web - SafariiOS_Safari.mp4DesktopMacOS_Desktop.mp4iOSiOS_Native.mp4AndroidAndroid_Native.mp4 |
|
We did not find an internal engineer to review this PR, trying to assign a random engineer to #26773 as well as to this PR... Please reach out for help on Slack if no one gets assigned! |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/amyevans in version: 1.3.76-0 🚀
|
|
🚀 Deployed to production by https://github.com/mountiny in version: 1.3.76-6 🚀
|
|
🚀 Deployed to staging by https://github.com/amyevans in version: 1.3.77-0 🚀
|
|
🚀 Deployed to production by https://github.com/mountiny in version: 1.3.77-7 🚀
|
Details
The PR addresses the issue in two ways:
Fixed Issues
$ #26773
PROPOSAL: no proposal
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)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
Web
web.desktop.mov
Mobile Web - Chrome
Mobile Web - Safari
Desktop
web.desktop.mov
iOS
Android