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

testHelper handleFindAll cannot handle RESTAdapter relationships #113

Closed
FilmKnurd opened this issue Jul 27, 2015 · 10 comments
Closed

testHelper handleFindAll cannot handle RESTAdapter relationships #113

FilmKnurd opened this issue Jul 27, 2015 · 10 comments

Comments

@FilmKnurd
Copy link

It looks like the intent of the testHelper is handle both JSONAPI and other adapters. However, the responseJSON built is incorrect for RESTAdapter relationships.

Currently

{
  posts: [
    {
      id: 1,
      author: {
        id: 1,
        name: "bob"
      }
    }
  ]
}

But should be

{
  posts: [
    {
      id: 1,
      author: 1
    }
  ],
 authors: [
  {
    id: 1,
   name: "bob"
  }
 ]
}
@danielspaniel
Copy link
Collaborator

hmm .. interesting .. I wonder why the tests were passing for the REST adapter .. I will investigate further .. ( do I have relationship test for them? ( not sure? ) ) but I am doing big refactor of the json responses right now, so this issue might be fixable in this refactor if I can get a failing test.

@FilmKnurd
Copy link
Author

Huh, interestingly, it seems to work if the relationship fields are defined with { async: true }

@danielspaniel
Copy link
Collaborator

That is kinda interesting .. I will factor that in when testing .. man .. that is going to be another whopper, testing sync and async for everything .. I am doing alittle bit now .. but for everything .. gulp ..

@FilmKnurd
Copy link
Author

I think it works because your test helper builds the models and adds them to the store. When the request is made, the async fields are ignored. When they are looked up, they already exist in the store, so ED does not make another network request.

@danielspaniel
Copy link
Collaborator

I don't think that is correct .. but feel free to slap some code in here to prove me wrong.
I will slap first:

  handleFindAll: function () {
    var args = Array.prototype.slice.call(arguments);
    var modelName = args.shift();
    var json = FactoryGuy.buildList.apply(FactoryGuy, arguments);

    var responseJson;
    if (!FactoryGuy.useJSONAPI()) {
      responseJson = this.mapFindAll(modelName, json);
    } else {
      responseJson = json;
    }
    var url = this.buildURL(modelName);
    this.stubEndpointForHttpRequest(url, responseJson);
  },

All this is doing is building json and returning it in mockjax call

@FilmKnurd
Copy link
Author

Ah yeah, buildList not makeList...

Looking again at my test, I see that the template does not render related model properties.

@danielspaniel
Copy link
Collaborator

So, is that a nice failure case that I can bank on?

@danielspaniel
Copy link
Collaborator

@FilmKnurd , I am working on this one now. Boy, this is such a good issue.
It's forcing me to change the way a few things work ( simplifying here, complicating there ) .. and now the build method will return correctly formatted REST or AMS data with sideloaded relationships that can be pushPayloaded right into the store or sent right to the adapter through mockjax response handler

@danielspaniel
Copy link
Collaborator

@FilmKnurd , finally got this one .. ( v2.0.1 ) Let me know if there are any problems. This was a big nternal refactor, but very very worth it, so thanks for pushing me along.

@FilmKnurd
Copy link
Author

All my tests are passing :-)

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

2 participants