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

[Erik Joelsson]

On Fri, 3 Dec 2021 17:17:35 GMT, Guoxiong Li <gli at openjdk.org> wrote:

> > If you look in JiraIssue::addWebLink, it already uses a comment instead of a link in certain cases (specifically when the link is considered confidential). This is the behavior we now wish to emulate all the time, but in addition to the weblink. IMO, we just need to change this method a bit, so that it always calls ::addWebLinkAsComment and then skips the weblink part if needsSecurity is true.
> 
> @erikj79 I re-read the code. If we make `addWebLinkAsComment` always be invoked, it may cause some unexpected behavior.
> 
> For example, when a patch integrates, a link will be added to the issue (The code is at `IssueNotifier#onIntegratedPullRequest`). At the same time, a comment will be added to the issue (The code is at `IssueNotifier#onNewCommits`). If we enable the `addWebLinkAsComment`, two similar comments would be added to the same issue.
> 
> We can remove the related code in `IssueNotifier#onNewCommits` to let the bot only add one comment. But actually, the comment message of the `IssueNotifier#onNewCommits` is more readable and precise. If we always remove the related code like `IssueNotifier#onNewCommits`, I can't find all the places to modify now and the change is more large and more difficult.
> 

I've now read through the code in question and you are partly right. First of all, internally at Oracle we are already using the code path where comments are used instead of issue links. The "needsSecurity" conditional basically means "is this Oracle confidential". You of course can't see this in action, but this makes me comfortable that it currently actually works. 

We did have an issue with duplicate comments being posted in the past. This was fixed in #784. I don't really like that solution, but it does the trick. Looking at actual (confidential) bugs, the onNewCommits call seems to run first, so we do get the nicer comment link.

Given this, we could get away with just changing addWebLink as I suggested. However, this would have the effect that also commit links would be added as both comments and links, which the enhancement request did not ask for. That's probably not desired.

> So I advice that we can add a warning comment in the api `removeComment` so that the developers use this api carefully.

If we need to add Issue::removeComment, then only implement it where it's going to be used, which is in JiraIssue. For GithubPullRequest and GitlabMergeRequest, better leave it not implemented than implemented but not tested. (Here the failed abstraction of Issue and lack of a sub interface for bug system issues strikes again.) If you go down this route, I won't be able to integrate this until I have time to test it properly. Given how little time I have to spend on Skara these days, that may be a while.

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

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