Rewrite comments on 'reopen' and 'issue_is_regression'

issues/models.py

@@ -190,13 +190,22 @@ class IssueStateManager(object):
 
     @staticmethod
     def reopen(issue):
+        # this is called "reopen", but since there's no UI for it, it's more like "deal with a regression" (i.e. that's
+        # the only way this gets called).
         issue.is_resolved = False
-        issue.is_resolved_by_next_release = False  # ?? echt?
-        # TODO and what about fixed_at ?
 
-        # as in IssueStateManager.resolve(), but not because a reopened issue cannot be muted (we could mute it soon
-        # after reopening) but because when reopening an issue you're doing this from a resolved state; calling unmute()
-        # here is done as a consistency-enforcement after the fact.
+        # we don't touch is_resolved_by_next_release (i.e. set to False) here. Why? The simple/principled answer is that
+        # observations that Bugsink can make can by definition not be about the future. If the user tells us "this
+        # is fixed in some not-yet-released version" there's just no information ever in Bugsink to refute that".
+        # (BTW this point in the code cannot be reached when issue.is_resolved_by_next_release is True anyway)
+
+        # we also don't touch `fixed_at`. The meaning of that field is "reports came in about fixes at these points in
+        # time", not "it actually _was_ fixed at all of those points" and the finer differences between those 2
+        # statements is precisely what we have quite some "is_regression" logic for.
+
+        # as in IssueStateManager.resolve(), but not because a reopened issue cannot be muted in principle (i.e. we
+        # could mute it soon after reopening) but because when reopening an issue you're doing this from a resolved
+        # state; calling unmute() here is done as an after-the-fact consistency-enforcement.
         IssueStateManager.unmute(issue)
 
     @staticmethod

issues/regressions.py

@@ -1,5 +1,9 @@
 from releases.models import ordered_releases
 
+# note that we don't check for is_muted anywhere in this file; this is because issues that are muted are unresolved (the
+# combination of muted & resolved is illegal and we enforce this elsewhere) and the unresolved case is trivially not a
+# regression. (first guard clause in `issue_is_regression`)
+
 
 def is_regression(sorted_releases, fixed_at, events_at, current_event_at):
     # NOTE: linear in time with the number of releases; however, for now it's a nice reference implementation.
@@ -26,19 +30,28 @@ def is_regression(sorted_releases, fixed_at, events_at, current_event_at):
 
 
 def issue_is_regression(issue, current_event_at):
-    # note that we don't check for is_muted anywhere in this file; this is because issues that are muted are unresolved
-    # (the combination of muted & resolved is illegal and we enforce this elsewhere) and the is_resolved == False case
-    # is basically the trivial case.
+    """
+    Given that a new event has just occurred, is this issue a regression?
+    `current_event_at` is the release at which the event occurred.
+
+    Must be called after `is_resolved_by_next_release`-handling for new releases (i.e. `create_release_if_needed`) to
+    ensure the logic surrounding `issue.is_resolved_by_next_release` is correct.
+    """
+
     if not issue.is_resolved:
+        # unresolved issues can't be regressions by definition
         return False
 
     if issue.is_resolved_by_next_release:
-        # i.e. this is solved, but only "in the future". The assumption (which is true in our code) here is: once this
-        # point is reached, all "actually seen releases" will have already been accounted for.
+        # if issue.is_resolved and issue.is_resolved_by_next_release <= implied because of the first guard clause.
+        # Which is to say the `is_resolved` marker is there, but another field qualifies it as "only in the future".
+        # Which means that seeing new events does not imply a regression, because that future hasn't arrived yet.
         return False
 
     if not issue.project.has_releases:
-        return True  # i.e. `return issue.is_resolved`, which is True if this point is reached.
+        # the simple case: no releases means that seeing new events implies a regression if the issue.is_resolved, which
+        # is True given the first guard clause.
+        return True
 
     sorted_releases = [r.version for r in ordered_releases(project=issue.project)]
     fixed_at = issue.get_fixed_at()