Yi EungJun 2015-04-22
NotificationMail: Merge mails if possible
When a user leaves a comment on an issue or review thread and close or
reopen it immediately, two events are created; one for comment and
another one for closing or reopening.

In such a case, Yobi have sent two emails to notify the two events. But
this patch makes Yobi sends only one email which has the message
generated by merging the two message of the two events.

For example, until now you have received two emails in such a case as
follows:

    From: Yi EungJun 
    To: me 
    Subject: [yobi] awesome feature request (#123)

    The feature is already implmented!

    ...

    From: Yi EungJun 
    To: me 
    Subject: [yobi] awesome feature request (#123)

    Issue has been closed

    ...

But now you receive only one email:

    From: Yi EungJun 
    To: me 
    Subject: [yobi] awesome feature request (#123)

    The feature is already implmented!

    ---

    Issue has been closed

    ...

Solution: Merge emails by NotificationMail.mergeMails() before they are
sent. The merged emails are instances of MergedNotificationMail whose
interface is same with existing NotificationMail so we didn't need to
modify the existing logic to send notification emails.

Note: Unfortunately, Two emails whose recipients differ from each other
are not merged. It means that if someone watches a project and changes
his or her notification setting not to receive notifications for new
comment then this feature does not work on the project because the user
exists in recipients of close/reopen events but not new comment events.
@96f7a521b5ca1d71942b7b16529331b0509489a9
app/models/NotificationEvent.java
--- app/models/NotificationEvent.java
+++ app/models/NotificationEvent.java
@@ -23,6 +23,7 @@
 import com.avaje.ebean.RawSqlBuilder;
 import controllers.UserApp;
 import controllers.routes;
+import notification.INotificationEvent;
 import models.enumeration.*;
 import models.resource.GlobalResource;
 import models.resource.Resource;
@@ -61,7 +62,7 @@
 import static models.enumeration.EventType.*;
 
 @Entity
-public class NotificationEvent extends Model {
+public class NotificationEvent extends Model implements INotificationEvent {
     private static final long serialVersionUID = 1L;
 
     @Id
@@ -464,6 +465,31 @@
         }
     }
 
+    @Override
+    public Date getCreatedDate() {
+        return created;
+    }
+
+    @Override
+    public String getTitle() {
+        return title;
+    }
+
+    @Override
+    public EventType getType() {
+        return eventType;
+    }
+
+    @Override
+    public ResourceType getResourceType() {
+        return resourceType;
+    }
+
+    @Override
+    public String getResourceId() {
+        return resourceId;
+    }
+
 
     /**
      * @see {@link models.PullRequest#merge(models.PullRequestEventMessage)}
app/models/NotificationMail.java
--- app/models/NotificationMail.java
+++ app/models/NotificationMail.java
@@ -22,11 +22,16 @@
 
 import com.google.common.collect.Lists;
 import info.schleichardt.play2.mailplugin.Mailer;
+import notification.INotificationEvent;
+import notification.INotificationMail;
 import mailbox.EmailAddressWithDetail;
+import models.enumeration.EventType;
 import models.enumeration.ResourceType;
 import models.enumeration.UserState;
 import models.resource.Resource;
+import notification.MergedNotificationMail;
 import org.apache.commons.lang.exception.ExceptionUtils;
+import org.apache.commons.lang3.ObjectUtils;
 import org.apache.commons.mail.HtmlEmail;
 import org.joda.time.DateTime;
 import org.jsoup.Jsoup;
@@ -57,8 +62,10 @@
 import java.util.*;
 import java.util.concurrent.TimeUnit;
 
+import static models.enumeration.EventType.*;
+
 @Entity
-public class NotificationMail extends Model {
+public class NotificationMail extends Model implements INotificationMail {
     private static final long serialVersionUID = 1L;
     private static final int RECIPIENT_NO_LIMIT = 0;
     static boolean hideAddress = true;
@@ -135,13 +142,19 @@
                 private void sendMail() {
                     Date createdUntil = DateTime.now().minusMillis
                             (MAIL_NOTIFICATION_DELAY_IN_MILLIS).toDate();
-                    List<NotificationMail> mails = find.where()
+                    List<? extends INotificationMail> mails = find.where()
                                     .lt("notificationEvent.created", createdUntil)
                                     .orderBy("notificationEvent.created ASC").findList();
 
-                    for (NotificationMail mail: mails) {
+                    try {
+                        mails = mergeMails(mails);
+                    } catch (Exception e) {
+                        play.Logger.warn("Failed to group mails", e);
+                    }
+
+                    for (INotificationMail mail: mails) {
                         try {
-                            NotificationEvent event = mail.notificationEvent;
+                            INotificationEvent event = mail.getEvent();
                             mail.delete();
                             if (event.resourceExists()) {
                                 sendNotification(event);
@@ -157,14 +170,109 @@
     }
 
     /**
+     * Groups mails by resource, sender and receivers
+     *
+     * Each mail is one of:
+     *
+     * a. a notification mail
+     * b. a merged notification mail of a pair: a notification to open or close
+     *    an issue or a review thread, and a previous notification to comment on
+     *    the resource by the same user
+     *
+     * @param mails orderd by created date
+     * @return
+     */
+    private static List<INotificationMail> mergeMails(List<? extends INotificationMail> mails) {
+        // Hash key to get a notification email by resource and sender
+        class EventHashKey {
+            private final Resource resource;
+            private final User sender;
+
+            @Override
+            public boolean equals(Object o) {
+                if (this == o) return true;
+                if (o == null || getClass() != o.getClass()) return false;
+
+                EventHashKey that = (EventHashKey) o;
+
+                if (resource != null ? !resource.equals(that.resource) : that.resource != null) return false;
+                if (sender != null ? !sender.equals(that.sender) : that.sender != null) return false;
+
+                return true;
+            }
+
+            @Override
+            public int hashCode() {
+                int result = resource != null ? resource.hashCode() : 0;
+                result = 31 * result + (sender != null ? sender.hashCode() : 0);
+                return result;
+            }
+
+            public EventHashKey(INotificationEvent event) {
+                this(event.getResource(), event.getSender());
+            }
+
+            public EventHashKey(Resource resource, User sender) {
+                this.resource = resource;
+                this.sender = sender;
+            }
+        }
+
+        List<INotificationMail> result = new ArrayList<>();
+        Map<EventHashKey, MergedNotificationMail> stateChangedMails = new HashMap<>();
+
+        // Merge events
+        for (Iterator<INotificationMail> iterator = new LinkedList<>(mails).descendingIterator();
+             iterator.hasNext();) {
+            INotificationMail mail = iterator.next();
+
+            INotificationEvent event = mail.getEvent();
+
+            if (event == null) {
+                play.Logger.warn(
+                        String.format("Possible bug: a notification event '%s' has null event.", mail));
+                continue;
+            }
+
+            if (event.getType().equals(ISSUE_STATE_CHANGED) ||
+                    event.getType().equals(REVIEW_THREAD_STATE_CHANGED)) {
+                // Collect state-change events
+                stateChangedMails.put(new EventHashKey(event), new MergedNotificationMail(mail));
+            } else if (event.getType().equals(EventType.NEW_COMMENT) ||
+                    event.getType().equals(EventType.NEW_REVIEW_COMMENT)) {
+                // If the current event is for commenting then find the matched
+                // state-change event and merge the two events.
+                EventHashKey key = new EventHashKey(
+                        event.getResource().getContainer(),
+                        event.getSender());
+                MergedNotificationMail stateChangedMail = stateChangedMails.remove(key);
+                if (stateChangedMail != null && ObjectUtils.equals(
+                        stateChangedMail.getEvent().findReceivers(), event.findReceivers())) {
+                    stateChangedMail.merge(mail);
+                    continue;
+                }
+            }
+
+            result.add(0, mail);
+        }
+
+        return result;
+    }
+
+    @Override
+    public INotificationEvent getEvent() {
+        return notificationEvent;
+    }
+
+    /**
      * An email which has Message-ID and/or References header based the given
      * NotificationEvent if possible. The headers help MUA to bind the emails
      * into a thread.
      */
     public static class EventEmail extends HtmlEmail {
-        private NotificationEvent event;
+        private INotificationEvent event;
 
-        public EventEmail(NotificationEvent event) {
+        public EventEmail(INotificationEvent event) {
             this.event = event;
         }
 
@@ -173,7 +281,7 @@
             return new MimeMessage(aSession) {
                 @Override
                 protected void updateMessageID() throws MessagingException {
-                    if (event != null && event.eventType.isCreating()) {
+                    if (event != null && event.getType().isCreating()) {
                         setHeader("Message-ID",
                                 event.getResource().getMessageId());
                     } else {
@@ -184,13 +292,13 @@
         }
 
         public void addReferences() {
-            if (event == null || event.resourceType == null ||
-                    event.resourceId == null) {
+            if (event == null || event.getResourceType() == null ||
+                    event.getResourceId() == null) {
                 return;
             }
 
             Resource resource = Resource.get(
-                    event.resourceType, event.resourceId);
+                    event.getResourceType(), event.getResourceId());
 
             if (resource == null) {
                 return;
@@ -221,7 +329,7 @@
      * @param event
      * @see <a href="https://github.com/nforge/yobi/blob/master/docs/technical/watch.md>watch.md</a>
      */
-    private static void sendNotification(NotificationEvent event) {
+    private static void sendNotification(INotificationEvent event) {
         Set<User> receivers = event.findReceivers();
 
         // Remove inactive users.
@@ -302,7 +410,7 @@
         return list;
     }
 
-    private static void sendMail(NotificationEvent event, Set<MailRecipient> toList, Set<MailRecipient> bccList, String langCode) {
+    private static void sendMail(INotificationEvent event, Set<MailRecipient> toList, Set<MailRecipient> bccList, String langCode) {
         if (toList.isEmpty()) {
             return;
         }
@@ -339,7 +447,7 @@
 
             String urlToView = event.getUrlToView();
 
-            email.setSubject(event.title);
+            email.setSubject(event.getTitle());
             Resource resource = event.getResource();
             if (resource.getType() == ResourceType.ISSUE_COMMENT) {
                 IssueComment issueComment = IssueComment.find.byId(Long.valueOf(resource.getId()));
@@ -349,7 +457,7 @@
             email.setTextMsg(getPlainMessage(lang, message, Url.create(urlToView), acceptsReply));
             email.setCharset("utf-8");
             email.addReferences();
-            email.setSentDate(event.created);
+            email.setSentDate(event.getCreatedDate());
             Mailer.send(email);
             String escapedTitle = email.getSubject().replace("\"", "\\\"");
             Set<InternetAddress> recipients = new HashSet<>();
@@ -485,4 +593,5 @@
 
         return msg;
     }
+
 }
 
app/notification/INotificationEvent.java (added)
+++ app/notification/INotificationEvent.java
@@ -0,0 +1,54 @@
+/**
+ * Yobi, Project Hosting SW
+ *
+ * Copyright 2015 NAVER Corp.
+ * http://yobi.io
+ *
+ * @author Yi EungJun
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package notification;
+
+import models.User;
+import models.enumeration.EventType;
+import models.enumeration.ResourceType;
+import models.resource.Resource;
+import play.api.i18n.Lang;
+
+import java.util.Date;
+import java.util.Set;
+
+public interface INotificationEvent {
+    User getSender();
+
+    Resource getResource();
+
+    String getMessage(Lang lang);
+
+    String getUrlToView();
+
+    Date getCreatedDate();
+
+    String getTitle();
+
+    EventType getType();
+
+    ResourceType getResourceType();
+
+    String getResourceId();
+
+    boolean resourceExists();
+
+    Set<User> findReceivers();
+}
 
app/notification/INotificationMail.java (added)
+++ app/notification/INotificationMail.java
@@ -0,0 +1,27 @@
+/**
+ * Yobi, Project Hosting SW
+ *
+ * Copyright 2015 NAVER Corp.
+ * http://yobi.io
+ *
+ * @author Yi EungJun
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package notification;
+
+public interface INotificationMail {
+    INotificationEvent getEvent();
+
+    void delete();
+}
 
app/notification/MergedNotificationEvent.java (added)
+++ app/notification/MergedNotificationEvent.java
@@ -0,0 +1,101 @@
+/**
+ * Yobi, Project Hosting SW
+ *
+ * Copyright 2015 NAVER Corp.
+ * http://yobi.io
+ *
+ * @author Yi EungJun
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package notification;
+
+import com.google.common.base.Joiner;
+import models.User;
+import models.enumeration.EventType;
+import models.enumeration.ResourceType;
+import models.resource.Resource;
+import play.api.i18n.Lang;
+
+import javax.annotation.Nonnull;
+import java.util.*;
+
+public class MergedNotificationEvent implements INotificationEvent {
+    private final List<INotificationEvent> messageSources;
+    private final INotificationEvent main;
+
+    public MergedNotificationEvent(@Nonnull INotificationEvent main,
+                                   @Nonnull List<INotificationEvent> messageSources) {
+        this.main = main;
+        this.messageSources = new ArrayList<>(messageSources);
+    }
+
+    @Override
+    public User getSender() {
+        return main.getSender();
+    }
+
+    @Override
+    public Resource getResource() {
+        return main.getResource();
+    }
+
+    @Override
+    public String getMessage(Lang lang) {
+        List<String> messages = new ArrayList<>();
+        for(INotificationEvent event : messageSources) {
+            messages.add(event.getMessage(lang));
+        }
+        return Joiner.on("\n\n---\n\n").join(messages);
+    }
+
+    @Override
+    public String getUrlToView() {
+        return main.getUrlToView();
+    }
+
+    @Override
+    public Date getCreatedDate() {
+        return main.getCreatedDate();
+    }
+
+    @Override
+    public String getTitle() {
+        return main.getTitle();
+    }
+
+    @Override
+    public EventType getType() {
+        return main.getType();
+    }
+
+    @Override
+    public ResourceType getResourceType() {
+        return main.getResourceType();
+    }
+
+    @Override
+    public String getResourceId() {
+        return main.getResourceId();
+    }
+
+    @Override
+    public boolean resourceExists() {
+        return main.resourceExists();
+    }
+
+    @Override
+    public Set<User> findReceivers() {
+        return main.findReceivers();
+    }
+}
 
app/notification/MergedNotificationMail.java (added)
+++ app/notification/MergedNotificationMail.java
@@ -0,0 +1,62 @@
+/**
+ * Yobi, Project Hosting SW
+ *
+ * Copyright 2015 NAVER Corp.
+ * http://yobi.io
+ *
+ * @author Yi EungJun
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package notification;
+
+import javax.annotation.Nonnull;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.List;
+
+public class MergedNotificationMail implements INotificationMail {
+    private final List<INotificationMail> mails;
+    private final INotificationMail main;
+
+    public MergedNotificationMail(@Nonnull INotificationMail main,
+                                  @Nonnull List<INotificationMail> mails) {
+        this.main = main;
+        this.mails = new ArrayList<>();
+        this.mails.addAll(mails);
+    }
+
+    public MergedNotificationMail(@Nonnull INotificationMail main) {
+        this(main, Arrays.asList(main));
+    }
+
+    @Override
+    public INotificationEvent getEvent() {
+        List<INotificationEvent> events = new ArrayList<>();
+        for(INotificationMail mail : mails) {
+            events.add(mail.getEvent());
+        }
+        return new MergedNotificationEvent(main.getEvent(), events);
+    }
+
+    @Override
+    public void delete() {
+        for(INotificationMail mail : mails) {
+            mail.delete();
+        }
+    }
+
+    public void merge(INotificationMail mail) {
+        mails.add(mail);
+    }
+}
Add a comment
List