From 9124310f2871117acaac98781be84c9fc016e2ad Mon Sep 17 00:00:00 2001
From: Callum Dryden <callum.dryden@rightscale.com>
Date: Thu, 6 Oct 2016 15:19:27 +0000
Subject: [PATCH] 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 +
app/models/concerns/expirable.rb | 6 ++-
app/models/event.rb | 9 +++-
app/models/members/project_member.rb | 6 ++-
app/services/event_create_service.rb | 4 ++
features/dashboard/dashboard.feature | 13 ------
features/steps/dashboard/dashboard.rb | 27 ------------
lib/event_filter.rb | 11 ++++-
.../project_member_activity_index_spec.rb | 41 +++++++++++++++++++
spec/models/concerns/expirable_spec.rb | 31 ++++++++++++++
spec/models/event_spec.rb | 27 ++++++++++++
spec/models/members/project_member_spec.rb | 11 +++++
spec/services/event_create_service_spec.rb | 19 +++++++++
13 files changed, 161 insertions(+), 45 deletions(-)
create mode 100644 spec/features/dashboard/project_member_activity_index_spec.rb
create mode 100644 spec/models/concerns/expirable_spec.rb
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 7b018fc0d57..8cb848c3957 100644
--- a/CHANGELOG.md
+++ b/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)
diff --git a/app/models/concerns/expirable.rb b/app/models/concerns/expirable.rb
index be93435453b..b66ba08dc59 100644
--- a/app/models/concerns/expirable.rb
+++ b/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
diff --git a/app/models/event.rb b/app/models/event.rb
index 0764cb8cabd..3993b35f96d 100644
--- a/app/models/event.rb
+++ b/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?
diff --git a/app/models/members/project_member.rb b/app/models/members/project_member.rb
index 125f26369d7..e4880973117 100644
--- a/app/models/members/project_member.rb
+++ b/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
diff --git a/app/services/event_create_service.rb b/app/services/event_create_service.rb
index 07fc77001a5..e24cc66e0fe 100644
--- a/app/services/event_create_service.rb
+++ b/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
diff --git a/features/dashboard/dashboard.feature b/features/dashboard/dashboard.feature
index 1f4c9020731..b1d5e4a7acb 100644
--- a/features/dashboard/dashboard.feature
+++ b/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
diff --git a/features/steps/dashboard/dashboard.rb b/features/steps/dashboard/dashboard.rb
index a7d61bc28e0..b2bec369e0f 100644
--- a/features/steps/dashboard/dashboard.rb
+++ b/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)
diff --git a/lib/event_filter.rb b/lib/event_filter.rb
index 96e70e37e8f..21f6a9a762b 100644
--- a/lib/event_filter.rb
+++ b/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)
diff --git a/spec/features/dashboard/project_member_activity_index_spec.rb b/spec/features/dashboard/project_member_activity_index_spec.rb
new file mode 100644
index 00000000000..ba77093a6d4
--- /dev/null
+++ b/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
diff --git a/spec/models/concerns/expirable_spec.rb b/spec/models/concerns/expirable_spec.rb
new file mode 100644
index 00000000000..f7b436f32e6
--- /dev/null
+++ b/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
diff --git a/spec/models/event_spec.rb b/spec/models/event_spec.rb
index 733b79079ed..aca49be2942 100644
--- a/spec/models/event_spec.rb
+++ b/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) }
diff --git a/spec/models/members/project_member_spec.rb b/spec/models/members/project_member_spec.rb
index d85a1c1e3b2..b2fe96e2e02 100644
--- a/spec/models/members/project_member_spec.rb
+++ b/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)
diff --git a/spec/services/event_create_service_spec.rb b/spec/services/event_create_service_spec.rb
index 16a9956fe7f..b7dc99ed887 100644
--- a/spec/services/event_create_service_spec.rb
+++ b/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
--
GitLab