RFR: 1683: The force push notifier needs an overhaul

[Erik Joelsson]

On Tue, 22 Nov 2022 21:57:58 GMT, Zhao Song <zsong at openjdk.org> wrote:

> A user reported a bug related with the force-push notification. Erik investigated it and found the implementation of this notification should not be in the `PullRequestBranchNotifier`. Also, users preferred to not get force-push notification if the force-push happens when the pr is in draft state.
> 
> In this patch, the implementation of checking force-push and notifying user are moved to `CheckWorkItem`. Also, force-pushing or rebasing a draft pr will not warn user any more.

Nice work!
I have some comments, but nothing really major. 

I put in some style corrections in some places. Please try to match the existing style with whitespace between keyword and opening paren as well as closing paren and opening brace.

bots/pr/src/main/java/org/openjdk/skara/bots/pr/CheckWorkItem.java line 392:

> 390:             }
> 391: 
> 392:             // Check Force push

I'm not sure if we should put this check here in `CheckWorkItem`. This will cause it to be run quite often. Maybe it would be better to put it in `CheckRun` where it would be guarded by the check metadata condition, so it would only need to be run if something significant has changed since the last check.

bots/pr/src/main/java/org/openjdk/skara/bots/pr/CheckWorkItem.java line 396:

> 394:                 var lastForcePushTime = pr.lastForcePushTime();
> 395:                 if (lastForcePushTime.isPresent()) {
> 396:                     var lastForcePushSuggestion = pr.comments().stream()

`pr.comments()` have already been fetched at the top, so no need to fetch them again.
Suggestion:

                    var lastForcePushSuggestion = comments.stream()

forge/src/main/java/org/openjdk/skara/forge/github/GitHubPullRequest.java line 802:

> 800:     private ZonedDateTime lastMarkedAsReadyTime() {
> 801:         ZonedDateTime readyTime = createdAt();
> 802:         Optional<ZonedDateTime> latestTime = request.get("issues/" + json.get("number").toString() + "/timeline")

This fetches the timeline a second time. Can you try to reuse the timeline result from lastForcePushTime()?

Could you also update the javadoc on `lastForcePushTime` that it's returning the last time something was force pushed while not in draft state.

test/src/main/java/org/openjdk/skara/test/TestPullRequest.java line 275:

> 273:     @Override
> 274:     public Optional<ZonedDateTime> lastForcePushTime() {
> 275:         if(store().lastForcePushTime()!= null && store().lastForcePushTime().isAfter(store().lastMarkedAsReadyTime())) {

Suggestion:

        if (store().lastForcePushTime()!= null && store().lastForcePushTime().isAfter(store().lastMarkedAsReadyTime())) {

test/src/main/java/org/openjdk/skara/test/TestPullRequestStore.java line 59:

> 57:         this.sourceRef = sourceRef;
> 58:         this.draft = draft;
> 59:         if(!draft){

Suggestion:

        if (!draft) {

test/src/main/java/org/openjdk/skara/test/TestPullRequestStore.java line 132:

> 130:     public void setDraft(boolean draft) {
> 131:         this.draft = draft;
> 132:         if(!draft){

Suggestion:

        if (!draft) {

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

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