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
[PLAT-1181] Waffle all Prereg Challenge specific parts of the codebase #8833
[PLAT-1181] Waffle all Prereg Challenge specific parts of the codebase #8833
Conversation
%if waffle.switch_is_active(features.OSF_PREREGISTRATION): | ||
<button type="button" class="btn btn-success pull-right" | ||
data-toggle="tooltip" data-placement="top" title="The Prereg Challenge has expired" | ||
style="margin-left: 5px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Weird use of style tags here because the disabled
class unjustly disables tooltips, so it's hacked to look and act disabled.
if not dry_run: | ||
add_file_logger(logger, __file__) | ||
main(dry_run=dry_run) | ||
if not waffle.switch_is_active(features.OSF_PREREGISTRATION): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might seem better to waffle this in the Celery config in defaults.py
then here inside the running task, but that causes a circular import in waffle
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed with David M. in person and we can completely remove this reminder email now (no sense reminding people to finish their prereg draft when there's two weeks remaining...). Go ahead and remove the script, task, and email template.
6878f0d
to
084e37c
Compare
2146788
to
297a59b
Compare
a84f7ea
to
9b18b28
Compare
9b18b28
to
c0514ba
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few minor suggestions and a bit more code that can be removed. Thanks for your work on this @Johnetordoff, looks good!
"""Landing page for the prereg challenge""" | ||
return views._view_registries_landing_page('prereg', **kwargs) | ||
"""Landing page for osf prereg""" | ||
if waffle.switch_is_active(features.OSF_PREREGISTRATION): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are the changes to this function necessary? The same mako template is returned regardless of the switch and it doesn't look like _view_registraties_landing_page
was changed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, campaign is used to determine which draft registrations to load on the landing page. Got it.
|
||
%if waffle.switch_is_active(features.OSF_PREREGISTRATION): | ||
<button type="button" class="btn btn-success pull-right" | ||
data-toggle="tooltip" data-placement="top" title="The Prereg Challenge has expired" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about "The Prereg Challenge has ended."? Ended sounds a bit more accurate than expired imo.
@@ -121,6 +123,9 @@ def submit_draft_for_review(auth, node, draft, *args, **kwargs): | |||
:rtype: dict | |||
:raises: HTTPError if embargo end date is invalid | |||
""" | |||
if waffle.switch_is_active(features.OSF_PREREGISTRATION): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a test to ensure that this function doesn't execute when the switch is active?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
if not dry_run: | ||
add_file_logger(logger, __file__) | ||
main(dry_run=dry_run) | ||
if not waffle.switch_is_active(features.OSF_PREREGISTRATION): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed with David M. in person and we can completely remove this reminder email now (no sense reminding people to finish their prereg draft when there's two weeks remaining...). Go ahead and remove the script, task, and email template.
32a9030
to
bc920db
Compare
@Johnetordoff, we should also go ahead and remove the custom messaging that's passed here to this template (it's also passed to 5 or so other templates -- do a global find to find them all). We don't want to mention the Prereg Challenge if someone registers an existing draft in January, |
When the Also, this modal appears when I click "Register for review" and should be removed. |
<span data-bind="if: (draft.metaSchema.name === 'Prereg Challenge' && !draft.isPendingApproval)"> | ||
<button id="register-submit" type="button" class="btn btn-primary pull-right" data-toggle="tooltip" data-placement="top" title="Not eligible for the Pre-Registration Challenge" data-bind="click: draft.registerWithoutReview.bind(draft)">Register without review</button> | ||
</span> | ||
%endif: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor: :
after endif
should be removed
@Johnetordoff, one minor comment and a merge conflict that needs resolving. Looks great when testing locally! |
into winddown-waffle-the-rest * 'develop' of https://github.com/CenterForOpenScience/osf.io: Bump version and update changelog [PLAT-1254] Fix embargoing pre-registrations Update export_user_account script for files-on-anything Add an absolute_url property to Institutions model Move call to delete files on a node to a celery task Expect 410 gone again rather than not permission for less confusion Allow sorting search results by "modified" Downgrade DRF to 3.8.2 Raise 410 instead of 404, include helpful message Update entity ID for Ferris When viewing a deleted file on a private node with no access, raise 404 If a node is deleted, show No files in the file browser when viewing tombstone metadata Update index settings, post-bulk indexing Limit the number of buckets in ES aggregation for preprints Upgrade django-elasticsearch-metrics; speed up backfill Reduce batch size; show progress Delete files on nodes removed from bulk destroy Fix related lookup contenttype restriction Change URL for captured raven message to send to Sentry chore: upgrade to osf-style@1.8.0 # Conflicts: # website/project/views/drafts.py
osf_tests/test_prereg.py
Outdated
'is_logged_in': False, | ||
} | ||
) | ||
|
||
cache.delete(Switch._cache_key(OSF_PREREGISTRATION)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this necessary? If you're using override_switch
to set the value of a switch, you shouldn't have to worry about the cache.
osf_tests/test_prereg.py
Outdated
} | ||
) | ||
|
||
cache.delete(Switch._cache_key(OSF_PREREGISTRATION)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are unnecessary, too, right?
website/project/views/drafts.py
Outdated
@@ -121,6 +123,12 @@ def submit_draft_for_review(auth, node, draft, *args, **kwargs): | |||
:rtype: dict | |||
:raises: HTTPError if embargo end date is invalid | |||
""" | |||
if waffle.switch_is_active(features.OSF_PREREGISTRATION): | |||
raise HTTPError(http.GONE, data={ | |||
'message_short': 'The Prereg Challenge has ended.', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: message_short
isn't usually a sentence, because it gets rendered as the page title when the OsfWebRenderer is used. Is this ever rendered to HTML?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No this should never actually be hittable by the UI.
website/project/views/drafts.py
Outdated
if waffle.switch_is_active(features.OSF_PREREGISTRATION): | ||
raise HTTPError(http.GONE, data={ | ||
'message_short': 'The Prereg Challenge has ended.', | ||
'message_long': 'The Prereg Challenge has ended. This endpoint is currently depreciated.' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo: "deprecated". That said, this endpoint isn't really deprecated (I mean...all of v1 is deprecated, but that's besides the point)--it is still used for other schemas.
Maybe it makes sense for message_long
to be The Prereg Challenge has ended. No new submissions are accepted at this time.
message_short` can be unset, I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is still used for other schemas.
Just to clarify Prereg Challenge is the only schema that uses this endpoint because it's the only one where requiresApproval
is set to true
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, ok. Then I suppose it's fine to deprecate this endpoint. Just need to make sure we weren't exposing language like "deprecated endpoint" in the UI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just took a quick glance. Looks good to go to staging. Just a few nits.
886f197
to
d9b5bf8
Compare
d9b5bf8
to
b021d50
Compare
Deployed to staging3 🎉 |
Remove mention of remind_draft_preregistrations script
Latest commits headed to staging3 |
OSF Preregistration form copy edits [PLAT-1279]
@@ -144,7 +144,7 @@ | |||
}, | |||
{ | |||
"text": "Registration following analysis of the data", | |||
"tooltip": "As of the date of submission, you have accessed and analyzed some of the data relevant to the research plan. This includes preliminary analysis of variables, calculation of descriptive statistics, and observation of data distributions. Please contact us (prereg@cos.io) and we will be happy to help you." | |||
"tooltip": "As of the date of submission, you have accessed and analyzed some of the data relevant to the research plan. This includes preliminary analysis of variables, calculation of descriptive statistics, and observation of data distributions. Please see https://cos.io/prereg for more information." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Johnetordoff Will these changes require a migration?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh that's my bad. I pushed a migration to release/next-platform but not to this branch because I thought this PR already had an ensure_schemas
migration. This PR will need a migration like this: 8cbe44b
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sloria Yes we'll have to run ensure_schemas to see any effect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh ok. @Johnetordoff Can you please add that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, please document in "Side Effects" that python manage.py migrate
needs to be run when this is released (and any other necessary steps).
Add migration to ensure schemas
I got this up to date with |
Purpose
We have to shut down all prereg challenge registration capbilities at midmight on New's Years Eve, the PR covers some of the gaps.
Changes
submit/
endpoint with waffle switch (not they can hit it through the browser anywayQA Notes
According to David the admin commenting ability doesn't effect users, so it can be totally removed in the future, so no need to waffle now.
Documentation
technical task, no docs.
Side Effects
No that I know of.
Ticket
https://openscience.atlassian.net/browse/PLAT-1181