From db486adb3540f8703d03fe5bb490d1b47d3ad920 Mon Sep 17 00:00:00 2001 From: Klaas van Schelven Date: Tue, 17 Sep 2024 23:01:41 +0200 Subject: [PATCH] Rewrite comments on 'reopen' and 'issue_is_regression' --- issues/models.py | 19 ++++++++++++++----- issues/regressions.py | 25 +++++++++++++++++++------ 2 files changed, 33 insertions(+), 11 deletions(-) diff --git a/issues/models.py b/issues/models.py index cfee863..4b780bd 100644 --- a/issues/models.py +++ b/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 diff --git a/issues/regressions.py b/issues/regressions.py index c47b799..e04e9c4 100644 --- a/issues/regressions.py +++ b/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()