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

Attribute typing #145

Merged
merged 2 commits into from
Nov 1, 2015
Merged

Conversation

wismer
Copy link
Contributor

@wismer wismer commented Nov 1, 2015

@danielspaniel I encountered a potential bug in factory guy - If a definition or a build (with custom attribute) contains an attribute with a default value that is false, that attribute does not get carried over into the store or get converted properly.

 // addon/rest-fixture-converter.js
  var extractAttributes = function (modelName, fixture) {
    var attributes = {};
    var transformFunction = getTransformFunction(modelName, 'Attribute');
    store.modelFor(modelName).eachAttribute(function (attribute) {
      var attributeKey = transformFunction(attribute);
      //console.log('attribute', attribute, attributeKey, fixture.hasOwnProperty(attribute), fixture.hasOwnProperty(attributeKey))
      if (fixture[attribute]) {
        attributes[attributeKey] = fixture[attribute];
      } else if (fixture[attributeKey]) {
        attributes[attributeKey] = fixture[attributeKey];
      }
    });
    return attributes;
  };

Whichever key is used, attribute or attributeKey, a value that is false will not copy with the conditionals set above. This only seems to affect the REST adapter. In order to make the tests pass, I had to slightly change some of the tests to include the aBooleanField. Please let me know if the changes would work or if there's something that I am missing.

@danielspaniel danielspaniel merged commit 1f90107 into adopted-ember-addons:master Nov 1, 2015
@danielspaniel
Copy link
Collaborator

Thanks for the fixer upper .. did you mean to test boolean being false or true .. not sure why you did the flip the bool field intead of adding another field so you would have:

  aBooleanField: false,
  bBooleanField: true

I am going to add that to the tests .. if you know what I mean

@danielspaniel
Copy link
Collaborator

or just testing both false and true .. is what I meant

@wismer
Copy link
Contributor Author

wismer commented Nov 1, 2015

oh! I should have thought of just testing out both false and true. Yea, that would be best.

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