From b4d614bdbcb50f506c56eaa180d7b3f3055884a8 Mon Sep 17 00:00:00 2001 From: Clement Ho Date: Mon, 29 Aug 2016 12:09:33 -0500 Subject: Fix inconsistent highlighting of already selected activity nav-links --- CHANGELOG | 1 + app/assets/javascripts/activities.js | 12 ++--- app/controllers/application_controller.rb | 3 +- app/views/shared/_event_filter.html.haml | 1 + lib/event_filter.rb | 23 +++++---- spec/javascripts/activities_spec.js.es6 | 61 ++++++++++++++++++++++++ spec/javascripts/fixtures/event_filter.html.haml | 21 ++++++++ spec/lib/event_filter_spec.rb | 49 +++++++++++++++++++ 8 files changed, 153 insertions(+), 18 deletions(-) create mode 100644 spec/javascripts/activities_spec.js.es6 create mode 100644 spec/javascripts/fixtures/event_filter.html.haml create mode 100644 spec/lib/event_filter_spec.rb diff --git a/CHANGELOG b/CHANGELOG index 07b2b23003b..fbafda4d9ad 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -22,6 +22,7 @@ v 8.13.0 (unreleased) - Added soft wrap button to repository file/blob editor - Add word-wrap to issue title on issue and milestone boards (ClemMakesApps) - Fix todos page mobile viewport layout (ClemMakesApps) + - Fix inconsistent highlighting of already selected activity nav-links (ClemMakesApps) - Fix robots.txt disallowing access to groups starting with "s" (Matt Harrison) - Close open merge request without source project (Katarzyna Kobierska Ula Budziszewska) - Fix that manual jobs would no longer block jobs in the next stage. !6604 diff --git a/app/assets/javascripts/activities.js b/app/assets/javascripts/activities.js index d5e11e22be5..f4f8cf04184 100644 --- a/app/assets/javascripts/activities.js +++ b/app/assets/javascripts/activities.js @@ -21,16 +21,14 @@ }; Activities.prototype.toggleFilter = function(sender) { - var event_filters, filter; + var filter = sender.attr("id").split("_")[0]; + $('.event-filter .active').removeClass("active"); - event_filters = $.cookie("event_filter"); - filter = sender.attr("id").split("_")[0]; - $.cookie("event_filter", (event_filters !== filter ? filter : ""), { + $.cookie("event_filter", filter, { path: gon.relative_url_root || '/' }); - if (event_filters !== filter) { - return sender.closest('li').toggleClass("active"); - } + + sender.closest('li').toggleClass("active"); }; return Activities; diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index bd4ba384b29..b3455e04c29 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -173,7 +173,8 @@ class ApplicationController < ActionController::Base end def event_filter - filters = cookies['event_filter'].split(',') if cookies['event_filter'].present? + # Split using comma to maintain backward compatibility Ex/ "filter1,filter2" + filters = cookies['event_filter'].split(',')[0] if cookies['event_filter'].present? @event_filter ||= EventFilter.new(filters) end diff --git a/app/views/shared/_event_filter.html.haml b/app/views/shared/_event_filter.html.haml index 8824bcc158e..3480800369a 100644 --- a/app/views/shared/_event_filter.html.haml +++ b/app/views/shared/_event_filter.html.haml @@ -1,4 +1,5 @@ %ul.nav-links.event-filter.scrolling-tabs + = event_filter_link EventFilter.all, 'All' = event_filter_link EventFilter.push, 'Push events' = event_filter_link EventFilter.merged, 'Merge events' = event_filter_link EventFilter.comments, 'Comments' diff --git a/lib/event_filter.rb b/lib/event_filter.rb index 668d2fa41b3..96e70e37e8f 100644 --- a/lib/event_filter.rb +++ b/lib/event_filter.rb @@ -2,8 +2,8 @@ class EventFilter attr_accessor :params class << self - def default_filter - %w{ push issues merge_requests team} + def all + 'all' end def push @@ -35,18 +35,21 @@ class EventFilter return events unless params.present? filter = params.dup - actions = [] - actions << Event::PUSHED if filter.include? 'push' - actions << Event::MERGED if filter.include? 'merged' - if filter.include? 'team' - actions << Event::JOINED - actions << Event::LEFT + case filter + when EventFilter.push + actions = [Event::PUSHED] + when EventFilter.merged + actions = [Event::MERGED] + when EventFilter.comments + actions = [Event::COMMENTED] + when EventFilter.team + actions = [Event::JOINED, Event::LEFT] + when EventFilter.all + actions = [Event::PUSHED, Event::MERGED, Event::COMMENTED, Event::JOINED, Event::LEFT] end - actions << Event::COMMENTED if filter.include? 'comments' - events.where(action: actions) end diff --git a/spec/javascripts/activities_spec.js.es6 b/spec/javascripts/activities_spec.js.es6 new file mode 100644 index 00000000000..743b15460c6 --- /dev/null +++ b/spec/javascripts/activities_spec.js.es6 @@ -0,0 +1,61 @@ +/*= require jquery.cookie.js */ +/*= require jquery.endless-scroll.js */ +/*= require pager */ +/*= require activities */ + +(() => { + window.gon || (window.gon = {}); + const fixtureTemplate = 'event_filter.html'; + const filters = [ + { + id: 'all', + }, { + id: 'push', + name: 'push events', + }, { + id: 'merged', + name: 'merge events', + }, { + id: 'comments', + },{ + id: 'team', + }]; + + function getEventName(index) { + let filter = filters[index]; + return filter.hasOwnProperty('name') ? filter.name : filter.id; + } + + function getSelector(index) { + let filter = filters[index]; + return `#${filter.id}_event_filter` + } + + describe('Activities', () => { + beforeEach(() => { + fixture.load(fixtureTemplate); + new Activities(); + }); + + for(let i = 0; i < filters.length; i++) { + ((i) => { + describe(`when selecting ${getEventName(i)}`, () => { + beforeEach(() => { + $(getSelector(i)).click(); + }); + + for(let x = 0; x < filters.length; x++) { + ((x) => { + let shouldHighlight = i === x; + let testName = shouldHighlight ? 'should highlight' : 'should not highlight'; + + it(`${testName} ${getEventName(x)}`, () => { + expect($(getSelector(x)).parent().hasClass('active')).toEqual(shouldHighlight); + }); + })(x); + } + }); + })(i); + } + }); +})(); diff --git a/spec/javascripts/fixtures/event_filter.html.haml b/spec/javascripts/fixtures/event_filter.html.haml new file mode 100644 index 00000000000..95e248cadf8 --- /dev/null +++ b/spec/javascripts/fixtures/event_filter.html.haml @@ -0,0 +1,21 @@ +%ul.nav-links.event-filter.scrolling-tabs + %li.active + %a.event-filter-link{ id: "all_event_filter", title: "Filter by all", href: "/dashboard/activity"} + %span + All + %li + %a.event-filter-link{ id: "push_event_filter", title: "Filter by push events", href: "/dashboard/activity"} + %span + Push events + %li + %a.event-filter-link{ id: "merged_event_filter", title: "Filter by merge events", href: "/dashboard/activity"} + %span + Merge events + %li + %a.event-filter-link{ id: "comments_event_filter", title: "Filter by comments", href: "/dashboard/activity"} + %span + Comments + %li + %a.event-filter-link{ id: "team_event_filter", title: "Filter by team", href: "/dashboard/activity"} + %span + Team \ No newline at end of file diff --git a/spec/lib/event_filter_spec.rb b/spec/lib/event_filter_spec.rb new file mode 100644 index 00000000000..a6d8e6927e0 --- /dev/null +++ b/spec/lib/event_filter_spec.rb @@ -0,0 +1,49 @@ +require 'spec_helper' + +describe EventFilter, lib: true do + describe '#apply_filter' do + let(:source_user) { create(:user) } + let!(:public_project) { create(:project, :public) } + + let!(:push_event) { create(:event, action: Event::PUSHED, project: public_project, target: public_project, author: source_user) } + let!(:merged_event) { create(:event, action: Event::MERGED, project: public_project, target: public_project, author: source_user) } + let!(:comments_event) { create(:event, action: Event::COMMENTED, project: public_project, target: public_project, author: source_user) } + let!(:joined_event) { create(:event, action: Event::JOINED, project: public_project, target: public_project, author: source_user) } + let!(:left_event) { create(:event, action: Event::LEFT, project: public_project, target: public_project, author: source_user) } + + it 'applies push filter' do + events = EventFilter.new(EventFilter.push).apply_filter(Event.all) + expect(events).to contain_exactly(push_event) + end + + it 'applies merged filter' do + events = EventFilter.new(EventFilter.merged).apply_filter(Event.all) + expect(events).to contain_exactly(merged_event) + end + + it 'applies comments filter' do + events = EventFilter.new(EventFilter.comments).apply_filter(Event.all) + expect(events).to contain_exactly(comments_event) + end + + it 'applies team filter' do + events = EventFilter.new(EventFilter.team).apply_filter(Event.all) + expect(events).to contain_exactly(joined_event, left_event) + end + + it 'applies all filter' do + events = EventFilter.new(EventFilter.all).apply_filter(Event.all) + expect(events).to contain_exactly(push_event, merged_event, comments_event, joined_event, left_event) + end + + it 'applies no filter' do + events = EventFilter.new(nil).apply_filter(Event.all) + expect(events).to contain_exactly(push_event, merged_event, comments_event, joined_event, left_event) + end + + it 'applies unknown filter' do + events = EventFilter.new('').apply_filter(Event.all) + expect(events).to contain_exactly(push_event, merged_event, comments_event, joined_event, left_event) + end + end +end -- cgit v1.2.1