Refactor EventFilter and increase its test coverage

Signed-off-by: Rémy Coutable <remy@rymai.me>

app/controllers/application_controller.rb

@@ -273,10 +273,10 @@ class ApplicationController < ActionController::Base
   end
   def event_filter
-    # Split using comma to maintain backward compatibility Ex/ "filter1,filter2"
-    filters = cookies['event_filter'].split(',')[0] if cookies['event_filter'].present?
-    filters = params[:event_filter].split(',')[0] if params[:event_filter].present?
-    @event_filter ||= EventFilter.new(filters)
+    @event_filter ||=
+      EventFilter.new(params[:event_filter].presence || cookies[:event_filter]).tap do |new_event_filter|
+        cookies[:event_filter] = new_event_filter.filter
+      end
   end
   # JSON for infinite scroll via Pager object

app/controllers/dashboard_controller.rb

@@ -40,7 +40,7 @@ class DashboardController < Dashboard::ApplicationController
       end
     @events = EventCollection
-      .new(projects, offset: params[:offset].to_i, filter: @event_filter)
+      .new(projects, offset: params[:offset].to_i, filter: event_filter)
       .to_a
     Events::RenderService.new(current_user).execute(@events)

app/views/shared/_event_filter.html.haml

@@ -2,13 +2,13 @@
   .fade-left= icon('angle-left')
   .fade-right= icon('angle-right')
   %ul.nav-links.event-filter.scrolling-tabs.nav.nav-tabs
-    = event_filter_link EventFilter.all, _('All'), s_('EventFilterBy|Filter by all')
+    = event_filter_link EventFilter::ALL, _('All'), s_('EventFilterBy|Filter by all')
     - if event_filter_visible(:repository)
-      = event_filter_link EventFilter.push, _('Push events'), s_('EventFilterBy|Filter by push events')
+      = event_filter_link EventFilter::PUSH, _('Push events'), s_('EventFilterBy|Filter by push events')
     - if event_filter_visible(:merge_requests)
-      = event_filter_link EventFilter.merged, _('Merge events'), s_('EventFilterBy|Filter by merge events')
+      = event_filter_link EventFilter::MERGED, _('Merge events'), s_('EventFilterBy|Filter by merge events')
     - if event_filter_visible(:issues)
-      = event_filter_link EventFilter.issue, _('Issue events'), s_('EventFilterBy|Filter by issue events')
+      = event_filter_link EventFilter::ISSUE, _('Issue events'), s_('EventFilterBy|Filter by issue events')
     - if comments_visible?
-      = event_filter_link EventFilter.comments, _('Comments'), s_('EventFilterBy|Filter by comments')
-    = event_filter_link EventFilter.team, _('Team'), s_('EventFilterBy|Filter by team')
+      = event_filter_link EventFilter::COMMENTS, _('Comments'), s_('EventFilterBy|Filter by comments')
+    = event_filter_link EventFilter::TEAM, _('Team'), s_('EventFilterBy|Filter by team')

lib/event_filter.rb

-class EventFilter
-  attr_accessor :params
-
-  class << self
-    def all
-      'all'
-    end
-
-    def push
-      'push'
-    end
-
-    def merged
-      'merged'
-    end
-    def issue
-      'issue'
-    end
-
-    def comments
-      'comments'
-    end
-
-    def team
-      'team'
-    end
+# frozen_string_literal: true
+class EventFilter
+  attr_accessor :filter
+
+  ALL = 'all'
+  PUSH = 'push'
+  MERGED = 'merged'
+  ISSUE = 'issue'
+  COMMENTS = 'comments'
+  TEAM = 'team'
+  FILTERS = [ALL, PUSH, MERGED, ISSUE, COMMENTS, TEAM].freeze
+
+  def initialize(filter)
+    # Split using comma to maintain backward compatibility Ex/ "filter1,filter2"
+    filter = filter.to_s.split(',')[0].to_s
+    @filter = FILTERS.include?(filter) ? filter : ALL
   end
-  def initialize(params)
-    @params = if params
-                params.dup
-              else
-                [] # EventFilter.default_filter
-              end
+  def active?(key)
+    filter == key.to_s
   end
   # rubocop: disable CodeReuse/ActiveRecord
   def apply_filter(events)
-    return events if params.blank? || params == EventFilter.all
-
-    case params
-    when EventFilter.push
+    case filter
+    when PUSH
       events.where(action: Event::PUSHED)
-    when EventFilter.merged
+    when MERGED
       events.where(action: Event::MERGED)
-    when EventFilter.comments
+    when COMMENTS
       events.where(action: Event::COMMENTED)
-    when EventFilter.team
+    when TEAM
       events.where(action: [Event::JOINED, Event::LEFT, Event::EXPIRED])
