[FEATURE ds-rollback-attribute] Add rolling back of a single model attribute #4246

courajs · 2016-03-18T18:45:48Z

First pass at #3705
First feature PR!
Is there any subtlety around in-flight records that I'm missing?

bcardarella · 2016-03-18T21:01:59Z

addon/-private/system/model/internal-model.js

@@ -435,6 +435,7 @@ InternalModel.prototype = {
    this.record._notifyProperties(dirtyKeys);

  },
+


Probably don't want to introduce additional whitespace

Actually, all other methods in this file have whitespace separators, so I left it in as a small style fix.

pangratz · 2016-03-18T21:24:39Z

Basically 👍 to the feature! My gut feeling says that this should be handled via the state machine though. Take for example the case where a record in in the invalid state and a property is reset: in this case the corresponding error is removed and the record could become valid. Not sure if there are other cases, but inFlight might also be a noteworthy scenario, as you @courajs already said.

I hope that I have time this weekend to take a deeper look. Will keep you posted!

pangratz · 2016-04-11T19:26:31Z

Hey @courajs,

finally I have time to get back to you here. I am very sorry for the delay 😔.

This has been discussed in the team meeting and we agreed that this functionality would be useful. We think that naming the method rollbackAttribute might be confusing though, so we suggest to name it resetAttribute. This method should basically set the attribute to the latest inFlight value or the canonical value from the adapter.

Can you update this PR and:

rename the method
add test for when an attribute is reset when the record is in flight

Thanks for your work so far! Looking forward to getting this merged!

pangratz · 2016-04-11T19:31:46Z

addon/-private/system/model/internal-model.js

+      this.send('propertyWasReset', attributeName);
+      this.record.notifyPropertyChange(attributeName);
+    }
+  }


To leave all the logic to the state machine I would suggest adding a new method to the InternalModel which returns the latest truth for an attribute: either the inFlight value or the canonical value. Then, in DS.Model#resetAttribute we simply get the latest value from the internal model and set it. By this everything should basically behave the same as if the attribute would be reset manually to the latest value...

What do you think?

Good suggestion!

courajs · 2016-04-11T23:46:32Z

@pangratz Sounds good to me

pangratz · 2016-05-08T18:34:49Z

@courajs kindly asking if you had any chance of getting this up to date? I would love to see this feature in master 😉 . Ping me if you need further assistance, happy to help!

courajs · 2016-05-10T15:32:10Z

@pangratz Thanks for the ping! This is mostly updated now, but I bumped into an existing bug with in-flight records we need to fix first. I'll have a PR for that soon and I'll link it here

courajs · 2016-05-10T16:09:21Z

I believe this build only failed because of #4379, and when it's merged these tests will pass

courajs · 2016-05-10T16:26:22Z

Confirmed locally, a rebase on top of #4379 and the tests pass.

courajs · 2016-05-12T19:05:29Z

@pangratz I think we're pretty much good here! Any final feedback?

pangratz · 2016-05-12T19:08:15Z

I meant to review but got caught up in work. I will try to review not later than ~~saturday~~ sunday. Thanks for the work so far!

courajs · 2016-05-12T19:19:05Z

sounds good :)

pangratz · 2016-05-15T16:42:11Z

addon/-private/system/model/internal-model.js

