Merge pull request #667 from sgfost/peer-review-improvements-1

major peer review workflow refactor from @sgfost 

- peer reviews only operate on unpublished draft code releases
- update guidance
- support closing / re-opening peer reviews
- handle invitation expirations better
- misc. bugfixes

django/core/jinja2/core/member_profiles/retrieve.jinja

@@ -137,9 +137,11 @@
                                     {% if invitation.accepted is none %}
                                         <span class="badge bg-warning">Waiting for response</span>
                                     {% elif invitation.review.is_complete %}
-                                        <span class="badge bg-light">Review complete</span>
+                                        <span class="badge bg-gray">Review complete</span>
                                     {% elif not invitation.accepted %}
                                         <span class="badge bg-primary">Declined review request</span>
+                                    {% elif invitation.review.closed %}
+                                        <span class="badge bg-danger">Review closed</span>
                                     {% else %}
                                         {% set badge_class="bg-warning" if invitation.review.is_awaiting_reviewer_feedback else "bg-info" %}
                                         <span class="badge {{badge_class}}">{{ invitation.review.get_status_display() }}</span>

django/core/jinja2/sidebar_layout.jinja

@@ -1,6 +1,13 @@
 {% extends 'base.jinja' %}
 
 {% block page %}
+    <section class='messages'>
+        {% for message in get_messages(request) %}
+            <div class='{{ message.tags }}'>
+                <i class='fas fa-info-circle'></i> {{ message }}
+            </div>
+        {% endfor %}
+    </section>
     <section class='page'>
         <div class="container">
             {% block top %}

django/curator/tests/test_dump_restore.py

@@ -51,7 +51,7 @@ def setUp(self):
         fs_api = self.release.get_fs_api()
         import_archive(
             codebase_release=self.release,
-            nestedcode_folder_name="library/tests/archives/nestedcode",
+            nested_code_folder_name="library/tests/archives/nestedcode",
             fs_api=fs_api,
         )
 

django/library/forms.py

@@ -8,7 +8,6 @@
     PeerReviewerFeedback,
     PeerReviewInvitation,
     ReviewerRecommendation,
-    ReviewStatus,
 )
 
 logger = logging.getLogger(__name__)
