Fixed some bugs related to activities

Besides the mentioned issues, this commit also fixes some activities when
deleting users. This commit also fixes most of the issues in #899, but
since this wasn't the goal of this commit, fixing this issue has been
left as something to be done in the near future (I'd still need to
properly review it).

Fixes #1104
Fixes #1106
See #899

Signed-off-by: Miquel Sabaté Solà <msabate@suse.com>

app/controllers/admin/users_controller.rb

@@ -36,29 +36,26 @@ def update
     attr = params.require(:user).permit([:email, :display_name])
 
     if @user.update_attributes(attr)
-      redirect_to admin_users_path, notice: "User '#{@user.username}' was updated successfully",
-                                    float:  true
+      redirect_to admin_users_path,
+                  notice: "User '#{@user.username}' was updated successfully",
+                  float:  true
     else
-      redirect_to edit_admin_user_path(@user), alert: @user.errors.full_messages, float: true
+      redirect_to edit_admin_user_path(@user),
+                  alert: @user.errors.full_messages,
+                  float: true
     end
   end
 
   # DELETE /admin/user/:id
   def destroy
     return if @user.nil?
 
-    PublicActivity::Activity.where(owner_id: @user.id).update_all(
-      owner_id:   nil,
-      owner_type: nil,
-      parameters: { owner_name: @user.username }
-    )
-    @user.create_activity :delete,
-                          owner:      current_user,
-                          parameters: { username: @user.username }
+    @user.update_activities!(current_user)
     @user.destroy!
 
-    redirect_to admin_users_path, notice: "User '#{@user.username}' was removed successfully",
-                                  float:  true
+    redirect_to admin_users_path,
+                notice: "User '#{@user.username}' was removed successfully",
+                float:  true
   end
 
   # PATCH/PUT /admin/user/1/toggle_admin

app/controllers/concerns/change_name_description.rb

@@ -2,38 +2,39 @@ module ChangeNameDescription
   extend ActiveSupport::Concern
 
   # Update the attributes name or description of a ActiveRecord-object.
-  def change_name_description(object, symbol, params)
+  def change_name_description(object, symbol, params, activity_params = {})
     authorize object
 
     new_name = params[:name]
     new_description = params[:description]
     old_description = object.description
     old_name = object.name
 
-    change_description(object, symbol, old_description, new_description)
-    change_name(object, symbol, old_name, new_name)
+    change_description(object, symbol, old_description, new_description, activity_params)
+    change_name(object, symbol, old_name, new_name, activity_params)
   end
 
   private
 
   # Create a PublicActivity for updating an attribute.
-  def create_activity(object, symbol, ctx, old_value, new_value)
-    object.create_activity "change_#{symbol}_#{ctx}".to_sym,
+  def create_activity(object, symbol, old_value, new_value, activity_params)
+    object.create_activity "change_#{symbol}".to_sym,
                                   owner:      current_user,
                                   recipient:  object,
-                                  parameters: { old: old_value, new: new_value }
+                                  parameters: activity_params.merge(old: old_value, new: new_value)
   end
 
   # Change description and track activity if successful.
-  def change_description(object, symbol, old_description, new_description)
+  def change_description(object, symbol, old_description, new_description, activity_params)
     return if new_description.nil? || old_description == new_description ||
         !object.update(description: new_description)
-    create_activity(object, symbol, "description", old_description, new_description)
+    create_activity(object, "#{symbol}_description", old_description,
+                    new_description, activity_params)
   end
 
   # Change name and track activity if successful.
-  def change_name(object, symbol, old_name, new_name)
+  def change_name(object, symbol, old_name, new_name, activity_params)
     return if old_name == new_name || new_name.blank? || !object.update(name: new_name)
-    create_activity(object, symbol, "name", old_name, new_name)
+    create_activity(object, "#{symbol}_name", old_name, new_name, activity_params)
   end
 end

app/controllers/namespaces_controller.rb

@@ -39,7 +39,9 @@ def create
 
     respond_to do |format|
       if @namespace.save
