Differentiate the expire from leave event

At the moment we cannot see weather a user left a project due to their
membership expiring of if they themselves opted to leave the project.
This adds a new event type that allows us to make this differentiation.
Note that is not really feasable to go back and reliably fix up the
previous events. As a result the events for previous expire removals
will remain the same however events of this nature going forward will be
correctly represented.

CHANGELOG.md

@@ -1,6 +1,7 @@
 Please view this file on the master branch, on stable branches it's out of date.
 
 ## 8.14.0 (2016-11-22)
+  - Adds user project membership expired event to clarify why user was removed (Callum Dryden)
 
 ## 8.13.0 (2016-10-22)
 

app/models/concerns/expirable.rb

@@ -5,11 +5,15 @@ module Expirable
     scope :expired, -> { where('expires_at <= ?', Time.current) }
   end
 
+  def expired?
+    expires? && expires_at <= Time.current
+  end
+
   def expires?
     expires_at.present?
   end
 
   def expires_soon?
-    expires_at < 7.days.from_now
+    expires? && expires_at < 7.days.from_now
   end
 end

app/models/event.rb

@@ -12,6 +12,7 @@ class Event < ActiveRecord::Base
   JOINED    = 8 # User joined project
   LEFT      = 9 # User left project
   DESTROYED = 10
+  EXPIRED   = 11 # User left project due to expiry
 
   RESET_PROJECT_ACTIVITY_INTERVAL = 1.hour
 
@@ -115,6 +116,10 @@ class Event < ActiveRecord::Base
     action == LEFT
   end
 
+  def expired?
+    action == EXPIRED
+  end
+
   def destroyed?
     action == DESTROYED
   end
@@ -124,7 +129,7 @@ class Event < ActiveRecord::Base
   end
 
   def membership_changed?
-    joined? || left?
+    joined? || left? || expired?
   end
 
   def created_project?
@@ -184,6 +189,8 @@ class Event < ActiveRecord::Base
       'joined'
     elsif left?
       'left'
+    elsif expired?
+      'removed due to membership expiration from'
     elsif destroyed?
       'destroyed'
     elsif commented?

app/models/members/project_member.rb

@@ -121,7 +121,11 @@ class ProjectMember < Member
   end
 
   def post_destroy_hook
-    event_service.leave_project(self.project, self.user)
+    if expired?
+      event_service.expired_leave_project(self.project, self.user)
+    else
+      event_service.leave_project(self.project, self.user)
+    end
 
     super
   end

app/services/event_create_service.rb

@@ -62,6 +62,10 @@ class EventCreateService
     create_event(project, current_user, Event::LEFT)
   end
 
+  def expired_leave_project(project, current_user)
+    create_event(project, current_user, Event::EXPIRED)
+  end
+
   def create_project(project, current_user)
     create_event(project, current_user, Event::CREATED)
   end

features/dashboard/dashboard.feature

@@ -31,19 +31,6 @@ Feature: Dashboard
     And I click "Create Merge Request" link
     Then I see prefilled new Merge Request page
 
-  @javascript
-  Scenario: I should see User joined Project event
-    Given user with name "John Doe" joined project "Shop"
-    When I visit dashboard activity page
-    Then I should see "John Doe joined project Shop" event
-
-  @javascript
-  Scenario: I should see User left Project event
-    Given user with name "John Doe" joined project "Shop"
-    And user with name "John Doe" left project "Shop"
-    When I visit dashboard activity page
-    Then I should see "John Doe left project Shop" event
-
   @javascript
   Scenario: Sorting Issues
     Given I visit dashboard issues page

features/steps/dashboard/dashboard.rb

@@ -33,33 +33,6 @@ class Spinach::Features::Dashboard < Spinach::FeatureSteps
     expect(find("input#merge_request_target_branch").value).to eq "master"
   end
 
-  step 'user with name "John Doe" joined project "Shop"' do
-    user = create(:user, { name: "John Doe" })
-    project.team << [user, :master]
-    Event.create(
-      project: project,
-      author_id: user.id,
-      action: Event::JOINED
-    )
-  end
-
-  step 'I should see "John Doe joined project Shop" event' do
-    expect(page).to have_content "John Doe joined project #{project.name_with_namespace}"
-  end
-
-  step 'user with name "John Doe" left project "Shop"' do
-    user = User.find_by(name: "John Doe")
-    Event.create(
-      project: project,
-      author_id: user.id,
-      action: Event::LEFT
-    )
-  end
-
-  step 'I should see "John Doe left project Shop" event' do
-    expect(page).to have_content "John Doe left project #{project.name_with_namespace}"
-  end
-
   step 'I have group with projects' do
     @group   = create(:group)
     @project = create(:project, namespace: @group)

lib/event_filter.rb

@@ -45,9 +45,16 @@ class EventFilter
     when EventFilter.comments
       actions = [Event::COMMENTED]
     when EventFilter.team
-      actions = [Event::JOINED, Event::LEFT]
+      actions = [Event::JOINED, Event::LEFT, Event::EXPIRED]
     when EventFilter.all