@@ -20,9 +19,7 @@ class PeerReviewInvitationForm(forms.ModelForm):
     """
 
     def save(self, commit=True):
-        invitation = super().save(commit)
-        invitation.send_candidate_reviewer_email()
-        return invitation
+        return super().save(commit)
 
     class Meta:
         model = PeerReviewInvitation
@@ -173,6 +170,7 @@ def __init__(self, **kwargs):
             feedback = kwargs["instance"]
             logger.debug("feedback %s", feedback.invitation.review.status)
             kwargs["initial"]["accept"] = feedback.invitation.review.is_complete
+        self.editor = kwargs.pop("editor", feedback.invitation.editor)
         super().__init__(**kwargs)
 
     accept = forms.BooleanField(label="Accept?", required=False)
@@ -183,11 +181,9 @@ def clean_notes_to_author(self):
     def save(self, commit=True):
         feedback = super().save(commit)
         if self.cleaned_data["accept"]:
-            feedback.invitation.review.set_complete_status(
-                editor=feedback.invitation.editor
-            )
+            feedback.invitation.review.set_complete_status(editor=self.editor)
         else:
-            feedback.editor_called_for_revisions()
+            feedback.editor_called_for_revisions(editor=self.editor)
         return feedback
 
     class Meta:
@@ -196,14 +192,15 @@ class Meta:
 
 
 class PeerReviewFilterForm(forms.Form):
+    include_closed = forms.BooleanField(required=False)
     requires_editor_input = forms.BooleanField(required=False)
-    include_dated_author_change_requests = forms.BooleanField(required=False)
-    include_dated_reviewer_feedback_requests = forms.BooleanField(required=False)
+    author_changes_requested = forms.BooleanField(required=False)
+    reviewer_feedback_requested = forms.BooleanField(required=False)
     order_by = forms.ChoiceField(
         choices=[
-            ("-max_last_modified", "Last Modified DESC"),
-            ("min_n_accepted_invites", "Min Accepted Invites ASC"),
-            ("title", "Title ASC"),
+            ("-max_last_modified", "Most recently modified"),
+            ("min_n_accepted_invites", "Least accepted invites"),
+            ("title", "Title"),
         ],
         required=False,
     )

django/library/fs.py

@@ -24,6 +24,10 @@
 
 logger = logging.getLogger(__name__)
 
+"""
+FIXME: consider refactoring to use pathlib.Path throughout this module instead of string paths
+"""
+
 
 class StagingDirectories(Enum):
     # Directory containing original files uploaded (such as zip files)
@@ -119,7 +123,7 @@ def __bool__(self):
         return len(self.msgs) > 0
 
     def __repr__(self):
-        return "<MessageGroup {0}>".format(repr(self.msgs))
+        return f"<MessageGroup {repr(self.msgs)}>"
 
     def append(self, msg: Optional):
         if msg is not None and msg:
@@ -141,10 +145,7 @@ def has_errors(self):
 
     def serialize(self):
         """Return a list of message along with message level"""
-        logs = []
-        for msg in self.msgs:
-            logs.append(msg.serialize())
-        return logs, self.level
+        return [msg.serialize() for msg in self.msgs], self.level
 
 
 class Message:
@@ -168,6 +169,10 @@ def create_fs_message(detail, stage: StagingDirectories, level: MessageLevels):
 
 
 class CodebaseReleaseStorage(FileSystemStorage):
+    """
+    storage abstraction for CodebaseRelease files
+    """
+
     stage = None
 
     def __init__(
@@ -187,9 +192,9 @@ def __init__(
         self.mimetype_mismatch_message_level = mimetype_mismatch_message_level
 
     def validate_system_file(self, name, content) -> Optional[Message]:
-        # FIXME: do we expect validate_file being run on full paths?
+        # FIXME: do we expect validate_file to be run on absolute paths?
         if fs.has_system_files(name):
-            return self.error("Ignored system file '{}'".format(name))
+            return self.error(f"Ignored system file '{name}'")
         return None
 
     def validate_mimetype(self, name):
@@ -198,7 +203,7 @@ def validate_mimetype(self, name):
         mimetype = mimetype if mimetype else "*/*"
         if not mimetype_matcher.match(mimetype):
             return create_fs_message(
-                "Ignored file '{}'. File type mismatch.".format(name),
+                f"Ignored file '{name}': invalid filetype {mimetype}",
                 self.stage,
                 self.mimetype_mismatch_message_level,
             )
@@ -438,7 +443,7 @@ def get_stage_storage(self, stage: StagingDirectories):
         elif stage == StagingDirectories.aip:
             return self.get_aip_storage()
         else:
-            raise ValueError("StageDirectories values {} not valid".format(stage))
+            raise ValueError(f"StageDirectories values {stage} not valid")
 
     def get_sip_list_url(self, category: FileCategoryDirectories):
         return reverse(
@@ -603,7 +608,7 @@ def delete(self, category: FileCategoryDirectories, relpath: Path):
             if not originals_storage.exists(str(relpath)):
                 logs.append(
                     create_fs_message(
-                        "No file at path {} to delete".format(str(relpath)),
+                        f"No file at path {relpath} to delete",
                         StagingDirectories.originals,
                         MessageLevels.error,
                     )
@@ -623,14 +628,14 @@ def _add_to_sip(self, name, content, category: FileCategoryDirectories):
             return sip_storage.log_save(name=name, content=content)
 
     def add_category(self, category: FileCategoryDirectories, src):
-        logger.info("adding category {}".format(category.name))
+        logger.info("adding category %s", category.name)
         originals_storage = self.get_originals_storage()
         msgs = self._create_msg_group()
         for dirpath, dirnames, filenames in os.walk(src):
             for filename in filenames:
                 filename = os.path.join(dirpath, filename)
                 name = os.path.join(category.name, str(Path(filename).relative_to(src)))
-                logger.debug("adding file {}".format(name))
+                logger.debug("adding file %s", name)
                 with open(filename, "rb") as content:
                     msgs.append(originals_storage.log_save(name, content))
         return msgs
@@ -652,6 +657,23 @@ def add(self, category: FileCategoryDirectories, content, name=None):
 
         return msgs
 
+    def copy_originals(self, source_release):
+        """copy all original files from a source CodebaseRelease to the calling release"""
+        logger.info(
+            "copying files from source version %s to version %s for codebase %s",
+            source_release.version_number,
+            self.version_number,
+            self.identifier,
+        )
+        source_fs_api = source_release.get_fs_api()
+        for category in FileCategoryDirectories:
+            source_files = source_fs_api.list(StagingDirectories.originals, category)
+            for relpath in source_files:
+                with source_fs_api.retrieve(
+                    StagingDirectories.originals, category, Path(relpath)
+                ) as file_content:
+                    self.add(category, file_content, name=relpath)
+
     def get_or_create_sip_bag(self, bagit_info=None):
         sip_dir = str(self.sip_dir)
         logger.info("creating bagit metadata at %s", sip_dir)
@@ -670,7 +692,7 @@ def build_sip(self, originals_dir: Optional[str] = None):
         msgs = self._create_msg_group()
         for name in originals_storage.list():
             path = self.originals_dir.joinpath(name)
-            logger.debug("adding file: {}".format(path.relative_to(self.originals_dir)))
+            logger.debug("adding file: %s", path.relative_to(self.originals_dir))
             category = get_category(Path(name).parts[0])
             with File(path.open("rb")) as f:
                 msgs.append(
@@ -739,7 +761,7 @@ def extractall(self, unpack_destination, filename):
                 r.extractall(path=unpack_destination)
 
         else:
-            return Message("Archive {} is unsupported".format(filename))
+            return Message(f"Archive {filename} is unsupported")
 
     def find_root_directory(self, basedir):
         for dirpath, dirnames, filenames in os.walk(basedir):
@@ -789,11 +811,12 @@ def process(self, category: FileCategoryDirectories, filename: str):
         return msgs
 
 
-def import_archive(codebase_release, nestedcode_folder_name, fs_api=None):
+def import_archive(codebase_release, nested_code_folder_name, fs_api=None):
+    """currently only used for tests"""
     if fs_api is None:
         fs_api = codebase_release.get_fs_api()
-    archive_name = "{}.zip".format(nestedcode_folder_name)
-    shutil.make_archive(nestedcode_folder_name, "zip", nestedcode_folder_name)
+    archive_name = f"{nested_code_folder_name}.zip"
+    shutil.make_archive(nested_code_folder_name, "zip", nested_code_folder_name)
     with open(archive_name, "rb") as f:
         msgs = fs_api.add(
             FileCategoryDirectories.code, content=f, name="nestedcode.zip"

django/library/jinja2/library/codebases/macros.jinja

@@ -61,7 +61,7 @@
                 <div class="title">
                     <h1>
                         {% if codebase.peer_reviewed %}
-                            <span class='badge bg-success'>Peer reviewed</span>
+                            <span class='badge bg-info'>Peer reviewed</span>
                         {% endif %}
                         <a href="{{ codebase.get_absolute_url() }}">{{ codebase.title }}</a>
                         {% if not codebase.live %}

django/library/jinja2/library/codebases/releases/edit.jinja

@@ -9,7 +9,9 @@
 <div id="release-editor" data-version-number="{{ release.version_number }}"
     data-identifier="{{ release.codebase.identifier }}"
     data-review-status="{{ release.get_review().get_status_display() if release.get_review() else 'Unreviewed' }}"
-    data-is-live="{{ release.live }}"></div>
+    data-is-live="{{ release.live }}"
+    data-can-edit-originals="{{ release.can_edit_originals }}"
+    ></div>
 {% endblock %}
 
 {% block js %}