+     @method latestTrueValue
+     @private
+  */
+  InternalModel.prototype.latestTrueValue = function latestTrueValue(key) {


Though this is an internal method, I am not not too happy about the name here to be honest. Coming up with a better one is difficult though 😔 What do you think of lastPreassignedAttributeValue() or even lastCanonicalOrInFlightAttributeValue() or something? I know this is a tiny nitpick, but I was hoping we find a better name here...

I could settle for lastCanonicalOrInFlightAttributeValue. What about latestSavedValue or latestComittedValue? Since it's not necessarily canonical yet, but it's trying to be.

I tend to favor the - admittedly verbose but expressive - lastCanonicalOrInFlightAttributeValue. We can change that always later once we find a better name...

Let's use lastAcknowledgedValue here. This should be clear enough.

pangratz · 2016-05-15T17:02:41Z

Overall, this looks very good 🚀

Some few additions and this is ready to

please add a test which asserts that nothing fancy happens if we reset an attribute, which hasn't changed and a test for reseting an attribute which doesn't exist (just want to make sure a future regression are caught)
add another test reseting an in flight attribute for a record which has been saved twice: the value of the attribute should be the one of the latest save()
add an entry to FEATURES.md which outlines the basic usage

Thanks!

courajs · 2016-05-15T17:04:42Z

👍

courajs · 2016-05-17T16:50:26Z

@pangratz updated!

pangratz · 2016-05-23T14:32:11Z

This just needs a rename for the latestTrueValue method (see my comment #4246 (comment)) and a rebase onto latest master, then this PR is good to go 🚀

courajs · 2016-05-23T16:03:44Z

Whoops! I thought I did that rename 😅
On it

leifdejong · 2016-05-26T14:09:14Z

@courajs how is this feature going?

courajs · 2016-05-26T16:49:20Z

Just needs the rename mentioned above and a rebase when I get a chance

courajs · 2016-05-28T18:42:03Z

@pangratz updated!

…bute

courajs · 2016-06-23T21:19:41Z

@pangratz I think we're finally ready for a merge here. Look good?

pangratz · 2016-06-24T11:59:32Z

Awesome! Thanks for the hard work on this @courajs 🚀 🎉

bcardarella reviewed Mar 18, 2016
View reviewed changes

bmac added the team-review label Mar 28, 2016

pangratz reviewed Apr 11, 2016
View reviewed changes

pangratz removed the team-review label Apr 11, 2016

courajs force-pushed the rollback-attribute branch from c5a8a0b to 3382029 Compare May 10, 2016 15:41

courajs mentioned this pull request May 10, 2016

[BUGFIX beta] Fix resetting of properties to in-flight values #4379

Merged

pangratz self-assigned this May 12, 2016

courajs force-pushed the rollback-attribute branch from 3382029 to 1776449 Compare May 12, 2016 13:35

pangratz reviewed May 15, 2016
View reviewed changes

courajs force-pushed the rollback-attribute branch 2 times, most recently from 38545cf to c372bb4 Compare May 17, 2016 16:09

courajs force-pushed the rollback-attribute branch from c372bb4 to f66a75b Compare May 28, 2016 18:16

courajs force-pushed the rollback-attribute branch from f66a75b to 6d510d3 Compare June 20, 2016 17:28

[FEATURE ds-reset-attribute] Add rolling back of a single model attri…

9cb1b33

…bute

courajs force-pushed the rollback-attribute branch from 6d510d3 to 9cb1b33 Compare June 23, 2016 20:42

pangratz merged commit 3a38988 into emberjs:master Jun 24, 2016

courajs deleted the rollback-attribute branch June 24, 2016 13:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FEATURE ds-rollback-attribute] Add rolling back of a single model attribute #4246

[FEATURE ds-rollback-attribute] Add rolling back of a single model attribute #4246

courajs commented Mar 18, 2016

bcardarella Mar 18, 2016

courajs Mar 18, 2016

pangratz commented Mar 18, 2016

pangratz commented Apr 11, 2016

pangratz Apr 11, 2016

courajs Apr 11, 2016

courajs commented Apr 11, 2016

pangratz commented May 8, 2016

courajs commented May 10, 2016

courajs commented May 10, 2016

courajs commented May 10, 2016

courajs commented May 12, 2016

pangratz commented May 12, 2016 •

edited

Loading

courajs commented May 12, 2016

pangratz May 15, 2016

courajs May 15, 2016

pangratz May 15, 2016

pangratz May 23, 2016

pangratz commented May 15, 2016

courajs commented May 15, 2016

courajs commented May 17, 2016

pangratz commented May 23, 2016 •

edited

Loading

courajs commented May 23, 2016

leifdejong commented May 26, 2016

courajs commented May 26, 2016

courajs commented May 28, 2016

courajs commented Jun 23, 2016

pangratz commented Jun 24, 2016

		@@ -435,6 +435,7 @@ InternalModel.prototype = {
		this.record._notifyProperties(dirtyKeys);

		},

[FEATURE ds-rollback-attribute] Add rolling back of a single model attribute #4246

[FEATURE ds-rollback-attribute] Add rolling back of a single model attribute #4246

Conversation

courajs commented Mar 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pangratz commented Mar 18, 2016

pangratz commented Apr 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

courajs commented Apr 11, 2016

pangratz commented May 8, 2016

courajs commented May 10, 2016

courajs commented May 10, 2016

courajs commented May 10, 2016

courajs commented May 12, 2016

pangratz commented May 12, 2016 • edited Loading

courajs commented May 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pangratz commented May 15, 2016

courajs commented May 15, 2016

courajs commented May 17, 2016

pangratz commented May 23, 2016 • edited Loading

courajs commented May 23, 2016

leifdejong commented May 26, 2016

courajs commented May 26, 2016

courajs commented May 28, 2016

courajs commented Jun 23, 2016

pangratz commented Jun 24, 2016

pangratz commented May 12, 2016 •

edited

Loading

pangratz commented May 23, 2016 •

edited

Loading