Delete user pulse/alert/dashboard sub subscriptions when User becomes inactive (#17809)

* Delete user pulse/alert/dashboard subscription subscriptions when they get archived

* Automatically archive Pulses when last subscription is deleted (#17812)

* Automatically archive Pulse when its last subscription is deleted

* Test fix 

* Fix occasionally-failing rotate-encryption-key!-test

* Update pulse_test.clj

src/metabase/events/activity_feed.clj

@@ -157,7 +157,7 @@
       (log/warn e (trs "Failed to process activity event {0}" (pr-str (:topic activity-event)))))))
 
 
-;;; ---------------------------------------------------- LIFECYLE ----------------------------------------------------
+;;; --------------------------------------------------- LIFECYCLE ----------------------------------------------------
 
 (defmethod events/init! ::ActivityFeed
   [_]

src/metabase/models/pulse.clj

@@ -136,6 +136,20 @@
     :can-write?        can-write?
     :perms-objects-set perms-objects-set}))
 
+(def ^:private ^:dynamic *automatically-archive-when-last-channel-is-deleted*
+  "Should we automatically archive a Pulse when its last `PulseChannel` is deleted? Normally we do, but this is disabled
+  in [[update-notification-channels!]] which creates/deletes/updates several channels sequentially."
+  true)
+
+(defn will-delete-channel
+  "This function is called by [[metabase.models.pulse-channel/pre-delete]] when the `PulseChannel` is about to be
+  deleted. Archives `Pulse` if the channel being deleted is its last channel."
+  [{pulse-id :pulse_id, pulse-channel-id :id, :as pulse-channel}]
+  (when *automatically-archive-when-last-channel-is-deleted*
+    (let [other-channels-count (db/count PulseChannel :pulse_id pulse-id, :id [:not= pulse-channel-id])]
+      (when (zero? other-channels-count)
+        (db/update! Pulse pulse-id :archived true)))))
+
 
 ;;; ---------------------------------------------------- Schemas -----------------------------------------------------
 
@@ -362,13 +376,12 @@
 ;;; ------------------------------------------ Other Persistence Functions -------------------------------------------
 
 (s/defn update-notification-cards!
-  "Update the PulseCards for a given `notification-or-id`.
-   `card-refs` should be a definitive collection of *all* Cards for the Notification in the desired order. They should
-  have keys like `id`, `include_csv`, and `include_xls`.
+  "Update the PulseCards for a given `notification-or-id`. `card-refs` should be a definitive collection of *all* Cards
+  for the Notification in the desired order. They should have keys like `id`, `include_csv`, and `include_xls`.
 
-   *  If a Card ID in `card-refs` has no corresponding existing `PulseCard` object, one will be created.
-   *  If an existing `PulseCard` has no corresponding ID in CARD-IDs, it will be deleted.
-   *  All cards will be updated with a `position` according to their place in the collection of `card-ids`"
+  *  If a Card ID in `card-refs` has no corresponding existing `PulseCard` object, one will be created.
+  *  If an existing `PulseCard` has no corresponding ID in CARD-IDs, it will be deleted.
+  *  All cards will be updated with a `position` according to their place in the collection of `card-ids`"
   [notification-or-id, card-refs :- (s/maybe [CardRef])]
   ;; first off, just delete any cards associated with this pulse (we add them again below)
   (db/delete! PulseCard :pulse_id (u/the-id notification-or-id))
@@ -384,9 +397,9 @@
                              card-refs)]
       (db/insert-many! PulseCard cards))))
 
-
 (defn- create-update-delete-channel!
-  "Utility function which determines how to properly update a single pulse channel."
+  "Utility function used by [[update-notification-channels!]] which determines how to properly update a single pulse
+  channel."
   [notification-or-id new-channel existing-channel]
   ;; NOTE that we force the :id of the channel being updated to the :id we *know* from our
   ;;      existing list of PulseChannels pulled from the db to ensure we affect the right record
@@ -426,9 +439,12 @@
                                                        (first (get old-channels %)))]
     (assert (zero? (count (get new-channels nil)))
       "Cannot have channels without a :channel_type attribute")
