From d2afddfeff29c15cad737db4898664381ce0f985 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Thu, 6 Sep 2018 12:07:33 +0200 Subject: Refactor EventFilter and increase its test coverage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Rémy Coutable --- app/controllers/application_controller.rb | 8 +- app/controllers/dashboard_controller.rb | 2 +- app/views/shared/_event_filter.html.haml | 12 +- lib/event_filter.rb | 86 ++++---------- qa/qa/page/project/activity.rb | 2 +- spec/factories/events.rb | 2 +- .../projects/activity/user_sees_activity_spec.rb | 12 +- spec/lib/event_filter_spec.rb | 131 +++++++++++++++------ spec/models/event_collection_spec.rb | 2 +- 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) -- cgit v1.2.1