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

Belongs to not updating when response contains a change with the previous value #4470

Merged
merged 1 commit into from
Jul 12, 2016

Conversation

chrisgame
Copy link
Contributor

@chrisgame chrisgame commented Jul 8, 2016

If the response to a save of a model that includes a belongs to relationship is successful but that response contains data that sets the belongs to back to it's original value the change is not applied to the store. #4469

@chrisgame chrisgame force-pushed the belongs-to-not-updating branch 2 times, most recently from 9bd817a to 3969de4 Compare July 11, 2016 09:11
@@ -31,11 +31,14 @@ BelongsToRelationship.prototype.setRecord = function(newRecord) {
};

BelongsToRelationship.prototype.setCanonicalRecord = function(newRecord) {
if (this.canonicalState) {
this.removeCanonicalRecord(this.canonicalState);
}
Copy link
Member

Choose a reason for hiding this comment

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

I think the real fix is the fact that removeCanonicalRecord ends up calling this.flushCanonicalLater(); somewhere in its call stack. Instead of moving this check up I'd prefer to see it left where it is an add an explicit call to this.flushCanonicalLater(); before this.setHasData(true);.

@chrisgame chrisgame force-pushed the belongs-to-not-updating branch from 3969de4 to a7b8ad9 Compare July 11, 2016 14:50
@jgwhite
Copy link
Contributor

jgwhite commented Jul 12, 2016

@bmac would you mind giving this another look over?

@bmac
Copy link
Member

bmac commented Jul 12, 2016

Looks good. 👍 Thanks @chrisgame.

@bmac bmac merged commit 02968c9 into emberjs:master Jul 12, 2016
@chrisgame
Copy link
Contributor Author

No problem, thank you @bmac

@corincerami
Copy link

I'm not sure if this is a breaking change or if our app was relying on a load bearing bug, but this is causing us some issues. We have a workaround, but I just wanted to check that this is indeed the intended behavior.

@bmac
Copy link
Member

bmac commented Nov 10, 2016

@chrisccerami I believe this is a bug fix as now relationship have the same behavior as attributes when the server returns a new value for the property. I am interested in hearing more about how this change is impacting your app. Do you mind sharing more details about your desired behavior and work around?

@corincerami
Copy link

Oh, it's not about desired behavior, and if this is the correct behavior then I'm sure it means someone did something silly in the way they built the particular part that broke. I believe it was something like we were updating a record and its hasMany association, saving the record, and then with the response we were saving the hasMany. This was working as of 2.6.2, but not 2.7.0. In order to work around it, we're now updating the record and saving it, and then updating the hasMany and saving that. I was only surprised to have something break in a minor release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants