Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[JENKINS-32301] Support more than one Reply-To Address #26

Merged
merged 2 commits into from Mar 28, 2016

Conversation

Projects
None yet
6 participants
@andresrc
Copy link
Collaborator

commented Mar 3, 2016

See JENKINS-32301

Some additional refactoring of MimeMessageBuilderTest included.

@reviewbybees

@reviewbybees

This comment has been minimized.

Copy link

commented Mar 3, 2016

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

@jenkinsadmin

This comment has been minimized.

Copy link
Member

commented Mar 3, 2016

Thank you for this pull request! Please check this document for how the Jenkins project handles pull requests.

return this;
}

public MimeMessageBuilder addReplyTo(@Nonnull String replyTo) {

This comment has been minimized.

Copy link
@armfergom

armfergom Mar 14, 2016

Used only for testing, right?

@@ -110,10 +118,27 @@ public MimeMessageBuilder setFrom(@Nonnull String from) {
}

public MimeMessageBuilder setReplyTo(@Nonnull String replyTo) {
this.replyTo = replyTo;
try {

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Mar 14, 2016

Member

MimeMessageBuilder is single-thread, right? If no, the new code introduces concurrency issues

@@ -164,4 +172,28 @@ public void test_addRecipients_tokenizer() throws Exception {
Assert.assertEquals("tom.yyyy@gmail.com", recipients[1].toString());
Assert.assertEquals("tom.zzzz@gmail.com", recipients[2].toString());

This comment has been minimized.

Copy link
@armfergom

armfergom Mar 14, 2016

You could use constants everywhere since you are doing it in other places, just to be consistent.

This comment has been minimized.

Copy link
@andresrc

andresrc Mar 14, 2016

Author Collaborator

I wanted to avoid "unrelated changes", will do a further clean up in another PR.

@armfergom

This comment has been minimized.

Copy link

commented Mar 14, 2016

LGTM 🐝 This is more a new feature rather than a bug fix unless the documentation says it should be working.

private void addRecipients(MimeMessage msg, Set<InternetAddress> recipientList, Message.RecipientType recipientType) throws UnsupportedEncodingException, MessagingException {
if (recipientList.isEmpty()) {
return;
}
final Collection<InternetAddress> recipients = recipientFilter != null ? recipientFilter.apply(recipientList) : recipientList;

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Mar 14, 2016

Member

Is this code related to the PR title? I doubt. Maybe a standalone bugfix

This comment has been minimized.

Copy link
@andresrc

andresrc Mar 14, 2016

Author Collaborator

It is a small refactor to share the code needed for the PR with the existing one.

@andresrc andresrc force-pushed the andresrc:JENKINS-32301 branch from 93027e8 to 5aebf60 Mar 14, 2016

@andresrc

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 14, 2016

Rebased and documented MimeMessageBuilder non-thread-safety.

@abayer

This comment has been minimized.

Copy link
Member

commented Mar 28, 2016

🐝

@andresrc

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 28, 2016

@andresrc andresrc merged commit 97f8508 into jenkinsci:master Mar 28, 2016

1 check passed

Jenkins This pull request looks good
Details

@andresrc andresrc deleted the andresrc:JENKINS-32301 branch Mar 28, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.