-        @namespace.create_activity :create, owner: current_user
+        @namespace.create_activity :create,
+                                   owner:      current_user,
+                                   parameters: { team: @namespace.team.name }
         @namespaces = policy_scope(Namespace)
         format.js { respond_with @namespace }
       else

app/controllers/team_users_controller.rb

@@ -13,7 +13,9 @@ def create
     @team_user.role = TeamUser.roles[:owner] if @promoted_role
 
     if @team_user.errors.empty? && @team_user.save
-      @team_user.create_activity!(:add_member, current_user)
+      @team_user.create_activity!(:add_member, current_user,
+                                  team_user: @team_user.user.username,
+                                  team:      @team_user.team.name)
       respond_with @team_user
     else
       respond_with @team_user.errors, status: :unprocessable_entity
@@ -34,14 +36,19 @@ def update
     old_role = @team_user.role
     @team_user.update(team_user_params)
     @team_user.create_activity! :change_member_role, current_user,
-      old_role: old_role, new_role: team_user_params["role"]
+                                old_role:  old_role,
+                                new_role:  team_user_params["role"],
+                                team:      @team_user.team.name,
+                                team_user: @team_user.user.username
 
     respond_on_update_or_destroy!("/teams/#{@team_user.team.id}")
   end
 
   # DELETE /team_users/1
   def destroy
-    @team_user.create_activity!(:remove_member, current_user)
+    @team_user.create_activity!(:remove_member, current_user,
+                                team_user: @team_user.user.username,
+                                team:      @team_user.team.name)
     @team_user.destroy
 
     respond_on_update_or_destroy!("/teams")

app/controllers/teams_controller.rb

@@ -29,7 +29,9 @@ def create
     @team.owners << current_user
 
     if @team.save
-      @team.create_activity :create, owner: current_user
+      @team.create_activity(:create,
+                            owner:      current_user,
+                            parameters: { team: @team.name })
       respond_with(@team)
     else
       respond_with @team.errors, status: :unprocessable_entity
@@ -40,7 +42,7 @@ def create
   # PATCH/PUT /teams/1.json
   def update
     p = params.require(:team).permit(:name, :description)
-    change_name_description(@team, :team, p)
+    change_name_description(@team, :team, p, team: @team.name)
   end
 
   # GET /teams/1/typeahead/%QUERY

app/helpers/activities_helper.rb

@@ -2,12 +2,43 @@
 module ActivitiesHelper
   # Returns a string containing the username of the owner of the activity.
   def activity_owner(activity)
-    if activity.owner
-      activity.owner.display_username
-    elsif activity.parameters[:owner_name].blank?
-      "Someone"
+    activity_user(activity, :owner, :owner_name, "Someone")
+  end
+
+  # Returns a string containing the username of the recipient of the activity.
+  def activity_user_recipient(activity, param)
+    activity_user(activity, :recipient, param, "a user")
+  end
+
+  # Returns a string containing the name of the team of the activity. If the
+  # `nested` parameter is provided, then it's supposed that the trackable has a
+  # nested `team` method that has to be called to obtain the name.
+  def activity_team(activity, nested = nil)
+    if activity.parameters[:team]
+      activity.parameters[:team]
+    elsif nested
+      activity.trackable.team.name
+    else
+      activity.trackable.name
+    end
+  end
+
+  protected
+
+  # Returns a string that represents the user of the given activity. The `method`
+  # parameter is used as the preferrable source of truth (e.g. `:owner` means to
+  # execute `activity.owner`). If this source is not available (e.g. the given
+  # resource has been removed), then the given `param` parameter is called
+  # (e.g. activity.parameters[:owner_name] if param was set to
+  # `:owner_name`). If that doesn't work either, then the given `empty_user`
+  # parameter is returned as-is.
+  def activity_user(activity, method, param, empty_user)
+    if activity.send(method)
+      activity.send(method).display_username
+    elsif activity.parameters[param].blank?
+      empty_user
     else
-      activity.parameters[:owner_name]
+      activity.parameters[param]
     end
   end
 end

