Fix failing test for model update with custom id attribute.

[ilkerde]

This commit fixes a failing test for model updates with a custom id
attribute. It changes the strategy how updated fields from server-side
are merged with locally synced models. Instead of merging objects with
_.extend(), the received response is merged through set() method.

Background

As far as I can judge (I'm very new to the codebase), this issue might
have been introduced with PR36, which introduced merging model after
update. Changing the type of the second argument to localsync from
bb.Model to object seemed to cause the unwanted side effect that the
custom id attribute is no longer discoverable from within localsync.

Hence, the fixing strategy has been developed around the idea to adhere
to localsync type signature. The easiest solution seems to just
overwrite the existing local model instance with the received response
from server through model.set(), after previously having parsed the
response through model.parse() (just to be compatible to the overall
idea of being agnostic from response formats).

Motivation

I developed this fix in order to get the test suite to all green back
again. I stumbled over the failing test after I checked out the code for
investigation (in pursuit of a completely unrelated thing).

Notes

This is my first contribution to the codebase. Please review my change
thoroughly. I ran the test suite against Chromium and Firefox browsers.
Thanks for your efforts on backbone.dualStorage!

[nilbus]

Nice. I'll take a look at this soon.

[nilbus]

Good, this is a step in the right direction toward having consistent expectations for the localsync arguments (#37). This fixes the idAttribute problem for update.

To fix this problem entirely, we need to update all the other localsync calls to pass in backbone model objects as well.

This patch makes forces a failure if attributes are ever passed in directly.

diff --git a/backbone.dualstorage.coffee b/backbone.dualstorage.coffee
index bfc90c7..8d172fd 100644
--- a/backbone.dualstorage.coffee
+++ b/backbone.dualstorage.coffee
@@ -145,6 +145,9 @@ callbackTranslator =
 # Override `Backbone.sync` to use delegate to the model or collection's
 # *localStorage* property, which should be an instance of `Store`.
 localsync = (method, model, options) ->
+  unless model.sync # will be defined on both collections and models
+    console.error "Model: ", model if console?.error
+    throw 'got attributes instead of a model'
   store = new Store options.storeName

   response = switch method

The first thing that you'll notice if you add this is that all tests fail because most of the tests pass an attributes object instead of a real backbone model. Once the tests pass in real models, it should also point out any other internal calls that pass attributes, including one that I see immediately in dualSync read, lines 244 and 246.

I'm noting that what you're doing here with parse and set also happens in Backbone.Model.save success callback, which we call immediately after set. This means that for each save (update), the change callback would be fired twice. I'd really like to avoid that if possible.

Maybe what we should do is define a function that takes a model and a server response that returns a clone of the model with attributes updated to reflect any changes from the server. We could use that method to generate an updated model suitable for passing into localsync every time we would have otherwise passed in a server response.

Let me know what you think of that, and if you see any issues I missed.

[ilkerde]

Thanks for your detailed review. All the points you made seem very valid to me. Hence, I created a new PR #55, where I updated all other localsync calls as well made sure that change event is not fired during model set. Thanks for your efforts.

Closing this PR in favor of #55.

[nilbus]

You might have done this intentionally (which is fine), but in case you didn't know, you can push new commits to the same branch after creating a pull request, and the new commits will go into the existing pull request. That's my personal preference, to keep the conversation and everything in context. This works fine though.

Anyway, thank you for your work! Taking a look now.