Merge pull request #840 from clojars/tobias/group-membership-emails

tobias · web-flow · commit 07b464522dcd · 2022-06-29T08:37:54.000-04:00
diff --git a/resources/queries/queryfile.sql b/resources/queries/queryfile.sql
@@ -111,7 +111,7 @@ JOIN (
 ) g
 ON (
   u.user = g.user
-)
+);
 
 --name: group-allnames
 SELECT "user"
@@ -141,6 +141,24 @@ WHERE (
       admin = true
 );
 
+--name: group-admin-emails
+SELECT u.email
+FROM users u
+JOIN (
+  SELECT "user"
+  FROM groups
+  WHERE (
+    name = :groupname
+    AND
+    inactive IS NOT true
+    AND
+    admin = true
+  )
+) g
+ON (
+  u.user = g.user
+);
+
 --name: inactivate-member!
 UPDATE groups
 SET inactive = true, inactivated_by = :inactivated_by
diff --git a/src/clojars/db.clj b/src/clojars/db.clj
@@ -197,6 +197,11 @@
                         {:connection db
                          :row-fn :user}))
 
+(defn group-admin-emails [db groupname]
+  (sql/group-admin-emails {:groupname groupname}
+                        {:connection db
+                         :row-fn :email}))
+
 (defn group-activenames [db groupname]
   (sql/group-activenames {:groupname groupname}
                          {:connection db
diff --git a/src/clojars/notifications/group.clj b/src/clojars/notifications/group.clj
@@ -0,0 +1,38 @@
+(ns clojars.notifications.group
+  (:require
+   [clojars.notifications :as notifications]
+   [clojars.notifications.common :as common]))
+
+(defmethod notifications/notification :group-member-added
+  [_type mailer
+   {:as _user username :user}
+   {:as data :keys [admin? group member admin-emails member-email]}]
+  (let [subject (format "A%s member was added to the group %s"
+                        (if admin? "n admin" "")
+                        group)
+        body [(format
+               "User '%s' was added%s to the %s group by %s."
+               member
+               (if admin? " as an admin" "")
+               group
+               username)
+              (common/details-table data)]
+        emails (set (concat admin-emails [member-email]))]
+    (doseq [email emails]
+      (notifications/send mailer email subject body))))
+
+(defmethod notifications/notification :group-member-removed
+  [_type mailer
+   {:as _user username :user}
+   {:as data :keys [group member admin-emails member-email]}]
+  (let [subject (format "A member was removed from the group %s"
+                        group)
+        body [(format
+               "User '%s' was removed from the %s group by %s."
+               member
+               group
+               username)
+              (common/details-table data)]
+        emails (set (concat admin-emails [member-email]))]
+    (doseq [email emails]
+      (notifications/send mailer email subject body))))
diff --git a/src/clojars/routes/group.clj b/src/clojars/routes/group.clj
@@ -2,24 +2,27 @@
   (:require
    [clojars.auth :as auth]
    [clojars.db :as db]
+   [clojars.event :as event]
+   [clojars.log :as log]
+   [clojars.routes.common :as common]
    [clojars.web.group :as view]
-   [compojure.core :as compojure :refer [GET POST DELETE]]
-   [clojars.log :as log]))
+   [compojure.core :as compojure :refer [GET POST DELETE]]))
 
 (defn- get-members [db groupname]
   (let [actives (seq (db/group-actives db groupname))]
     (when (seq actives)
       (auth/try-account
        #(view/show-group db % groupname actives)))))
 
-(defn- toggle-or-add-member [db groupname username make-admin?]
+(defn- toggle-or-add-member [db groupname username make-admin? details]
   (let [actives (seq (db/group-actives db groupname))
         membernames (->> actives
                          (filter (complement view/is-admin?))
                          (map :user))
         adminnames (->> actives
                         (filter view/is-admin?)
                         (map :user))
+        user-to-add (db/find-user db username)
         handler-fn (fn [account admin? group-users]
                      #(log/with-context {:tag :toggle-or-add-group-member
                                          :group groupname
@@ -41,9 +44,18 @@
                             (view/show-group db account groupname actives
                                              (format "They're already an %s!" (if admin? "admin" "member"))))
 
-                          (db/find-user db username)
+                          user-to-add
                           (let [add-fn (if admin? db/add-admin db/add-member)]
                             (add-fn db groupname username account)
+                            (event/emit :group-member-added
+                                        (merge
+                                         details
+                                         {:admin? admin?
+                                          :admin-emails (db/group-admin-emails db groupname)
+                                          :group groupname
+                                          :member username
+                                          :member-email (:email user-to-add)
+                                          :username account}))
                             (log/info {:status :success})
                             (log/audit db {:tag :member-added
                                            :message (format "user '%s' added as %s" username
@@ -71,7 +83,7 @@
                       make-admin?
                       (if make-admin? adminnames membernames))))))))
 
-(defn- remove-member [db groupname username]
+(defn- remove-member [db groupname username details]
   (let [actives (seq (db/group-actives db groupname))]
     (when (seq actives)
       (auth/try-account
@@ -95,6 +107,14 @@
                (some #{username} (map :user actives))
                (do
                  (db/inactivate-member db groupname username account)
+                 (event/emit :group-member-removed
+                             (merge
+                              details
+                              {:admin-emails (db/group-admin-emails db groupname)
+                               :group groupname
+                               :member username
+                               :member-email (:email (db/find-user db username))
+                               :username account}))
                  (log/info {:status :success})
                  (log/audit db {:tag :member-removed
                                 :message (format "user '%s' removed" username)})
@@ -113,7 +133,7 @@
   (compojure/routes
    (GET ["/groups/:groupname", :groupname #"[^/]+"] [groupname]
         (get-members db groupname))
-   (POST ["/groups/:groupname", :groupname #"[^/]+"] [groupname username admin]
-         (toggle-or-add-member db groupname username (= "1" admin)))
-   (DELETE ["/groups/:groupname", :groupname #"[^/]+"] [groupname username]
-           (remove-member db groupname username))))
+   (POST ["/groups/:groupname", :groupname #"[^/]+"] [groupname username admin :as request]
+         (toggle-or-add-member db groupname username (= "1" admin) (common/request-details request)))
+   (DELETE ["/groups/:groupname", :groupname #"[^/]+"] [groupname username :as request]
+           (remove-member db groupname username (common/request-details request)))))
diff --git a/src/clojars/system.clj b/src/clojars/system.clj
@@ -4,6 +4,7 @@
    [clojars.notifications :as notifications]
    ;; for defmethods
    [clojars.notifications.deploys]
+   [clojars.notifications.group]
    [clojars.notifications.token]
    [clojars.notifications.user]
    [clojars.oauth.github :as github]
diff --git a/test/clojars/integration/users_test.clj b/test/clojars/integration/users_test.clj
@@ -4,8 +4,10 @@
    [clojars.email :as email]
    [clojars.integration.steps :refer [disable-mfa enable-mfa login-as register-as]]
    ;; for defmethods
+   [clojars.notifications.group]
    [clojars.notifications.user]
    [clojars.test-helper :as help :refer [with-test-system]]
+   [clojure.string :as str]
    [clojure.test :refer [deftest is testing use-fixtures]]
    [kerodon.core :refer [check fill-in follow follow-redirect
                          press session visit within]]
@@ -278,72 +280,107 @@
         (has (text? "The reset code was not found. Please ask for a new code in the forgot password page")))))
 
 (deftest admin-can-add-member-to-group
-  (-> (session (help/app))
-      (register-as "fixture" "fixture@example.org" "password"))
-  (-> (session (help/app))
-      (register-as "dantheman" "test@example.org" "password")
-      (visit "/groups/org.clojars.dantheman")
-      (fill-in [:#username] "fixture")
-      (press "Add Member")
-      ;;(follow-redirect)
-      (within [:table.group-member-list
-               [:tr enlive/last-of-type]
-               [:td enlive/first-of-type]]
-              (has (text? "fixture")))
-      (within [:table.group-member-list
-               [:tr enlive/last-of-type]
-               [:td (enlive/nth-of-type 2)]]
-              (has (text? "No"))))
-
-  (is (some #{"fixture"} (db/group-membernames help/*db* "org.clojars.dantheman")))
-
-  (help/match-audit {:username "dantheman"}
-                    {:tag "member-added"
-                     :user "dantheman"
-                     :group_name "org.clojars.dantheman"
-                     :message "user 'fixture' added as member"}))
+  (with-test-system
+    (-> (session (help/app))
+        (register-as "fixture" "fixture@example.org" "password"))
+    (-> (session (help/app))
+        (register-as "dantheman" "test@example.org" "password")
+        ((fn [session] (email/expect-mock-emails 2) session))
+        (visit "/groups/org.clojars.dantheman")
+        (fill-in [:#username] "fixture")
+        (press "Add Member")
+        ;;(follow-redirect)
+        (within [:table.group-member-list
+                 [:tr enlive/last-of-type]
+                 [:td enlive/first-of-type]]
+                (has (text? "fixture")))
+        (within [:table.group-member-list
+                 [:tr enlive/last-of-type]
+                 [:td (enlive/nth-of-type 2)]]
+                (has (text? "No"))))
+
+    (is (some #{"fixture"} (db/group-membernames help/*db* "org.clojars.dantheman")))
+
+    (help/match-audit {:username "dantheman"}
+                      {:tag "member-added"
+                       :user "dantheman"
+                       :group_name "org.clojars.dantheman"
+                       :message "user 'fixture' added as member"})
+
+    (is (true? (email/wait-for-mock-emails)))
+    (is (= 2 (count @email/mock-emails)))
+    (is (= #{"fixture@example.org" "test@example.org"}
+           (into #{} (map first) @email/mock-emails)))
+    (is (every? #(= "A member was added to the group org.clojars.dantheman"
+                    %)
+                (into [] (map second) @email/mock-emails)))
+    (is (every? #(str/starts-with? % "User 'fixture' was added to the org.clojars.dantheman group by dantheman.\n\n")
+                (into [] (map #(nth % 2)) @email/mock-emails)))))
 
 (deftest admin-can-add-admin-to-group
-  (-> (session (help/app))
-      (register-as "fixture" "fixture@example.org" "password"))
-  (-> (session (help/app))
-      (register-as "dantheman" "test@example.org" "password")
-      (visit "/groups/org.clojars.dantheman")
-      (fill-in [:#username] "fixture")
-      (check [:#admin])
-      (press "Add Member")
-      ;;(follow-redirect)
-      (within [:table.group-member-list
-               [:tr enlive/last-of-type]
-               [:td enlive/first-of-type]]
-              (has (text? "fixture")))
-      (within [:table.group-member-list
-               [:tr enlive/last-of-type]
-               [:td (enlive/nth-of-type 2)]]
-              (has (text? "Yes"))))
-
-  (is (some #{"fixture"} (db/group-adminnames help/*db* "org.clojars.dantheman")))
-
-  (help/match-audit {:username "dantheman"}
-                    {:tag "member-added"
-                     :user "dantheman"
-                     :group_name "org.clojars.dantheman"
-                     :message "user 'fixture' added as admin"}))
+  (with-test-system
+    (-> (session (help/app))
+        (register-as "fixture" "fixture@example.org" "password"))
+    (-> (session (help/app))
+        (register-as "dantheman" "test@example.org" "password")
+        ((fn [session] (email/expect-mock-emails 2) session))
+        (visit "/groups/org.clojars.dantheman")
+        (fill-in [:#username] "fixture")
+        (check [:#admin])
+        (press "Add Member")
+        (within [:table.group-member-list
+                 [:tr enlive/last-of-type]
+                 [:td enlive/first-of-type]]
+                (has (text? "fixture")))
+        (within [:table.group-member-list
+                 [:tr enlive/last-of-type]
+                 [:td (enlive/nth-of-type 2)]]
+                (has (text? "Yes"))))
+
+    (is (some #{"fixture"} (db/group-adminnames help/*db* "org.clojars.dantheman")))
+
+    (help/match-audit {:username "dantheman"}
+                      {:tag "member-added"
+                       :user "dantheman"
+                       :group_name "org.clojars.dantheman"
+                       :message "user 'fixture' added as admin"})
+
+    (is (true? (email/wait-for-mock-emails)))
+    (is (= 2 (count @email/mock-emails)))
+    (is (= #{"fixture@example.org" "test@example.org"}
+           (into #{} (map first) @email/mock-emails)))
+    (is (every? #(= "An admin member was added to the group org.clojars.dantheman"
+                    %)
+                (into [] (map second) @email/mock-emails)))
+    (is (every? #(str/starts-with? % "User 'fixture' was added as an admin to the org.clojars.dantheman group by dantheman.\n\n")
+                (into [] (map #(nth % 2)) @email/mock-emails)))))
 
 (deftest admin-can-remove-user-from-group
-  (-> (session (help/app))
-      (register-as "fixture" "fixture@example.org" "password"))
-  (-> (session (help/app))
-      (register-as "dantheman" "test@example.org" "password")
-      (visit "/groups/org.clojars.dantheman")
-      (fill-in [:#username] "fixture")
-      (press "Add Member")
-      (press "Remove Member"))
-  (help/match-audit {:username "dantheman"}
-                    {:tag "member-removed"
-                     :user "dantheman"
-                     :group_name "org.clojars.dantheman"
-                     :message "user 'fixture' removed"}))
+  (with-test-system
+    (-> (session (help/app))
+        (register-as "fixture" "fixture@example.org" "password"))
+    (-> (session (help/app))
+        (register-as "dantheman" "test@example.org" "password")
+        (visit "/groups/org.clojars.dantheman")
+        (fill-in [:#username] "fixture")
+        (press "Add Member")
+        ((fn [session] (email/expect-mock-emails 2) session))
+        (press "Remove Member"))
+    (help/match-audit {:username "dantheman"}
+                      {:tag "member-removed"
+                       :user "dantheman"
+                       :group_name "org.clojars.dantheman"
+                       :message "user 'fixture' removed"})
+
+    (is (true? (email/wait-for-mock-emails)))
+    (is (= 2 (count @email/mock-emails)))
+    (is (= #{"fixture@example.org" "test@example.org"}
+           (into #{} (map first) @email/mock-emails)))
+    (is (every? #(= "A member was removed from the group org.clojars.dantheman"
+                    %)
+                (into [] (map second) @email/mock-emails)))
+    (is (every? #(str/starts-with? % "User 'fixture' was removed from the org.clojars.dantheman group by dantheman.\n\n")
+                (into [] (map #(nth % 2)) @email/mock-emails)))))
 
 (deftest user-must-exist-to-be-added-to-group
   (-> (session (help/app))