-      actions = [Event::PUSHED, Event::MERGED, Event::COMMENTED, Event::JOINED, Event::LEFT]
+      actions = [
+        Event::PUSHED,
+        Event::MERGED,
+        Event::COMMENTED,
+        Event::JOINED,
+        Event::LEFT,
+        Event::EXPIRED
+      ]
     end
 
     events.where(action: actions)

spec/features/dashboard/project_member_activity_index_spec.rb

@@ -0,0 +1,41 @@
+require 'spec_helper'
+
+feature 'Project member activity', feature: true, js: true do
+  include WaitForAjax
+
+  let(:user)            { create(:user) }
+  let(:project)         { create(:empty_project, :public, name: 'x', namespace: user.namespace) }
+
+  before do
+    project.team << [user, :master]
+  end
+
+  def visit_activities_and_wait_with_event(event_type)
+    Event.create(project: project, author_id: user.id, action: event_type)
+    visit activity_namespace_project_path(project.namespace.path, project.path)
+    wait_for_ajax
+  end
+
+  subject { page.find(".event-title").text }
+
+  context 'when a user joins the project' do
+    before { visit_activities_and_wait_with_event(Event::JOINED) }
+
+    it { is_expected.to eq("#{user.name} joined project") }
+  end
+
+  context 'when a user leaves the project' do
+    before { visit_activities_and_wait_with_event(Event::LEFT) }
+
+    it { is_expected.to eq("#{user.name} left project") }
+  end
+
+  context 'when a users membership expires for the project' do
+    before { visit_activities_and_wait_with_event(Event::EXPIRED) }
+
+    it "presents the correct message" do
+      message = "#{user.name} removed due to membership expiration from project"
+      is_expected.to eq(message)
+    end
+  end
+end

spec/models/concerns/expirable_spec.rb

@@ -0,0 +1,31 @@
+require 'spec_helper'
+
+describe Expirable do
+  describe 'ProjectMember' do
+    let(:no_expire) { create(:project_member) }
+    let(:expire_later) { create(:project_member, expires_at: Time.current + 6.days) }
+    let(:expired) { create(:project_member, expires_at: Time.current - 6.days) }
+
+    describe '.expired' do
+      it { expect(ProjectMember.expired).to match_array([expired]) }
+    end
+
+    describe '#expired?' do
+      it { expect(no_expire.expired?).to eq(false) }
+      it { expect(expire_later.expired?).to eq(false) }
+      it { expect(expired.expired?).to eq(true) }
+    end
+
+    describe '#expires?' do
+      it { expect(no_expire.expires?).to eq(false) }
+      it { expect(expire_later.expires?).to eq(true) }
+      it { expect(expired.expires?).to eq(true) }
+    end
+
+    describe '#expires_soon?' do
+      it { expect(no_expire.expires_soon?).to eq(false) }
+      it { expect(expire_later.expires_soon?).to eq(true) }
+      it { expect(expired.expires_soon?).to eq(true) }
+    end
+  end
+end

spec/models/event_spec.rb

@@ -40,6 +40,33 @@ describe Event, models: true do
     end
   end
 
+  describe '#membership_changed?' do
+    context "created" do
+      subject { build(:event, action: Event::CREATED).membership_changed? }
+      it { is_expected.to be_falsey }
+    end
+
+    context "updated" do
+      subject { build(:event, action: Event::UPDATED).membership_changed? }
+      it { is_expected.to be_falsey }
+    end
+
+    context "expired" do
+      subject { build(:event, action: Event::EXPIRED).membership_changed? }
+      it { is_expected.to be_truthy }
+    end
+
+    context "left" do
+      subject { build(:event, action: Event::LEFT).membership_changed? }
+      it { is_expected.to be_truthy }
+    end
+
+    context "joined" do
+      subject { build(:event, action: Event::JOINED).membership_changed? }
+      it { is_expected.to be_truthy }
+    end
+  end
+
   describe '#note?' do
     subject { Event.new(project: target.project, target: target) }
 

spec/models/members/project_member_spec.rb

@@ -54,6 +54,17 @@ describe ProjectMember, models: true do
       master_todos
     end
 
+    it "creates an expired event when left due to expiry" do
+      expired = create(:project_member, project: project, expires_at: Time.now - 6.days)
+      expired.destroy
+      expect(Event.first.action).to eq(Event::EXPIRED)
+    end
+
+    it "creates a left event when left due to leave" do
+      master.destroy
+      expect(Event.first.action).to eq(Event::LEFT)
+    end
+
     it "destroys itself and delete associated todos" do
       expect(owner.user.todos.size).to eq(2)
       expect(master.user.todos.size).to eq(3)

spec/services/event_create_service_spec.rb

@@ -110,4 +110,23 @@ describe EventCreateService, services: true do
       end
     end
   end
+
+  describe 'Project' do
+    let(:user) { create :user }
+    let(:project) { create(:empty_project) }
+
+    describe '#join_project' do
+      subject { service.join_project(project, user) }
+
+      it { is_expected.to be_truthy }
+      it { expect { subject }.to change { Event.count }.from(0).to(1) }
+    end
+
+    describe '#expired_leave_project' do
+      subject { service.expired_leave_project(project, user) }
+
+      it { is_expected.to be_truthy }
+      it { expect { subject }.to change { Event.count }.from(0).to(1) }
+    end
+  end
 end