app/helpers/teams_helper.rb

@@ -20,7 +20,7 @@ def role_within_team(team)
 
   # Render the namespace scope icon.
   def team_scope_icon(team)
-    if team.team_users.count > 1
+    if team.team_users.enabled.count > 1
       icon = "fa-users"
       title = "Team"
     else

app/models/team_user.rb

@@ -38,7 +38,7 @@ class TeamUser < ActiveRecord::Base
   # Create the activity regarding this team member. If no parameters are
   # specified, then it's assumed: { role: role }.
   def create_activity!(type, owner, parameters = nil)
-    params = parameters || { role: role }
+    params = parameters ? parameters.merge(role: role) : { role: role }
     team.create_activity type, owner: owner, recipient: user, parameters: params
   end
 

app/models/user.rb

@@ -192,6 +192,27 @@ def update_tags!
     Tag.where(user_id: id).update_all(user_id: nil, username: username)
   end
 
+  # Update the activities owned by this user. This method should only be called
+  # before destroying this user.
+  def update_activities!(owner)
+    # Originally this was handled in a single query, but with that is was not
+    # possible to fix a bug as specified in PR #1144. Now it's handled in a
+    # block that ends up performing multiple queries, which we want to perform
+    # atomically (thus the transaction).
+    ActiveRecord::Base.transaction do
+      PublicActivity::Activity.where(owner_id: id).find_each do |a|
+        a.owner_id   = nil
+        a.owner_type = nil
+        a.parameters = a.parameters.merge(owner_name: display_username)
+        a.save
+      end
+    end
+
+    create_activity :delete,
+                    owner:      owner,
+                    parameters: { username: username }
+  end
+
   protected
 
   # Returns whether the given user can be disabled or not. The following rules

app/views/public_activity/namespace/_change_team.csv.slim

@@ -1 +1 @@
-= CSV.generate_line(['namespace', activity.trackable.name, 'change team', '-', activity.owner.username, activity.created_at, "owned by team #{activity.trackable.team.name}"])
+= CSV.generate_line(['namespace', activity.trackable.name, 'change team', '-', activity.owner.username, activity.created_at, "owned by team #{activity_team(activity, true)}"])

app/views/public_activity/namespace/_change_team.html.slim

@@ -10,7 +10,7 @@ li
           = "#{activity.owner.username} moved the "
         = link_to activity.trackable.name , activity.trackable
         = " namespace to the "
-        = link_to activity.trackable.team.name, activity.trackable.team
+        = link_to activity_team(activity, true), activity.trackable
         = " team"
       small
         i.fa.fa-clock-o

app/views/public_activity/namespace/_create.csv.slim

@@ -1 +1 @@
-= CSV.generate_line(['namespace', activity.trackable.name, 'create', '-', activity_owner(activity), activity.created_at, "owned by team #{activity.trackable.team.name}"])
+= CSV.generate_line(['namespace', activity.trackable.name, 'create', '-', activity_owner(activity), activity.created_at, "owned by team #{activity_team(activity, true)}"])

app/views/public_activity/namespace/_create.html.slim

@@ -10,7 +10,7 @@ li
           = "#{activity_owner(activity)} created the "
         = link_to activity.trackable.name , activity.trackable
         = " namespace under the "
-        = link_to activity.trackable.team.name, activity.trackable.team
+        = link_to activity_team(activity, true), activity.trackable.team
         = " team"
       small
         i.fa.fa-clock-o

app/views/public_activity/namespace/_delete.csv.slim

@@ -0,0 +1,4 @@
+- if activity.parameters[:repository_name]
+  = CSV.generate_line(['repository', "#{activity.trackable.clean_name}", 'delete repository', "#{activity.parameters[:repository_name]}", activity_owner(activity), activity.created_at, "-"])
+- else
+  = CSV.generate_line(['repository', "#{activity.trackable.clean_name}", 'delete repository', "-", activity_owner(activity), activity.created_at, "-"])

app/views/public_activity/team/_add_member.csv.slim

