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

[IN-63][Reviews] Use ember-osf queryHasMany #120

Conversation

fabmiz
Copy link
Member

@fabmiz fabmiz commented Mar 8, 2018

Purpose

Use ember-osf version of queryHasMany

Summary of Changes

  • Update usage of queryHasMany to model.queryHasMany instead of queryHasMany from reviews store.
  • Delete store
  • Update tests

Side Effects / Testing Notes

These changes affect the preprint detail page and the moderation list page.
On the preprint detail page:

  • Make sure it works as usual with a simple preprint.
  • Create a preprint with more than 10 contributors, and see if the preprint detail page works
    with no errors (please check the console as well.)

On the moderation list page:

  • Simply make sure it works as usual, no errors.

Ticket

IN-63

Reviewer Checklist

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

@coveralls
Copy link

coveralls commented Mar 8, 2018

Coverage Status

Coverage increased (+1.5%) to 52.952% when pulling 063c7a3 on fabmiz:feature/IN-63_use_eosf_queryHasMany into 98e8513 on CenterForOpenScience:release/next-interfaces.

Copy link
Contributor

@alexschiller alexschiller left a comment

Choose a reason for hiding this comment

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

swap to next interfaces for ember-osf otherwise looks fine

package.json Outdated
@@ -17,7 +17,7 @@
"test:cover": "COVERAGE=true yarn test"
},
"devDependencies": {
"@centerforopenscience/ember-osf": "https://github.com/CenterForOpenScience/ember-osf#feature/reviews#2017-12-21",
"@centerforopenscience/ember-osf": "https://github.com/CenterForOpenScience/ember-osf#df7716240c43af4ef42a191985436279e6e510ec",
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you run:
yarn use-ember-osf-next-interfaces
to set this to next interfaces instead of a pinned commit?

Copy link
Member Author

@fabmiz fabmiz Mar 13, 2018

Choose a reason for hiding this comment

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

Sure. it does not seem like use-ember-osf-next-interfaces script is defined here yet both develop and release/next-interfaces. I will get it from https://github.com/CenterForOpenScience/ember-osf-preprints/blob/develop/package.json#L19.

@alexschiller alexschiller changed the base branch from develop to release/next-interfaces March 13, 2018 13:48
@alexschiller alexschiller changed the base branch from release/next-interfaces to develop March 13, 2018 13:51
@alexschiller alexschiller changed the base branch from develop to release/next-interfaces March 13, 2018 13:51
Copy link
Contributor

@alexschiller alexschiller left a comment

Choose a reason for hiding this comment

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

CHANGELOG please

@alexschiller alexschiller merged commit 40cf9a3 into CenterForOpenScience:release/next-interfaces Mar 16, 2018
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.

None yet

3 participants