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

Changing modelName to type #39

Merged
merged 1 commit into from
Dec 5, 2014

Conversation

ksykulev
Copy link
Contributor

@ksykulev ksykulev commented Dec 4, 2014

modelName does not appear to be in scope in the pushPayload function.
It appears that this is a typo for type which is the first argument to
the pushPayload function.

modelName does not appear to be in scope in the pushPayload function.
It appears that this is a typo for type which is the first argument to
the pushPayload function.
danielspaniel added a commit that referenced this pull request Dec 5, 2014
Changing modelName to type
@danielspaniel danielspaniel merged commit 5c229c6 into adopted-ember-addons:master Dec 5, 2014
@danielspaniel
Copy link
Collaborator

@ksykulev Good find, I have been ignoring the fixture adapter for a while though and considering dropping support for it altogether. Are you using fixture adapter for testing?

@ksykulev
Copy link
Contributor Author

ksykulev commented Dec 5, 2014

Yes unfortunately we are. I would say 90%-95% of our tests use the rest adapter(ActiveModelAdapter), but there are cases when we need to drop down to the fixture adapter. Most notably when we do stuff with findQuery

Ember.Route.extend({
...
  model: function() {
    var params = { zip: '94804', code: 'RM' };
    return this.store.find('facility', params);
  }
...
});

Normally we would use mockjax or sinon to create a fake server/response.. but some of our models have lots of embedded associations so mocking out such a response gets tedious and difficult to manage. Thus we drop down to the fixture adapter.
I have a few more pull requests I need to wrap up to enhance fixture adapter support. But if you have other more clever ways of handling these types of situations, i'll be more than happy to switch my test code.

@danielspaniel
Copy link
Collaborator

Hmm.. this is an interesting use case. I have been pondering a few ideas, but nothing super fantastic has hit me yet, but I will let you know if something does.

@danielspaniel
Copy link
Collaborator

@ksykulev .. I added a handleFindQuery method to FactoryGuyTestHelperMixin just now.
It works very very similarly to the way you are using the fixture adapter for a store.findQuery call.
Check it out in the current source ( tag 0.9.2 ) and in the documentation .. shows how to use it.
Can you let me know if this works for you? Or give me any feedback and ideas for improvement.

@ksykulev
Copy link
Contributor Author

@danielspaniel - This is a great.

I have two fixes to the fixture adapter I have been working on.
One to fix and enable tests:
https://github.com/ksykulev/ember-data-factory-guy/tree/fixture-adapter-tests
One for pushPayload:
https://github.com/ksykulev/ember-data-factory-guy/tree/pushpayload-embedded

I will try to wrap those up this week and get them in a pull requestable state.

Then I will run through all of our tests with the new handleFindQuery method to see if I can get rid of the fixture adapter. Will let you know what I discover. Thanks again!

@danielspaniel
Copy link
Collaborator

awesome @ksykulev look forward to your updates to fixture adapter .. and getting those tests passing again.

@ksykulev
Copy link
Contributor Author

@danielspaniel - I went through a lot of tests with the new handleFindQuery code. I was able to get rid of the fixture adapter in a lot of places. Nice!

I think relationships were a bit tricky to figure out..

With embedded relationships I hydrated the data store with the embedded models and just return the parent model with ids:

//Models
Manufacturer = DS.Model.extend({
  name: DS.attr('string'),
  cars: DS.hasMany('car', { embedded: 'always' })
});
Car = DS.Model.extend({
  name: DS.attr('name'),
  year: DS.attr('number'),
  manufacturer: DS.belongsTo('manufacturer', { embedded: 'always' })
  parts: DS.hasMany('part', { embedded: 'always' })
});
Part = DS.Model.extend({
  name: DS.attr('string'),
  cars: DS.hasMany('car', { embedded: 'always' })
});

var manufacturer = FactoryGuy.make('manufacturer');
var parts = FactoryGuy.makeList('part', 3);
var cars = FactoryGuy.buildList('car', 2, { manufacturer: manufacturer.id, parts: parts });

testHelper.handleFindQuery('car', ['year', 'manufacturer'], cars);

Not bad. I'm not sure if this is going to run me into trouble later, but it seems to be working fine for now.

Async relationships worked in a similar way, except I would have multiple handleFindQuery calls.

//Models
Car = DS.Model.extend({
  name: DS.attr('name'),
  year: DS.attr('number'),
  parts: DS.hasMany('part', { embedded: 'async' })
});
Part = DS.Model.extend({
  name: DS.attr('string'),
  cars: DS.hasMany('car')
});

