From 960fddde74105069b1560275dadbfc1fe67851a4 Mon Sep 17 00:00:00 2001 From: Greybird Date: Sun, 8 Jan 2023 09:02:30 +0100 Subject: [PATCH 1/4] Use standard check for isAnonymous --- .../java/hudson/plugins/claim/AbstractClaimBuildAction.java | 3 ++- .../java/hudson/plugins/claim/CommonMessagesProvider.java | 4 +--- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/src/main/java/hudson/plugins/claim/AbstractClaimBuildAction.java b/src/main/java/hudson/plugins/claim/AbstractClaimBuildAction.java index cdbd87e2..03c90987 100755 --- a/src/main/java/hudson/plugins/claim/AbstractClaimBuildAction.java +++ b/src/main/java/hudson/plugins/claim/AbstractClaimBuildAction.java @@ -2,6 +2,7 @@ import groovy.lang.Binding; import hudson.model.*; +import hudson.security.ACL; import jenkins.model.Jenkins; import org.apache.commons.lang.StringUtils; import org.jenkinsci.plugins.scriptsecurity.sandbox.groovy.SecureGroovyScript; @@ -337,7 +338,7 @@ public final boolean canRelease() { } public final boolean isUserAnonymous() { - return Jenkins.getAuthentication2().getName().equals("anonymous"); + return ACL.isAnonymous2(Jenkins.getAuthentication2()); } @Exported diff --git a/src/main/java/hudson/plugins/claim/CommonMessagesProvider.java b/src/main/java/hudson/plugins/claim/CommonMessagesProvider.java index c7da765e..e603a245 100644 --- a/src/main/java/hudson/plugins/claim/CommonMessagesProvider.java +++ b/src/main/java/hudson/plugins/claim/CommonMessagesProvider.java @@ -12,8 +12,6 @@ import java.util.Date; import java.util.function.Supplier; -import static hudson.security.ACL.ANONYMOUS_USERNAME; - public final class CommonMessagesProvider { private static final int DATA_PRESENT = 1; @@ -68,7 +66,7 @@ private Formatter getFormat(MessagesProvider messagesProvider) { Authentication auth = Jenkins.getAuthentication2(); String currentUser = auth.getName(); boolean isAutoAssigned = claimedBy.equals(assignedBy); - if (!currentUser.equals(ANONYMOUS_USERNAME)) { + if (!ACL.isAnonymous2(auth)) { if (currentUser.equals(assignedBy)) { if (isAutoAssigned) { return messagesProvider.claimedBySelf(); From 8c187d6beca0ca7570f210a00685e1dc67854af8 Mon Sep 17 00:00:00 2001 From: Greybird Date: Sun, 8 Jan 2023 19:23:09 +0100 Subject: [PATCH 2/4] Switch to User class for claim actions --- .../claim/AbstractClaimBuildAction.java | 73 ++++++++++++++---- .../plugins/claim/ClaimBuildAction.java | 3 +- .../hudson/plugins/claim/ClaimEmailer.java | 29 +++---- .../hudson/plugins/claim/ClaimPublisher.java | 9 ++- .../hudson/plugins/claim/ClaimTestAction.java | 5 +- .../plugins/claim/ClaimTestDataPublisher.java | 19 +++-- .../plugins/claim/ClaimEmailerTest.java | 77 ++++++++++++++----- .../hudson/plugins/claim/ClaimReportTest.java | 2 +- .../java/hudson/plugins/claim/ClaimTest.java | 47 ++++++++++- .../plugins/claim/UserClaimsReportTest.java | 9 ++- 10 files changed, 203 insertions(+), 70 deletions(-) diff --git a/src/main/java/hudson/plugins/claim/AbstractClaimBuildAction.java b/src/main/java/hudson/plugins/claim/AbstractClaimBuildAction.java index 03c90987..b2be9625 100755 --- a/src/main/java/hudson/plugins/claim/AbstractClaimBuildAction.java +++ b/src/main/java/hudson/plugins/claim/AbstractClaimBuildAction.java @@ -17,7 +17,9 @@ import jakarta.mail.MessagingException; import org.springframework.security.core.Authentication; +import org.springframework.security.core.userdetails.UsernameNotFoundException; +import javax.annotation.Nonnull; import javax.servlet.ServletException; import java.io.IOException; import java.util.Collections; @@ -89,7 +91,7 @@ public final void doClaim(StaplerRequest req, StaplerResponse resp) String assignee = req.getSubmittedForm().getString("assignee"); if (!StringUtils.isEmpty(assignee) && !claimedUser.equals(assignee)) { // Validate the specified assignee. - User resolvedAssignee = User.get(assignee, false, Collections.emptyMap()); + User resolvedAssignee = getUserFromId(assignee, false); if (resolvedAssignee == null) { LOGGER.log(Level.WARNING, "Invalid username specified for assignment: {0}", assignee); resp.forwardToPreviousPage(req); @@ -132,7 +134,7 @@ public final void doClaim(StaplerRequest req, StaplerResponse resp) private static User getCurrentUser() { Authentication authentication = Jenkins.getAuthentication2(); - return User.getById(authentication.getName(), false); + return User.get2(authentication); } /** @@ -158,9 +160,28 @@ public final void claim(String claimedByUser, String providedReason, String assi * @param isSticky true if the claim has to be kept until resolution * @param isPropagated true if the claim has to be propagated to following builds * @param notify true if notifications have to be sent + * @deprecated use {@link #claim(User, String, User, Date, boolean, boolean, boolean)} */ + @Deprecated public final void claim(String claimedByUser, String providedReason, String assignedByUser, Date date, boolean isSticky, boolean isPropagated, boolean notify) { + User claimedBy = getUserFromId(claimedByUser); + User assignedBy = getUserFromId(assignedByUser); + claim(claimedBy, providedReason, assignedBy, date, isSticky, isPropagated, notify); + } + + /** + * Claims a {@link Saveable}, and optionally notifies of the claim. + * @param claimedByUser claiming user + * @param providedReason reason for the claim + * @param assignedByUser assigner user + * @param date date of the claim + * @param isSticky true if the claim has to be kept until resolution + * @param isPropagated true if the claim has to be propagated to following builds + * @param notify true if notifications have to be sent + */ + public final void claim(User claimedByUser, String providedReason, User assignedByUser, Date date, + boolean isSticky, boolean isPropagated, boolean notify) { applyClaim(claimedByUser, providedReason, assignedByUser, date, isSticky, isPropagated); if (notify) { try { @@ -175,33 +196,33 @@ public final void claim(String claimedByUser, String providedReason, String assi /** * Sends an initial claim email. - * @param claimedByUser name of the claiming user + * @param claimedByUser the claiming user * @param providedReason reason for the claim - * @param assignedByUser name of the assigner user + * @param assignedByUser the assigner user * @throws MessagingException if there has been some problem with sending the email * @throws IOException if there is an IO problem when sending the mail * @throws InterruptedException if the send operation is interrupted */ - protected abstract void sendInitialClaimEmail(String claimedByUser, String providedReason, String assignedByUser) + protected abstract void sendInitialClaimEmail(User claimedByUser, String providedReason, User assignedByUser) throws MessagingException, IOException, InterruptedException; /** * Applies the claim data to the {@link AbstractClaimBuildAction}. - * @param claimedByUser name of the claiming user + * @param claimedByUser the claiming user * @param providedReason reason for the claim - * @param assignedByUser name of the assigner user + * @param assignedByUser the assigner user * @param date date of the claim * @param isSticky true if the claim has to be kept until resolution * @param isPropagated true if the claim has to be propagated to following builds */ - protected void applyClaim(String claimedByUser, String providedReason, String assignedByUser, Date date, + protected void applyClaim(@Nonnull User claimedByUser, String providedReason, @Nonnull User assignedByUser, Date date, boolean isSticky, boolean isPropagated) { this.claimed = true; - this.claimedBy = claimedByUser; + this.claimedBy = claimedByUser.getId(); this.reason = providedReason; this.transientClaim = !isSticky; this.claimDate = date; - this.assignedBy = assignedByUser; + this.assignedBy = assignedByUser.getId(); if (isPropagated) { getNextAction().ifPresent(action -> { if (!action.isClaimed()) { @@ -273,7 +294,7 @@ public final String getAssignedBy() { // used by groovy scripts ? public final String getClaimedByName() { - User user = User.get(claimedBy, false, Collections.emptyMap()); + User user = getUserFromId(claimedBy, false); if (user != null) { return user.getDisplayName(); } else { @@ -283,7 +304,7 @@ public final String getClaimedByName() { // used by groovy scripts ? public final String getAssignedByName() { - User user = User.get(assignedBy, false, Collections.emptyMap()); + User user = getUserFromId(assignedBy, false); if (user != null) { return user.getDisplayName(); } else { @@ -309,9 +330,21 @@ public final boolean isClaimed() { /** * Claim a new {@link Saveable} with the same settings as this one. * @param other the source data + * @return true if claim has been copied, false otherwise */ - protected void copyTo(AbstractClaimBuildAction other) { - other.applyClaim(getClaimedBy(), getReason(), getAssignedBy(), getClaimDate(), isSticky(), false); + protected boolean copyTo(AbstractClaimBuildAction other) { + User claimedBy = getUserFromId(getClaimedBy(), false); + if (claimedBy == null) + { + return false; + } + User assignedBy = getUserFromId(getAssignedBy(), false); + if (assignedBy == null) + { + assignedBy = User.getUnknown(); + } + other.applyClaim(claimedBy, getReason(), assignedBy, getClaimDate(), isSticky(), false); + return true; } public final boolean isClaimedByMe() { @@ -454,4 +487,16 @@ protected final void evalGroovyScript() { } } } + + protected final User getUserFromId(String userId) { + return getUserFromId(userId, true); + } + + protected final User getUserFromId(String userId, boolean throwIfNotFound) { + User resolved = User.get(userId, false, Collections.emptyMap()); + if (resolved == null && throwIfNotFound) { + throw new UsernameNotFoundException("Unknown user: " + userId); + } + return resolved; + } } diff --git a/src/main/java/hudson/plugins/claim/ClaimBuildAction.java b/src/main/java/hudson/plugins/claim/ClaimBuildAction.java index 034acd68..c266a3e4 100644 --- a/src/main/java/hudson/plugins/claim/ClaimBuildAction.java +++ b/src/main/java/hudson/plugins/claim/ClaimBuildAction.java @@ -1,6 +1,7 @@ package hudson.plugins.claim; import hudson.model.Run; +import hudson.model.User; import jenkins.model.RunAction2; import jakarta.mail.MessagingException; @@ -53,7 +54,7 @@ protected Optional getNextAction() { } @Override - protected void sendInitialClaimEmail(String claimedByUser, String providedReason, String assignedByUser) + protected void sendInitialClaimEmail(User claimedByUser, String providedReason, User assignedByUser) throws MessagingException, IOException { ClaimEmailer.sendInitialBuildClaimEmailIfConfigured( claimedByUser, diff --git a/src/main/java/hudson/plugins/claim/ClaimEmailer.java b/src/main/java/hudson/plugins/claim/ClaimEmailer.java index d0201b78..a776458b 100644 --- a/src/main/java/hudson/plugins/claim/ClaimEmailer.java +++ b/src/main/java/hudson/plugins/claim/ClaimEmailer.java @@ -1,5 +1,6 @@ package hudson.plugins.claim; +import hudson.model.User; import hudson.plugins.claim.messages.InitialBuildClaimMessage; import hudson.plugins.claim.messages.InitialTestClaimMessage; import hudson.plugins.claim.messages.RepeatedBuildClaimMessage; @@ -8,6 +9,8 @@ import hudson.tasks.junit.CaseResult; import jakarta.mail.MessagingException; + +import javax.annotation.Nonnull; import java.io.IOException; import java.util.List; import java.util.logging.Logger; @@ -40,22 +43,22 @@ private ClaimEmailer() { /** * Sends an email to the assignee indicating that the given build has been assigned. - * @param claimedByUser name of the claiming user - * @param assignedByUser name of the assigner user + * @param claimedByUser the claiming user + * @param assignedByUser the assigner user * @param action the build/action which has been assigned * @param reason the reason given for the assignment * @param url the URL the user can view for the assigned build * @throws MessagingException if there has been some problem with sending the email * @throws IOException if there is an IO problem when sending the mail */ - public static void sendInitialBuildClaimEmailIfConfigured(String claimedByUser, String assignedByUser, + public static void sendInitialBuildClaimEmailIfConfigured(@Nonnull User claimedByUser, @Nonnull User assignedByUser, String action, String reason, String url) throws MessagingException, IOException { ClaimConfig config = ClaimConfig.get(); if (config.getSendEmails() && MAILER_LOADED) { InitialBuildClaimMessage message = new InitialBuildClaimMessage( - action, url, reason, claimedByUser, assignedByUser + action, url, reason, claimedByUser.getDisplayName(), assignedByUser.getDisplayName() ); message.send(); } @@ -63,22 +66,22 @@ public static void sendInitialBuildClaimEmailIfConfigured(String claimedByUser, /** * Sends an email to the assignee indicating that the given test has been assigned. - * @param claimedByUser name of the claiming user - * @param assignedByUser name of the assigner user + * @param claimedByUser the claiming user + * @param assignedByUser the assigner user * @param action the build/action which has been assigned * @param reason the reason given for the assignment * @param url the URL the user can view for the assigned build * @throws MessagingException if there has been some problem with sending the email * @throws IOException if there is an IO problem when sending the mail */ - public static void sendInitialTestClaimEmailIfConfigured(String claimedByUser, String assignedByUser, + public static void sendInitialTestClaimEmailIfConfigured(@Nonnull User claimedByUser, @Nonnull User assignedByUser, String action, String reason, String url) throws MessagingException, IOException { ClaimConfig config = ClaimConfig.get(); if (config.getSendEmails() && MAILER_LOADED) { InitialTestClaimMessage message = new InitialTestClaimMessage( - action, url, reason, claimedByUser, assignedByUser + action, url, reason, claimedByUser.getDisplayName(), assignedByUser.getDisplayName() ); message.send(); } @@ -86,18 +89,18 @@ public static void sendInitialTestClaimEmailIfConfigured(String claimedByUser, S /** * Sends an email to the assignee indicating that the given build is still failing. - * @param claimedByUser name of the claiming user + * @param claimedByUser the claiming user * @param action the build/action which has been assigned * @param url the URL the user can view for the assigned build * @throws MessagingException if there has been some problem with sending the email * @throws IOException if there is an IO problem when sending the mail */ - public static void sendRepeatedBuildClaimEmailIfConfigured(String claimedByUser, String action, String url) + public static void sendRepeatedBuildClaimEmailIfConfigured(@Nonnull User claimedByUser, String action, String url) throws MessagingException, IOException { ClaimConfig config = ClaimConfig.get(); if (config.getSendEmailsForStickyFailures() && MAILER_LOADED) { - RepeatedBuildClaimMessage message = new RepeatedBuildClaimMessage(action, url, claimedByUser); + RepeatedBuildClaimMessage message = new RepeatedBuildClaimMessage(action, url, claimedByUser.getDisplayName()); message.send(); } } @@ -111,13 +114,13 @@ public static void sendRepeatedBuildClaimEmailIfConfigured(String claimedByUser, * @throws MessagingException if there has been some problem with sending the email * @throws IOException if there is an IO problem when sending the mail */ - public static void sendRepeatedTestClaimEmailIfConfigured(String claimedByUser, String action, String url, + public static void sendRepeatedTestClaimEmailIfConfigured(@Nonnull User claimedByUser, String action, String url, List failedTests) throws MessagingException, IOException { ClaimConfig config = ClaimConfig.get(); if (config.getSendEmailsForStickyFailures() && MAILER_LOADED) { - RepeatedTestClaimMessage message = new RepeatedTestClaimMessage(action, url, claimedByUser, failedTests); + RepeatedTestClaimMessage message = new RepeatedTestClaimMessage(action, url, claimedByUser.getDisplayName(), failedTests); message.send(); } } diff --git a/src/main/java/hudson/plugins/claim/ClaimPublisher.java b/src/main/java/hudson/plugins/claim/ClaimPublisher.java index 6e29e2c0..ee7d02f8 100644 --- a/src/main/java/hudson/plugins/claim/ClaimPublisher.java +++ b/src/main/java/hudson/plugins/claim/ClaimPublisher.java @@ -6,6 +6,7 @@ import hudson.model.Result; import hudson.model.Run; import hudson.model.TaskListener; +import hudson.model.User; import hudson.tasks.BuildStepDescriptor; import hudson.tasks.BuildStepMonitor; import hudson.tasks.Notifier; @@ -73,14 +74,16 @@ static void addClaimBuildAction(Run build) throws IOException { if (previousBuild != null) { ClaimBuildAction c = previousBuild.getAction(ClaimBuildAction.class); if (c != null && c.isClaimed() && c.isSticky()) { - c.copyTo(action); - sendEmailsForStickyFailure(build, c.getClaimedBy()); + if (c.copyTo(action)) { + sendEmailsForStickyFailure(build, c.getUserFromId(c.getClaimedBy())); + } } } } - private static void sendEmailsForStickyFailure(Run build, String claimedByUser) { + private static void sendEmailsForStickyFailure(Run build, User claimedByUser) { try { + ClaimEmailer.sendRepeatedBuildClaimEmailIfConfigured(claimedByUser, build.toString(), build.getUrl()); } catch (MessagingException | IOException e) { LOGGER.log(Level.WARNING, "Exception when sending build failure reminder email. Ignoring.", e); diff --git a/src/main/java/hudson/plugins/claim/ClaimTestAction.java b/src/main/java/hudson/plugins/claim/ClaimTestAction.java index 41c308f8..854bb759 100644 --- a/src/main/java/hudson/plugins/claim/ClaimTestAction.java +++ b/src/main/java/hudson/plugins/claim/ClaimTestAction.java @@ -1,6 +1,7 @@ package hudson.plugins.claim; import hudson.model.Run; +import hudson.model.User; import hudson.plugins.claim.ClaimTestDataPublisher.Data; import hudson.tasks.junit.TestResultAction; import hudson.tasks.test.TestResult; @@ -25,7 +26,7 @@ public String getDisplayName() { } @Override - protected void applyClaim(String claimedByUser, String providedReason, String assignedByUser, Date date, + protected void applyClaim(User claimedByUser, String providedReason, User assignedByUser, Date date, boolean isSticky, boolean isPropagated) { data.addClaim(testObjectId, this); super.applyClaim(claimedByUser, providedReason, assignedByUser, date, isSticky, isPropagated); @@ -63,7 +64,7 @@ protected Run getOwner() { } @Override - protected void sendInitialClaimEmail(String claimedByUser, String providedReason, String assignedByUser) + protected void sendInitialClaimEmail(User claimedByUser, String providedReason, User assignedByUser) throws MessagingException, IOException { ClaimEmailer.sendInitialTestClaimEmailIfConfigured( claimedByUser, diff --git a/src/main/java/hudson/plugins/claim/ClaimTestDataPublisher.java b/src/main/java/hudson/plugins/claim/ClaimTestDataPublisher.java index 4aed2837..21ae6f60 100644 --- a/src/main/java/hudson/plugins/claim/ClaimTestDataPublisher.java +++ b/src/main/java/hudson/plugins/claim/ClaimTestDataPublisher.java @@ -5,10 +5,7 @@ import hudson.FilePath; import hudson.Functions; import hudson.Launcher; -import hudson.model.Descriptor; -import hudson.model.Run; -import hudson.model.Saveable; -import hudson.model.TaskListener; +import hudson.model.*; import hudson.tasks.junit.*; import org.kohsuke.stapler.DataBoundConstructor; @@ -34,16 +31,18 @@ public Data contributeTestData(Run run, @NonNull FilePath workspace, Launc throws IOException, InterruptedException { Data data = new Data(run); - Map> claimedFailuresByUser = new HashMap<>(); + Map> claimedFailuresByUser = new HashMap<>(); for (CaseResult result: testResult.getFailedTests()) { CaseResult previous = result.getPreviousResult(); if (previous != null) { ClaimTestAction previousAction = previous.getTestAction(ClaimTestAction.class); if (previousAction != null && previousAction.isClaimed() && previousAction.isSticky()) { ClaimTestAction action = new ClaimTestAction(data, result.getId()); - previousAction.copyTo(action); - data.addClaim(result.getId(), action); - putAsListElement(claimedFailuresByUser, action.getClaimedBy(), result); + if (previousAction.copyTo(action)) { + data.addClaim(result.getId(), action); + User user = action.getUserFromId(action.getClaimedBy()); + putAsListElement(claimedFailuresByUser, user, result); + } } } } @@ -63,9 +62,9 @@ private void putAsListElement(Map> map, K key, V value) { } private void sendEmailsForStickyFailuresIfPresent(Run run, TestResult testResult, - Map> claimedFailuresByUser) { + Map> claimedFailuresByUser) { try { - for (Entry> entry : claimedFailuresByUser.entrySet()) { + for (Entry> entry : claimedFailuresByUser.entrySet()) { String url = Functions.joinPath(run.getUrl(), testResult.getParentAction().getUrlName()); ClaimEmailer.sendRepeatedTestClaimEmailIfConfigured( entry.getKey(), run.toString(), url, entry.getValue() diff --git a/src/test/java/hudson/plugins/claim/ClaimEmailerTest.java b/src/test/java/hudson/plugins/claim/ClaimEmailerTest.java index b830082e..6f273709 100644 --- a/src/test/java/hudson/plugins/claim/ClaimEmailerTest.java +++ b/src/test/java/hudson/plugins/claim/ClaimEmailerTest.java @@ -38,6 +38,7 @@ public class ClaimEmailerTest { public void testSendEmailNotConfigured() throws Exception { final String assigneeId = "assignee"; + final String assignedById = "assignedByMe"; JenkinsLocationConfiguration.get().setAdminAddress("test "); ClaimConfig config = ClaimConfig.get(); @@ -47,12 +48,16 @@ public void testSendEmailNotConfigured() throws Exception { Mailbox yourInbox = Mailbox.get(new InternetAddress(recipient)); yourInbox.clear(); - // ensure the user is existing + // ensure the users are existing User assignee = User.get(assigneeId, true, Collections.emptyMap()); + assignee.setFullName("Assignee User"); + User assignedBy = User.get(assignedById, true, Collections.emptyMap()); + assignedBy.setFullName("AssignedBy User"); UserProperty p = new Mailer.UserProperty("assignee@test.com"); assignee.addProperty(p); - ClaimEmailer.sendInitialBuildClaimEmailIfConfigured(assigneeId, "assignedByMe", + + ClaimEmailer.sendInitialBuildClaimEmailIfConfigured(assignee, assignedBy, "Test build", "test reason", "jobs/TestBuild/"); assertEquals(0, yourInbox.size()); @@ -62,6 +67,7 @@ public void testSendEmailNotConfigured() throws Exception { public void testSendEmailOnInitialBuildFailureConfigured() throws Exception { final String assigneeId = "assignee"; + final String assignedById = "assignedByMe"; JenkinsLocationConfiguration.get().setAdminAddress("test "); JenkinsLocationConfiguration.get().setUrl("http://localhost:8080/jenkins/"); @@ -74,11 +80,15 @@ public void testSendEmailOnInitialBuildFailureConfigured() throws Exception { yourInbox.clear(); // ensure the user is existing + // ensure the users are existing User assignee = User.get(assigneeId, true, Collections.emptyMap()); + assignee.setFullName("Assignee User"); + User assignedBy = User.get(assignedById, true, Collections.emptyMap()); + assignedBy.setFullName("AssignedBy User"); UserProperty p = new Mailer.UserProperty("assignee@test.com"); assignee.addProperty(p); - ClaimEmailer.sendInitialBuildClaimEmailIfConfigured(assigneeId, "assignedBy", "Test build", + ClaimEmailer.sendInitialBuildClaimEmailIfConfigured(assignee, assignedBy, "Test build", "test reason", "jobs/TestBuild/"); assertEquals(1, yourInbox.size()); @@ -97,13 +107,14 @@ public void testSendEmailOnInitialBuildFailureConfigured() throws Exception { assertTrue("Mail content should contain the details", content.contains(Messages .ClaimEmailer_Details("http://localhost:8080/jenkins/jobs/TestBuild/"))); assertTrue("Mail content should assignment text", content.contains(Messages - .ClaimEmailer_Build_Initial_Text("Test build", "assignedBy"))); + .ClaimEmailer_Build_Initial_Text("Test build", "AssignedBy User"))); } @Test public void testSendEmailOnInitialTestFailureConfigured() throws Exception { final String assigneeId = "assignee"; + final String assignedById = "assignedByMe"; JenkinsLocationConfiguration.get().setAdminAddress("test "); JenkinsLocationConfiguration.get().setUrl("http://localhost:8080/jenkins/"); @@ -115,12 +126,15 @@ public void testSendEmailOnInitialTestFailureConfigured() throws Exception { Mailbox yourInbox = Mailbox.get(new InternetAddress(recipient)); yourInbox.clear(); - // ensure the user is existing + // ensure the users are existing User assignee = User.get(assigneeId, true, Collections.emptyMap()); + assignee.setFullName("Assignee User"); + User assignedBy = User.get(assignedById, true, Collections.emptyMap()); + assignedBy.setFullName("AssignedBy User"); UserProperty p = new Mailer.UserProperty("assignee@test.com"); assignee.addProperty(p); - ClaimEmailer.sendInitialTestClaimEmailIfConfigured(assigneeId, "assignedBy", "Test Test", + ClaimEmailer.sendInitialTestClaimEmailIfConfigured(assignee, assignedBy, "Test Test", "test reason", "jobs/TestBuild/testReport/TestTest"); assertEquals(1, yourInbox.size()); @@ -139,7 +153,7 @@ public void testSendEmailOnInitialTestFailureConfigured() throws Exception { assertTrue("Mail content should contain the details", content.contains(Messages .ClaimEmailer_Details("http://localhost:8080/jenkins/jobs/TestBuild/testReport/TestTest"))); assertTrue("Mail content should assignment text", - content.contains(Messages.ClaimEmailer_Test_Initial_Text("Test Test", "assignedBy"))); + content.contains(Messages.ClaimEmailer_Test_Initial_Text("Test Test", "AssignedBy User"))); } /* @@ -160,12 +174,13 @@ public void shouldNotSendEmailWhenSelfClaiming() throws Exception { Mailbox yourInbox = Mailbox.get(new InternetAddress(recipient)); yourInbox.clear(); - // ensure the user is existing + // ensure the users are existing User assignee = User.get(assigneeId, true, Collections.emptyMap()); + assignee.setFullName("Assignee User"); UserProperty p = new Mailer.UserProperty("assignee@test.com"); assignee.addProperty(p); - ClaimEmailer.sendInitialBuildClaimEmailIfConfigured(assigneeId, assigneeId, "Test build", + ClaimEmailer.sendInitialBuildClaimEmailIfConfigured(assignee, assignee, "Test build", "test reason", "jobs/TestBuild/"); assertEquals(0, yourInbox.size()); @@ -178,6 +193,7 @@ public void shouldNotSendEmailWhenSelfClaiming() throws Exception { public void shouldNotFailWhenRecipientEmailAddressIsNull() throws Exception { final String assigneeId = "sarah connor"; + final String assignedById = "assignedByMe"; // given JenkinsLocationConfiguration.get().setAdminAddress("test "); @@ -186,11 +202,15 @@ public void shouldNotFailWhenRecipientEmailAddressIsNull() throws Exception { ClaimConfig config = ClaimConfig.get(); config.setSendEmails(true); - // ensure the user is existing, will generate invalid default mail address with a space - User.get(assigneeId, true, Collections.emptyMap()); + // ensure the users are existing + // will generate invalid default mail address with a space + User assignee = User.get(assigneeId, true, Collections.emptyMap()); + assignee.setFullName("Assignee User"); + User assignedBy = User.get(assignedById, true, Collections.emptyMap()); + assignedBy.setFullName("AssignedBy User"); // when - ClaimEmailer.sendInitialBuildClaimEmailIfConfigured(assigneeId, "assignedBy", "Test build", + ClaimEmailer.sendInitialBuildClaimEmailIfConfigured(assignee, assignedBy, "Test build", "test reason", "jobs/TestBuild/"); // then @@ -201,6 +221,7 @@ public void shouldNotFailWhenRecipientEmailAddressIsNull() throws Exception { public void testSendEmailOnRepeatedBuildFailureConfigured() throws Exception { final String assigneeId = "assignee"; + final String assignedById = "assignedByMe"; JenkinsLocationConfiguration.get().setAdminAddress("test "); JenkinsLocationConfiguration.get().setUrl("http://localhost:8080/jenkins/"); @@ -213,12 +234,15 @@ public void testSendEmailOnRepeatedBuildFailureConfigured() throws Exception { Mailbox yourInbox = Mailbox.get(new InternetAddress(recipient)); yourInbox.clear(); - // ensure the user is existing + // ensure the users are existing User assignee = User.get(assigneeId, true, Collections.emptyMap()); + assignee.setFullName("Assignee User"); + User assignedBy = User.get(assignedById, true, Collections.emptyMap()); + assignedBy.setFullName("AssignedBy User"); UserProperty p = new Mailer.UserProperty("assignee@test.com"); assignee.addProperty(p); - ClaimEmailer.sendRepeatedBuildClaimEmailIfConfigured(assigneeId, "Test build", "jobs/TestBuild/"); + ClaimEmailer.sendRepeatedBuildClaimEmailIfConfigured(assignee, "Test build", "jobs/TestBuild/"); assertEquals(1, yourInbox.size()); Message mailMessage = yourInbox.get(0); @@ -243,6 +267,7 @@ public void testSendEmailOnRepeatedBuildFailureConfigured() throws Exception { public void shouldNotFailOnRepeatedTestFailureWhenNoTestsAreFailing() throws Exception { final String assigneeId = "assignee"; + final String assignedById = "assignedByMe"; JenkinsLocationConfiguration.get().setAdminAddress("test "); JenkinsLocationConfiguration.get().setUrl("http://localhost:8080/jenkins/"); @@ -255,14 +280,17 @@ public void shouldNotFailOnRepeatedTestFailureWhenNoTestsAreFailing() throws Exc Mailbox yourInbox = Mailbox.get(new InternetAddress(recipient)); yourInbox.clear(); - // ensure the user is existing + // ensure the users are existing User assignee = User.get(assigneeId, true, Collections.emptyMap()); + assignee.setFullName("Assignee User"); + User assignedBy = User.get(assignedById, true, Collections.emptyMap()); + assignedBy.setFullName("AssignedBy User"); UserProperty p = new Mailer.UserProperty("assignee@test.com"); assignee.addProperty(p); List tests = new ArrayList(); - ClaimEmailer.sendRepeatedTestClaimEmailIfConfigured(assigneeId, "Test Test", + ClaimEmailer.sendRepeatedTestClaimEmailIfConfigured(assignee, "Test Test", "jobs/TestBuild/testReport/TestTest", tests); assertEquals(0, yourInbox.size()); @@ -272,6 +300,7 @@ public void shouldNotFailOnRepeatedTestFailureWhenNoTestsAreFailing() throws Exc public void testSendEmailOnRepeatedTestFailureConfigured() throws Exception { final String assigneeId = "assignee"; + final String assignedById = "assignedByMe"; JenkinsLocationConfiguration.get().setAdminAddress("test "); JenkinsLocationConfiguration.get().setUrl("http://localhost:8080/jenkins/"); @@ -284,8 +313,11 @@ public void testSendEmailOnRepeatedTestFailureConfigured() throws Exception { Mailbox yourInbox = Mailbox.get(new InternetAddress(recipient)); yourInbox.clear(); - // ensure the user is existing + // ensure the users are existing User assignee = User.get(assigneeId, true, Collections.emptyMap()); + assignee.setFullName("Assignee User"); + User assignedBy = User.get(assignedById, true, Collections.emptyMap()); + assignedBy.setFullName("AssignedBy User"); UserProperty p = new Mailer.UserProperty("assignee@test.com"); assignee.addProperty(p); @@ -297,7 +329,7 @@ public void testSendEmailOnRepeatedTestFailureConfigured() throws Exception { List tests = new ArrayList<>(); tests.add(caseResult1); tests.add(caseResult2); - ClaimEmailer.sendRepeatedTestClaimEmailIfConfigured(assigneeId, "Test Build", + ClaimEmailer.sendRepeatedTestClaimEmailIfConfigured(assignee, "Test Build", "jobs/TestBuild/testReport/TestTest", tests); assertEquals(1, yourInbox.size()); @@ -327,6 +359,7 @@ public void emailShouldBeSentForStickyBuildClaimWhenReminderConfigured() throws FreeStyleProject job = createFailingJobWithName("test-" + System.currentTimeMillis()); final String assigneeId = "assignee"; + final String assignedById = "assignedByMe"; JenkinsLocationConfiguration.get().setAdminAddress("test "); JenkinsLocationConfiguration.get().setUrl("localhost:8080/jenkins/"); @@ -339,12 +372,16 @@ public void emailShouldBeSentForStickyBuildClaimWhenReminderConfigured() throws Mailbox recipientInbox = Mailbox.get(new InternetAddress(recipient)); recipientInbox.clear(); - // ensure the user is existing + // ensure the users are existing User assignee = User.get(assigneeId, true, Collections.emptyMap()); + assignee.setFullName("Assignee User"); + User assignedBy = User.get(assignedById, true, Collections.emptyMap()); + assignedBy.setFullName("AssignedBy User"); + assignee.addProperty(new Mailer.UserProperty("assignee@test.com")); ClaimBuildAction claimAction = job.getLastBuild().getAction(ClaimBuildAction.class); - claimAction.claim(assignee.getId(), "some reason", "assignedByUser", new Date(), + claimAction.claim(assignee, "some reason", assignedBy, new Date(), true, true, false); job.scheduleBuild2(0).get(); diff --git a/src/test/java/hudson/plugins/claim/ClaimReportTest.java b/src/test/java/hudson/plugins/claim/ClaimReportTest.java index 2878145c..18596534 100644 --- a/src/test/java/hudson/plugins/claim/ClaimReportTest.java +++ b/src/test/java/hudson/plugins/claim/ClaimReportTest.java @@ -45,7 +45,7 @@ public void claimedFailedJobShouldBeVisibleInClaimReport() throws Exception { User user1 = User.get("test-user1", true, Collections.emptyMap()); User user2 = User.get("test-user2", true, Collections.emptyMap()); - claimAction.applyClaim(user1.getId(), "test reason", user2.getId(), new Date(), true, true); + claimAction.applyClaim(user1, "test reason", user2, new Date(), true, true); try(JenkinsRule.WebClient client = j.createWebClient()) { HtmlPage page = client.goTo("claims/"); diff --git a/src/test/java/hudson/plugins/claim/ClaimTest.java b/src/test/java/hudson/plugins/claim/ClaimTest.java index be63a526..d213c661 100644 --- a/src/test/java/hudson/plugins/claim/ClaimTest.java +++ b/src/test/java/hudson/plugins/claim/ClaimTest.java @@ -27,6 +27,7 @@ import hudson.model.Build; import hudson.model.Project; import hudson.model.Result; +import hudson.model.User; import hudson.plugins.claim.utils.TestBuilder; import hudson.security.FullControlOnceLoggedInAuthorizationStrategy; import org.junit.Before; @@ -152,6 +153,45 @@ public void stickyClaimPropagatesToNextBuild() throws Exception { assertThat(action2.getClaimDate(), is(action.getClaimDate())); } + @Test + public void stickyClaimDoesPropagatesToNextBuildWithUnknownAssignedByWhenAssignedByUserHasBeenDeleted() throws Exception { + final int waitTime = 2_000; + // Given: + givenBuildClaimedByOtherUser(firstBuild); + User user = User.getById("user1", false); + user.delete(); + + // When: + Thread.sleep(waitTime); + Build nextBuild = project.scheduleBuild2(0).get(); + // Then: + ClaimBuildAction action = firstBuild.getAction(ClaimBuildAction.class); + ClaimBuildAction action2 = nextBuild.getAction(ClaimBuildAction.class); + assertThat(action2.isClaimed(), is(true)); + assertThat(action2.getClaimedBy(), is("user2")); + assertThat(action2.getReason(), is("reason")); + assertThat(action2.isSticky(), is(true)); + assertThat(action2.getAssignedBy(), is(User.getUnknown().getId())); + assertThat(action2.getClaimDate(), is(action.getClaimDate())); + } + + @Test + public void stickyClaimDoesNotPropagatesToNextBuildWhenClaimedByUserHasBeenDeleted() throws Exception { + final int waitTime = 2_000; + // Given: + givenBuildClaimedByCurrentUser(firstBuild); + User user = User.getById("user1", false); + user.delete(); + + // When: + Thread.sleep(waitTime); + Build nextBuild = project.scheduleBuild2(0).get(); + // Then: + ClaimBuildAction action = firstBuild.getAction(ClaimBuildAction.class); + ClaimBuildAction action2 = nextBuild.getAction(ClaimBuildAction.class); + assertThat(action2.isClaimed(), is(false)); + } + @Test public void stickyClaimOnPreviousBuildPropagatesToFollowingFailedBuilds() throws Exception { // Given: @@ -258,14 +298,17 @@ public void nonStickyClaimDoesNotPropagateToNextBuild() throws Exception { private ClaimBuildAction givenBuildClaimedByOtherUser(Build build) { ClaimBuildAction action = build.getAction(ClaimBuildAction.class); - action.claim("user2", "reason", "user1", new Date(), true, + User user1 = User.getById("user1", true); + User user2 = User.getById("user2", true); + action.claim(user2, "reason", user1, new Date(), true, false, false); return action; } private ClaimBuildAction givenBuildClaimedByCurrentUser(Build build) { ClaimBuildAction action = build.getAction(ClaimBuildAction.class); - action.claim("user1", "reason", "user1", new Date(), true, + User user1 = User.getById("user1", true); + action.claim(user1, "reason", user1, new Date(), true, false, false); return action; } diff --git a/src/test/java/hudson/plugins/claim/UserClaimsReportTest.java b/src/test/java/hudson/plugins/claim/UserClaimsReportTest.java index 5cb542c7..131422b0 100644 --- a/src/test/java/hudson/plugins/claim/UserClaimsReportTest.java +++ b/src/test/java/hudson/plugins/claim/UserClaimsReportTest.java @@ -55,21 +55,21 @@ public void userAssignedFailedJobsShouldBeVisibleInUserClaimReport() throws Exce // job1 claimed by A ClaimBuildAction claimAction1 = job1.getLastBuild().getAction(ClaimBuildAction.class); - claimAction1.applyClaim(userA.getId(), "test reason", userB.getId(), new Date(), true, true); + claimAction1.applyClaim(userA, "test reason", userB, new Date(), true, true); verifyUserClaims(userA, 1, 0); verifyUserClaims(userB, 0, 0); verifyUserClaims(userC, 0, 0); // job2 claimed by C ClaimBuildAction claimAction2 = job2.getLastBuild().getAction(ClaimBuildAction.class); - claimAction2.applyClaim(userC.getId(), "test reason", userB.getId(), new Date(), true, true); + claimAction2.applyClaim(userC, "test reason", userB, new Date(), true, true); verifyUserClaims(userA, 1, 0); verifyUserClaims(userB, 0, 0); verifyUserClaims(userC, 1, 0); // job3 claimed by C ClaimBuildAction claimAction3 = job3.getLastBuild().getAction(ClaimBuildAction.class); - claimAction3.applyClaim(userC.getId(), "test reason", userA.getId(), new Date(), true, true); + claimAction3.applyClaim(userC, "test reason", userA, new Date(), true, true); verifyUserClaims(userA, 1, 0); verifyUserClaims(userB, 0, 0); verifyUserClaims(userC, 2, 0); @@ -83,13 +83,14 @@ public void userAssignedFailedJobsUnderADifferentIdForSameUserShouldBeVisibleInU User userA = User.get("userA-" + timestamp, true, Collections.emptyMap()); assertNotEquals(userA.getId(), userA.getId().toLowerCase(), "Fix the test setup to ensure this condition"); + User userALowerCasedId = User.get(userA.getId().toLowerCase(), true, Collections.emptyMap()); // none claimed verifyUserClaims(userA, 0, 0); // job1 claimed by A ClaimBuildAction claimAction1 = job1.getLastBuild().getAction(ClaimBuildAction.class); - claimAction1.applyClaim(userA.getId().toLowerCase(), "test reason", userA.getId(), new Date(), true, true); + claimAction1.applyClaim(userALowerCasedId, "test reason", userA, new Date(), true, true); verifyUserClaims(userA, 1, 0); } From a3e5fc9dd24056623a2477182f370261e5904d0e Mon Sep 17 00:00:00 2001 From: Greybird Date: Sun, 8 Jan 2023 22:13:54 +0100 Subject: [PATCH 3/4] Remove obsolete methods --- .../claim/AbstractClaimBuildAction.java | 62 +------------------ 1 file changed, 3 insertions(+), 59 deletions(-) diff --git a/src/main/java/hudson/plugins/claim/AbstractClaimBuildAction.java b/src/main/java/hudson/plugins/claim/AbstractClaimBuildAction.java index b2be9625..41e4975a 100755 --- a/src/main/java/hudson/plugins/claim/AbstractClaimBuildAction.java +++ b/src/main/java/hudson/plugins/claim/AbstractClaimBuildAction.java @@ -41,8 +41,6 @@ public abstract class AbstractClaimBuildAction private String assignedBy; private Date claimDate; private boolean transientClaim = !ClaimConfig.get().isStickyByDefault(); - @Deprecated - private transient boolean reclaim; private ClaimBuildFailureAnalyzer bfaClaimer = null; private String reason; @@ -56,17 +54,6 @@ public final CommonMessagesProvider getMessageProvider() { return CommonMessagesProvider.build(this); } - /** - * Indicates if the {@link Saveable} is claimed. - * - * @deprecated use {@link #isClaimed()} instead - * @return true if the {@link Saveable} is claimed, else false - */ - @Deprecated - public final boolean isReclaim() { - return isClaimed(); - } - public final ClaimBuildFailureAnalyzer getBfaClaimer() { return bfaClaimer; } @@ -86,8 +73,7 @@ public final String getUrlName() { public final void doClaim(StaplerRequest req, StaplerResponse resp) throws Exception { User currentUser = getCurrentUser(); - String currentUserId = currentUser.getId(); - String claimedUser = currentUserId; // Default to self-assignment + User claimedUser = currentUser; // Default to self-assignment String assignee = req.getSubmittedForm().getString("assignee"); if (!StringUtils.isEmpty(assignee) && !claimedUser.equals(assignee)) { // Validate the specified assignee. @@ -97,7 +83,7 @@ public final void doClaim(StaplerRequest req, StaplerResponse resp) resp.forwardToPreviousPage(req); return; } - claimedUser = assignee; + claimedUser = resolvedAssignee; } String reasonProvided = req.getSubmittedForm().getString("reason"); @@ -125,7 +111,7 @@ public final void doClaim(StaplerRequest req, StaplerResponse resp) if (StringUtils.isEmpty(reasonProvided)) { reasonProvided = null; } - claim(claimedUser, reasonProvided, currentUserId, new Date(), sticky, propagated, true); + claim(claimedUser, reasonProvided, currentUser, new Date(), sticky, propagated, true); this.getOwner().save(); evalGroovyScript(); @@ -137,39 +123,6 @@ private static User getCurrentUser() { return User.get2(authentication); } - /** - * Claims a {@link Saveable}. - * @param claimedByUser name of the claiming user - * @param providedReason reason for the claim - * @param assignedByUser name of the assigned user - * @param isSticky true if the claim has to be kept until resolution - * @deprecated use {@link #claim(String, String, String, Date, boolean, boolean, boolean)} - */ - @Deprecated - public final void claim(String claimedByUser, String providedReason, String assignedByUser, boolean isSticky) { - claim(claimedByUser, providedReason, assignedByUser, new Date(), isSticky, - ClaimConfig.get().isPropagateToFollowingBuildsByDefault(), false); - } - - /** - * Claims a {@link Saveable}, and optionally notifies of the claim. - * @param claimedByUser name of the claiming user - * @param providedReason reason for the claim - * @param assignedByUser name of the assigner user - * @param date date of the claim - * @param isSticky true if the claim has to be kept until resolution - * @param isPropagated true if the claim has to be propagated to following builds - * @param notify true if notifications have to be sent - * @deprecated use {@link #claim(User, String, User, Date, boolean, boolean, boolean)} - */ - @Deprecated - public final void claim(String claimedByUser, String providedReason, String assignedByUser, Date date, - boolean isSticky, boolean isPropagated, boolean notify) { - User claimedBy = getUserFromId(claimedByUser); - User assignedBy = getUserFromId(assignedByUser); - claim(claimedBy, providedReason, assignedBy, date, isSticky, isPropagated, notify); - } - /** * Claims a {@link Saveable}, and optionally notifies of the claim. * @param claimedByUser claiming user @@ -252,15 +205,6 @@ public final void doUnclaim(StaplerRequest req, StaplerResponse resp) resp.forwardToPreviousPage(req); } - /** - * Unclaims a {@link Saveable}. - * @deprecated use {@link #unclaim(boolean)} - */ - @Deprecated - public final void unclaim() { - unclaim(false); - } - /** * Unclaims a {@link Saveable}, and optionally notifies of the unclaim. * @param notify true if notifications have to be sent From 95342e1e5825b48edb7a91f7c66ff86f9e55995c Mon Sep 17 00:00:00 2001 From: Greybird Date: Mon, 9 Jan 2023 16:25:09 +0100 Subject: [PATCH 4/4] Fis spotbugs issue --- .../java/hudson/plugins/claim/AbstractClaimBuildAction.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/main/java/hudson/plugins/claim/AbstractClaimBuildAction.java b/src/main/java/hudson/plugins/claim/AbstractClaimBuildAction.java index 41e4975a..0242a0f9 100755 --- a/src/main/java/hudson/plugins/claim/AbstractClaimBuildAction.java +++ b/src/main/java/hudson/plugins/claim/AbstractClaimBuildAction.java @@ -75,7 +75,8 @@ public final void doClaim(StaplerRequest req, StaplerResponse resp) User currentUser = getCurrentUser(); User claimedUser = currentUser; // Default to self-assignment String assignee = req.getSubmittedForm().getString("assignee"); - if (!StringUtils.isEmpty(assignee) && !claimedUser.equals(assignee)) { + + if (!StringUtils.isEmpty(assignee) && !claimedUser.getId().equals(assignee)) { // Validate the specified assignee. User resolvedAssignee = getUserFromId(assignee, false); if (resolvedAssignee == null) {