-    ;; for each of our possible channel types call our handler function
-    (doseq [[channel-type] pulse-channel/channel-types]
-      (handle-channel channel-type))))
+    ;; don't automatically archive this Pulse if we end up deleting its last PulseChannel -- we're probably replacing
+    ;; it with a new one immediately thereafter.
+    (binding [*automatically-archive-when-last-channel-is-deleted* false]
+      ;; for each of our possible channel types call our handler function
+      (doseq [[channel-type] pulse-channel/channel-types]
+        (handle-channel channel-type)))))
 
 (s/defn ^:private create-notification-and-add-cards-and-channels!
   "Create a new Pulse/Alert with the properties specified in `notification`; add the `card-refs` to the Notification and

src/metabase/models/pulse_channel.clj

@@ -5,6 +5,7 @@
             [metabase.models.interface :as i]
             [metabase.models.pulse-channel-recipient :refer [PulseChannelRecipient]]
             [metabase.models.user :as user :refer [User]]
+            [metabase.plugins.classloader :as classloader]
             [metabase.util :as u]
             [schema.core :as s]
             [toucan.db :as db]
@@ -83,7 +84,7 @@
   {:email {:type              "email"
            :name              "Email"
            :allows_recipients true
-           :recipients        ["user", "email"]
+           :recipients        ["user" "email"]
            :schedules         [:hourly :daily :weekly :monthly]}
    :slack {:type              "slack"
            :name              "Slack"
@@ -126,13 +127,20 @@
                 :order-by [[:u.id :asc]]})]
      (user/add-common-name user))))
 
+(defn- pre-delete [pulse-channel]
+  ;; Call [[metabase.models.pulse/will-delete-channel]] to let it know we're about to delete a PulseChannel; that
+  ;; function will decide whether or not to automatically archive the Pulse as well.
+  (classloader/require 'metabase.models.pulse)
+  ((resolve 'metabase.models.pulse/will-delete-channel) pulse-channel))
+
 (u/strict-extend (class PulseChannel)
   models/IModel
   (merge
    models/IModelDefaults
    {:hydration-keys (constantly [:pulse_channel])
     :types          (constantly {:details :json, :channel_type :keyword, :schedule_type :keyword, :schedule_frame :keyword})
-    :properties     (constantly {:timestamped? true})})
+    :properties     (constantly {:timestamped? true})
+    :pre-delete     pre-delete})
 
   i/IObjectPermissions
   (merge
@@ -140,6 +148,21 @@
    {:can-read?  (constantly true)
     :can-write? i/superuser?}))
 
+(defn will-delete-recipient
+  "This function is called by [[metabase.models.pulse-channel-recipient/pre-delete]] when a `PulseChannelRecipient` is
+  about to be deleted. Deletes `PulseChannel` if the recipient being deleted is its last recipient. (This only applies
+  to PulseChannels with User subscriptions; Slack PulseChannels and ones with email address subscriptions are not
+  automatically deleted.)"
+  [{channel-id :pulse_channel_id, pulse-channel-recipient-id :id}]
+  (let [other-recipients-count (db/count PulseChannelRecipient :pulse_channel_id channel-id, :id [:not= pulse-channel-recipient-id])
+        last-recipient?        (zero? other-recipients-count)]
+    (when last-recipient?
+      ;; make sure this channel doesn't have any email-address (non-User) recipients.
+      (let [details              (db/select-one-field :details PulseChannel :id channel-id)
+            has-email-addresses? (seq (:emails details))]
+        (when-not has-email-addresses?
+          (db/delete! PulseChannel :id channel-id))))))
+
 
 ;; ## Persistence Functions
 
@@ -171,18 +194,18 @@
                                       weekday)]
     (db/select [PulseChannel :id :pulse_id :schedule_type :channel_type]
       {:where [:and [:= :enabled true]
-                    [:or [:= :schedule_type "hourly"]
-                         [:and [:= :schedule_type "daily"]
-                               [:= :schedule_hour hour]]
-                         [:and [:= :schedule_type "weekly"]
-                               [:= :schedule_hour hour]
-                               [:= :schedule_day weekday]]
-                         [:and [:= :schedule_type "monthly"]
-                               [:= :schedule_hour hour]
-                               [:= :schedule_frame schedule-frame]
-                               [:or [:= :schedule_day weekday]
-                                    ;; this is here specifically to allow for cases where day doesn't have to match
-                                    [:= :schedule_day monthly-schedule-day-or-nil]]]]]})))
+               [:or [:= :schedule_type "hourly"]
+                [:and [:= :schedule_type "daily"]
+                 [:= :schedule_hour hour]]
+                [:and [:= :schedule_type "weekly"]
+                 [:= :schedule_hour hour]
+                 [:= :schedule_day weekday]]
+                [:and [:= :schedule_type "monthly"]
+                 [:= :schedule_hour hour]
+                 [:= :schedule_frame schedule-frame]
+                 [:or [:= :schedule_day weekday]
+                  ;; this is here specifically to allow for cases where day doesn't have to match
+                  [:= :schedule_day monthly-schedule-day-or-nil]]]]]})))
 
 
 (defn update-recipients!

src/metabase/models/pulse_channel_recipient.clj

 (ns metabase.models.pulse-channel-recipient
-  (:require [toucan.models :as models]))
+  (:require [metabase.plugins.classloader :as classloader]
+            [metabase.util :as u]
+            [toucan.models :as models]))
 
 (models/defmodel PulseChannelRecipient :pulse_channel_recipient)
