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.

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

Warn users when before signing out with unsynced notes #702

Merged
merged 10 commits into from
Feb 7, 2018
8 changes: 7 additions & 1 deletion lib/app.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -517,7 +517,13 @@ export const App = connect(mapStateToProps, mapDispatchToProps)(
throw new Error('Unknown dialog type.');
}

return <DialogComponent {...this.props} {...{ key, dialog, params }} />;
return (
<DialogComponent
isElectron={isElectron()}
{...this.props}
{...{ key, dialog, params }}
/>
);
},
})
);
Expand Down
69 changes: 68 additions & 1 deletion lib/dialogs/settings.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ export const SettingsDialog = React.createClass({
propTypes: {
actions: PropTypes.object.isRequired,
onSignOut: PropTypes.func.isRequired,
isElectron: PropTypes.bool.isRequired,
},

onDone() {
Expand All @@ -37,6 +38,72 @@ export const SettingsDialog = React.createClass({
});
},

onSignOutRequested() {
// Safety first! Check for any unsynced notes before signing out.
const { onSignOut, noteBucket } = this.props;
const { notes } = this.props.appState;
const { getVersion } = noteBucket;

noteBucket.hasLocalChanges((error, hasChanges) => {
if (hasChanges) {
this.showUnsyncedWarning();
return;
}

// Also check persisted store for any notes with version 0
const noteHasSynced = note =>
new Promise((resolve, reject) =>
getVersion(note.id, (e, v) => (e || v === 0 ? reject() : resolve()))
);

Promise.race(notes.map(noteHasSynced)).then(
Copy link
Contributor

@beaucollins beaucollins Feb 7, 2018

Choose a reason for hiding this comment

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

Looks like I commented on an outdated change but the Promise.race remains.

It seems you want to check if any of the notes have a version of 0 and if any of them do have a version of 0 then you will want to show showUnsyncedWarning.

Promise.race will return the first promise that resolves or rejects and ignore all others where as Promise.all will wait until all Promises resolve or fail early upon the first reject.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm ok, I think I indeed want Promise.all then. If we ever see a version of zero we should stop and show the warning. Otherwise, keep checking.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in 5324686. Nice catch!

Copy link
Member

@dmsnell dmsnell Feb 7, 2018

Choose a reason for hiding this comment

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

good catch @beaucollins - the race was from my original idea where only the unsync'd notes would resolve and so we wouldn't have needed to wait for all Promises - an early abort. this was a wrong idea. 😄

it would also work if we only reject()ed on unsync'd notes and had a timeout, but that's another kind of race we don't want.

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've learned a lot about Promise via this PR! :)

Copy link
Member

Choose a reason for hiding this comment

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

@roundhill you don't need to learn that much about promises. just let your "yes" be "yes" and your "no" be "no" 😉

() => onSignOut(), // All good, sign out now!
() => this.showUnsyncedWarning() // Show a warning to the user
);
});
},

showUnsyncedWarning() {
const { isElectron } = this.props;
isElectron ? this.showElectronWarningDialog() : this.showWebWarningDialog();
},

showElectronWarningDialog() {
const { onSignOut } = this.props;
const dialog = __non_webpack_require__('electron').remote.dialog; // eslint-disable-line no-undef
dialog.showMessageBox(
{
type: 'warning',
buttons: ['Delete Notes', 'Cancel', 'Visit Web App'],
title: 'Unsynced Notes Detected',
message:
'Logging out will delete any unsynced notes. You can verify your ' +
'synced notes by logging in to the Web App.',
},
response => {
if (response === 0) {
onSignOut();
} else if (response === 2) {
viewExternalUrl('https://app.simplenote.com');
}
}
);
},

showWebWarningDialog() {
const { onSignOut } = this.props;
const shouldReallySignOut = confirm(
'Warning: Unsynced notes were detected.\n\n' +
'Logging out will delete any notes that have not synced. ' +
'Check your connection and visit app.simplenote.com to verify synced notes.' +
'\n\nClick OK to delete unsynced notes and Log Out.'
);

if (shouldReallySignOut) {
onSignOut();
}
},

render() {
var dialog = this.props.dialog;

Expand Down Expand Up @@ -91,7 +158,7 @@ export const SettingsDialog = React.createClass({
<button
type="button"
className="button button-primary"
onClick={this.props.onSignOut}
onClick={this.onSignOutRequested}
>
Log Out
</button>
Expand Down
12 changes: 6 additions & 6 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -14,19 +14,19 @@
"build": "NODE_ENV=development npm run build:dll && npm run build:app",
"build:app": "webpack --config ./webpack.config.js",
"build:dll": "webpack --config ./webpack.config.dll.js",
"build:prod":
"NODE_ENV=production webpack -p --config ./webpack.config.dll.js && webpack -p --config ./webpack.config.js",
"build:prod": "NODE_ENV=production webpack -p --config ./webpack.config.dll.js && webpack -p --config ./webpack.config.js",
"format": "prettier --write {desktop,lib,sass}/{**/,*}.{js,json,jsx,sass}",
"lint": "eslint --ext .js --ext .jsx lib",
"start":
"NODE_ENV=hot npm run build && webpack-dev-server --config ./webpack.config.js --content-base dist --host 0.0.0.0 --port 4000 --hot --inline"
"start": "NODE_ENV=hot npm run build && webpack-dev-server --config ./webpack.config.js --content-base dist --host 0.0.0.0 --port 4000 --hot --inline"
},
"license": "ISC",
"engines": {
"node": "7.9.0",
"npm": "4.2.0"
},
"browserslist": ["Chrome >= 58"],
"browserslist": [
"Chrome >= 58"
],
"jest": {
"rootDir": "lib",
"testRegex": "(/test/.*\\.jsx?)|(test\\.jsx?)$"
Expand Down Expand Up @@ -99,7 +99,7 @@
"serve-favicon": "2.4.3",
"showdown": "1.7.2",
"showdown-xss-filter": "0.2.0",
"simperium": "0.2.9"
"simperium": "0.3.1"
},
"optionalDependencies": {
"appdmg": "0.4.5"
Expand Down