fix: Make review settings history usable (fixes #3213).

ietf/doc/templatetags/ietf_filters.py

@@ -881,3 +881,21 @@ def badgeify(blob):
             )
 
     return text
+
+@register.filter
+def simple_history_delta_changes(history):
+    """Returns diff between given history and previous entry."""
+    prev = history.prev_record
+    if prev:
+        delta = history.diff_against(prev)
+        return delta.changes
+    return []
+
+@register.filter
+def simple_history_delta_change_cnt(history):
+    """Returns number of changes between given history and previous entry."""
+    prev = history.prev_record
+    if prev:
+        delta = history.diff_against(prev)
+        return len(delta.changes)
+    return 0

ietf/review/policies.py

@@ -131,12 +131,15 @@ def _update_skip_next(self, rotation_pks, assignee_person):
         assignee_index = rotation_pks.index(assignee_person.pk)
         skipped = rotation_pks[0:assignee_index]
         skipped_settings = self.team.reviewersettings_set.filter(person__in=skipped)  # list of PKs is valid here
+        changed = []
         for ss in skipped_settings:
-            ss.skip_next = max(0, ss.skip_next - 1)  # ensure we don't go negative
-        bulk_update_with_history(skipped_settings,
+            if ss.skip_next > 0:
+                ss.skip_next = max(0, ss.skip_next - 1)  # ensure we don't go negative
+                ss._change_reason = "Skip count decremented"
+                changed.append(ss)
+        bulk_update_with_history(changed,
                                  ReviewerSettings,
-                                 ['skip_next'],
-                                 default_change_reason='skipped')
+                                 ['skip_next'])
 
     def _assignment_in_order(self, rotation_pks, assignee_person):
         """Is this an in-order assignment?"""
@@ -262,12 +265,15 @@ def _filter_unavailable_reviewers(self, reviewers, review_req=None):
 
     def _clear_request_next_assignment(self, person):
         s = self._reviewer_settings_for(person)
-        s.request_assignment_next = False
-        s.save()
+        if s.request_assignment_next:
+            s.request_assignment_next = False
+            s._change_reason = "Clearing request next assignment"
+            s.save()
 
     def _add_skip(self, person):
         s = self._reviewer_settings_for(person)
         s.skip_next += 1
+        s._change_reason = "Incrementing skip count"
         s.save()
 
     def _reviewer_settings_for(self, person):
@@ -484,6 +490,7 @@ def set_wants_to_be_next(self, reviewer_person):
         # Instead, the "assign me next" flag is set.
         settings = self._reviewer_settings_for(reviewer_person)
         settings.request_assignment_next = True
+        settings._change_reason = "Setting request next assignment"
         settings.save()
 
     def _update_skip_next(self, rotation_pks, assignee_person):
@@ -523,20 +530,22 @@ def _update_skip_next(self, rotation_pks, assignee_person):
             min_skip_next = min([rs.skip_next for rs in rotation_settings.values()])
 
         next_reviewer_index = None
+        changed = []
         for index, pk in enumerate(unfolded_rotation_pks):
             rs = rotation_settings.get(pk)
             if (rs is None) or (rs.skip_next == min_skip_next):
                 next_reviewer_index = index
                 break
             else:
-                rs.skip_next = max(0, rs.skip_next - 1)  # ensure never negative
+                if rs.skip_next > 0:
+                    rs.skip_next = max(0, rs.skip_next - 1)  # ensure never negative
+                    rs._change_reason = "Skip count decremented"
+                    changed.append(rs)
 
         log.assertion('next_reviewer_index is not None')  # some entry in the list must have the minimum value
-
-        bulk_update_with_history(rotation_settings.values(),
+        bulk_update_with_history(changed,
                                  ReviewerSettings,
-                                 ['skip_next'],
-                                 default_change_reason='skipped')
+                                 ['skip_next'])
 
         next_reviewer_pk = unfolded_rotation_pks[next_reviewer_index]
         NextReviewerInTeam.objects.update_or_create(
@@ -578,6 +587,7 @@ def set_wants_to_be_next(self, reviewer_person):
         # who rejected a review and no further action is needed.
         settings = self._reviewer_settings_for(reviewer_person)
         settings.request_assignment_next = True
+        settings._change_reason = "Setting request next assignment"
         settings.save()
 
 

ietf/templates/group/change_reviewer_settings.html

@@ -105,11 +105,17 @@ <h2 class="mt-5">History of settings</h2>
             {% if reviewersettings.history.all %}
                 <tbody>
                     {% for h in reviewersettings.history.all %}
+                        {% if h|simple_history_delta_change_cnt > 0 or h.history_change_reason != "skipped" and h.history_change_reason %}
                         <tr>
                             <td>{{ h.history_date|date }}</td>
                             <td>{% person_link h.history_user.person %}</td>
-                            <td>{{ h.history_change_reason }}</td>
+                            <td>{% if h.history_change_reason != "skipped" and h.history_change_reason %} {{ h.history_change_reason }}<br> {% endif %}
+                                {% for change in h|simple_history_delta_changes %}
+                                   {{ change.field }} changed from "{{change.old}}" to "{{change.new}}"<br>
+                                {% endfor %}
+                            </td>
                         </tr>
+                        {% endif %}
                     {% endfor %}
                 </tbody>
             {% endif %}