@@ -1 +1,6 @@
-= CSV.generate_line(['team', activity.trackable.name, 'add member', activity.recipient.display_username, activity_owner(activity), activity.created_at, "role #{activity.parameters[:role]}"])
+- if activity.recipient
+  = CSV.generate_line(['team', activity_team(activity), 'add member', activity.recipient.display_username, activity_owner(activity), activity.created_at, "role #{activity.parameters[:role]}"])
+- elsif activity.parameters[:team_user]
+  = CSV.generate_line(['team', activity_team(activity), 'add member', activity.parameters[:team_user], activity_owner(activity), activity.created_at, "role #{activity.parameters[:role]}"])
+- else
+  = CSV.generate_line(['team', activity_team(activity), 'add member', "a user", activity_owner(activity), activity.created_at, "role #{activity.parameters[:role]}"])

app/views/public_activity/team/_add_member.html.slim

@@ -7,9 +7,15 @@ li
     .description
       h6
         strong
-          = "#{activity_owner(activity)} added user #{activity.recipient.display_username} "
+          - if activity.recipient
+            = "#{activity_owner(activity)} added user #{activity.recipient.display_username} "
+          - else
+            - if activity.parameters[:team_user]
+              = "#{activity_owner(activity)} added user #{activity.parameters[:team_user]} "
+            - else
+              = "#{activity_owner(activity)} added a user "
         = "with role #{activity.parameters[:role]} to the "
-        = link_to activity.trackable.name, activity.trackable
+        = link_to activity_team(activity), activity.trackable
         |  team
       small
         i.fa.fa-clock-o

app/views/public_activity/team/_change_member_role.csv.slim

@@ -1,8 +1 @@
-= CSV.generate_line(['team',
-                      activity.trackable.name,
-                      'change member role',
-                      activity.recipient.display_username,
-                      activity_owner(activity),
-                      activity.created_at,
-                      "from #{activity.parameters[:old_role]} to \
-                      #{activity.parameters[:new_role]}"])
+= CSV.generate_line(['team', activity_team(activity), 'change member role', activity_user_recipient(activity, :team_user), activity_owner(activity), activity.created_at, "from #{activity.parameters[:old_role]} to #{activity.parameters[:new_role]}"])

app/views/public_activity/team/_change_member_role.html.slim

@@ -7,13 +7,15 @@ li
     .description
       h6
         strong
-          - if activity.owner == activity.recipient
-            = "#{activity_owner(activity)} changed its role "
+          - if activity.parameters[:team_user] && activity.parameters[:owner_name]
+            = "#{activity_owner(activity)} changed the role of #{activity_user_recipient(activity, :team_user)} "
+          - elsif activity.owner == activity.recipient
+            = "#{activity_owner(activity)} changed their role "
           - else
-            = "#{activity_owner(activity)} changed role of user #{activity.recipient.display_username} "
+            = "#{activity_owner(activity)} changed the role of #{activity_user_recipient(activity, :team_user)} "
         = "from #{activity.parameters[:old_role]} to #{activity.parameters[:new_role]} "
         = "within the team "
-        = link_to activity.trackable.name, activity.trackable
+        = link_to activity_team(activity), activity.trackable
       small
         i.fa.fa-clock-o
         = activity_time_tag activity.created_at

app/views/public_activity/team/_change_team_description.csv.slim

@@ -1,8 +1 @@
-= CSV.generate_line(['team',
-                     activity.trackable.name,
-                     'change the description of the team',
-                     activity.recipient.name,
-                     activity_owner(activity),
-                     activity.created_at,
-                     "from #{activity.parameters[:old_description]} to \
-                     #{activity.parameters[:new_description]}"])
+= CSV.generate_line(['team', activity_team(activity), 'change the description of the team', activity.recipient.name, activity_owner(activity), activity.created_at, "from #{activity.parameters[:old_description]} to #{activity.parameters[:new_description]}"])

app/views/public_activity/team/_change_team_description.html.slim

@@ -12,7 +12,7 @@ li
         - else
           strong
             = "#{activity_owner(activity)} edited the description of the team "
