RFR: 1258: Add review link/details to comments section

[Guoxiong Li]

On Fri, 3 Dec 2021 12:55:03 GMT, Guoxiong Li <gli at openjdk.org> wrote:

> Hi all,
> 
> This patch adds a comment to the jira issue when the corresponding PR is submitted. Because currently, the class `Issue` has no an api to remove a comment. So I need to create and implement it, too. And the test cases are added.
> 
> Thanks for taking the time to review.
> 
> Best Regards,
> -- Guoxiong

> > From the code, I guess you can see the second style. Is my guess right?
>
> You see the first one, as onNewCommit seems to be executing first. I have searched back JBS and the newest bugs with where the second style comment can be found were fixed for JDK 16, so before #784 (in these issues both comments are present).

The `see the first one` confused me. Please see the following code analysis.

The method `parseWebLinkComment` can only identify the comment lines `Remote link:` and `URL:`. 


    private static final Pattern titlePattern = Pattern.compile("^Remote link: (.*)");
    private static final Pattern urlPattern = Pattern.compile("^URL: (.*)");
    private static final Pattern summaryPattern = Pattern.compile("^Summary: (.*)");
    private static final Pattern relationshipPattern = Pattern.compile("^Relationship: (.*)");

    private Optional<Link> parseWebLinkComment(Comment comment) {
        var lines = comment.body().lines().collect(Collectors.toList());
        if ((lines.size() < 2 ) || (lines.size() > 4)) {
            return Optional.empty();
        }
        var titleMatcher = titlePattern.matcher(lines.get(0));  // <----  Remote link: XXXX
        var urlMatcher = urlPattern.matcher(lines.get(1));   // <----  URL: XXXX
        if (!titleMatcher.matches() || !urlMatcher.matches()) {   // <---- Both `Remote link:` and `URL:` should exist
            return Optional.empty();   // <---- if not, it will return Optional.empty()
        }
        // ignore other code...
    }
``` 

So when the #784 added the following code, the variable `alreadyPosted` just can identify the comment which has `Remote link:` and `URL:`. But it can't identify the comment created by the method `onNewCommit`, which has `Changeset: Author: Committer: Date: URL: `.


    private void addWebLinkAsComment(Link link) {
        var alreadyPosted = comments().stream()
                                      .map(this::parseWebLinkComment)
                                      .filter(Optional::isPresent)
                                      .map(Optional::get)
                                      .anyMatch(l -> l.uri().equals(link.uri()) && l.title().equals(link.title()));
        if (alreadyPosted) {
            return;
        }
        // ignore other code...
    }


As @rwestberg mentioned in the #784:

> Please review this change that avoids potentially posting the same Jira link comment multiple times.

The #784 only can avoid potentially posting **the same** Jira link comment multiple times.

And the method `onNewCommit` has the following code which can identify the comment created by `addWebLinkAsComment`. It only checks the url and not checks the other pattern, which is not like `parseWebLinkComment`.

``` 
    // only check the url
    var alreadyPostedComment = existingComments.stream()
            .filter(comment -> comment.author().equals(issueProject.issueTracker().currentUser()))
            .anyMatch(comment -> comment.body().contains(hashUrl) || comment.body().contains(shortHashUrl));
    if (!alreadyPostedComment) {
        issue.addComment(commitNotification);
    }


---

>From the code, we can simulate the run result.

**If the issues are public (I can see):**
1. if the `onNewCommit` runs earier than `onIntegratedPullRequest`.
  (1) The `onNewCommit` will create the comment as `Changeset: Author: Committer: Date: URL:`.
  (2) Then the `onIntegratedPullRequest` will  create the issue link for the commit.

  It is reasonable and expected.

2. if the `onIntegratedPullRequest` runs earier than `onNewCommit`.
  (1) The `onIntegratedPullRequest` will  create the issue link for the commit.
  (2) Then the `onNewCommit` will create the comment as `Changeset: Author: Committer: Date: URL:`.

  The result is the same as the point 1. And it is the situation I always see in the OpenJDK JBS.


**If the issues are confidential (not public) (I can't see):**
1. if the `onNewCommit` runs earier than `onIntegratedPullRequest`.
  (1) The `onNewCommit` will create the comment as `Changeset: Author: Committer: Date: URL:`.
  (2) Then the `onIntegratedPullRequest` runs. Because `parseWebLinkComment` can't identify the comment `Changeset: Author: Committer: Date: URL:`, so `onIntegratedPullRequest` creates another comment `Remote link:` and `URL:` for the commit.

  In this situation, there are two comments added. It is unexpected.

2. if the `onIntegratedPullRequest` runs earier than `onNewCommit`.  
  (1) The `onIntegratedPullRequest` runs. Because there is no comment now, so `onIntegratedPullRequest` creates comment `Remote link:  URL:` for the commit.
  (2) Then the `onNewCommit` runs. It only identifies the url, which can identify the comment `Remote link: URL:`. So it won't create the comment again.

  In this situation, only one comment like `Remote link:  URL:` is added to the issue. It is unexpected, too. Because @erikj79 said `see the first one`.

----

I think the problem is not in method `parseWebLinkComment` and `addWebLink`.  I re-read the code, it seems the problem is caused by the config `commitLink`.

In the public issues, the `commitLink` is `true`, so the issue link can be created by using the method `onIntegratedPullRequest`.
So the analysis above about this situation is right.


// method onIntegratedPullRequest
            if (commitLink) {   // <---- Here
                var linkBuilder = Link.create(repository.webUrl(hash), "Commit")
                                      .summary(repository.name() + "/" + hash.abbreviate());
                if (commitIcon != null) {
                    linkBuilder.iconTitle("Commit");
                    linkBuilder.iconUrl(commitIcon);
                }
                issue.addLink(linkBuilder.build());  // <---- if the commitLink is false, this line never runs.
            }


In the confidential issues, the `commitLink` is false, so the issue comment `Remote link:  URL:`  never be created because the `issue.addLink` never runs. And the method `onNewCommit` will always create the comment `Changeset: Author: Committer: Date: URL:`, which you can see in current confidential (oracle internal) issues.

Could I get your help to read the configuration about the `commitLink` to verify my suspection?

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

PR: https://git.openjdk.java.net/skara/pull/1250