[MOD-109][Feature] Reviews, Volume I: Moderation by aaxelb · Pull Request #7807 · CenterForOpenScience/osf.io

aaxelb · 2017-10-12T17:12:12Z

Replaces #7708

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

Purpose

All the backend changes required for OSF Reviews. Adds workflows for moderators to control which preprints are publicly available in their branded interface.

Changes

New changes since #7708 was approved

Allow including a top-level meta object in API responses
- override get_renderer_context in the view, something like:
```
def get_renderer_context(self):
    context = super(...).get_renderer_context()
    context['meta'] = { ... }
    return context
```
- At /preprint_providers/<id>/preprints/, add meta[reviews_state_counts]=true in the query string to get counts of that provider's preprints grouped by reviews_state to top-level meta
Make all a user's preprints visible on their "My Projects -> All my preprints" page, regardless whether they've been approved/rejected by a moderator
- Set _has_abandoned_preprint=False on submission to providers using a reviews workflow
Send email notifications when a preprint transitions states
Mint DOIs with EZID when a preprint first transitions into a public state (should match existing behavior on unreviewed providers)
Create a "preprint initiated" NodeLog when a preprint is submitted for review
Keep previously published preprints public when a provider first sets up a reviews workflow
- Migration to set all preprints for unreviewed providers to "accepted" state
- When a preprint is published to an unreviewed provider, set "accepted" state
Allow filtering preprint lists with filter[node_is_public], so we can exclude preprints with private nodes from the reviews interface
Various bugfixes in the new reviews code

Original changes

Add Action top-level object
- Each action represents a transition, a state change on an object
- e.g. "Abram submitted preprint X for review at time Y, changing its state from initial to pending"
- Once created, actions are meant to be immutable
- New API endpoints:
  - /actions/
    - Write-only endpoint for creating actions
    - GET returns an empty list for the sake of the browsable API
  - /actions/<id>/
    - Read-only action detail
  - /preprints/<guid>/actions/
    - Read-only list of actions which target the given preprint
  - /users/me/actions/
    - Read-only list of actions from providers for which the user is a moderator
Add ReviewableMixin for objects which can be reviewed
- Right now just PreprintService
- Fields:
  - reviews_state: Current state of the preprint
- Includes methods to trigger transitions and create corresponding Actions
Add ReviewProviderMixin for objects which own Reviewables and can choose a workflow for them
- Right now just PreprintProvider
- Fields:
  - reviews_workflow: Provider's chosen workflow (see below)
  - reviews_comments_private: Whether comments made by moderators are visible to the submitter/contributors
  - reviews_comments_anonymous: Whether the name/ID of the moderators who accept/reject/act on a preprint are visible to the submitter/contributors
- Modified API endpoints:
  - /preprint_providers/<id>/ is once-writable
    - Provider admins can set the three reviews fields exactly once to set up reviews
    - If reviews_workflow is non-null, the endpoint is read-only
Add workflows that define valid transitions
- Workflows:
  - pre-moderation: Preprints are publicly visible as soon as they're submitted, but can later be removed by moderators
  - post-moderation: Preprints are visible only after approval by a moderator
- Valid states:
  - initial: Default state, created but not yet submitted
  - pending: Submitted for and awaiting review, may be publicly visible depending on the workflow
  - accepted: Has explicit moderator approval, publicly visible under the provider
  - rejected: Has explicit moderator disapproval, only visible to contributors and moderators
- Valid action triggers
  - submit
    - Move from inital to pending, or from rejected to pending if resubmission is allowed by the workflow
    - Node admins and moderators can submit
  - accept
    - Move from pending or rejected to accepted
    - Only moderators can accept
  - reject
    - Move from pending or accepted to rejected
    - Only moderators can reject
  - edit_comment
    - Modify the comment visible to moderators (and possibly contributors) without changing the preprint's state
    - Only moderators can edit_comment
Use django guardian for permissions
- Create admin and moderator groups for each provider, assign group various object permissions on that provider
- Add users to groups to make them admin and/or moderator

Side Effects

Preprints workflow

If a provider has non-null reviews_workflow, authors can no longer publish their preprints directly (by setting is_published). Instead, they must create a submit action at /actions/. The corresponding changes to ember-preprints are here: CenterForOpenScience/ember-osf-preprints#465