-        =  link_to activity.trackable.name, activity.trackable
+        =  link_to activity_team(activity), activity.trackable
       small
         i.fa.fa-clock-o
         = activity_time_tag activity.created_at

app/views/public_activity/team/_change_team_name.csv.slim

@@ -1,8 +1 @@
-= CSV.generate_line(['team',
-                     activity.trackable.name,
-                     'change the name of the team',
-                     activity.recipient.name,
-                     activity_owner(activity),
-                     activity.created_at,
-                     "from #{activity.parameters[:old_name]} to \
-                     #{activity.parameters[:new_name]}"])
+= CSV.generate_line(['team', activity_team(activity), 'change the name of the team', activity.recipient.name, activity_owner(activity), activity.created_at, "from #{activity.parameters[:old_name]} to #{activity.parameters[:new_name]}"])

app/views/public_activity/team/_create.csv.slim

@@ -1 +1 @@
-= CSV.generate_line(['team', activity.trackable.name, 'create', '-', activity_owner(activity), activity.created_at, '-'])
+= CSV.generate_line(['team', activity_team(activity), 'create', '-', activity_owner(activity), activity.created_at, '-'])

app/views/public_activity/team/_create.html.slim

@@ -8,7 +8,7 @@ li
       h6
         strong
           = "#{activity_owner(activity)} created team "
-        = link_to activity.trackable.name, activity.trackable
+        = link_to activity_team(activity), activity.trackable
       small
         i.fa.fa-clock-o
         = activity_time_tag activity.created_at

app/views/public_activity/team/_remove_member.csv.slim

@@ -1 +1 @@
-= CSV.generate_line(['team', activity.trackable.name, 'remove member', activity.recipient.display_username, activity_owner(activity), activity.created_at, "role #{activity.parameters[:role]}"])
+= CSV.generate_line(['team', activity_team(activity), 'remove member', activity.recipient.display_username, activity_owner(activity), activity.created_at, "role #{activity.parameters[:role]}"])

app/views/public_activity/team/_remove_member.html.slim

@@ -12,7 +12,7 @@ li
           - else
             = "#{activity_owner(activity)} removed user #{activity.recipient.display_username} "
         = "from the "
-        = link_to activity.trackable.name, activity.trackable
+        = link_to activity_team(activity), activity.trackable
         |  team
       small
         i.fa.fa-clock-o

app/views/public_activity/user/_delete.csv.slim

@@ -0,0 +1 @@
+= CSV.generate_line(['user', "-", 'removed user', activity.parameters[:username], activity_owner(activity), activity.created_at, "role #{activity.parameters[:role]}"])

app/views/public_activity/webhook/_create.csv.slim

@@ -1,4 +1,4 @@
 - if activity.trackable
-  = CSV.generate_line(['webhook', activity.trackable.url, 'create', '-', activity.owner.username, activity.created_at, "owned by namespace #{activity.trackable.namespace.name}"])
+  = CSV.generate_line(['webhook', activity.trackable.url, 'create', '-', activity_owner(activity), activity.created_at, "owned by namespace #{activity.trackable.namespace.name}"])
 - else
-  = CSV.generate_line(['webhook', activity.parameters[:webhook_host], 'create', '-', activity.owner.username, activity.created_at, "owned by namespace #{activity.parameters[:namespace_name]}"])
+  = CSV.generate_line(['webhook', activity.parameters[:webhook_host], 'create', '-', activity_owner(activity), activity.created_at, "owned by namespace #{activity.parameters[:namespace_name]}"])

app/views/public_activity/webhook/_destroy.csv.slim

@@ -1 +1 @@
-= CSV.generate_line(["webhook", activity.parameters[:webhook_host], "destroy", "-", activity.owner.username, activity.created_at, "owned by namespace #{activity.parameters[:namespace_name]}"])
+= CSV.generate_line(["webhook", activity.parameters[:webhook_host], "destroy", "-", activity_owner(activity), activity.created_at, "owned by namespace #{activity.parameters[:namespace_name]}"])