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.

Sign up for GitHub

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

Jump to bottom

[ENG-4460] Toggle assertions based on preprint-provider flag #756

Merged
merged 4 commits into from
Sep 12, 2023
Merged

[ENG-4460] Toggle assertions based on preprint-provider flag #756

merged 4 commits into from
Sep 12, 2023

Conversation

futa-ikeda
Copy link
Contributor

@futa-ikeda futa-ikeda commented Sep 7, 2023 •
edited
Loading

Purpose

  • Show/Hide Assertions/COI panels in submit page based on a preprint-provider flag (assertions_enabled)

Summary of Changes/Side Effects

  • Add assertionsEnabled flag to submit page controller
    • controls which panels are visible (hide COI and Assertions based on this flag)
    • controls which validations occur
    • controls which validation errors are shown
  • Is based pretty heavily on un-doing a prior PR to remove the reliance on sloan feature-flags

Testing Notes

  • Will need to toggle the assertions_enabled flag on the preprint_provider page in the admin app
  • The assertions_enabled does not control whether we show/hide COI and/or Author Assertions on the preprint-detail page (e.g. osf.io/preprints//), so if a preprint is submitted with COI or Author Assertions, it will show up on the detail page regardless of the assertions_enabled flag on the provider

Ticket

https://openscience.atlassian.net/browse/ENG-4460

Notes for Reviewer

  • It's easiest to read if you hide whitespace in the diff
  • This will be released in conjunction with the changes in ember-osf found in this PR to add the assertionsEnabled flag to the preprint-provider model

Reviewer Checklist

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

futa-ikeda
futa-ikeda commented Sep 8, 2023
View reviewed changes
tests/unit/controllers/submit-test.js Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

A real fix would be to make two separate test cases for the PreprintProver.assertionsEnabled flag turned on and off, but I can't seem to get the changes in my preprint-provider factory in eosf to show up here :/

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this still true that you can't get the changes to show?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not getting the changes to show, but I can just hack it with something like ctrl.set('selectedProvider', { assertionsEnabled: true }); so I'm adding a couple tests that do that.

futa-ikeda
futa-ikeda commented Sep 8, 2023
View reviewed changes
package.json
@@ -26,7 +26,7 @@
"coveralls": "cat ./coverage/lcov.info | coveralls"
},
"devDependencies": {
"@centerforopenscience/ember-osf": "https://github.com/CenterForOpenScience/ember-osf.git#v0.36.0",
"@centerforopenscience/ember-osf": "https://github.com/CenterForOpenScience/ember-osf.git#feature\/custom-preprint-citation",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note to self: make sure this is updated on release

brianjgeiger
brianjgeiger requested changes Sep 8, 2023
View reviewed changes
Copy link
Contributor

@brianjgeiger brianjgeiger left a comment

Choose a reason for hiding this comment

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

Approved except for the pointing to a feature branch of ember-osf and mayyybe updating the test if you've figured out your problems there.

tests/unit/controllers/submit-test.js Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this still true that you can't get the changes to show?

@futa-ikeda futa-ikeda merged commit 2d8fe52 into CenterForOpenScience:feature/provider-defaults Sep 12, 2023
2 checks passed
@futa-ikeda futa-ikeda mentioned this pull request Sep 12, 2023
Merged
5 tasks
futa-ikeda added a commit that referenced this pull request Sep 12, 2023
@futa-ikeda
[ENG-4460] Toggle assertions based on preprint-provider flag (#756)
35d8f26 
* Toggle assertions based on preprint-provider flag

* Update test

* Point package.json to eosf feature branch temporarily

* Add tests for assertionsEnabled
adlius added a commit that referenced this pull request Sep 15, 2023
@futa-ikeda @adlius
Feature/provider defaults (#760)
684a30a 
* [ENG-4460] Toggle assertions based on preprint-provider flag (#756)

* Toggle assertions based on preprint-provider flag

* Update test

* Point package.json to eosf feature branch temporarily

* Add tests for assertionsEnabled

* Update yarn.lock (#757)

* [ENG-4585] Remove Chronos (#759)

* remove chronos

* remove styles and translations

---------

Co-authored-by: Yuhuai Liu <yuhuai@cos.io>
@futa-ikeda futa-ikeda deleted the assertions-enabled branch October 27, 2023 14:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brianjgeiger brianjgeiger brianjgeiger approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@futa-ikeda @brianjgeiger