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

handleCreate().match does not work with ActiveModelSerializer #165

Closed
gleseur opened this issue Dec 23, 2015 · 10 comments
Closed

handleCreate().match does not work with ActiveModelSerializer #165

gleseur opened this issue Dec 23, 2015 · 10 comments

Comments

@gleseur
Copy link
Contributor

gleseur commented Dec 23, 2015

Hi,

I'm using ActiveModelSerializer and I have an issue when using handleCreate.match.
It raises an error https://github.com/danielspaniel/ember-data-factory-guy/blob/master/addon/mock-create-request.js#L81 because expectedData.data is undefined.
I have :

requestData == expectedData == {
  order: "1",
  orderItems: Array[2],
  shipItems: true,
  shipOrder: true, 
  shippingMethod: "1",
  shippingRef: "AZE123",
}

(so you can see there is no data node)

but going through normalize at https://github.com/danielspaniel/ember-data-factory-guy/blob/master/addon/mock-create-request.js#L76 and https://github.com/danielspaniel/ember-data-factory-guy/blob/master/addon/mock-create-request.js#L79 does not add a data node.

The test code is:

TestHelper.handleCreate('shipment').match({
  order: this.pack.get('order'),
  shipOrder: true,
  shipItems: true,
  shippingMethod: this.methods[0],
  shippingRef: this.ref,
  orderItems: this.pack.get('orderItems'),
});

I don't think anything in the app is interfering with this.

Is it a workaround to this ? (or maybe I'm not using it right ?)

Thanks

@danielspaniel
Copy link
Collaborator

It's alway nice to try to throw out more hay when trying to find a needle in the haystack.

You're making my life difficult by doing stuff like

TestHelper.handleCreate('shipment').match({
  order: this.pack.get('order'),
  shipOrder: true,
  shipItems: true,
  shippingMethod: this.methods[0],
  shippingRef: this.ref,
  orderItems: this.pack.get('orderItems'),
});

because I have no idea what is in: this.pack.get('orderItems'), or this.methods[0], or this.pack.get('order').
How can I figure out what you are really matching .. I can't .. so that is problem numbero uno.

What I suggest is throwing out hay:

just do this:

TestHelper.handleCreate('shipment')

and see what happens .. forget the match .. and see if it works .. then add one item to match at a time till it fails. That's what I would do. And if still does not work .. your going to have to lay out json so I can see the values .. and not => this.pack.get('orderItems').

speaking of which .. you should not match on belongs to or has many relations .. just attributes

@danielspaniel
Copy link
Collaborator

oops .. did you just say your data looked like this:

expectedData == {
  order: "1",
  orderItems: Array[2],
  shipItems: true,
  shipOrder: true, 
  shippingMethod: "1",
  shippingRef: "AZE123",
}

I think you did .. so sorry about that .. did not mean to say you did not show data .. cause there it is.
Same advise still applies though .. try simplest match

@gleseur
Copy link
Contributor Author

gleseur commented Dec 23, 2015

Thanks for taking a look.

The no match solution is working and I defaulted to that for now. However the whole point of the form I'm testing is to fill in this data. So not asserting the sent data makes the test way less useful.
Trying only a single basic field fails the same way (logical given the shape of the data)

All associations are serialized as ids so they should work (and it's quite an important data to test in that request).

Defaulting the attributes to Object.keys(expectedData) and ObjectKeys(requestData) would work in my case from what I see in the debugger but I don't know enough the code here to say if this would be a valid fix.

To complete the data, here is the code of the serializer for this model:

import DS from 'ember-data';

export default DS.ActiveModelSerializer.extend(DS.EmbeddedRecordsMixin, {
  attrs: {
    orderItems: {
      serialize: 'ids',
      deserialize: 'ids',
    },
  },
});

@danielspaniel
Copy link
Collaborator

Hmm ... good point .. about no match being useless for this case .. I did not mean to suggest it was the end of the game .. just throwing out hay.

So you are saying that one field causes error also .. was that an attribute field or an association.
Can you clarify.
I am kinda curious why the store.normalize method is not producing json api data .. it usually does.
What version of ember/ember data/factory guy are you on?

@gleseur
Copy link
Contributor Author

gleseur commented Dec 23, 2015

Any field triggers the same error.
Also tried match({}), result is the same.

Here is everything we have (still on the 1.13 branch, so not too late):
Only other notable thing I see is that we're using ember-cli-rails to integrate with the backend

"bower": "^1.5.2",
"broccoli-asset-rev": "^2.1.2",
"broccoli-jscs": "^1.1.0",
"ember-bootstrap": "0.3.0",
"ember-browserify": "^1.0.5",
"ember-cli": "1.13.8",
"ember-cli-app-version": "0.5.0",
"ember-cli-babel": "^5.1.3",
"ember-cli-content-security-policy": "0.4.0",
"ember-cli-dependency-checker": "^1.0.1",
"ember-cli-htmlbars": "0.7.9",
"ember-cli-htmlbars-inline-precompile": "^0.2.0",
"ember-cli-ic-ajax": "0.2.1",
"ember-cli-inject-live-reload": "^1.3.1",
"ember-cli-qunit": "1.0.1",
"ember-cli-rails-addon": "0.0.12",
"ember-cli-release": "0.2.3",
"ember-cli-sass": "^4.2.0",
"ember-cli-sri": "^1.0.3",
"ember-cli-uglify": "^1.2.0",
"ember-cli-uploader": "^0.3.11",
"ember-data": "1.13.8",
"ember-data-factory-guy": "1.13.10",
"ember-disable-proxy-controllers": "^1.0.0",
"ember-export-application-global": "^1.0.3",
"ember-i18n": "4.1.4",
"ember-local-storage": "0.0.9",
"ember-sinon": "0.2.1",
"font-awesome": "4.4.0",
"jquery-mockjax": "2.0.1",
"karma-phantomjs-launcher": "^0.2.1",
"lodash": "^3.10.1",
"papaparse": "^4.1.2"

@danielspaniel
Copy link
Collaborator

I don't think ember-cli-rails should be a problem, but do try to upgrade to factory guy v2.1.3 .. it supports all the way back to ember data 1.13.5 .. you are using pretty old version of FG

@gleseur
Copy link
Contributor Author

gleseur commented Dec 24, 2015

Upgrading to 2.1.3 then prevents the whole test suite to load. I believe due to some ES6 stuff. It's time for us to upgrade our ember-cli version anyway, so I'll come back to you when that's done.

@gleseur
Copy link
Contributor Author

gleseur commented Dec 31, 2015

Upgraded ember, ember-cli and factory guy. It almost fixed my issue. Now I only have it for compound class names

For instance if the model file is app/models/foo-bar.js then either I do

TestHelper.handleCreate('fooBar').match({a: 1} and it fails with the same error because in https://github.com/danielspaniel/ember-data-factory-guy/blob/master/addon/mock-create-request.js#L79 the modelName ('fooBar') does not match the model name in the request params ('foo_bar')

or

TestHelper.handleCreate('foo_bar').match({a: 1}) then the match is ok, but it's now failing at https://github.com/danielspaniel/ember-data-factory-guy/blob/master/addon/mock-create-request.js#L106 because definition is empty because the model definition keys are camelCase.

I'm not sure what's the right fix. Maybe, it's the factory definition that is not correct. For the compound models I do:

FactoryGuy.define('fooBar', ...

In the meanwhile, the workaround I've found is forcing the id with a andReturn({ id: ...} but it's not very satisfactory

Maybe a more robust solution would be to try harder in https://github.com/danielspaniel/ember-data-factory-guy/blob/master/addon/factory-guy.js#L130 by trying the various options (camelize, underscore, dasherize). If that's ok with you, I'll do a PR.

@ccleung
Copy link
Contributor

ccleung commented Apr 12, 2016

I think @gleseur has the right idea, the underlying issue seems to be that this line: https://github.com/danielspaniel/ember-data-factory-guy/blob/master/addon/mocks/mock-create-request.js#L106 is using the modelName which would be a string, formatted like my-model-name and does not convert/use the active model key format, which would need to be my_model_name as the root key of the requested data payload in order to properly extract the requested data.

I made this change on my branch and it fixed the issue for me: #187 the request of the mock-create-request will rely on the modelName as is, e.g., for me it was dasherized my-model-name because i call mockCreate('my-model-name').match(...)

@danielspaniel
Copy link
Collaborator

I think v2.6.2 fixed this issue .. let me know if I am wrong and I will fix it ASAP.

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