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

[Guoxiong Li]

On Fri, 3 Dec 2021 14:18:34 GMT, Erik Joelsson <erikj 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.

I agree to adjust the method `addWebLinkAsComment` to complete this enhancement.

> I don't think we should remove comments when removing links. Issue comments are meant to represent a log of things happening to an Issue. The links section is meant to represent the current valid/relevant links. Removing the link when it's not relevant is good, but removing comments is like editing history and should only be done rare cases. In this situation, I would leave it to the user to clean up if they think it's needed.

Curently, the method `removeWebLink` will remove the corresponding comment. Please see the code below. The line `request.delete("/comment/" + comment.id()).execute()` will remove the related web link comment.

Because such comment is related to the weblink, I think this step is reasonable. But removing the arbitrary comment is not good. I agree that we shouldn't provide the api about removing comment.


    private void removeWebLink(Link link) {
        request.delete("/remotelink")
               .param("globalId", "skaralink=" + link.uri().orElseThrow().toString())
               .onError(e -> e.statusCode() == 404 ? Optional.of(JSON.object().put("already_deleted", true)) : Optional.empty())
               .execute();

        for (var comment : comments()) {
            var commentLink = parseWebLinkComment(comment);
            if (commentLink.isEmpty()) {
                continue;
            }
            if (!commentLink.get().uri().equals(link.uri())) {
                continue;
            }
            request.delete("/comment/" + comment.id()).execute();  // <---- here, remove the comment.
        }
    }



> Also, if you add new API calls to various REST providers, those need thorough testing which can be hard to perform. We unfortunately do not have any automated way of testing the interaction between Skara and Github/Gitlab/Jira.

I noticed it. Hope the automated tests can be built in the future.

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

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