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

Add ability to rollback specific attributes #3705

Closed
Panman82 opened this issue Aug 26, 2015 · 21 comments
Closed

Add ability to rollback specific attributes #3705

Panman82 opened this issue Aug 26, 2015 · 21 comments

Comments

@Panman82
Copy link

TL;DR model.rollbackAttributes(['attrA','attrC'])

It would be nice if the .rollbackAttributes() method accepted an array of attribute names that would be rolled back, leaving the others as-is. Ex: model.rollbackAttributes(['attrA','attrC'])

So in the example above, attributes 'attrA' and 'attrC' would have their values reverted to the previous, if changed. Other attributes, such as 'attrB', would retain their new value, if changed.

Related state information should properly reflect the rollback; hasDirtyAttributes and changedAttributes(). If there are other attributes not rolled back ('attrB') that have changed values, then hasDirtyAttributes would remain true. And calls to changedAttributes() would not return the rolled back attributes, but would return others that were changed.

Passing in an empty array should trigger the default behavior, as though no array was passed in at all, rolling back all attributes.

@Panman82
Copy link
Author

Another situation to be aware of, if model.get('isNew') === true and model.rollbackAttributes(), the model is put in a "deleted" state. So, if model.rollbackAttributes(['attrA','attrC']) but "attrB" is also changed, the model should not be rolled back into a "deleted" state.

@bmac
Copy link
Member

bmac commented Aug 27, 2015

Interesting idea @Panman8201. Would you be interested in working on an implementation?

@Panman82
Copy link
Author

@bmac I took a look at the internal-model.js rollbackAttributes() but I'm having a tough time following along. The whole internal model thing confuses me. Perhaps with direction I could figure it out.

@courajs
Copy link
Contributor

courajs commented Mar 18, 2016

I'd be interested in taking a stab at this. I propose the following:

  • We add model.rollbackAttribute('attr').
  • This should be equivalent to model.set('attr', originalValue). That is, it should update isDirty appropriately, but would not roll a newly created record back into a deleted state. It would be very confusing to change the state of the record based on whether other attributes have been set or not.
  • We don't add model.rollbackAttributes(['attrA', 'attrB']). This is debatable, but given the point above, I wouldn't want to muddle the rollbackAttributes API. It would act very differently in the array-arg and no-arg cases, and this would be confusing.

@Panman82
Copy link
Author

@courajs I'd still love to have this feature. While I'm not part of the ember-data team at all, I wouldn't mind if you took a stab at it!

@Panman82
Copy link
Author

@courajs Do you know if this made it in ember data 2.8.0-beta.1? I just tried and it seems like it didn't, but it is in the changelog... Perhaps it's still under a feature flag and needs to be enabled explicitly?

@courajs
Copy link
Contributor

courajs commented Jul 26, 2016

Yeah - the commits are included, but the feature is still behind a disabled feature flag: https://github.com/emberjs/data/blob/v2.8.0-beta.1/config/features.json#L10
You'd have to use a canary build until the ember-data team discuss at a meeting and enable the feature flag in your config.

@pangratz
Copy link
Member

@Panman8201 @courajs I will bring this up in the next ember-data weekly call to discuss if this should be enabled in the next beta release. For now you'd need a canary build and explicitly enable the feature...

@bluscreen
Copy link

I had the same issue and managed to create a little workaround via changedAttributes like so:
rollbackSpecificAttribs: function (specificAttribs) { const changedAttribs = this.changedAttributes() Object.keys(changedAttribs).forEach(attrib => { if (specificAttribs.includes(attrib)) { const oldVal = changedAttribs[attrib][0]; this.set(attrib, oldVal) } }) }
currently works only for flat structures and might be risky but it seems sufficient for my case..

@courajs
Copy link
Contributor

courajs commented Oct 26, 2016

@bluscreen That's pretty close to what has been implemented behind the ds-reset-attribute feature flag. I think you're safe using that for now if you don't want to use a canary build with feature flags.

@courajs
Copy link
Contributor

courajs commented Oct 26, 2016

@pangratz Did this fall off the agenda?

@pangratz
Copy link
Member

@courajs 😔 yes, unfortunately it did. I am sorry.

We discussed this recently and despite our initial idea about naming this resetAttribute to avoid confusion with rollback, it does introduce a new verb reset which might be even more confusing. So we decided that your initial proposal of rollbackAttribute might actually be the better name.

@courajs can I get you one more time to change this back to rollbackAttribute and rename the feature to ds-rollback-attribute? Thank you so much for your patience on this.

@thomaswrenn
Copy link

+1

@Panman82
Copy link
Author

Panman82 commented Apr 12, 2017

@courajs it has been for some time. Any status update for when this might be enabled and available in release? (from the ED team obviously)

@Panman82
Copy link
Author

Any specific reason for closing @stefanpenner ? I realize the feature has been merged but it's not yet enabled for beta/production. I was hoping to use this issue to keep track of status updates..

@Techn1x
Copy link

Techn1x commented Oct 24, 2017

2 years later and this is still behind a feature flag :(

What needs to happen to get this into ember stable?

@rinoldsimon
Copy link

a solution is to create a new model every time when the user hits discard changes:

  model.rollbackAttributes(); 
  model = this.store.createRecord('...');

@Techn1x
Copy link

Techn1x commented Mar 13, 2018

@stefanpenner can we get some more info on this? Why is this still behind a Canary Build? Why was this issue closed? I was really hoping this would be released with Ember 3.0. This feature is particularly relevant for having an attribute on a model that has it's own modify/rollback mechanism (i.e. edit and cancel buttons).

For example, I have a Truck model, that has a Mapping attribute. On the Truck Edit form, the user can modify most simple attributes normally (eg type, color, icon, etc.), but there is a 'Change Mapping' button for if they want to show & modify the mapping attr, along with a Cancel button. I need this Cancel button to rollback this single attribute to its original state.

This Mapping attribute is fairly simple and isn't complex enough to require full blown ember-data-model-fragments which relies on private APIs.

This is just one of many uses for model.rollbackAttribute()

@runspired
Copy link
Contributor

We recently removed the feature flag. With the impending merge of the RecordData RFC, alternative model layers that are better suited for buffering and undoing edits can be explored :D We may want to still revisit this as an RFC in the near future.

@seanCodes
Copy link
Contributor

@stefanpenner @runspired Revisiting this after a couple years. Any update on this front? Is there a better/alternative way to do this or is it on the roadmap?

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

No branches or pull requests