Fix all the things... #256

Closed
wants to merge 10 commits into from
@ghempton
Ember.js member

This is an attempt at making things tenable for common transactional scenarios. This PR supersedes #253, #252, #248 as well as a fix for #254.

Specifically, the following scenarios were previously broken, but are now fixed:

  1. Modifying the parent and deleting a child. Logic was added inside Model.dataDidChange to check if the association data is stale and contains deleted records.

  2. New parent-child hierarchies. Pending records are handled inside Model.dataDidChange.

  3. Records can be added and deleted in Many Arrays. This previously worked only for record arrays returned by Store.findAll. All many arrays are now properly tracked by the store.

  4. Embedded associations now work. Some special handling was needed inside Model.dataDidChange

  5. Modifying the parent and adding a child. For now, newly added child records will be put into the pending state even if the parent already has an id. This forces the child record to wait for the parent to be updated. Not ideal, but it works without ambiguity.

P.S. Sorry about the large number of commits. I can rebase into a single commit if the powers that be so desire.

@heeton heeton commented on the diff May 21, 2012
...ges/ember-data/lib/system/record_arrays/many_array.js
@@ -31,8 +46,9 @@ DS.ManyArray = DS.RecordArray.extend({
// Overrides Ember.Array's replace method to implement
replace: function(index, removed, added) {
var parentRecord = get(this, 'parentRecord');
- var pendingParent = parentRecord && !get(parentRecord, 'id');
+ var pendingParent = parentRecord && get(parentRecord, 'isDirty'); // && !get(parentRecord, 'id');
@heeton
heeton added a note May 21, 2012

// Whoops

that seems dangerous and I'd say it's wrong. The parent is only pending if it does not have an ID, from what I have seen so far.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@endash

+1 The parent/child hierarchy create fix FTW. Only issue: there's a 'blip' where the record resets and any collectionviews bound to the children of the parent element redraw.

@xdissent

This PR fixes embedded associations for me except it doesn't update computed properties based on the associated properties. For example, see this test which will pass all assertions but the last 2: https://gist.github.com/2860403

@xdissent

Here's a fiddle showing the test mentioned above failing: http://jsfiddle.net/xdissent/Y3rAa/

@guilhermeaiolfi

I'm not sure if that's related to associations at all. I think that's how ember works. At least for cached properties. See how all tests pass using volatile(): http://jsfiddle.net/Y3rAa/7/

@skurfuerst

I have another use-case where I can confirm that this fix solves a bug:

  • I have two classes A and B, where A.elements is a hasMany relation of B objects. the relation is a non-embedded relation, i.e. it's the m:n case.
  • now, open a new transaction, put in A; create a number of new B-objects inside that transaction and add them to A.elements
  • when now reverting the transaction, A and all new B objects are reverted to the initial state (and the new B's are marked as deleted) -- so that works as expected. However, the new B objects were not removed from A.elements.

This pull request fixes this issue.

Should I try to create a unit test reproducing this behavior and add it to this pull request?

Greets, Sebastian

@ralfschimmel

Seems that this would also fix the issue I just posted; #331

@renajohn

+1
This pull request would solve my current problem.

@sly7-7

@ghempton @wagenet

What the status of this ? I know this probably won't merge with the relationship-improvements branch, but for now, I encounter some problems with ember-data, which seems to be related to this PR.

Modifying an attribute of a parent + adding/removing a child in a single transaction (with store.commit()) results in two requests, PUT parent, POST child. As a consequence the parent response comes without the new child, and the related ember-data model does not contains the child. When the POST response arrives, the parent model is not updated against it.

I tried to cut into several transactions (first transaction commit the children, second transaction deals with parent). It seems to work at the first sight, but playing a little with that, and I encounter some "ManyArrayStateManager: cannot send childWasSaved to state clean"

Could you give me some hints ? Could I try to merge this PR in a fork ?

@wagenet
Ember.js member

@ghempton Can you revisit this now that we've merged the RI branch?

@wagenet
Ember.js member

Closing due to inactivity.

@wagenet wagenet closed this Feb 5, 2013
@ghempton
Ember.js member

@wagenet sorry for the inactivity. this PR is way outdated

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment