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

HandleUpdate builds the wrong url #29

Closed
tmcgilchrist opened this issue Oct 29, 2014 · 8 comments
Closed

HandleUpdate builds the wrong url #29

tmcgilchrist opened this issue Oct 29, 2014 · 8 comments

Comments

@tmcgilchrist
Copy link

How handleUpdate builds the url for mocking doesn't seem to take into account the namespace preference supplied on an adapter.
If try mocking out an update from ember-data like this testHelper.handleUpdate('album', 1, {type: 'PUT'}) it builds a url for the model /albums/1' when it should really be/api/albums/1. I'm not really sure if this is an ember-data-factory-guy bug or something wrong with ember since it seems to be using the per adapterbuildUrl()` function which should work.

Also it seems that the default type for handleUpdate should be PUT since that's what ember-data uses by default when you do a save() on en existing model.

test 'update playCount', ->

  testHelper.handleUpdate('album', 1, {type: 'PUT'})
  # really gives testHelper.stubEndpointForHttpRequest('/albums/1', "{}", {type: 'PUT'})
  # should be testHelper.stubEndpointForHttpRequest('/api/albums/1', "{}", {type: 'PUT'})

  visit '/album/1'
  click 'button.play-album' # Generates a PUT to the server to update the album

  andThen ->
    equal find('#playCount').text(), "Played 1 times", "Updates the play count"
`import DS from 'ember-data'`

Album = DS.Model.extend
  name: DS.attr('string')
  artist: DS.attr('string')
  coverImage: DS.attr('string')
  year: DS.attr('date')
  playCount: DS.attr('number', {defaultValue: 0})
  tracks: DS.hasMany('track', { async: true })

`export default Album`
-------------------------------
`import DS from 'ember-data'`

AlbumAdapter = DS.RESTAdapter.extend
  namespace: 'api'

`export default AlbumAdapter`
@danielspaniel
Copy link
Collaborator

Are you sure handleUpdate is not using PUT ?
Not sure why you think it does not .. I am curious. But the code looks like this:

  handleUpdate: function (type, id, status) {
    this.stubEndpointForHttpRequest(
      this.buildURL(type, id),
      {},
      {type: 'PUT', status: (status || 200)}
    )
  },

And .. hmm .. that sure is a puzzler .. that the url is wrong .. since the buildURL method is a proxy to ember-data's buildUrl method ( which should create the correct url from the namespace which is set in the adapter .. you noticed this too )

  buildURL: function (type, id) {
    return this.getStore().adapterFor('application').buildURL(type, id);
  },

So I am a bit puzzled there .. but will try and run a few tests to see if I can reproduce what you are saying about the url being off. If the handleFind makes the correct url ( uses the same buildUrl method, not sure why handleUpdate would not? ) .. anyway .. for the sake of being super clear .. what ember-data-factory-guy version are you using, and it is the ruby gem or the bower version?

@tmcgilchrist
Copy link
Author

Sorry @danielspaniel you are correct, it uses PUT.

For this example I'm using embercli with Ember: 1.7.0, Ember Data : 1.0.0-beta.11 and ember-data-factory-guy "~0.7.3", with bower obviously.

The weird thing is that I see it building the correct url when it does a store.find('album', 1) inside the tests as the Chrome network tab shows PUT http://localhost:4200/api/albums/1 404 (Not Found)

Seems that ember-data-factory-guy uses this code to generate the url
this.getStore().adapterFor('application').buildURL(type, id)
while when I debug through ember it uses this
this.ajax(this.buildURL(type.typeKey, id, record), "PUT", { data: data })
where typeKey = "album"
id = "1"
record = EmberData record

I naively modified the ember-data-factory-guy call to pass through the same args but that didn't work.

@danielspaniel
Copy link
Collaborator

@tmcgilchrist .. let me know if the new version 0.7.7 ( which has the new buildUrl method ) fixes this issue for you .. not 100% sure it will .. but it is an improvement form the old buildUrl method

@tmcgilchrist
Copy link
Author

I'll give it a go this week and report back @danielspaniel

@indirect
Copy link
Contributor

I figured out why this problem is happening. When you're using a real backend, the response JSON includes an id attribute, and that attribute is set on the Ember model. The id attribute is then used to generate the handleUpdate URL.

When you use handleCreate to create the Ember model, there is no ID supplied, and the Ember model will still have an ID of null, even after it has been saved. With an ID of null, the URL that is generated for PUT will be missing an ID, and look like the IDs described above. I'm currently hacking around this with this really gross thing, but I'm open to suggestions for how to do it better:

// make the record and load it in the store
var model = store.makeFixture.apply(store,args);
// respond with the record plus an ID higher than existing records
var nextId = Math.max.apply(this, store.all('collection').mapBy('id'))
responseJson[modelName]=model.toJSON();
responseJson[modelName]['id'] = nextId;

@indirect
Copy link
Contributor

annnd I just noticed that this is fixed in master, with a much less hacky solution. :)

responseJson[modelName] = $.extend(options,{id: definition.nextId()});

@danielspaniel
Copy link
Collaborator

yes, that handleCreate took me forever to get right. I think I finally got it.

@danielspaniel
Copy link
Collaborator

@indirect .. in fixing up handleCreate to allow exact match and returns parameters (version 0.8.3) , I had to take out the part where I set the id for the simplest cases like handleCreate('project') , since if I return and value in the json ( even just the id ) .. all attributes you set in the store.createRecord('project', {attr1: 1, attr2: 2}) will be set to null ( since they are overwritten by returning json with nothing but an id ).
So, I will close this but want to alert you of this issue. To avoid this ( and get the id, and all your attributes, use the match parameter to make sure you get all your attributes back.

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

3 participants