var parts = FactoryGuy.buildList('part', 3);
var cars = FactoryGuy.buildList('car', 2);
cars[0]['parts'] = parts.slice(0, 1);
cars[0]['parts] = parts.slice(1);

testHelper.handleFindQuery('car', ['year', 'manufacturer'], cars);
testHelper.handleFindQuery('part', ['car'], parts);

The tricky part was when I made use of links for async models.

{
  cars: [
    { id: 1, name: 'Model S', year: 2014, 'links': { 'parts': '/api/cars/1/parts' } },
    { id: 2, name: 'Model X', year: 2014, 'links': { 'parts': '/api/cars/2/parts' } }
  ]
}

So instead of going to /api/parts?car_id=1 we use /api/cars/1/parts. Again not too bad:

var cars = FactoryGuy.buildList('car', 2);
cars.forEach(function(car){
  car['links']['parts'] =  '/api/cars/'+car.id+'/parts';
});

@danielspaniel
Copy link
Collaborator

@ksykulev .. I will look into the embedded relationships and see what I can do.

@ksykulev
Copy link
Contributor Author

@danielspaniel - I wonder if it could be something as simple as...

var manufacturer = FactoryGuy.make('manufacturer');
var parts = FactoryGuy.buildList('part', 3);
var cars = FactoryGuy.buildList('car', 2, { manufacturer: manufacturer.id, parts: parts });
var embedded = {
  parts: parts,
  manufacturer: [manufacturer]
}
testHelper.handleFindQuery('car', ['year', 'manufacturer'], cars, embedded);

And then just merge the embedded hash into the response along with the base.

@danielspaniel
Copy link
Collaborator

Hmm .. your experiments remind of the things I do to get this stuff working .. I am on a bit of a holiday ( so sorry if I am slow to respond ) ... Does it work to "make" the records with the hasMany and belongsTo like

var manufacturer = FactoryGuy.make('manufacturer');
var parts = FactoryGuy.makeList('part', 3);
var cars = FactoryGuy.makeList('car', 2, { manufacturer: manufacturer, parts: parts });
testHelper.handleFindQuery('car', ['year', 'manufacturer'], cars)

Have you tried that? I am guessing it does not work? But that should and I will fix things so that it does if not, because that was my intent .. to be able to create instances and use handlefindQuery to return them

@danielspaniel
Copy link
Collaborator

Also, you could cut down the code with traits like:

var cars = FactoryGuy.makeList('car', 2, 'with_manufacturer', 'with_parts');
testHelper.handleFindQuery('car', ['year', 'manufacturer'], cars)

@ksykulev
Copy link
Contributor Author

@danielspaniel - I think I tried that and I don't remember it working..
But no worries. No rush.
Have a good holiday!

@danielspaniel
Copy link
Collaborator

@ksykulev, I just did a test and this style worked for me .. the one like:

var cars = FactoryGuy.makeList('car', 2, 'with_manufacturer', 'with_parts');
testHelper.handleFindQuery('car', ['year', 'manufacturer'], cars)

I made sure to make the associations ( parts, and manufacturer ) embedded as well.
So, can you give it a try and let me know if that works for you too? If not, can you send me the tests your trying ( and the setup ) .. so I can compare and see what I am doing differently.

@danielspaniel
Copy link
Collaborator

@ksykulev , Is {embedded:'always'} even supported anymore in association definition. I know its in use in the serializer definitions( as of 1.0.0.beta.10), so it makes me wonder if the other is even relevant? Can you confirm that.

@ksykulev
Copy link
Contributor Author

ksykulev commented Jan 6, 2015

@danielspaniel - Hey sorry for the delayed response. Here is some tests I came up with:
https://gist.github.com/ksykulev/998583603bb8df423f81

Let me know which ones are completely out of bounds and which ones make sense. I can definitely use the pattern of usage you recommend for handleFindQuery.

RE: embedded - good question. I need to look more deeply into this.

Unfortunately i've been really busy at work and haven't had a chance to regroup about the fixture adapter changes or this new embedded question. I'll try to allocate some time soon to both of these things.

@danielspaniel
Copy link
Collaborator

No problem @ksykulev. You know, now that I think about it and see your tests, I am thinking of removing the 'passing in json' altogether, and just have the array you pass in be records already created with make or makeList ( third parameter to handleFindQuery ).

Do you care? Is there any great reason to pass in json fixtures. I only supported that because you were using it that way, but the reason to not support it anymore is because I am not sure how reliable it is, whereas I can almost guarantee that passing in records will work every time.

@ksykulev
Copy link
Contributor Author

ksykulev commented Jan 7, 2015

@danielspaniel - I think it's totally reasonable to not support JSON with handleFindQuery for now/until we find a good reason for it. If the user needs that fine grained control they should just use mockjax/sinon on their own. no? They could just as easily use FactoryGuy.build(list) to help them create the JSON as well..

@danielspaniel
Copy link
Collaborator

Ok @ksykulev, I will drop support for the json array ( of fixtures ), so you will have to make the records you want to be returned by the handleFindQuery before you call it. We can always bring it back if there is need.
I will push out the new version tomorrow. ( 0.9.3 )

@ksykulev
Copy link
Contributor Author

@danielspaniel - With the last round of changes things have been working really smoothly. Thanks for your help.

@danielspaniel
Copy link
Collaborator

Your welcome @ksykulev :)

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

Successfully merging this pull request may close these issues.

2 participants