+
+(defn- pre-delete [{id :id, pulse-channel-id :pulse_channel_id, :as pcr}]
+  ;; call [[metabase.models.pulse-channel/will-delete-recipient]] to let it know we're about to delete this
+  ;; PulseChannelRecipient; that function will decide whether to automatically delete the PulseChannel as well.
+  (classloader/require 'metabase.models.pulse-channel)
+  ((resolve 'metabase.models.pulse-channel/will-delete-recipient) pcr))
+
+(u/strict-extend (class PulseChannelRecipient)
+  models/IModel
+  (merge
+   models/IModelDefaults
+   {:pre-delete pre-delete}))

src/metabase/models/user.clj

@@ -65,34 +65,38 @@
         :user_id  user-id
         :group_id (:id (group/admin))))))
 
-(defn- pre-update [{:keys [email reset_token is_superuser id locale] :as user}]
+(defn- pre-update
+  [{reset-token :reset_token, superuser? :is_superuser, active? :is_active, :keys [email id locale], :as user}]
   ;; when `:is_superuser` is toggled add or remove the user from the 'Admin' group as appropriate
-  (when-not (nil? is_superuser)
+  (when (some? superuser?)
     (let [membership-exists? (db/exists? PermissionsGroupMembership
                                :group_id (:id (group/admin))
                                :user_id  id)]
       (cond
-        (and is_superuser
+        (and superuser?
              (not membership-exists?))
         (db/insert! PermissionsGroupMembership
           :group_id (u/the-id (group/admin))
           :user_id  id)
-
         ;; don't use `delete!` here because that does the opposite and tries to update this user
         ;; which leads to a stack overflow of calls between the two
         ;; TODO - could we fix this issue by using `post-delete!`?
-        (and (not is_superuser)
+        (and (not superuser?)
              membership-exists?)
         (db/simple-delete! PermissionsGroupMembership
           :group_id (u/the-id (group/admin))
           :user_id  id))))
+  ;; make sure email and locale are valid if set
   (when email
     (assert (u/email? email)))
   (when locale
     (assert (i18n/available-locale? locale)))
+  ;; delete all subscriptions to pulses/alerts/etc. if the User is getting archived (`:is_active` status changes)
+  (when (false? active?)
+    (db/delete! 'PulseChannelRecipient :user_id id))
   ;; If we're setting the reset_token then encrypt it before it goes into the DB
   (cond-> user
-    reset_token (update :reset_token creds/hash-bcrypt)
+    reset-token (update :reset_token creds/hash-bcrypt)
     locale      (update :locale i18n/normalized-locale-string)
     email       (update :email u/lower-case-en)))
 

test/metabase/cmd/rotate_encryption_key_test.clj

@@ -13,11 +13,14 @@
             [metabase.models.interface :as interface]
             [metabase.test :as mt]
             [metabase.test.data.interface :as tx]
+            [metabase.test.fixtures :as fixtures]
             [metabase.util.encryption :as encrypt]
             [metabase.util.encryption-test :as eu]
             [toucan.db :as db]
             [toucan.models :as models]))
 
+(use-fixtures :once (fixtures/initialize :db))
+
 (defn do-with-model-type
   [mtype in-type-fns f]
   (let [type-fns        (var-get #'models/type-fns)

test/metabase/models/pulse_test.clj

@@ -2,18 +2,11 @@
   (:require [clojure.test :refer :all]
             [medley.core :as m]
             [metabase.api.common :as api]
-            [metabase.models.card :refer [Card]]
-            [metabase.models.collection :refer [Collection]]
-            [metabase.models.dashboard :refer [Dashboard]]
-            [metabase.models.dashboard-card :refer [DashboardCard]]
-            [metabase.models.database :refer [Database]]
+            [metabase.models :refer [Card Collection Dashboard DashboardCard Database Pulse PulseCard PulseChannel
+                                     PulseChannelRecipient Table User]]
             [metabase.models.interface :as mi]
             [metabase.models.permissions :as perms]
-            [metabase.models.pulse :as pulse :refer [Pulse]]
-            [metabase.models.pulse-card :refer [PulseCard]]
-            [metabase.models.pulse-channel :refer [PulseChannel]]
-            [metabase.models.pulse-channel-recipient :refer [PulseChannelRecipient]]
-            [metabase.models.table :refer [Table]]
+            [metabase.models.pulse :as pulse]
             [metabase.test :as mt]
             [metabase.test.mock.util :refer [pulse-channel-defaults]]
             [metabase.util :as u]
@@ -274,6 +267,84 @@
       (is (= 1
              (count (:cards (pulse/retrieve-pulse (u/the-id pulse)))))))))
 
+(deftest archive-pulse-when-last-user-unsubscribes-test
+  (letfn [(do-with-objects [f]
+            (mt/with-temp* [User                  [{user-id :id}]
+                            Pulse                 [{pulse-id :id}]
+                            PulseChannel          [{pulse-channel-id :id} {:pulse_id pulse-id}]
+                            PulseChannelRecipient [_ {:pulse_channel_id pulse-channel-id, :user_id user-id}]]
+              (f {:user-id          user-id
+                  :pulse-id         pulse-id
+                  :pulse-channel-id pulse-channel-id
+                  :archived?        (fn []
+                                      (db/select-one-field :archived Pulse :id pulse-id))})))]
+    (testing "automatically archive a Pulse when the last user unsubscribes"
+      (testing "one subscriber"
+        (do-with-objects
+         (fn [{:keys [archived? user-id pulse-id]}]
+           (testing "make the User inactive"
+             (is (db/update! User user-id :is_active false)))
+           (testing "Pulse should be archived"
+             (is (archived?))))))
+      (testing "multiple subscribers"
+        (do-with-objects
+         (fn [{:keys [archived? user-id pulse-id pulse-channel-id]}]
+           ;; create a second user + subscription so we can verify that we don't archive the Pulse if a User unsubscribes
+           ;; but there is still another subscription.
+           (mt/with-temp* [User                  [{user-2-id :id}]
+                           PulseChannelRecipient [_ {:pulse_channel_id pulse-channel-id, :user_id user-2-id}]]
+             (is (not (archived?)))
+             (testing "User 1 becomes inactive: Pulse should not be archived yet (because User 2 is still a recipient)"
+               (is (db/update! User user-id :is_active false))
+               (is (not (archived?))))
+             (testing "User 2 becomes inactive: Pulse should now be archived because it has no more recipients"
+               (is (db/update! User user-2-id :is_active false))
+               (is (archived?))
+               (testing "PulseChannel & PulseChannelRecipient rows should have been archived as well."
+                 (is (not (db/exists? PulseChannel :id pulse-channel-id)))
+                 (is (not (db/exists? PulseChannelRecipient :pulse_channel_id pulse-channel-id))))))))))
+    (testing "Don't archive Pulse if it has still has recipients after deleting User subscription\n"
+      (testing "another User subscription exists on a DIFFERENT channel\n"
+        (do-with-objects
+         (fn [{:keys [archived? user-id pulse-id]}]
+           (mt/with-temp* [User                  [{user-2-id :id}]
+                           PulseChannel          [{channel-2-id :id} {:pulse_id pulse-id}]
+                           PulseChannelRecipient [_ {:pulse_channel_id channel-2-id, :user_id user-2-id}]]
+             (testing "make User 1 inactive"
+               (is (db/update! User user-id :is_active false)))
+             (testing "Pulse should not be archived"
+               (is (not (archived?))))))))
+      (testing "still sent to a Slack channel"
+        (do-with-objects
+         (fn [{:keys [archived? user-id pulse-id]}]
+           (mt/with-temp PulseChannel [_ {:channel_type "slack"
+                                          :details      {:channel "#general"}
+                                          :pulse_id     pulse-id}]
+             (testing "make the User inactive"
+               (is (db/update! User user-id :is_active false)))
+             (testing "Pulse should not be archived"
+               (is (not (archived?))))))))
+      (testing "still sent to email addresses\n"
+        (testing "emails on the same channel as deleted User\n"
+          (do-with-objects
+           (fn [{:keys [archived? user-id pulse-id pulse-channel-id]}]
+             (db/update! PulseChannel pulse-channel-id :details {:emails ["foo@bar.com"]})
+             (testing "make the User inactive"
+               (is (db/update! User user-id :is_active false)))
+             (testing "Pulse should not be archived"
+               (is (not (archived?)))))))
+        (testing "emails on a different channel\n"
+          (do-with-objects
+           (fn [{:keys [archived? user-id pulse-id]}]
+             (mt/with-temp PulseChannel [_ {:channel_type "email"
+                                            :details      {:emails ["foo@bar.com"]}
+                                            :pulse_id     pulse-id}]
+               (testing "make the User inactive"
+                 (is (db/update! User user-id :is_active false)))
+               (testing "Pulse should not be archived"
+                 (is (not (archived?))))))))))))
+
+
 ;;; +----------------------------------------------------------------------------------------------------------------+
 ;;; |                                   Pulse Collections Permissions Tests                                          |
 ;;; +----------------------------------------------------------------------------------------------------------------+

test/metabase/models/user_test.clj

@@ -3,15 +3,23 @@
             [clojure.string :as str]
             [clojure.test :refer :all]
             [metabase.http-client :as http]
-            [metabase.models.collection :as collection :refer [Collection]]
+            [metabase.models
+             :refer
+             [Collection
+              Database
+              PermissionsGroup
+              PermissionsGroupMembership
+              Pulse
+              PulseChannel
+              PulseChannelRecipient
+              Session
+              Table
+              User]]
+            [metabase.models.collection :as collection]
             [metabase.models.collection-test :as collection-test]
-            [metabase.models.database :refer [Database]]
             [metabase.models.permissions :as perms]
-            [metabase.models.permissions-group :as group :refer [PermissionsGroup]]
-            [metabase.models.permissions-group-membership :refer [PermissionsGroupMembership]]
-            [metabase.models.session :refer [Session]]
-            [metabase.models.table :refer [Table]]
-            [metabase.models.user :as user :refer [User]]
+            [metabase.models.permissions-group :as group]
+            [metabase.models.user :as user]
             [metabase.test :as mt]
             [metabase.test.data.users :as test-users]
             [metabase.util :as u]
@@ -433,3 +441,21 @@
         (db/update! User user-id :locale "en-GB")
         (is (= "en_GB"
                (db/select-one-field :locale User :id user-id)))))))
+
+(deftest delete-pulse-subscriptions-when-archived-test
+  (testing "Delete a User's Pulse/Alert/Dashboard Subscription subscriptions when they get archived"
+    (mt/with-temp* [User                  [{user-id :id}]
+                    Pulse                 [{pulse-id :id}]
+                    PulseChannel          [{pulse-channel-id :id} {:pulse_id pulse-id}]
+                    PulseChannelRecipient [_ {:pulse_channel_id pulse-channel-id, :user_id user-id}]]
+      (letfn [(subscription-exists? []
+                (db/exists? PulseChannelRecipient :pulse_channel_id pulse-channel-id, :user_id user-id))]
+        (testing "Sanity check: subscription should exist"
+          (is (subscription-exists?)))
+        (testing "user is updated but not archived: don't delete the subscription"
+          (is (db/update! User user-id :is_active true))
+          (is (subscription-exists?)))
+        (testing "archive the user"
+          (is (db/update! User user-id :is_active false)))
+        (testing "subscription should no longer exist"
+          (is (not (subscription-exists?))))))))