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

can.Model#save: values for attributes not found in response are replaced with their defaults #560

Closed
ghost opened this Issue Nov 19, 2013 · 8 comments

Comments

Projects
None yet
2 participants
@ghost

ghost commented Nov 19, 2013

I have a model with default values for some attributes.
The problem is that after model update I don't return all attribute values in response, only required (time stamp etc...) and my current attribute values are replaced with default values.

For example I have a model with attribute 'a' and default value for that attribute is 100. I set the value to 90 and call model save that performs update operation. On update I return some attributes and 'a' is not there.
After update I have my 'a' attribute set to 100 again.

After looking into code I found that if attributes from server are supplied then 'model' method is called and new model instance with default attribute values is created. The created instance is then used to update current model instance replacing current values with defaults.

I think the behavior is incorrect and default attribute values should not be applied.
I will add a pull request for this issue.

@ghost ghost referenced this issue Nov 20, 2013

Closed

Added test to issue #560 #561

@ghost ghost assigned daffl Nov 20, 2013

@daffl

This comment has been minimized.

Show comment
Hide comment
@daffl

daffl Nov 20, 2013

Contributor

Defaults should only work with the attributes plugin anwyay. I'll look into why it isn't updating that properly.

Contributor

daffl commented Nov 20, 2013

Defaults should only work with the attributes plugin anwyay. I'll look into why it isn't updating that properly.

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Nov 20, 2013

Contributor

Not anymore. You can add defaults to the prototype directly now.

Sent from my iPhone

On Nov 20, 2013, at 1:59 PM, David Luecke notifications@github.com wrote:

Defaults should only work with the attributes plugin anwyay. I'll look into why it isn't updating that properly.


Reply to this email directly or view it on GitHub.

Contributor

justinbmeyer commented Nov 20, 2013

Not anymore. You can add defaults to the prototype directly now.

Sent from my iPhone

On Nov 20, 2013, at 1:59 PM, David Luecke notifications@github.com wrote:

Defaults should only work with the attributes plugin anwyay. I'll look into why it isn't updating that properly.


Reply to this email directly or view it on GitHub.

@daffl

This comment has been minimized.

Show comment
Hide comment
@daffl

daffl Nov 20, 2013

Contributor

We should document that ;)

Contributor

daffl commented Nov 20, 2013

We should document that ;)

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Nov 23, 2013

Contributor

http://canjs.com/docs/can.Map.prototype.DEFAULT-ATTR.html

documented. I'll change the test to reflect.

Contributor

justinbmeyer commented Nov 23, 2013

http://canjs.com/docs/can.Map.prototype.DEFAULT-ATTR.html

documented. I'll change the test to reflect.

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Nov 23, 2013

Contributor

Arg, I should have sorted all this out for 2.0. The problem is that updated / created is ran through Model.model() which creates a new instance. That new instance will have default properties that will overwrite the original properties.

Contributor

justinbmeyer commented Nov 23, 2013

Arg, I should have sorted all this out for 2.0. The problem is that updated / created is ran through Model.model() which creates a new instance. That new instance will have default properties that will overwrite the original properties.

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Nov 23, 2013

Contributor

@daffl I'm not sure the best way of fixing this and keeping backwards compatibility. The problem is that
.models should not be called by update and create. But this was forced for #301.

The long-term fix is to support a .parseModel() and .parseModels(). These would be called to get data from the server. .model and .models() would call this and so would update and create.

For 2.0.3, I am thinking that if .parseModel or .parseModels is provided things will work. If .model and .models are not provided, things will work. But, if .model and .models are provided without .parseModel and .parseModels it will break.

Contributor

justinbmeyer commented Nov 23, 2013

@daffl I'm not sure the best way of fixing this and keeping backwards compatibility. The problem is that
.models should not be called by update and create. But this was forced for #301.

The long-term fix is to support a .parseModel() and .parseModels(). These would be called to get data from the server. .model and .models() would call this and so would update and create.

For 2.0.3, I am thinking that if .parseModel or .parseModels is provided things will work. If .model and .models are not provided, things will work. But, if .model and .models are provided without .parseModel and .parseModels it will break.

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Nov 23, 2013

Contributor

Oh, if .models is a string, I will set that as .parseModels too.

Contributor

justinbmeyer commented Nov 23, 2013

Oh, if .models is a string, I will set that as .parseModels too.

justinbmeyer added a commit that referenced this issue Nov 23, 2013

@daffl

This comment has been minimized.

Show comment
Hide comment
@daffl

daffl Mar 21, 2014

Contributor

Looks like this has been fixed via 9c6a55a

Contributor

daffl commented Mar 21, 2014

Looks like this has been fixed via 9c6a55a

@daffl daffl closed this Mar 21, 2014

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