Merge pull request #2452 from metabase/fix-duplicate-join-activity-entry

Fix duplicate "user joined" activity feed entries

src/metabase/api/session.clj

@@ -45,7 +45,7 @@
       (throw (ex-info "Password did not match stored password." {:status-code 400
                                                                  :errors      {:password "did not match stored password"}})))
     (let [session-id (create-session (:id user))]
-      (events/publish-event :user-login {:user_id (:id user) :session_id session-id})
+      (events/publish-event :user-login {:user_id (:id user), :session_id session-id, :first_login (not (boolean (:last_login user)))})
       {:id session-id})))
 
 
@@ -85,30 +85,30 @@
   "Number of milliseconds a password reset is considered valid."
   (* 48 60 60 1000)) ; token considered valid for 48 hours
 
-(defn- valid-reset-token->user-id
+(defn- valid-reset-token->user
   "Check if a password reset token is valid. If so, return the `User` ID it corresponds to."
   [^String token]
   (when-let [[_ user-id] (re-matches #"(^\d+)_.+$" token)]
     (let [user-id (Integer/parseInt user-id)]
-      (when-let [{:keys [reset_token reset_triggered]} (sel :one :fields [User :reset_triggered :reset_token] :id user-id)]
+      (when-let [{:keys [reset_token reset_triggered], :as user} (sel :one :fields [User :id :last_login :reset_triggered :reset_token] :id user-id)]
         ;; Make sure the plaintext token matches up with the hashed one for this user
         (when (try (creds/bcrypt-verify token reset_token)
                    (catch Throwable _))
           ;; check that the reset was triggered within the last 48 HOURS, after that the token is considered expired
           (let [token-age (- (System/currentTimeMillis) reset_triggered)]
             (when (< token-age reset-token-ttl-ms)
-              user-id)))))))
+              user)))))))
 
 (defendpoint POST "/reset_password"
   "Reset password with a reset token."
   [:as {{:keys [token password]} :body}]
   {token    Required
    password [Required ComplexPassword]}
-  (or (when-let [user-id (valid-reset-token->user-id token)]
+  (or (when-let [{user-id :id, :as user} (valid-reset-token->user token)]
         (set-user-password user-id password)
         ;; after a successful password update go ahead and offer the client a new session that they can use
         (let [session-id (create-session user-id)]
-          (events/publish-event :user-login {:user_id user-id :session_id session-id})
+          (events/publish-event :user-login {:user_id user-id, :session_id session-id, :first_login (not (boolean (:last_login user)))})
           {:success    true
            :session_id session-id}))
       (throw (invalid-param-exception :password "Invalid reset token"))))
@@ -118,7 +118,7 @@
   "Check is a password reset token is valid and isn't expired."
   [token]
   {token Required}
-  {:valid (boolean (valid-reset-token->user-id token))})
+  {:valid (boolean (valid-reset-token->user token))})
 
 
 (defendpoint GET "/properties"

src/metabase/api/setup.clj

@@ -57,7 +57,7 @@
       :user_id (:id new-user))
     ;; notify that we've got a new user in the system AND that this user logged in
     (events/publish-event :user-create {:user_id (:id new-user)})
-    (events/publish-event :user-login {:user_id (:id new-user) :session_id session-id})
+    (events/publish-event :user-login {:user_id (:id new-user), :session_id session-id, :first_login true})
     {:id session-id}))
 
 

src/metabase/events/activity_feed.clj

@@ -101,7 +101,7 @@
 (defn- process-user-activity [topic object]
   ;; we only care about login activity when its the users first session (a.k.a. new user!)
   (when (and (= :user-login topic)
-             (= (:session_id object) (first-session-for-user (:user_id object))))
+             (:first_login object))
     (activity/record-activity
       :topic    :user-joined
       :user-id  (:user_id object)

src/metabase/events/last_login.clj

@@ -25,7 +25,6 @@
   ;; try/catch here to prevent individual topic processing exceptions from bubbling up.  better to handle them here.
   (try
     (when-let [{object :item} last-login-event]
-      (log/info object)
       ;; just make a simple attempt to set the `:last_login` for the given user to now
       (when-let [user-id (:user_id object)]
         (db/upd User user-id :last_login (u/new-sql-timestamp))))

src/metabase/models/session.clj

 (ns metabase.models.session
   (:require [korma.core :as k]
-            [metabase.db :refer [sel]]
+            [metabase.db :as db]
             (metabase.models [interface :as i]
                              [user :refer [User]])
             [metabase.util :as u]))
@@ -22,4 +22,4 @@
   "Retrieves the first Session `:id` for a given user (if available), or nil otherwise."
   [user-id]
   {:pre [(integer? user-id)]}
-  (sel :one :id Session, :user_id user-id, (k/order :created_at :ASC)))
+  (db/sel :one :field [Session :id], :user_id user-id, (k/order :created_at :ASC)))

test/metabase/events/activity_feed_test.clj

@@ -428,7 +428,8 @@
   (do
     (k/delete Activity)
     (process-activity-event {:topic :user-login
-                             :item  {:user_id    user-id
-                                     :session_id session-id}})
+                             :item  {:user_id     user-id
+                                     :session_id  session-id
+                                     :first_login true}})
     (-> (db/sel :one Activity :topic "user-joined")
         (select-keys [:topic :user_id :model :model_id :details]))))

test/metabase/models/session_test.clj

@@ -16,7 +16,10 @@
     (k/insert Session
               (k/values [{:id         "the-greatest-day-ever"
                           :user_id    user-id
-                          :created_at (metabase.util/->Timestamp "1980-10-19")}
+                          :created_at (metabase.util/->Timestamp "1980-10-19T05:05:05.000Z")}
+                         {:id         "even-more-greatness"
+                          :user_id    user-id
+                          :created_at (metabase.util/->Timestamp "1980-10-19T05:08:05.000Z")}
                          {:id         "the-world-of-bi-changes-forever"
                           :user_id    user-id
                           :created_at (metabase.util/->Timestamp "2015-10-21")}

test_resources/log4j.properties

@@ -16,3 +16,4 @@ log4j.appender.file.layout.ConversionPattern=%d [%t] %-5p%c - %m%n
 
 # customizations to logging by package
 log4j.logger.metabase=INFO
+log4j.logger.metabase.sync-database=WARN