RFR: 1584: Require re-review of a PR if the target branch changes [v3]

[Erik Helin]

On Tue, 4 Oct 2022 18:16:09 GMT, Erik Joelsson <erikj at openjdk.org> wrote:

>> This change improves reviewer handling when the target branch of a PR is changed. Currently this only invalidates a review on GitHub, but not on GitLab. With this change the behavior is made uniform. In addition to this, the following new behavior is added.
>> 
>> 1. When the target branch of a PR is changed, any existing reviews in the Reviewers list will be flagged as "Re-review required (review applies to pull request targeting <branch>)" to make what happened clearer.
>> 2. If the target branch of a PR is changed back to a previously targeted branch, any reviews made against that branch may now become valid again (if all other requirements are met).
>> 
>> Note that changes to and from pr/X branches are ignored, just like before.
>> 
>> PullRequest::reviews is no longer specifying the order of reviews returned. This order was actually not consistent between implementations and I would rather we use explicit sorts when needed instead of assuming an order.
>> 
>> I'm not really happy with the placement of the new static method `calculateReviewTargetRefs`, but I couldn't really find a better location, and I really wanted to share this between different `PullRequest` implementations. I think the correct solution would be to introduce an `AbstractPullRequest` common super class, but I didn't want to do this for a single method.
>> 
>> I have verified this with the new and modified tests, as well as by running against the playground repo(s).
>
> Erik Joelsson has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains three commits:
> 
>  - Merge branch 'master' into SKARA-1584-rereview-targetref
>  - Merge branch 'master' into SKARA-1584-rereview-targetref
>  - SKARA-1584

This patch looks good, just some stylistic nits.

I don't really mind the static method, in general I prefer that over an abstract base class. I personally think that an abstract base class makes it so much harder to follow the control flow in sub-classes, whereas now it is very clear in `GitLabMergeRequest` and `GitHubPullRequest` how the static helper method is used.

bots/pr/src/main/java/org/openjdk/skara/bots/pr/CheckRun.java line 505:

> 503:                                    if (!review.targetRef().equals(pr.targetRef())) {
> 504:                                        entry += " 🔄 Re-review required (review applies to pull request targeting [" + review.targetRef()
> 505:                                                + "](" + pr.repository().webUrl(new Branch(review.targetRef())) + "))";

I would probably have phrased this as `"Re-review required (review was made when pull request targeted the " + review.targetRef() + " branch)";`, but that is just my style 😸

forge/src/main/java/org/openjdk/skara/forge/RefChange.java line 5:

> 3: import java.time.ZonedDateTime;
> 4: 
> 5: public record RefChange(String prevRefName, String curRefName, ZonedDateTime createdAt) {

Again, I'm more for shorter names, so I would have gone with `ReferenceChange(String from, String to, ZonedDateTime at)`

forge/src/main/java/org/openjdk/skara/forge/Review.java line 52:

> 50:     }
> 51: 
> 52:     public Review(Review other, String targetRef) {

I would probably have used a static method like `public static Review withTargetRef(Review r, String targetRef)` or potentially have a instance method `public Review withTargetRef(String targetRef)`.

-------------

Marked as reviewed by ehelin (Reviewer).

PR: https://git.openjdk.org/skara/pull/1383