Fix inconsistent highlighting of already selected activity nav-links

8 files changed, 153 insertions, 18 deletions

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