[FEAT BUGFIX] enable canonical state updates to deleted records #5408
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
hjdivad
approved these changes
Apr 3, 2018
tests/unit/model-test.js
Outdated
| } | ||
| }) | ||
| } catch (e) { | ||
| assert.ok(false, e); |
There was a problem hiding this comment.
what does this try/catch achieve?
hjdivad
reviewed
Apr 3, 2018
tests/unit/model-test.js
Outdated
| set(record, 'isArchived', true); | ||
| assert.ok(false, 'Was unable to dirty a deleted record'); | ||
| } catch (e) { | ||
| assert.ok(true, e.message); |
There was a problem hiding this comment.
let's assert the specific error rather than merely that there was an error
tests/unit/model-test.js
Outdated
| } | ||
| }); | ||
| } catch (e) { | ||
| assert.ok(false, e); |
There was a problem hiding this comment.
what does this try/catch achieve?
There was a problem hiding this comment.
I think I can use expectAssertion here instead
There was a problem hiding this comment.
@hjdivad oh wait, actually, I can't unless there's a corollary? I basically want a "expect no assertion"
bmac
approved these changes
Apr 4, 2018
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
resolves #4995
replaces #5231
Problem
Records in the
root.deleted.savedstate currently error if new data is received from the server for the record while the record is still in the store.For instance, a user may desire the following flow, which is now possible with this PR:
Use Case
Sometimes deletions are "soft deletes", and sometimes deletions may be queued on the server and the deleted state not reflect immediately in other requests.
2.13 Regression
Prior to 2.13 we would unload records whose deletion had been persisted. While this enabled folks to push server updates back into the store for either use case above, this would create a new record in the
root.loaded.savedstate, which is also unexpected.This Fix
This fix enables canonical (but not dirty) state updates to
root.deleted.savedrecords to still occur. Records are still available to the UI until the user opts to callunloadRecord. This means that end users may themselves decide whether they want to unload and treat these records asundeletedagain, or maintain the deleted state.Soft Delete
User's desiring
soft deleteakaarchivingwith the ability to resurrect a model should not be usingdeleteRecordordestroyRecord. While this PR will make soft-deletions via this mechanism work slightly better up until the point that resurrection is desired, users should instead fire an action to update a flag of their own making, and only fire a DELETE for a permanent deletion.