diff --git a/bots/notify/src/main/java/org/openjdk/skara/bots/notify/issue/IssueNotifier.java b/bots/notify/src/main/java/org/openjdk/skara/bots/notify/issue/IssueNotifier.java index 321e03807..80ad27e3a 100644 --- a/bots/notify/src/main/java/org/openjdk/skara/bots/notify/issue/IssueNotifier.java +++ b/bots/notify/src/main/java/org/openjdk/skara/bots/notify/issue/IssueNotifier.java @@ -257,9 +257,11 @@ public void onNewCommits(HostedRepository repository, Repository localRepository var existingComments = issue.comments(); var hashUrl = repository.webUrl(commit.hash()).toString(); + // We used to store URLs with just the abbreviated hash, so need to check for both + var shortHashUrl = repository.webUrl(new Hash(commit.hash().abbreviate())).toString(); var alreadyPostedComment = existingComments.stream() - .filter(comment -> comment.author().equals(issueProject.issueTracker().currentUser())) - .anyMatch(comment -> comment.body().contains(hashUrl)); + .filter(comment -> comment.author().equals(issueProject.issueTracker().currentUser())) + .anyMatch(comment -> comment.body().contains(hashUrl) || comment.body().contains(shortHashUrl)); if (!alreadyPostedComment) { issue.addComment(commitNotification); } diff --git a/bots/notify/src/test/java/org/openjdk/skara/bots/notify/issue/IssueNotifierTests.java b/bots/notify/src/test/java/org/openjdk/skara/bots/notify/issue/IssueNotifierTests.java index d978ac4f6..e3bd6ee35 100644 --- a/bots/notify/src/test/java/org/openjdk/skara/bots/notify/issue/IssueNotifierTests.java +++ b/bots/notify/src/test/java/org/openjdk/skara/bots/notify/issue/IssueNotifierTests.java @@ -23,6 +23,7 @@ package org.openjdk.skara.bots.notify.issue; import org.junit.jupiter.api.*; +import org.openjdk.skara.bots.notify.CommitFormatters; import org.openjdk.skara.bots.notify.NotifyBot; import org.openjdk.skara.forge.HostedRepository; import org.openjdk.skara.issuetracker.*; @@ -415,15 +416,15 @@ void testMultipleIssues(TestInfo testInfo) throws IOException { var comments1 = updatedIssue1.comments(); assertEquals(1, comments1.size()); var comment1 = comments1.get(0); - assertTrue(comment1.body().contains(editHash.abbreviate())); + assertTrue(comment1.body().contains(editHash.toString())); var comments2 = updatedIssue2.comments(); assertEquals(1, comments2.size()); var comment2 = comments2.get(0); - assertTrue(comment2.body().contains(editHash.abbreviate())); + assertTrue(comment2.body().contains(editHash.toString())); var comments3 = updatedIssue3.comments(); assertEquals(1, comments3.size()); var comment3 = comments3.get(0); - assertTrue(comment3.body().contains(editHash.abbreviate())); + assertTrue(comment3.body().contains(editHash.toString())); // As well as a fixVersion and a resolved in build assertEquals(Set.of("0.1"), fixVersions(updatedIssue1)); @@ -472,7 +473,7 @@ void testIssue(TestInfo testInfo) throws IOException { var comments = updatedIssue.comments(); assertEquals(1, comments.size()); var comment = comments.get(0); - assertTrue(comment.body().contains(editHash.abbreviate())); + assertTrue(comment.body().contains(editHash.toString())); // As well as a fixVersion and a resolved in build assertEquals(Set.of("0.1"), fixVersions(updatedIssue)); @@ -530,7 +531,7 @@ void testIssueBuildAfterMerge(TestInfo testInfo) throws IOException { var comments = updatedIssue.comments(); assertEquals(1, comments.size()); var comment = comments.get(0); - assertTrue(comment.body().contains(editHash.abbreviate())); + assertTrue(comment.body().contains(editHash.toString())); // As well as a fixVersion and a resolved in build assertEquals(Set.of("0.1"), fixVersions(updatedIssue)); @@ -618,7 +619,7 @@ void testIssueBuildAfterTag(TestInfo testInfo) throws IOException { var comments = updatedIssue.comments(); assertEquals(1, comments.size()); var comment = comments.get(0); - assertTrue(comment.body().contains(editHash.abbreviate())); + assertTrue(comment.body().contains(editHash.toString())); // As well as a fixVersion and a resolved in build assertEquals(Set.of("0.1"), fixVersions(updatedIssue)); @@ -699,12 +700,12 @@ void testIssueBuildAfterTagMultipleBranches(TestInfo testInfo) throws IOExceptio var comments = updatedIssue.comments(); assertEquals(1, comments.size()); var comment = comments.get(0); - assertTrue(comment.body().contains(editHash.abbreviate())); + assertTrue(comment.body().contains(editHash.toString())); var backportComments = backportIssue.comments(); assertEquals(1, backportComments.size()); var backportComment = backportComments.get(0); - assertTrue(backportComment.body().contains(editHash.abbreviate())); + assertTrue(backportComment.body().contains(editHash.toString())); // As well as a fixVersion and a resolved in build assertEquals(Set.of("16.0.2"), fixVersions(updatedIssue)); @@ -789,7 +790,7 @@ void testIssueRetryTag(TestInfo testInfo) throws IOException { var comments = updatedIssue.comments(); assertEquals(1, comments.size()); var comment = comments.get(0); - assertTrue(comment.body().contains(editHash.abbreviate())); + assertTrue(comment.body().contains(editHash.toString())); // As well as a fixVersion and a resolved in build assertEquals(Set.of("0.1"), fixVersions(updatedIssue)); @@ -886,7 +887,7 @@ void testIssueOtherDomain(TestInfo testInfo) throws IOException { var comments = updatedIssue.comments(); assertEquals(1, comments.size()); var comment = comments.get(0); - assertTrue(comment.body().contains(editHash.abbreviate())); + assertTrue(comment.body().contains(editHash.toString())); // As well as a fixVersion assertEquals(Set.of("0.1"), fixVersions(updatedIssue)); @@ -925,7 +926,7 @@ void testIssueNoVersion(TestInfo testInfo) throws IOException { var comments = issue.comments(); assertEquals(1, comments.size()); var comment = comments.get(0); - assertTrue(comment.body().contains(editHash.abbreviate())); + assertTrue(comment.body().contains(editHash.toString())); // But not in the fixVersion assertEquals(Set.of(), fixVersions(issue)); @@ -960,7 +961,7 @@ void testIssueConfiguredVersionNoCommit(TestInfo testInfo) throws IOException { var comments = issue.comments(); assertEquals(1, comments.size()); var comment = comments.get(0); - assertTrue(comment.body().contains(editHash.abbreviate())); + assertTrue(comment.body().contains(editHash.toString())); // As well as a fixVersion - but not the one from the repo assertEquals(Set.of("2.0"), fixVersions(issue)); @@ -1002,6 +1003,66 @@ void testIssueIdempotence(TestInfo testInfo) throws IOException { var comments = issue.comments(); assertEquals(1, comments.size()); var comment = comments.get(0); + assertTrue(comment.body().contains(editHash.toString())); + + // As well as a fixVersion + assertEquals(Set.of("0.1"), fixVersions(issue)); + + // Wipe the history + localRepo.push(historyState, repo.url(), "history", true); + + // Run it again + TestBotRunner.runPeriodicItems(notifyBot); + + // There should be no new comments or fixVersions + var updatedIssue = issueProject.issue(issue.id()).orElseThrow(); + assertEquals(1, updatedIssue.comments().size()); + assertEquals(Set.of("0.1"), fixVersions(updatedIssue)); + } + } + + /** + * The format used for commit URLs in bug comments and elsewhere was changed + * to use the full hash instead of an abbreviated hash. This test verifies + * that the idempotence of the IssueNotifier holds true even when + * encountering old bug comments containing the old commit URL format. + */ + @Test + void testIssueIdempotenceOldUrlFormat(TestInfo testInfo) throws IOException { + try (var credentials = new HostCredentials(testInfo); + var tempFolder = new TemporaryDirectory()) { + var repo = credentials.getHostedRepository(); + var repoFolder = tempFolder.path().resolve("repo"); + var localRepo = CheckableRepository.init(repoFolder, repo.repositoryType()); + credentials.commitLock(localRepo); + localRepo.pushAll(repo.url()); + + var storageFolder = tempFolder.path().resolve("storage"); + var issueProject = credentials.getIssueProject(); + var jbsNotifierConfig = JSON.object().put("fixversions", JSON.object()); + var notifyBot = testBotBuilder(repo, issueProject, storageFolder, jbsNotifierConfig).create("notify", JSON.object()); + + // Initialize history + TestBotRunner.runPeriodicItems(notifyBot); + + // Save the state + var historyState = localRepo.fetch(repo.url(), "history"); + + // Create an issue and commit a fix + var issue = issueProject.createIssue("This is an issue", List.of("Indeed"), Map.of("issuetype", JSON.of("Enhancement"))); + var editHash = CheckableRepository.appendAndCommit(localRepo, "Another line", issue.id() + ": Fix that issue"); + localRepo.push(editHash, repo.url(), "master"); + + // Add a comment for the fix with the old url hash format + var lastCommit = localRepo.commits().stream().findFirst().orElseThrow(); + issue.addComment(CommitFormatters.toTextBrief(repo, lastCommit).replace(editHash.toString(), editHash.abbreviate())); + TestBotRunner.runPeriodicItems(notifyBot); + + // Verify that the planted comment is still the only one + var comments = issue.comments(); + assertEquals(1, comments.size()); + var comment = comments.get(0); + // We expect the abbreviated hash in the planted comment assertTrue(comment.body().contains(editHash.abbreviate())); // As well as a fixVersion diff --git a/forge/src/main/java/org/openjdk/skara/forge/github/GitHubRepository.java b/forge/src/main/java/org/openjdk/skara/forge/github/GitHubRepository.java index d55d56342..4a6c0c21f 100644 --- a/forge/src/main/java/org/openjdk/skara/forge/github/GitHubRepository.java +++ b/forge/src/main/java/org/openjdk/skara/forge/github/GitHubRepository.java @@ -208,7 +208,7 @@ public URI nonTransformedWebUrl() { @Override public URI webUrl(Hash hash) { - var endpoint = "/" + repository + "/commit/" + hash.abbreviate(); + var endpoint = "/" + repository + "/commit/" + hash; return gitHubHost.getWebURI(endpoint); } diff --git a/forge/src/main/java/org/openjdk/skara/forge/gitlab/GitLabRepository.java b/forge/src/main/java/org/openjdk/skara/forge/gitlab/GitLabRepository.java index 1d53c4a47..e6ce8af54 100644 --- a/forge/src/main/java/org/openjdk/skara/forge/gitlab/GitLabRepository.java +++ b/forge/src/main/java/org/openjdk/skara/forge/gitlab/GitLabRepository.java @@ -199,7 +199,7 @@ public URI nonTransformedWebUrl() { @Override public URI webUrl(Hash hash) { return URIBuilder.base(gitLabHost.getUri()) - .setPath("/" + projectName + "/commit/" + hash.abbreviate()) + .setPath("/" + projectName + "/commit/" + hash) .build(); }