Fix failing tests after #16749 (#17274)

* Revert changes to activity and activity-test namespaces

* Add some sanity-checks to help debug the failing test

* Delete accidental newline

src/metabase/api/activity.clj

@@ -7,7 +7,6 @@
             [metabase.models.dashboard :refer [Dashboard]]
             [metabase.models.interface :as mi]
             [metabase.models.view-log :refer [ViewLog]]
-            [metabase.server.middleware.offset-paging :as offset-paging]
             [toucan.db :as db]
             [toucan.hydrate :refer [hydrate]]))
 
@@ -62,11 +61,9 @@
 (defendpoint GET "/"
   "Get recent activity."
   []
-  (filter mi/can-read? (-> (db/select Activity {:order-by [[:timestamp :desc]]
-                                                :limit    offset-paging/*limit*
-                                                :offset   offset-paging/*offset*})
-                           (hydrate :user :table :database)
-                           add-model-exists-info)))
+  (filter mi/can-read? (-> (db/select Activity, {:order-by [[:timestamp :desc]], :limit 40})
+                               (hydrate :user :table :database)
+                               add-model-exists-info)))
 
 (defn- view-log-entry->matching-object [{:keys [model model_id]}]
   (when (contains? #{"card" "dashboard"} model)

test/metabase/api/activity_test.clj

@@ -6,6 +6,7 @@
             [metabase.models.activity :refer [Activity]]
             [metabase.models.card :refer [Card]]
             [metabase.models.dashboard :refer [Dashboard]]
+            [metabase.models.interface :as models]
             [metabase.models.view-log :refer [ViewLog]]
             [metabase.test :as mt]
             [metabase.test.fixtures :as fixtures]
@@ -72,7 +73,7 @@
                   :user    nil})]
                ;; remove other activities from the API response just in case -- we're not interested in those
                (let [these-activity-ids (set (map u/the-id [activity1 activity2 activity3]))]
-                 (for [activity (mt/user-http-request :crowberto :get 200 "activity" :limit Integer/MAX_VALUE)
+                 (for [activity (mt/user-http-request :crowberto :get 200 "activity")
                        :when    (contains? these-activity-ids (u/the-id activity))]
                    (dissoc activity :timestamp)))))))))
 
@@ -191,21 +192,25 @@
                                                    :details  {:dashcards [{:card_id card-id}
                                                                           {:card_id 0}]}}])))))
 
-(defn- user-can-see-user-joined-activity? [user]
+(deftest activity-visibility-test
   ;; clear out all existing Activity entries
+  ;;
+  ;; TODO -- this is a bad pattern -- test shouldn't wipe out a bunch of other stuff like this. Just fetch all the
+  ;; activities and look for the presence of this specific one.
   (db/delete! Activity)
-  (-> (mt/with-temp Activity [activity {:topic     "user-joined"
-                                        :details   {}
-                                        :timestamp #t "2019-02-15T11:55:00.000Z"}]
-        (mt/user-http-request user :get 200 "activity"))
-      seq
-      boolean))
-
-(deftest activity-visibility-test
-  (testing "Only admins should get to see user-joined activities"
-    (testing "admin should see `:user-joined` activities"
-      (is (= true
-             (user-can-see-user-joined-activity? :crowberto))))
-    (testing "non-admin should *not* see `:user-joined` activities"
-      (is (= false
-             (user-can-see-user-joined-activity? :rasta))))))
+  (mt/with-temp Activity [activity {:topic     "user-joined"
+                                    :details   {}
+                                    :timestamp #t "2019-02-15T11:55:00.000Z"}]
+    (letfn [(user-can-see-user-joined-activity? [user]
+              (seq (mt/user-http-request user :get 200 "activity")))]
+      (testing "Only admins should get to see user-joined activities"
+        (testing "admin should see `:user-joined` activities"
+          (testing "Sanity check: admin should be able to read the activity"
+            (mt/with-test-user :crowberto
+              (is (models/can-read? activity))))
+          (is (user-can-see-user-joined-activity? :crowberto)))
+        (testing "non-admin should *not* see `:user-joined` activities"
+          (testing "Sanity check: non-admin should *not* be able to read the activity"
+            (mt/with-test-user :rasta
+              (is (not (models/can-read? activity)))))
+          (is (not (user-can-see-user-joined-activity? :rasta))))))))