Do not close entry page if it failed to save or delete. #566
Conversation
tech4him1
changed the title
src/containers/EntryPage.js
Outdated
| @@ -97,7 +97,7 @@ class EntryPage extends React.Component { | |||
| handlePersistEntry = () => { | |||
| const { persistEntry, collection } = this.props; | |||
| setTimeout(() => { | |||
| persistEntry(collection).then(() => this.handleCloseEntry()); | |||
| persistEntry(collection).then(() => this.handleCloseEntry()).catch(() => {/* Don't do anything, the error will be shown by the action. */}); | |||
There was a problem hiding this comment.
I think the error here should continue to propagate, rather than be caught by the empty catch block, unless there's a particular reason you caught this here.
There was a problem hiding this comment.
The only reason I did this was that I figured since the action is already showing a notification, we didn't need to do anything. I'll go ahead and test it without the .catch and make sure that there are no side effects, then I can remove it.
There was a problem hiding this comment.
Looks like the only "downside" is that you get duplicate error messages in the console, so I'll go ahead and remove this.
src/containers/EntryPage.js
Outdated
| @@ -110,7 +110,7 @@ class EntryPage extends React.Component { | |||
| const { deleteEntry, entry, collection } = this.props; | |||
| const slug = entry.get('slug'); | |||
| setTimeout(() => { | |||
| deleteEntry(collection, slug).then(() => this.handleCloseEntry()); | |||
| deleteEntry(collection, slug).then(() => this.handleCloseEntry()).catch(() => {/* Don't do anything, the error will be shown by the action. */}); | |||
There was a problem hiding this comment.
I think the error here should continue to propagate, rather than be caught by the empty catch block, unless there's a particular reason you caught this here.
src/actions/entries.js
Outdated
| @@ -288,7 +288,7 @@ export function persistEntry(collection) { | |||
| kind: 'success', | |||
| dismissAfter: 4000, | |||
| })); | |||
| return dispatch(entryPersisted(collection, serializedEntry)); | |||
| return Promise.resolve(dispatch(entryPersisted(collection, serializedEntry))); | |||
There was a problem hiding this comment.
I don't think the Promise.resolve here isn't doing anything - dispatch(entryDeleted(...)) should already be returning a promise.
There was a problem hiding this comment.
Good point -- since its returning from a then, it already gets wrapped in a promise.
| @@ -297,7 +297,7 @@ export function persistEntry(collection) { | |||
| kind: 'danger', | |||
| dismissAfter: 8000, | |||
| })); | |||
| return dispatch(entryPersistFail(collection, serializedEntry, error)); | |||
| return Promise.reject(dispatch(entryPersistFail(collection, serializedEntry, error))); | |||
src/actions/entries.js
Outdated
| @@ -310,7 +310,7 @@ export function deleteEntry(collection, slug) { | |||
| dispatch(entryDeleting(collection, slug)); | |||
| return backend.deleteEntry(state.config, collection, slug) | |||
| .then(() => { | |||
| return dispatch(entryDeleted(collection, slug)); | |||
| return Promise.resolve(dispatch(entryDeleted(collection, slug))); | |||
There was a problem hiding this comment.
I don't think the Promise.resolve here isn't doing anything - dispatch(entryDeleted(...)) should already be returning a promise.
| @@ -319,7 +319,7 @@ export function deleteEntry(collection, slug) { | |||
| dismissAfter: 8000, | |||
| })); | |||
| console.error(error); | |||
| return dispatch(entryDeleteFail(collection, slug, error)); | |||
| return Promise.reject(dispatch(entryDeleteFail(collection, slug, error))); | |||
tech4him1
force-pushed
the
entry-stop-navigation-on-fail
branch
from
8ae941f
to
74662cb
Compare
|
@Benaiah Should be updated now -- makes for a simple PR! |
- Summary
Fixes #555:
- Test plan
Manually tested with the
test-repobackend by returning a rejected promise frompersistEntryanddeleteFile.- Description for the changelog
Do not close entry page if the entry failed to save or delete.
- A picture of a cute animal (not mandatory but encouraged)