Merge branch 'events-cache-invalidation' into 'master'

Remove caching of events data

This MR removes the caching of events data as this was deemed unnecessary while increasing load on the database. See https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/6578#note_18864037 and 5371da34 for more information.

See merge request !6578

app/controllers/profiles/avatars_controller.rb

@@ -4,7 +4,6 @@ class Profiles::AvatarsController < Profiles::ApplicationController
     @user.remove_avatar!
 
     @user.save
-    @user.reset_events_cache
 
     redirect_to profile_path
   end

app/controllers/projects/avatars_controller.rb

@@ -20,7 +20,6 @@ class Projects::AvatarsController < Projects::ApplicationController
     @project.remove_avatar!
 
     @project.save
-    @project.reset_events_cache
 
     redirect_to edit_project_path(@project)
   end

app/models/event.rb

@@ -43,12 +43,6 @@ class Event < ActiveRecord::Base
   scope :for_milestone_id, ->(milestone_id) { where(target_type: "Milestone", target_id: milestone_id) }
 
   class << self
-    def reset_event_cache_for(target)
-      Event.where(target_id: target.id, target_type: target.class.to_s).
-        order('id DESC').limit(100).
-        update_all(updated_at: Time.now)
-    end
-
     # Update Gitlab::ContributionsCalendar#activity_dates if this changes
     def contributions
       where("action = ? OR (target_type in (?) AND action in (?))",
@@ -353,6 +347,10 @@ class Event < ActiveRecord::Base
       update_all(last_activity_at: created_at)
   end
 
+  def authored_by?(user)
+    user ? author_id == user.id : false
+  end
+
   private
 
   def recent_update?

app/models/issue.rb

@@ -182,18 +182,6 @@ class Issue < ActiveRecord::Base
     branches_with_iid - branches_with_merge_request
   end
 
-  # Reset issue events cache
-  #
-  # Since we do cache @event we need to reset cache in special cases:
-  # * when an issue is updated
-  # Events cache stored like  events/23-20130109142513.
-  # The cache key includes updated_at timestamp.
-  # Thus it will automatically generate a new fragment
-  # when the event is updated because the key changes.
-  def reset_events_cache
-    Event.reset_event_cache_for(self)
-  end
-
   # To allow polymorphism with MergeRequest.
   def source_project
     project

app/models/merge_request.rb

@@ -605,18 +605,6 @@ class MergeRequest < ActiveRecord::Base
     self.target_project.repository.branch_names.include?(self.target_branch)
   end
 
-  # Reset merge request events cache
-  #
-  # Since we do cache @event we need to reset cache in special cases:
-  # * when a merge request is updated
-  # Events cache stored like  events/23-20130109142513.
-  # The cache key includes updated_at timestamp.
-  # Thus it will automatically generate a new fragment
-  # when the event is updated because the key changes.
-  def reset_events_cache
-    Event.reset_event_cache_for(self)
-  end
-
   def merge_commit_message
     message = "Merge branch '#{source_branch}' into '#{target_branch}'\n\n"
     message << "#{title}\n\n"

app/models/note.rb

@@ -201,19 +201,6 @@ class Note < ActiveRecord::Base
     super(noteable_type.to_s.classify.constantize.base_class.to_s)
   end
 
-  # Reset notes events cache
-  #
-  # Since we do cache @event we need to reset cache in special cases:
-  # * when a note is updated
-  # * when a note is removed
-  # Events cache stored like  events/23-20130109142513.
-  # The cache key includes updated_at timestamp.
-  # Thus it will automatically generate a new fragment
-  # when the event is updated because the key changes.
-  def reset_events_cache
-    Event.reset_event_cache_for(self)
-  end
-
   def editable?
     !system?
   end

app/models/project.rb

@@ -976,7 +976,6 @@ class Project < ActiveRecord::Base
       begin
         gitlab_shell.mv_repository(repository_storage_path, "#{old_path_with_namespace}.wiki", "#{new_path_with_namespace}.wiki")
         send_move_instructions(old_path_with_namespace)
-        reset_events_cache
 
         @old_path_with_namespace = old_path_with_namespace
 
@@ -1043,22 +1042,6 @@ class Project < ActiveRecord::Base
     attrs
   end
 
-  # Reset events cache related to this project
-  #
-  # Since we do cache @event we need to reset cache in special cases:
-  # * when project was moved
-  # * when project was renamed
-  # * when the project avatar changes
-  # Events cache stored like  events/23-20130109142513.
-  # The cache key includes updated_at timestamp.
-  # Thus it will automatically generate a new fragment
-  # when the event is updated because the key changes.
-  def reset_events_cache
-    Event.where(project_id: self.id).
-      order('id DESC').limit(100).
-      update_all(updated_at: Time.now)
-  end
-
   def project_member(user)
     project_members.find_by(user_id: user)
   end

app/models/user.rb

@@ -708,20 +708,6 @@ class User < ActiveRecord::Base
       project.project_member(self)
   end
 
-  # Reset project events cache related to this user
-  #
-  # Since we do cache @event we need to reset cache in special cases:
-  # * when the user changes their avatar
-  # Events cache stored like  events/23-20130109142513.
-  # The cache key includes updated_at timestamp.
-  # Thus it will automatically generate a new fragment
-  # when the event is updated because the key changes.
-  def reset_events_cache
-    Event.where(author_id: id).
-      order('id DESC').limit(1000).
-      update_all(updated_at: Time.now)
-  end
-
   def full_website_url
     return "http://#{website_url}" if website_url !~ /\Ahttps?:\/\//
 

app/services/issuable_base_service.rb

@@ -184,8 +184,6 @@ class IssuableBaseService < BaseService
     params[:label_ids] = process_label_ids(params, existing_label_ids: issuable.label_ids)
 
     if params.present? && update_issuable(issuable, params)
-      issuable.reset_events_cache
-
       # We do not touch as it will affect a update on updated_at field
       ActiveRecord::Base.no_touching do
         handle_common_system_notes(issuable, old_labels: old_labels)

app/services/notes/delete_service.rb

@@ -2,7 +2,6 @@ module Notes
   class DeleteService < BaseService
     def execute(note)
       note.destroy
-      note.reset_events_cache
     end
   end
 end

app/services/notes/update_service.rb

@@ -5,7 +5,6 @@ module Notes
 
       note.update_attributes(params.merge(updated_by: current_user))
       note.create_new_cross_references!(current_user)
-      note.reset_events_cache
 
       if note.previous_changes.include?('note')
         TodoService.new.update_note(note, current_user)

app/services/projects/transfer_service.rb

@@ -61,9 +61,6 @@ module Projects
         # Move missing group labels to project
         Labels::TransferService.new(current_user, old_group, project).execute
 
-        # clear project cached events
-        project.reset_events_cache
-
         # Move uploads
         Gitlab::UploadsTransfer.new.move_project(project.path, old_namespace.path, new_namespace.path)
 

app/uploaders/avatar_uploader.rb

@@ -3,16 +3,10 @@ class AvatarUploader < CarrierWave::Uploader::Base
 
   storage :file
 
-  after :store, :reset_events_cache
-
   def store_dir
     "uploads/#{model.class.to_s.underscore}/#{mounted_as}/#{model.id}"
   end
 
-  def reset_events_cache(file)
-    model.reset_events_cache if model.is_a?(User)
-  end
-
   def exists?
     model.avatar.file && model.avatar.file.exists?
   end

app/views/events/_event.html.haml

@@ -3,7 +3,6 @@
     .event-item-timestamp
       #{time_ago_with_tooltip(event.created_at)}
 
-    = cache [event, current_application_settings, "v2.2"] do
     = author_avatar(event, size: 40)
 
     - if event.created_project?

app/views/events/event/_push.html.haml

@@ -18,7 +18,7 @@
       - few_commits.each do |commit|
         = render "events/commit", commit: commit, project: project, event: event
 
-      - create_mr = event.new_ref? && create_mr_button?(project.default_branch, event.ref_name, project)
+      - create_mr = event.new_ref? && create_mr_button?(project.default_branch, event.ref_name, project) && event.authored_by?(current_user)
       - if event.commits_count > 1
         %li.commits-stat
           - if event.commits_count > 2
@@ -35,12 +35,12 @@
             Compare #{from_label}...#{truncate_sha(event.commit_to)}
 
           - if create_mr
-            %span{"data-user-is" => event.author_id, "data-display" => "inline"}
+            %span
               or
               = link_to create_mr_path(project.default_branch, event.ref_name, project) do
                 create a merge request
       - elsif create_mr
-        %li.commits-stat{"data-user-is" => event.author_id}
+        %li.commits-stat
           = link_to create_mr_path(project.default_branch, event.ref_name, project) do
             Create Merge Request
 - elsif event.rm_ref?

app/views/layouts/_head.html.haml

@@ -56,5 +56,3 @@
   = render 'layouts/google_analytics' if extra_config.has_key?('google_analytics_id')
   = render 'layouts/piwik' if extra_config.has_key?('piwik_url') && extra_config.has_key?('piwik_site_id')
   = render 'layouts/bootlint' if Rails.env.development?
-
-  = render 'layouts/user_styles'

app/views/layouts/_user_styles.html.haml

-:css
-  [data-user-is] {
-    display: none !important;
-  }
-
-  [data-user-is="#{current_user.try(:id)}"] {
-    display: block !important;
-  }
-
-  [data-user-is="#{current_user.try(:id)}"][data-display="inline"] {
-    display: inline !important;
-  }
-
-  [data-user-is-not] {
-    display: block !important;
-  }
-
-  [data-user-is-not][data-display="inline"] {
-    display: inline !important;
-  }
-
-  [data-user-is-not="#{current_user.try(:id)}"] {
-    display: none !important;
-  }

changelogs/unreleased/events-cache-invalidation.yml

+---
+title: Remove caching of events data
+merge_request: 6578
+author: 

spec/models/event_spec.rb

@@ -260,6 +260,24 @@ describe Event, models: true do
     end
   end
 
+  describe '#authored_by?' do
+    let(:event) { build(:event) }
+
+    it 'returns true when the event author and user are the same' do
+      expect(event.authored_by?(event.author)).to eq(true)
+    end
+
+    it 'returns false when passing nil as an argument' do
+      expect(event.authored_by?(nil)).to eq(false)
+    end
+
+    it 'returns false when the given user is not the author of the event' do
+      user = double(:user, id: -1)
+
+      expect(event.authored_by?(user)).to eq(false)
+    end
+  end
+
   def create_event(project, user, attrs = {})
     data = {
       before: Gitlab::Git::BLANK_SHA,

spec/views/layouts/_head.html.haml_spec.rb

 require 'spec_helper'
 
 describe 'layouts/_head' do
-  before do
-    stub_template 'layouts/_user_styles.html.haml' => ''
-  end
-
   it 'escapes HTML-safe strings in page_title' do
     stub_helper_with_safe_string(:page_title)