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

Fix the Refresh of Lists when an optimistic update fails #4179

Merged
merged 1 commit into from
Feb 13, 2020
Merged

Fix the Refresh of Lists when an optimistic update fails #4179

merged 1 commit into from
Feb 13, 2020

Conversation

m4theushw
Copy link
Contributor

Fixes #4176

@m4theushw m4theushw changed the title Refresh list on failure [RFR] Refresh list when an update fails Dec 17, 2019
ThieryMichel
ThieryMichel requested changes Jan 3, 2020
View reviewed changes
Copy link
Contributor

@ThieryMichel ThieryMichel left a comment

Choose a reason for hiding this comment

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

Nice one. Could you add test ? Please.

@fzaninotto
Copy link
Member

You must also remove the call to the refresh() side effect in useEditController and useShowController, otherwise the refresh will occur twice.

@m4theushw
Copy link
Contributor Author

@fzaninotto I'm calling refresh only on undoable operations. I think that is not the case of useEditController and useShowController since they refresh when getOne fails. Am I right?

@fzaninotto
Copy link
Member

Right, but then with your patch there will still be two refreshes in case of optimistic error.

So the right way to fix this bug is to add a refresh as onFailure side effect in the save function of useEditController and useCreateController.

@m4theushw m4theushw force-pushed the refresh-list-on-failure branch from 4d24f30 to feeb0e0 Compare January 3, 2020 18:20
@m4theushw m4theushw requested a review from ThieryMichel January 3, 2020 18:22
@m4theushw
Copy link
Contributor Author

I didn't add a refresh in useCreateController because it's not undoable and this bug only occurs with optimistic saving.

@JulienMattiussi
Copy link
Contributor

Closes #3244

@JulienMattiussi JulienMattiussi added the RFR Ready For Review label Feb 12, 2020
@JulienMattiussi JulienMattiussi changed the title [RFR] Refresh list when an update fails Fix the Refresh of Lists when an optimistic update fails Feb 12, 2020
@fzaninotto fzaninotto merged commit 709be90 into marmelab:master Feb 13, 2020
@fzaninotto
Copy link
Member

Thanks!

@fzaninotto fzaninotto added this to the 3.2.2 milestone Feb 13, 2020
@fzaninotto
Copy link
Member

hm, this PR, once merged to master, breaks the e2e tests. I'll take a look.

@fzaninotto
Copy link
Member

Fixed in b232120

@JulienMattiussi JulienMattiussi mentioned this pull request Feb 14, 2020
Closed
This was referenced Apr 10, 2020
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ThieryMichel ThieryMichel ThieryMichel approved these changes

@JulienMattiussi JulienMattiussi JulienMattiussi approved these changes

Assignees
No one assigned
Labels
RFR Ready For Review
Projects
None yet
Milestone
3.2.2
Development

Successfully merging this pull request may close these issues.

List does not refresh after API error (video included)
4 participants
@m4theushw @fzaninotto @JulienMattiussi @ThieryMichel