-    when EventFilter.issue
+    when ISSUE
       events.where(action: [Event::CREATED, Event::UPDATED, Event::CLOSED, Event::REOPENED])
-    end
-  end
-  # rubocop: enable CodeReuse/ActiveRecord
-
-  def options(key)
-    filter = params.dup
-
-    if filter.include? key
-      filter.delete key
     else
-      filter << key
-    end
-
-    filter
-  end
-
-  def active?(key)
-    if params.present?
-      params.include? key
-    else
-      key == EventFilter.all
+      events
     end
   end
+  # rubocop: enable CodeReuse/ActiveRecord
 end

qa/qa/page/project/activity.rb

@@ -3,7 +3,7 @@ module QA
     module Project
       class Activity < Page::Base
         view 'app/views/shared/_event_filter.html.haml' do
-          element :push_events, "event_filter_link EventFilter.push, _('Push events')"
+          element :push_events, "event_filter_link EventFilter::PUSH, _('Push events')"
         end
         def go_to_push_events

spec/factories/events.rb

@@ -24,7 +24,7 @@ FactoryBot.define do
   factory :push_event, class: PushEvent do
     project factory: :project_empty_repo
-    author factory: :user
+    author(factory: :user) { project.creator }
     action Event::PUSHED
   end

spec/features/projects/activity/user_sees_activity_spec.rb

@@ -3,8 +3,10 @@ require 'spec_helper'
 describe 'Projects > Activity > User sees activity' do
   let(:project) { create(:project, :repository, :public) }
   let(:user) { project.creator }
+  let(:issue) { create(:issue, project: project) }
   before do
+    create(:event, :created, project: project, target: issue, author: user)
     event = create(:push_event, project: project, author: user)
     create(:push_event_payload,
            event: event,
@@ -12,10 +14,18 @@ describe 'Projects > Activity > User sees activity' do
            commit_to: '6d394385cf567f80a8fd85055db1ab4c5295806f',
            ref: 'fix',
            commit_count: 1)
-    visit activity_project_path(project)
   end
   it 'shows the last push in the activity page', :js do
+    visit activity_project_path(project)
+
     expect(page).to have_content "#{user.name} pushed new branch fix"
   end
+
+  it 'allows to filter event with the "event_filter=issue" URL param', :js do
+    visit activity_project_path(project, event_filter: 'issue')
+
+    expect(page).not_to have_content "#{user.name} pushed new branch fix"
+    expect(page).to have_content "#{user.name} opened issue #{issue.to_reference}"
+  end
 end

spec/lib/event_filter_spec.rb

 require 'spec_helper'
 describe EventFilter do
+  describe 'FILTERS' do
+    it 'returns a definite list of filters' do
+      expect(described_class::FILTERS).to eq(%w[all push merged issue comments team])
+    end
+  end
+
+  describe '#filter' do
+    it 'returns "all" if given filter is nil' do
+      expect(described_class.new(nil).filter).to eq(described_class::ALL)
+    end
+
+    it 'returns "all" if given filter is ""' do
+      expect(described_class.new('').filter).to eq(described_class::ALL)
+    end
+
+    it 'returns "all" if given filter is "foo"' do
+      expect(described_class.new('foo').filter).to eq('all')
+    end
+  end
+
   describe '#apply_filter' do
-    let(:source_user) { create(:user) }
-    let!(:public_project) { create(:project, :public) }
-    let!(:push_event)     { create(:push_event,        project: public_project, author: source_user) }
-    let!(:merged_event)   { create(:event, :merged,    project: public_project, target: public_project, author: source_user) }
-    let!(:created_event)  { create(:event, :created,   project: public_project, target: public_project, author: source_user) }
-    let!(:updated_event)  { create(:event, :updated,   project: public_project, target: public_project, author: source_user) }
-    let!(:closed_event)   { create(:event, :closed,    project: public_project, target: public_project, author: source_user) }
-    let!(:reopened_event) { create(:event, :reopened,  project: public_project, target: public_project, author: source_user) }
-    let!(:comments_event) { create(:event, :commented, project: public_project, target: public_project, author: source_user) }
-    let!(:joined_event)   { create(:event, :joined,    project: public_project, target: public_project, author: source_user) }
-    let!(:left_event)     { create(:event, :left,      project: public_project, target: public_project, author: source_user) }
-    it 'applies push filter' do
-      events = described_class.new(described_class.push).apply_filter(Event.all)
-      expect(events).to contain_exactly(push_event)
+    set(:public_project) { create(:project, :public) }
+
+    set(:push_event)     { create(:push_event,        project: public_project) }
+    set(:merged_event)   { create(:event, :merged,    project: public_project, target: public_project) }
+    set(:created_event)  { create(:event, :created,   project: public_project, target: public_project) }
+    set(:updated_event)  { create(:event, :updated,   project: public_project, target: public_project) }
+    set(:closed_event)   { create(:event, :closed,    project: public_project, target: public_project) }
+    set(:reopened_event) { create(:event, :reopened,  project: public_project, target: public_project) }
+    set(:comments_event) { create(:event, :commented, project: public_project, target: public_project) }
+    set(:joined_event)   { create(:event, :joined,    project: public_project, target: public_project) }
+    set(:left_event)     { create(:event, :left,      project: public_project, target: public_project) }
+    let(:filtered_events) { described_class.new(filter).apply_filter(Event.all) }
+    context 'with the "push" filter' do
+      let(:filter) { described_class::PUSH }
+
+      it 'filters push events only' do
+        expect(filtered_events).to contain_exactly(push_event)
+      end
     end
