mockCreate is not deserializing date properties correctly

[mariokostelac]

Turns out that mockCreate('modelName').returns({attrs: build('model')}) is not deserializing dates correctly. It always sets them to null. I've done a little bit of digging and will leave information I've gathered in comments below.

Failing spec #400

[danielspaniel]

Spec looks good, @mariokostelac , when you finish discovering .. submit the fix in the PR ( with the failing test )

[mariokostelac]

The problem lies in convertForBuild being called twice, both times with transformKeys: true.

• The first call is done when build method is called.
• The second time it get's called when mockCreateRequest.getResponse is called

That makes transform:date.serialize() being called twice as well, first time converting Date object into string (as part of build method) and second time string being converted to null (as part of getResponse method).

[mariokostelac]

@danielspaniel I've nailed exactly why it's happening, but it seems to me that I do not understand interfaces enough to understand how the correct fix looks like. Is that anything you could help with?

For example. What exactly should build return? Should it return what ember data would send to the server? Or what server would return for that ember model? Maybe something third?

[danielspaniel]

I am pondering this one @mariokostelac

[mariokostelac]

I am pretty new in this space but it seems to me that build should be returning whatever server would return for that instance of the model. That makes me think that getResponse should not really be doing any real transformations of keys and values but return whatever was passed in. It is quite possible the reality is quite more complex and I am willing to change my view 📚 .

[danielspaniel]

I guess it's tricky .. this is actually very tricky and there is a lot of magic going on to make this all work like you would expect. One of the magics is that builder. When you build a review like that

 let review = build('review', {id: 1, rating: 3, date: new Date(), description: "very good"});

and then call ===> review.get()

what you are seeing there is a list of attributes that the server would be sending to you ( serialized ).

Sooooooooo therefore , in the examples I gave for mock create / update I never did what you are doing ( though it is a nice idea )
because you are passing the serialized version of the attributes to the .returns({attrs: serialized already => bad idea }})

What I normally do for your cases when i am just saying "hey i have a review here and just pass it back after pretending to create it " ( and works fine ) is this:

 let review = make('review', {id: 1, rating: 3, date: new Date(), description: "very good"});
mockCreate(review);  

Or when i am just looking for a few attrs, and usually just one or two so for that case I always do this

 let date = newDate();
 mockCreate('review').returns({attrs: {date: date}});

this works as expected

so .. for this case, i am not going to fix it because there is nothing particular to fix. it is just the nature of the game here to use the build in the certain way intended. If you really can not stand this your welcome to try harder, but I don't think its worth it.

[mariokostelac]

Ah that makes total sense. Should we change https://github.com/danielspaniel/ember-data-factory-guy#mockcreate then?

There are several of them using build and buildList.

[danielspaniel]

no no .. those builds are there to return "serialized" data if you don't already pass in data of your own. and they don't actually return much ( just empty I think )

[mariokostelac]

I've added another commit in that PR. Is that a valid test or I am still doing something wrong?

The idea is that I can mock the server response that's retrieved first time something returns creates a review.

[danielspaniel]

no, i guess it's an ok test of that functionality .. but I already have a test for that .. so I don't need this one .. just delete the PR really .. cause we don't need it anymore. Was perfect to show your issue though.

[mariokostelac]

My issue is that the test is actually failing, I am probably lacking some basic understanding of the API.

Is there anything I can read to understand when I can pass a result of build, when a result of make, makeNew. I'm quite confused. (And thank you for your time).

[danielspaniel]

here is the test ( i used make instead of makeNew ( by mistake )

    let description          = "special description",
        camelCaseDescription = "special camelcase description",
        profile              = makeNew('profile', {description, camelCaseDescription});

    mockCreate(profile).match({camelCaseDescription, description});
    await profile.save();

    assert.ok(!profile.get('isNew'), 'Profile is saved');
    assert.deepEqual(
      profile.getProperties(['description', 'camelCaseDescription']),
      {camelCaseDescription, description},
      'correct model attributes present'
    );
  });

[danielspaniel]

this works with dates or you can do what I suggested with passing {attrs: {date: myDate}})
either one

[mariokostelac]

I think I will have to spend more time to understand this fully, but I am slowly getting it - we were using returns interface incorrectly.

It is fair to say that I found many instances of m = build('x'); mockCreate(modelName).returns({ attrs: m }) in our codebase (where I work), which means other developers potentially do not understand it either. I'll close the PR since it's not a bug, but will try to write up my thoughts on the interface and documentations examples. I think there is a way to make it more clear and concise.

[danielspaniel]

For sure , yes and yes, this is not easy to understand and any work you do to make it clearer is valuable and we can put that in the README.
And yes, many people are using factory guy less than correctly or optimally and it will throw everyone off who reads those tests in the future, so even you understanding will help ( at least you! if not others )

[danielspaniel]

closing this since we kinda got the date stuff worked out