If a provider has null reviews_workflow, the existing behavior is unchanged. Workflows are entirely opt-in per provider, for now.

Ember OSF

ember-osf changes: CenterForOpenScience/ember-osf#272

[#OSF-8498] Return relationship data for those relations of the form... proto://host:port/v2/<namespace>/<namespace_id>/

* [MOD-20][Feature] Add PreprintService.state field * [MOD-21][Feature] Add ReviewProviderMixin * Responding to review * Use Enum * Isolate transitions machine * comments_public -> comments_private, for clarity

* [MOD-12] Setup ember-osf-reviews app *Update docker setup *Add osf-reviews campaign *Add reviews route *Add osf-reviews logo *Update registration templates *Update osf navbar *Add mako landing page * fix

* Add reviews fields to preprint/provider endpoints * Add permissions... * Cleaner permissions * Apply permissions to ReviewLog endpoints * Preprint permissions... * Use reviews permissions on all preprint endpoints * Filter preprint providers by reviews permission * Add provider setup endpoint * Allow node admins to see logs on their submissions Assuming the provider settings allow it * Hide log creators if comments are anonymous * Add script to generate fake ReviewLogs * Replace log state fields with action * Creating review logs to trigger state transitions * Allow resubmitting * Fix admin * Filter permissions correctly * Responding to review * Fix reviews migration * Responding to review * Fix stupid mistakes * Cleaner PermissionHelper * Fix provider log list view * Add preprint list permissions tests * Fix migrations after rebase * Fix a couple things... * No reviews workflow is None instead of 'none' * Create review logs at /v2/reviews/review_logs/ * Add read-only title and contributors to preprints * Hide preprints with private nodes from moderators * Add date_last_transitioned field to preprints * Expose live reload port for reviews * Better permission names * Tests for creating review logs, transitions

* Add related counts for reviewables * Fixes * CR

* More reviews tests * plac8 flake8 * Filter providers by multiple permissions with OR * Tests and cleanup... * More cleanup * Responding to review

* ReviewLog => Action * Move Action model to osf.models * Fix up migrations * Fix failing tests * Fix up migrations again * Make /actions/ useful in the browsable API

This reverts commit 51d8b4d.

* Set abandoned to False on submission * Optimize query

brianjgeiger · 2017-10-18T15:34:02Z

+        target = validated_data.pop('target')
+        comment = validated_data.pop('comment', '')
+        try:
+            if trigger == Triggers.ACCEPT.value:


I feel like this will be simplified one day, as it seems like it's having to maintain a redundant list. But not a big deal right now.

brianjgeiger · 2017-10-18T17:39:37Z


-    this.nextLink = link ? 
+    this.nextLink = link ?
        link + '&version=2.2' :


It wouldn't be a bad idea to upgrade to 2.6 for this, but I don't think it would hold anything up leaving it at 2.2.

sloria

Pass finished.

sloria · 2017-10-18T16:03:04Z

+<div style="margin: 40px;">
+    <p>Hello ${user.fullname},</p>
+    % if workflow == 'pre-moderation':
+        <p>Your submission "${reviewable.node.title}", submitted to ${reviewable.provider.name} has ${'not been accepted. You may edit the '+ reviewable.provider.preprint_word+ ' and resubmit, at which time it will becoming pending moderation.' if is_rejected else 'been accepted by the moderator and is now discoverable to others.'}</p>


Almost always prefer str.format to string concatenation, even in templates.

http://cosdev.readthedocs.io/en/latest/style_guides/python.html#string-formatting

sloria · 2017-10-18T16:51:49Z

@@ -0,0 +1,31 @@
+# -*- coding: utf-8 -*-


Minor: Can you rename this file to something more descriptive? Custom migrations should not use the autogenerated names. Something like 0065_set_abandoned_false_for_all_submitted_preprints.py

sloria · 2017-10-18T17:14:17Z

@@ -0,0 +1,49 @@
+# -*- coding: utf-8 -*-


Same here: Can you rename this file to something more descriptive?

sloria · 2017-10-18T17:32:55Z

+    ]
+
+    operations = [
+        migrations.RunPython(add_reviews_notification_setting),


This is a very slow migration--after 30 minutes, I'm still in the users whose GUIDS start with "a". And since there are schema changes in later migrations, the app would need to be down until this migration finished.

One workaround would be to make this a mgmt command instead of a migration. Just be clear document in the ticket that the command needs to be run when this is deployed.

sloria · 2017-10-18T17:42:20Z

    send_type = models.CharField(max_length=50, db_index=True, validators=[validate_subscription_type, ])
    event = models.CharField(max_length=50)
-    message = models.CharField(max_length=2048)
+    message = models.CharField(max_length=10000)


I think this field should just be changed to a TextField, which doesn't have a db-level limit on character length.

Besides, there's no performance difference between the vachar and text columns in Postgres: https://www.postgresql.org/docs/9.0/static/datatype-character.html

sloria · 2017-10-18T17:46:44Z

+    for user_id in context['email_recipients']:
+        user = OSFUser.load(user_id)
+        context['is_creator'] = user == context.get('reviewable').node.creator
+        email = mails.Mail(template, subject='Confirmation of your submission to {provider}'.format(provider=context.get('reviewable').provider.name))


This should be a constant in website.mails.mails.

sloria · 2017-10-18T17:58:50Z

    title = ser.CharField(source='node.title', required=False)
    description = ser.CharField(required=False, allow_blank=True, allow_null=True, source='node.description')
    tags = JSONAPIListField(child=NodeTagField(), required=False, source='node.tags')
+    node_is_public = ser.BooleanField(read_only=True, source='node__is_public')


Do we actually need this field? If you're just using it for filtering, can you do ?filter[node__is_public]=1 instead?

If there's a way to make that work, that'd be lovely, but I get a 400:

{ "errors": [ { "source": { "parameter": "filter" }, "meta": {}, "detail": "'node__is_public' is not a valid field for this endpoint." } ] }

even if I add node__is_public to filterable_fields

I was about to be very surprised and pleased and possibly a little concerned if that worked.

I don't think it would take a lot to update the filtering to handle this; it's probably just the verification that a field is named what you think it is, but it would be a bit to do.

Ah, it looks like we check if the passed filter name is one of the serializer fields here:

def _get_field_or_error(self, field_name): """ Check that the attempted filter field is valid :raises InvalidFilterError: If the filter field is not valid """ serializer_class = self.serializer_class if field_name not in serializer_class._declared_fields: raise InvalidFilterError(detail="'{0}' is not a valid field for this endpoint.".format(field_name)) if field_name not in getattr(serializer_class, 'filterable_fields', set()): raise InvalidFilterFieldError(parameter='filter', value=field_name) return serializer_class._declared_fields[field_name]

Does DRF support hidden fields? Maybe if read_only=True and write_only=True?

No need for this to block merging this PR, but it's worth a few minutes to see if we can avoid adding the extra field, especially if we intend to ever support nested filtering (node__is_public), which I think we should.

sloria · 2017-10-18T18:15:44Z

    def save_changes(self, ev):
        now = self.action.date_created if self.action is not None else timezone.now()
        should_publish = self.reviewable.in_public_reviews_state
+        self.reviewable.node._has_abandoned_preprint = False


Minor: to avoid potentially repeating queries, save self.reviewable.node to a variable.

node = self.reviewable.node node._has_abandoned_preprint = False .... node.save()

chennan47 · 2017-10-19T16:05:03Z

+
+Your ${reviewable.provider.name} and OSF teams
+
+Center for Open Science


You need to put one empty line between 'Center for Open Science' and '210 Ridge McIntire Road, Suite 500, Charlottesville, VA 22903-5083' or gmail render will just think they are at the same line.

chennan47 · 2017-10-19T16:05:55Z

+
+Center for Open Science
+210 Ridge McIntire Road, Suite 500, Charlottesville, VA 22903-5083
+


You need to add a link to the privacy police here. An example will be https://github.com/CenterForOpenScience/osf.io/pull/7613/files#diff-dfa2a5104ecad07e986bd711b1d9464fR10

chennan47 · 2017-10-19T16:06:52Z

+
+Your ${reviewable.provider.name} and OSF teams
+
+Center for Open Science


One line space.

chennan47 · 2017-10-19T16:07:22Z

+
+Center for Open Science
+210 Ridge McIntire Road, Suite 500, Charlottesville, VA 22903-5083
+


A link to privacy policy.

chennan47 · 2017-10-19T16:08:03Z

+
+Your ${reviewable.provider.name} and OSF teams
+
+Center for Open Science


One line space.

chennan47 · 2017-10-19T16:08:24Z

+
+Center for Open Science
+210 Ridge McIntire Road, Suite 500, Charlottesville, VA 22903-5083
+


A link to privacy policy

chennan47 · 2017-10-19T17:12:03Z

+        Your ${reviewable.provider.name} and OSF teams
+        <p>
+        Center for Open Science<br>
+        210 Ridge McIntire Road, Suite 500, Charlottesville, VA 22903-5083


This is fine for now. But after #7613 got merged in. There will be duplicate in html email templates for COS physical address and privacy policy. Create a ticket for the later on clean up. https://openscience.atlassian.net/browse/OSF-8826

chennan47 · 2017-10-19T17:13:57Z

@@ -0,0 +1,31 @@
+Hello ${user.fullname},


Product team has specified all emails to be in html email template instead of txt. There is a ticket to change all current existing txt email to html, which is https://openscience.atlassian.net/browse/OSF-8770. This is fine for now. But we should stop writing any more txt email template.

Rename migrations Use variable for related node Use constant in mails.py Change message field on NotificationDigest from char to text Add management command for adding subscriptions to users

* Update templates and mixins * Remove .txt templates and Apply other requested changes * Apply requested changes

Respond to Reviews CR

* * Add new re-submission template * Add new method notify_resubmit * Update workflow * Remove zipcode * Apply requested changes * Apply requested changes

aaxelb · 2017-10-20T15:11:18Z

+
+# Handle email notifications including: update comment, accept, and reject of submission.
+@reviews_signals.reviews_email.connect
+def reviews_notification(self, context):


Note: This will be refactored in the 0.1.0 release to use a mails.notify-like function instead of checking subscriptions and creating NotificationDigests manually.
https://openscience.atlassian.net/browse/MOD-150

sloria · 2017-10-23T18:22:10Z

+
+REVIEWS_SUBMISSION_CONFIRMATION = lambda provider_name: Mail(
+    'reviews_submission_confirmation',
+    subject='Confirmation of your submission to {}'.format(provider_name)


@laurenbarker Did subject='Confirmation of your submission to ${provider_name}' not work?

Woops, disregard. Didn't see this one done in a future commit.

sloria · 2017-10-23T18:23:23Z

-        mails.send_mail(user.username, email, mimetype='html', user=user, **context)
+        mails.send_mail(
+            user.username,
+            getattr(mails, 'REVIEWS_SUBMISSION_CONFIRMATION')(context.get('reviewable').provider.name),


Why is getattr necessary?

sloria · 2017-10-23T18:26:28Z

🚢 🇮🇹 !

TomBaxter and others added 30 commits October 3, 2017 17:30

Update API to return ...{relationship: {data: {type: , id:}...

cc6ba73

[#OSF-8498] Return relationship data for those relations of the form... proto://host:port/v2/<namespace>/<namespace_id>/

[MOD-29] Reviews Initial Commit

8165183

[MOD-18] Add reviews to EXTERNAL_EMBER_APPS

0251b49

[MOD-20][MOD-21][Feature] Add reviews mixins (#21)

8cda742

* [MOD-20][Feature] Add PreprintService.state field * [MOD-21][Feature] Add ReviewProviderMixin * Responding to review * Use Enum * Isolate transitions machine * comments_public -> comments_private, for clarity

Log model and read-only endpoints (#24)

cb8a053

[MOD-12][MOD-17][MOD-39]Setup ember-osf-reviews app (OSF-side) (#22)

5b97814

* [MOD-12] Setup ember-osf-reviews app *Update docker setup *Add osf-reviews campaign *Add reviews route *Add osf-reviews logo *Update registration templates *Update osf navbar *Add mako landing page * fix

Expose date_last_transitioned in API (#27)

7fa6611

[MOD-61] Set reviews settings by PATCHing provider (#28)

919295a

Nullable provider settings, set all at once (#29)

ac81090

Add related counts for reviewables (#30)

262367f

* Add related counts for reviewables * Fixes * CR

[Fix] Hack around GuidQuerySet (#31)

75eba5c

[Feature] More API tests and some cleanup (#32)

97a9548

* More reviews tests * plac8 flake8 * Filter providers by multiple permissions with OR * Tests and cleanup... * More cleanup * Responding to review

Post-rebase cleanup

30c7872

Make travis happy

3582395

Remove osf-reviews campaign

6382c2e

Make travis happier...

b5a85d6

Add can_view_reviews to /users/me/ (#1)

b059ee9

[Feature][MOD-73] Rename log to action (#2)

c103cb6

* ReviewLog => Action * Move Action model to osf.models * Fix up migrations * Fix failing tests * Fix up migrations again * Make /actions/ useful in the browsable API

Cleanup for consistency

e194199

Add action relationship to users serializer

9d609db

Comment out reviews-specific dev docker stuff

481ed25

Responding to review

38b28fa

Remove OSFReviews from navbar (#3)

bc68494

Fix provider detail endpoint

20421be

Responding to review...

c73320e

Keep permissions in permissions classes

3d6eb79

Move scripts to management commands

7275977

Revert "Keep permissions in permissions classes"

2e040d0

This reverts commit 51d8b4d.

Reenable sorting tests

7c15538

aaxelb and others added 2 commits October 17, 2017 11:06

Fix email template

9bd39aa

[MOD-147][Improvement] Set abandoned to False on submission (#7816)

34e0f67

* Set abandoned to False on submission * Optimize query

brianjgeiger approved these changes Oct 18, 2017

View reviewed changes

sloria suggested changes Oct 18, 2017

View reviewed changes

Include no_future_emails in submission confirmation email (#7823)

3e17965

chennan47 reviewed Oct 19, 2017

View reviewed changes

laurenbarker mentioned this pull request Oct 19, 2017

Respond to Reviews CR #7825

Merged

laurenbarker and others added 6 commits October 19, 2017 14:54

Update global_reviews notification display text

1e82ff7

Rename migrations Use variable for related node Use constant in mails.py Change message field on NotificationDigest from char to text Add management command for adding subscriptions to users

Add provider_name to the context

7472a82

Clean up the migrations

25e9014

[Improvement][MOD-154] Update wording in emails (#7820)

dbfed68

* Update templates and mixins * Remove .txt templates and Apply other requested changes * Apply requested changes

Merge pull request #7825 from laurenbarker/improvement/respond-cr

0e7089f

Respond to Reviews CR

[Improvement][MOD-152] Update re-submisison email notificaiton (#7821)

8323a94

* * Add new re-submission template * Add new method notify_resubmit * Update workflow * Remove zipcode * Apply requested changes * Apply requested changes

aaxelb commented Oct 20, 2017

View reviewed changes

laurenbarker added 2 commits October 20, 2017 13:17

Don't say user will get email if no_future_emails (#7833)

63872d1

Fix rejected wording (#7834)

1bc9d27

sloria reviewed Oct 23, 2017

View reviewed changes

sloria approved these changes Oct 23, 2017

View reviewed changes

sloria merged commit b7f857f into develop Oct 23, 2017

binoculars deleted the feature/reviews branch October 30, 2017 18:18

TomBaxter mentioned this pull request Nov 14, 2017

[RFR] [OSF-8498] API: Add id and type to relationships #7702

Closed


		Your ${reviewable.provider.name} and OSF teams

		Center for Open Science


		Center for Open Science
		210 Ridge McIntire Road, Suite 500, Charlottesville, VA 22903-5083

Uh oh!

Conversation

aaxelb commented Oct 12, 2017 • edited by laurenbarker Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Purpose

Changes

New changes since #7708 was approved

Original changes

Side Effects

Preprints workflow

Ember OSF

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sloria left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sloria Oct 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sloria Oct 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sloria Oct 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sloria commented Oct 23, 2017

Uh oh!

Reviewers

Assignees

Labels

aaxelb commented Oct 12, 2017 •

edited by laurenbarker

Loading

sloria Oct 18, 2017 •

edited

Loading

sloria Oct 18, 2017 •

edited

Loading

sloria Oct 18, 2017 •

edited

Loading