-    it 'applies merged filter' do
-      events = described_class.new(described_class.merged).apply_filter(Event.all)
-      expect(events).to contain_exactly(merged_event)
+    context 'with the "merged" filter' do
+      let(:filter) { described_class::MERGED }
+
+      it 'filters merged events only' do
+        expect(filtered_events).to contain_exactly(merged_event)
+      end
     end
-    it 'applies issue filter' do
-      events = described_class.new(described_class.issue).apply_filter(Event.all)
-      expect(events).to contain_exactly(created_event, updated_event, closed_event, reopened_event)
+    context 'with the "issue" filter' do
+      let(:filter) { described_class::ISSUE }
+
+      it 'filters issue events only' do
+        expect(filtered_events).to contain_exactly(created_event, updated_event, closed_event, reopened_event)
+      end
     end
-    it 'applies comments filter' do
-      events = described_class.new(described_class.comments).apply_filter(Event.all)
-      expect(events).to contain_exactly(comments_event)
+    context 'with the "comments" filter' do
+      let(:filter) { described_class::COMMENTS }
+
+      it 'filters comment events only' do
+        expect(filtered_events).to contain_exactly(comments_event)
+      end
     end
-    it 'applies team filter' do
-      events = described_class.new(described_class.team).apply_filter(Event.all)
-      expect(events).to contain_exactly(joined_event, left_event)
+    context 'with the "team" filter' do
+      let(:filter) { described_class::TEAM }
+
+      it 'filters team events only' do
+        expect(filtered_events).to contain_exactly(joined_event, left_event)
+      end
     end
-    it 'applies all filter' do
-      events = described_class.new(described_class.all).apply_filter(Event.all)
-      expect(events).to contain_exactly(push_event, merged_event, created_event, updated_event, closed_event, reopened_event, comments_event, joined_event, left_event)
+    context 'with the "all" filter' do
+      let(:filter) { described_class::ALL }
+
+      it 'returns all events' do
+        expect(filtered_events).to eq(Event.all)
+      end
+    end
+
+    context 'with an unknown filter' do
+      let(:filter) { 'foo' }
+
+      it 'returns all events' do
+        expect(filtered_events).to eq(Event.all)
+      end
+    end
+
+    context 'with a nil filter' do
+      let(:filter) { nil }
+
+      it 'returns all events' do
+        expect(filtered_events).to eq(Event.all)
+      end
+    end
+  end
+
+  describe '#active?' do
+    let(:event_filter) { described_class.new(described_class::TEAM) }
+
+    it 'returns false if filter does not include the given key' do
+      expect(event_filter.active?('foo')).to eq(false)
     end
-    it 'applies no filter' do
-      events = described_class.new(nil).apply_filter(Event.all)
-      expect(events).to contain_exactly(push_event, merged_event, created_event, updated_event, closed_event, reopened_event, comments_event, joined_event, left_event)
+    it 'returns false if the given key is nil' do
+      expect(event_filter.active?(nil)).to eq(false)
     end
-    it 'applies unknown filter' do
-      events = described_class.new('').apply_filter(Event.all)
-      expect(events).to contain_exactly(push_event, merged_event, created_event, updated_event, closed_event, reopened_event, comments_event, joined_event, left_event)
+    it 'returns true if filter does not include the given key' do
+      expect(event_filter.active?(described_class::TEAM)).to eq(true)
     end
   end
 end

spec/models/event_collection_spec.rb

@@ -41,7 +41,7 @@ describe EventCollection do
     end
     it 'allows filtering of events using an EventFilter' do
-      filter = EventFilter.new(EventFilter.issue)
+      filter = EventFilter.new(EventFilter::ISSUE)
       events = described_class.new(projects, filter: filter).to_a
       expect(events.length).to eq(1)