summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorRémy Coutable <remy@rymai.me>2018-09-06 12:07:33 +0200
committerRémy Coutable <remy@rymai.me>2018-09-26 18:34:26 +0200
commitd2afddfeff29c15cad737db4898664381ce0f985 (patch)
tree3fd5b7f6329bf4fe4ce9851e8bc16e92def8d205
parent65cf805278c1d865ee0543508de92d5f4ecae4f0 (diff)
downloadgitlab-ce-d2afddfeff29c15cad737db4898664381ce0f985.tar.gz
Refactor EventFilter and increase its test coverage1801-allow-event_filter-to-be-set-in-the-url
Signed-off-by: Rémy Coutable <remy@rymai.me>
-rw-r--r--app/controllers/application_controller.rb8
-rw-r--r--app/controllers/dashboard_controller.rb2
-rw-r--r--app/views/shared/_event_filter.html.haml12
-rw-r--r--lib/event_filter.rb86
-rw-r--r--qa/qa/page/project/activity.rb2
-rw-r--r--spec/factories/events.rb2
-rw-r--r--spec/features/projects/activity/user_sees_activity_spec.rb12
-rw-r--r--spec/lib/event_filter_spec.rb131
-rw-r--r--spec/models/event_collection_spec.rb2
9 files changed, 147 insertions, 110 deletions
diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb
index 81b538b97ba..b87034d10b6 100644
--- a/app/controllers/application_controller.rb
+++ b/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
diff --git a/app/controllers/dashboard_controller.rb b/app/controllers/dashboard_controller.rb
index 241753a505a..c032fb2efb5 100644
--- a/app/controllers/dashboard_controller.rb
+++ b/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)
diff --git a/app/views/shared/_event_filter.html.haml b/app/views/shared/_event_filter.html.haml
index 7afb7b3a93b..6612497e7e2 100644
--- a/app/views/shared/_event_filter.html.haml
+++ b/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')
diff --git a/lib/event_filter.rb b/lib/event_filter.rb
index f756a211a12..24fdcd6fbb1 100644
--- a/lib/event_filter.rb
+++ b/lib/event_filter.rb
@@ -1,76 +1,42 @@
-class EventFilter
- attr_accessor :params
-
- class << self
- def all
- 'all'
- end
-
- def push
- 'push'
- end
-
- def merged
- 'merged'
- end
+# frozen_string_literal: true
- def issue
- 'issue'
- end
-
- def comments
- 'comments'
- end
-
- def team
- 'team'
- end
+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
diff --git a/qa/qa/page/project/activity.rb b/qa/qa/page/project/activity.rb
index 0196922c889..a0500b4d31a 100644
--- a/qa/qa/page/project/activity.rb
+++ b/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
diff --git a/spec/factories/events.rb b/spec/factories/events.rb
index 5798b81ecad..bf8411b1894 100644
--- a/spec/factories/events.rb
+++ b/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
diff --git a/spec/features/projects/activity/user_sees_activity_spec.rb b/spec/features/projects/activity/user_sees_activity_spec.rb
index e0248911b5f..ebaa137772d 100644
--- a/spec/features/projects/activity/user_sees_activity_spec.rb
+++ b/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
diff --git a/spec/lib/event_filter_spec.rb b/spec/lib/event_filter_spec.rb
index 87ae6b6cf01..30016da6828 100644
--- a/spec/lib/event_filter_spec.rb
+++ b/spec/lib/event_filter_spec.rb
@@ -1,58 +1,119 @@
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) }
+ 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!(: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) }
+ let(:filtered_events) { described_class.new(filter).apply_filter(Event.all) }
- it 'applies push filter' do
- events = described_class.new(described_class.push).apply_filter(Event.all)
- expect(events).to contain_exactly(push_event)
+ 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
diff --git a/spec/models/event_collection_spec.rb b/spec/models/event_collection_spec.rb
index e0a87c18cc7..6078f429bdc 100644
--- a/spec/models/event_collection_spec.rb
+++ b/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)