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

[BUGFIX] has-many relationship: set hasData when fetching link #5053

Merged
merged 1 commit into from
Jul 12, 2017
Merged

[BUGFIX] has-many relationship: set hasData when fetching link #5053

merged 1 commit into from
Jul 12, 2017

Conversation

csantero
Copy link
Contributor

@csantero csantero commented Jul 4, 2017

I believe this fixes #4845. The problem is that setHasData was only being called when at least one internal model was being added, which is not the case when the server returns an empty array.

@csantero
Copy link
Contributor Author

csantero commented Jul 4, 2017

Not sure what's wrong with Travis. npm test passes locally.

assert.equal(get(records, 'length'), 0);
assert.equal(get(personsReference.value(), 'length'), 0);

done();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lets not use done, instead use promises directly. That way when the promise rejects, the test fails.

return Ember.RSVP.resolve({ data: [] });
};

var family;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let

};

var family;
run(function() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

=>

var personsReference = family.hasMany('persons');
assert.equal(personsReference.remoteType(), "link");

run(function() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

=>

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return (so promises work)

assert.equal(personsReference.remoteType(), "link");

run(function() {
personsReference.load().then(function(records) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return (so promises work)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

=>

@stefanpenner
Copy link
Member

stefanpenner commented Jul 4, 2017

@csantero this looks reasonable, but we should double check.

@igorT / @runspired / @hjdivad / @bmac r?

@csantero
Copy link
Contributor Author

csantero commented Jul 5, 2017

@stefanpenner I've updated the test.

Copy link
Member

@hjdivad hjdivad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm looks okay but I wonder if we need to handle the case of preloading with an empty array as well

Copy link
Member

@bmac bmac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@hjdivad
Copy link
Member

hjdivad commented Jul 12, 2017

Looks great, thanks for the contribution @csantero

@hjdivad hjdivad merged commit f77dbf9 into emberjs:master Jul 12, 2017
hjdivad added a commit that referenced this pull request Jul 12, 2017
See #5053 for a similar fix in the fetchLink case.  See #4845 for the
initially reported bug.
@csantero csantero deleted the has-many-link-returns-empty-array branch July 12, 2017 23:16
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.

HasManyReference property _isLoaded return false when it had been loaded once (with empty result)
4 participants