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

APERTA-10963 preprint doi in paper tracker requires flag #3410

Merged
merged 1 commit into from
Aug 29, 2017

Conversation

amaxsurge
Copy link

@amaxsurge amaxsurge commented Aug 28, 2017

JIRA issue: https://jira.plos.org/jira/browse/APERTA-10963

What this PR does:

At /paper_tracker the preprint doi column is now hidden unless the corresponding preprint flag is checked

Special instructions for Review or PO:

Toggle the flag checkbox, view the column

Code Review Tasks:

Author tasks (delete tasks that don't apply to your PR, this list should be finished before code review):

  • If I made any UI changes, I've let QA know.

Reviewer tasks (these should be checked or somehow noted before passing on to PO):

  • I read through the JIRA ticket's AC before doing the rest of the review
  • I ran the code (in the review environment or locally). I agree the running code fulfills the Acceptance Criteria as stated on the JIRA ticket
  • I read the code; it looks good
  • I have found the tests to be sufficient for both positive and negative test cases

@phoca2004 phoca2004 temporarily deployed to plos-ciagent-pr-3410 August 28, 2017 10:38 Inactive
@amaxsurge
Copy link
Author

amaxsurge commented Aug 28, 2017

Notes to reviewer.

  1. For reasons that I cannot fathom the code changes (not the test changes) are causing failures here in the paper tracker test with no meaningful error message except a reference to a 404 that does not show up in the network console

  2. I went down a whole rabbit hole here trying to test the positive case where you need inject and update a service into the global container. As far as I can tell based on this
    https://stackoverflow.com/questions/28422375/how-to-mock-an-ember-cli-service-in-an-acceptance-test
    and
    Allow overriding services/factories in the registry. emberjs/ember-test-helpers#96

this is only possible in a moduleForComponent based test but not a plain acceptance test. Would be happy to be corrected on this if anybody knows how to do it.

@egh egh self-assigned this Aug 28, 2017
</th>
{{#if preprintsEnabled}}
<th class="paper-tracker-paper-preprint-id-column">
{{sort-link text="Manuscript Preprint ID"
Copy link
Contributor

Choose a reason for hiding this comment

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

Per AC 2 this should be "Pre-print DOI"

Copy link
Author

Choose a reason for hiding this comment

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

crazy, I swear I did that. Must have stashed it or something. thanks

@egh
Copy link
Contributor

egh commented Aug 28, 2017

@amaxsurge I believe the test problem is due to the feature flag service not being stubbed out. See https://github.com/Tahi-project/tahi/blob/master/client/tests/integration/pods/components/admin-page/component-test.js#L19 for an example, but I can't get this to work directly with your test, probably because of some strange difference between qunit test types.

@amaxsurge amaxsurge force-pushed the features/APERTA-10963-flag-preprint-doi-search branch from becfc52 to 98f3b0b Compare August 28, 2017 18:39
@phoca2004 phoca2004 temporarily deployed to plos-ciagent-pr-3410 August 28, 2017 18:39 Inactive
@amaxsurge
Copy link
Author

@egh yeah I basically wrote that exact same code that you put in the example and could not get it to work. Let me keep reading and playing with it

@amaxsurge amaxsurge force-pushed the features/APERTA-10963-flag-preprint-doi-search branch from 98f3b0b to 6449d59 Compare August 28, 2017 19:42
@amaxsurge
Copy link
Author

@egh all tests are passing here. Is this still in review?

@@ -45,6 +45,7 @@ module('Integration: Paper Tracker', {
Factory.resetFactoryIds();
App = startApp();
server = setupMockServer();
$.mockjax({url: '/api/feature_flags.json', status: 200, responseText: {PREPRINT: true}});
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@egh egh merged commit 0cfc624 into master Aug 29, 2017
@egh egh deleted the features/APERTA-10963-flag-preprint-doi-search branch August 29, 2017 19:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants