Skip to content

Conversation

@aaxelb
Copy link
Contributor

@aaxelb aaxelb commented Jan 11, 2018

Purpose

Move the queryHasMany helper from ember-osf-reviews to ember-osf. Provides a way to query relationships without going through ember-data, which sometimes decides it has all the data it needs and doesn't make the request.

Summary of Changes

  • Add queryHasMany method to osf-model
    • Allows querying relationships with custom query params and ajax options
      e.g. user.queryHasMany('nodes', { filter: { title: "My title!", public: true } })
  • Remove ember-data-has-many-query

Side Effects / Testing Notes

Code that uses model.query() has to be updated to use model.queryHasMany(). In most cases, it's a drop-in replacement, but I didn't want to replace it silently in case someone tries to query a belongsTo or somehow depends on ember-data-has-many-query-specific behavior.

Ticket

https://openscience.atlassian.net/browse/MOD-176

Reviewer Checklist

  • meets requirements
  • easy to understand
  • DRY
  • testable and includes test(s)
  • changes described in CHANGELOG.md

@aaxelb aaxelb force-pushed the feature/MOD-176--queryHasMany branch from 42c98e6 to c716b56 Compare January 12, 2018 16:09
@aaxelb aaxelb changed the title [WIP][MOD-176][Feature] Replace has-many-query with queryHasMany [MOD-176][Feature] Replace has-many-query with queryHasMany Jan 12, 2018
};

var other = this.get('_dirtyRelationships.${rel}') || {};
var other = this.get(`_dirtyRelationships.${rel}`) || {};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just a drive-by fix -- not sure if it actually caused misbehavior, though.

Copy link
Member

Choose a reason for hiding this comment

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

Wow, been broke for 2 years: https://github.com/aaxelb/ember-osf/blame/06d0f12f7b486e75fc97d5e8d93b829636e02d90/addon/models/osf-model.js#L68

But, like you said, this is likely not causing any misbehavior, yet...

Copy link
Member

@jamescdavis jamescdavis left a comment

Choose a reason for hiding this comment

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

This looks like a nice solution! Thanks for this!

};

var other = this.get('_dirtyRelationships.${rel}') || {};
var other = this.get(`_dirtyRelationships.${rel}`) || {};
Copy link
Member

Choose a reason for hiding this comment

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

Wow, been broke for 2 years: https://github.com/aaxelb/ember-osf/blame/06d0f12f7b486e75fc97d5e8d93b829636e02d90/addon/models/osf-model.js#L68

But, like you said, this is likely not causing any misbehavior, yet...

return ArrayPromiseProxy.create({ promise });
},

__queryHasManyDone(resolve, payload) {
Copy link
Member

Choose a reason for hiding this comment

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

I like the double underscore prefix for "superprivate" methods that should never be called directly (only as a callback). AFAIK, this is not a convention in JS, but I like it.

@jamescdavis jamescdavis merged commit ca3e407 into CenterForOpenScience:develop Jan 17, 2018
@aaxelb aaxelb deleted the feature/MOD-176--queryHasMany branch January 17, 2018 14:25
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