Remove default value for project argument on subscribable concern

This commit is contained in:
Douglas Barbosa Alexandre 2016-11-04 16:19:08 -02:00
parent b3249bc28f
commit 0c052f116c
22 changed files with 96 additions and 98 deletions

View file

@ -12,7 +12,7 @@ class SentNotificationsController < ApplicationController
def unsubscribe_and_redirect
noteable = @sent_notification.noteable
noteable.unsubscribe(@sent_notification.recipient)
noteable.unsubscribe(@sent_notification.recipient, @sent_notification.project)
flash[:notice] = "You have been unsubscribed from this thread."

View file

@ -12,45 +12,43 @@ module Subscribable
has_many :subscriptions, dependent: :destroy, as: :subscribable
end
def subscribed?(user, to_project = nil)
if subscription = subscriptions.find_by(user: user, project: (to_project || project))
def subscribed?(user, project)
if subscription = subscriptions.find_by(user: user, project: project)
subscription.subscribed
else
subscribed_without_subscriptions?(user, to_project)
subscribed_without_subscriptions?(user, project)
end
end
# Override this method to define custom logic to consider a subscribable as
# subscribed without an explicit subscription record.
def subscribed_without_subscriptions?(user, to_project = nil)
def subscribed_without_subscriptions?(user, project)
false
end
def subscribers(to_project = nil)
subscriptions.where(project: (to_project || project), subscribed: true).map(&:user)
def subscribers(project)
subscriptions.where(project: project, subscribed: true).map(&:user)
end
def toggle_subscription(user, to_project = nil)
subscribed = subscribed?(user, (to_project || project))
find_or_initialize_subscription(user, to_project).
update(subscribed: !subscribed)
def toggle_subscription(user, project)
find_or_initialize_subscription(user, project).
update(subscribed: !subscribed?(user, project))
end
def subscribe(user, to_project = nil)
find_or_initialize_subscription(user, to_project).
def subscribe(user, project)
find_or_initialize_subscription(user, project).
update(subscribed: true)
end
def unsubscribe(user, to_project = nil)
find_or_initialize_subscription(user, to_project).
def unsubscribe(user, project)
find_or_initialize_subscription(user, project).
update(subscribed: false)
end
private
def find_or_initialize_subscription(user, to_project = nil)
def find_or_initialize_subscription(user, project)
subscriptions.
find_or_initialize_by(user_id: user.id, project_id: (to_project || project).id)
find_or_initialize_by(user_id: user.id, project_id: project.id)
end
end

View file

@ -266,7 +266,7 @@ class Issue < ActiveRecord::Base
def as_json(options = {})
super(options).tap do |json|
json[:subscribed] = subscribed?(options[:user]) if options.has_key?(:user) && options[:user]
json[:subscribed] = subscribed?(options[:user], project) if options.has_key?(:user) && options[:user]
if options.has_key?(:labels)
json[:labels] = labels.as_json(

View file

@ -212,9 +212,9 @@ class IssuableBaseService < BaseService
def change_subscription(issuable)
case params.delete(:subscription_event)
when 'subscribe'
issuable.subscribe(current_user)
issuable.subscribe(current_user, project)
when 'unsubscribe'
issuable.unsubscribe(current_user)
issuable.unsubscribe(current_user, project)
end
end

View file

@ -193,7 +193,7 @@ module SlashCommands
desc 'Subscribe'
condition do
issuable.persisted? &&
!issuable.subscribed?(current_user)
!issuable.subscribed?(current_user, project)
end
command :subscribe do
@updates[:subscription_event] = 'subscribe'
@ -202,7 +202,7 @@ module SlashCommands
desc 'Unsubscribe'
condition do
issuable.persisted? &&
issuable.subscribed?(current_user)
issuable.subscribed?(current_user, project)
end
command :unsubscribe do
@updates[:subscription_event] = 'unsubscribe'

View file

@ -140,7 +140,7 @@
= render "shared/issuable/participants", participants: issuable.participants(current_user)
- if current_user
- subscribed = issuable.subscribed?(current_user)
- subscribed = issuable.subscribed?(current_user, @project)
.block.light.subscription{data: {url: toggle_subscription_path(issuable)}}
.sidebar-collapsed-icon
= icon('rss')

View file

@ -218,7 +218,7 @@ module API
expose :assignee, :author, using: Entities::UserBasic
expose :subscribed do |issue, options|
issue.subscribed?(options[:current_user])
issue.subscribed?(options[:current_user], options[:project] || issue.project)
end
expose :user_notes_count
expose :upvotes, :downvotes
@ -248,7 +248,7 @@ module API
expose :diff_head_sha, as: :sha
expose :merge_commit_sha
expose :subscribed do |merge_request, options|
merge_request.subscribed?(options[:current_user])
merge_request.subscribed?(options[:current_user], options[:project])
end
expose :user_notes_count
expose :should_remove_source_branch?, as: :should_remove_source_branch

View file

@ -120,7 +120,7 @@ module API
issues = issues.reorder(issuable_order_by => issuable_sort)
present paginate(issues), with: Entities::Issue, current_user: current_user
present paginate(issues), with: Entities::Issue, current_user: current_user, project: user_project
end
# Get a single project issue
@ -132,7 +132,7 @@ module API
# GET /projects/:id/issues/:issue_id
get ":id/issues/:issue_id" do
@issue = find_project_issue(params[:issue_id])
present @issue, with: Entities::Issue, current_user: current_user
present @issue, with: Entities::Issue, current_user: current_user, project: user_project
end
# Create a new project issue
@ -174,7 +174,7 @@ module API
end
if issue.valid?
present issue, with: Entities::Issue, current_user: current_user
present issue, with: Entities::Issue, current_user: current_user, project: user_project
else
render_validation_error!(issue)
end
@ -217,7 +217,7 @@ module API
issue = ::Issues::UpdateService.new(user_project, current_user, attrs).execute(issue)
if issue.valid?
present issue, with: Entities::Issue, current_user: current_user
present issue, with: Entities::Issue, current_user: current_user, project: user_project
else
render_validation_error!(issue)
end
@ -239,7 +239,7 @@ module API
begin
issue = ::Issues::MoveService.new(user_project, current_user).execute(issue, new_project)
present issue, with: Entities::Issue, current_user: current_user
present issue, with: Entities::Issue, current_user: current_user, project: user_project
rescue ::Issues::MoveService::MoveError => error
render_api_error!(error.message, 400)
end

View file

@ -60,7 +60,7 @@ module API
end
merge_requests = merge_requests.reorder(issuable_order_by => issuable_sort)
present paginate(merge_requests), with: Entities::MergeRequest, current_user: current_user
present paginate(merge_requests), with: Entities::MergeRequest, current_user: current_user, project: user_project
end
desc 'Create a merge request' do
@ -87,7 +87,7 @@ module API
merge_request = ::MergeRequests::CreateService.new(user_project, current_user, mr_params).execute
if merge_request.valid?
present merge_request, with: Entities::MergeRequest, current_user: current_user
present merge_request, with: Entities::MergeRequest, current_user: current_user, project: user_project
else
handle_merge_request_errors! merge_request.errors
end
@ -120,7 +120,7 @@ module API
get path do
merge_request = user_project.merge_requests.find(params[:merge_request_id])
authorize! :read_merge_request, merge_request
present merge_request, with: Entities::MergeRequest, current_user: current_user
present merge_request, with: Entities::MergeRequest, current_user: current_user, project: user_project
end
desc 'Get the commits of a merge request' do
@ -167,7 +167,7 @@ module API
merge_request = ::MergeRequests::UpdateService.new(user_project, current_user, mr_params).execute(merge_request)
if merge_request.valid?
present merge_request, with: Entities::MergeRequest, current_user: current_user
present merge_request, with: Entities::MergeRequest, current_user: current_user, project: user_project
else
handle_merge_request_errors! merge_request.errors
end
@ -212,7 +212,7 @@ module API
execute(merge_request)
end
present merge_request, with: Entities::MergeRequest, current_user: current_user
present merge_request, with: Entities::MergeRequest, current_user: current_user, project: user_project
end
desc 'Cancel merge if "Merge when build succeeds" is enabled' do

View file

@ -114,7 +114,7 @@ module API
}
issues = IssuesFinder.new(current_user, finder_params).execute
present paginate(issues), with: Entities::Issue, current_user: current_user
present paginate(issues), with: Entities::Issue, current_user: current_user, project: user_project
end
end
end

View file

@ -25,7 +25,7 @@ describe Projects::Boards::IssuesController do
create(:labeled_issue, project: project, labels: [planning])
create(:labeled_issue, project: project, labels: [development], due_date: Date.tomorrow)
create(:labeled_issue, project: project, labels: [development], assignee: johndoe)
issue.subscribe(johndoe)
issue.subscribe(johndoe, project)
list_issues user: user, board: board, list: list2

View file

@ -3,7 +3,7 @@ require 'rails_helper'
describe SentNotificationsController, type: :controller do
let(:user) { create(:user) }
let(:project) { create(:empty_project) }
let(:sent_notification) { create(:sent_notification, noteable: issue, recipient: user) }
let(:sent_notification) { create(:sent_notification, project: project, noteable: issue, recipient: user) }
let(:issue) do
create(:issue, project: project, author: user) do |issue|
@ -17,7 +17,7 @@ describe SentNotificationsController, type: :controller do
before { get(:unsubscribe, id: sent_notification.reply_key, force: true) }
it 'unsubscribes the user' do
expect(issue.subscribed?(user)).to be_falsey
expect(issue.subscribed?(user, project)).to be_falsey
end
it 'sets the flash message' do
@ -33,7 +33,7 @@ describe SentNotificationsController, type: :controller do
before { get(:unsubscribe, id: sent_notification.reply_key) }
it 'does not unsubscribe the user' do
expect(issue.subscribed?(user)).to be_truthy
expect(issue.subscribed?(user, project)).to be_truthy
end
it 'does not set the flash message' do
@ -53,7 +53,7 @@ describe SentNotificationsController, type: :controller do
before { get(:unsubscribe, id: sent_notification.reply_key.reverse) }
it 'does not unsubscribe the user' do
expect(issue.subscribed?(user)).to be_truthy
expect(issue.subscribed?(user, project)).to be_truthy
end
it 'does not set the flash message' do
@ -69,7 +69,7 @@ describe SentNotificationsController, type: :controller do
before { get(:unsubscribe, id: sent_notification.reply_key, force: true) }
it 'unsubscribes the user' do
expect(issue.subscribed?(user)).to be_falsey
expect(issue.subscribed?(user, project)).to be_falsey
end
it 'sets the flash message' do
@ -88,11 +88,11 @@ describe SentNotificationsController, type: :controller do
merge_request.subscriptions.create(user: user, project: project, subscribed: true)
end
end
let(:sent_notification) { create(:sent_notification, noteable: merge_request, recipient: user) }
let(:sent_notification) { create(:sent_notification, project: project, noteable: merge_request, recipient: user) }
before { get(:unsubscribe, id: sent_notification.reply_key) }
it 'unsubscribes the user' do
expect(merge_request.subscribed?(user)).to be_falsey
expect(merge_request.subscribed?(user, project)).to be_falsey
end
it 'sets the flash message' do

View file

@ -26,11 +26,11 @@ describe 'Unsubscribe links', feature: true do
expect(current_path).to eq unsubscribe_sent_notification_path(SentNotification.last)
expect(page).to have_text(%(Unsubscribe from issue #{issue.title} (#{issue.to_reference})))
expect(page).to have_text(%(Are you sure you want to unsubscribe from issue #{issue.title} (#{issue.to_reference})?))
expect(issue.subscribed?(recipient)).to be_truthy
expect(issue.subscribed?(recipient, project)).to be_truthy
click_link 'Unsubscribe'
expect(issue.subscribed?(recipient)).to be_falsey
expect(issue.subscribed?(recipient, project)).to be_falsey
expect(current_path).to eq new_user_session_path
end
@ -38,11 +38,11 @@ describe 'Unsubscribe links', feature: true do
visit body_link
expect(current_path).to eq unsubscribe_sent_notification_path(SentNotification.last)
expect(issue.subscribed?(recipient)).to be_truthy
expect(issue.subscribed?(recipient, project)).to be_truthy
click_link 'Cancel'
expect(issue.subscribed?(recipient)).to be_truthy
expect(issue.subscribed?(recipient, project)).to be_truthy
expect(current_path).to eq new_user_session_path
end
end
@ -51,7 +51,7 @@ describe 'Unsubscribe links', feature: true do
visit header_link
expect(page).to have_text('unsubscribed')
expect(issue.subscribed?(recipient)).to be_falsey
expect(issue.subscribed?(recipient, project)).to be_falsey
end
end
@ -62,14 +62,14 @@ describe 'Unsubscribe links', feature: true do
visit body_link
expect(page).to have_text('unsubscribed')
expect(issue.subscribed?(recipient)).to be_falsey
expect(issue.subscribed?(recipient, project)).to be_falsey
end
it 'unsubscribes from the issue when visiting the link from the header' do
visit header_link
expect(page).to have_text('unsubscribed')
expect(issue.subscribed?(recipient)).to be_falsey
expect(issue.subscribed?(recipient, project)).to be_falsey
end
end
end

View file

@ -182,19 +182,19 @@ describe Issue, "Issuable" do
before { allow(issue).to receive(:participants).with(user).and_return([]) }
it 'returns false when no subcription exists' do
expect(issue.subscribed?(user)).to be_falsey
expect(issue.subscribed?(user, project)).to be_falsey
end
it 'returns true when a subcription exists and subscribed is true' do
issue.subscriptions.create(user: user, project: project, subscribed: true)
expect(issue.subscribed?(user)).to be_truthy
expect(issue.subscribed?(user, project)).to be_truthy
end
it 'returns false when a subcription exists and subscribed is false' do
issue.subscriptions.create(user: user, project: project, subscribed: false)
expect(issue.subscribed?(user)).to be_falsey
expect(issue.subscribed?(user, project)).to be_falsey
end
end
@ -202,19 +202,19 @@ describe Issue, "Issuable" do
before { allow(issue).to receive(:participants).with(user).and_return([user]) }
it 'returns false when no subcription exists' do
expect(issue.subscribed?(user)).to be_truthy
expect(issue.subscribed?(user, project)).to be_truthy
end
it 'returns true when a subcription exists and subscribed is true' do
issue.subscriptions.create(user: user, project: project, subscribed: true)
expect(issue.subscribed?(user)).to be_truthy
expect(issue.subscribed?(user, project)).to be_truthy
end
it 'returns false when a subcription exists and subscribed is false' do
issue.subscriptions.create(user: user, project: project, subscribed: false)
expect(issue.subscribed?(user)).to be_falsey
expect(issue.subscribed?(user, project)).to be_falsey
end
end
end

View file

@ -637,7 +637,7 @@ describe API::API, api: true do
it "sends notifications for subscribers of newly added labels" do
label = project.labels.first
label.toggle_subscription(user2)
label.toggle_subscription(user2, project)
perform_enqueued_jobs do
post api("/projects/#{project.id}/issues", user),
@ -828,7 +828,7 @@ describe API::API, api: true do
it "sends notifications for subscribers of newly added labels when issue is updated" do
label = create(:label, title: 'foo', color: '#FFAABB', project: project)
label.toggle_subscription(user2)
label.toggle_subscription(user2, project)
perform_enqueued_jobs do
put api("/projects/#{project.id}/issues/#{issue.id}", user),

View file

@ -339,7 +339,7 @@ describe API::API, api: true do
end
context "when user is already subscribed to label" do
before { label1.subscribe(user) }
before { label1.subscribe(user, project) }
it "returns 304" do
post api("/projects/#{project.id}/labels/#{label1.id}/subscription", user)
@ -358,7 +358,7 @@ describe API::API, api: true do
end
describe "DELETE /projects/:id/labels/:label_id/subscription" do
before { label1.subscribe(user) }
before { label1.subscribe(user, project) }
context "when label_id is a label title" do
it "unsubscribes from the label" do
@ -381,7 +381,7 @@ describe API::API, api: true do
end
context "when user is already unsubscribed from label" do
before { label1.unsubscribe(user) }
before { label1.unsubscribe(user, project) }
it "returns 304" do
delete api("/projects/#{project.id}/labels/#{label1.id}/subscription", user)

View file

@ -260,7 +260,7 @@ describe Issuable::BulkUpdateService, services: true do
it 'subscribes the given user' do
bulk_update(issues, subscription_event: 'subscribe')
expect(issues).to all(be_subscribed(user))
expect(issues).to all(be_subscribed(user, project))
end
end
@ -275,7 +275,7 @@ describe Issuable::BulkUpdateService, services: true do
bulk_update(issues, subscription_event: 'unsubscribe')
issues.each do |issue|
expect(issue).not_to be_subscribed(user)
expect(issue).not_to be_subscribed(user, project)
end
end
end

View file

@ -215,7 +215,7 @@ describe Issues::UpdateService, services: true do
let!(:subscriber) do
create(:user).tap do |u|
label.toggle_subscription(u)
label.toggle_subscription(u, project)
project.team << [u, :developer]
end
end

View file

@ -199,7 +199,7 @@ describe MergeRequests::UpdateService, services: true do
context 'when the issue is relabeled' do
let!(:non_subscriber) { create(:user) }
let!(:subscriber) { create(:user).tap { |u| label.toggle_subscription(u) } }
let!(:subscriber) { create(:user) { |u| label.toggle_subscription(u, project) } }
before do
project.team << [non_subscriber, :developer]

View file

@ -385,7 +385,7 @@ describe NotificationService, services: true do
label = create(:label, project: project, issues: [issue])
group_label = create(:group_label, group: group, issues: [issue])
issue.reload
label.toggle_subscription(user_1)
label.toggle_subscription(user_1, project)
group_label.toggle_subscription(user_2, project)
group_label.toggle_subscription(user_3, another_project)
@ -411,12 +411,12 @@ describe NotificationService, services: true do
label = create(:label, project: project, issues: [confidential_issue])
confidential_issue.reload
label.toggle_subscription(non_member)
label.toggle_subscription(author)
label.toggle_subscription(assignee)
label.toggle_subscription(member)
label.toggle_subscription(guest)
label.toggle_subscription(admin)
label.toggle_subscription(non_member, project)
label.toggle_subscription(author, project)
label.toggle_subscription(assignee, project)
label.toggle_subscription(member, project)
label.toggle_subscription(guest, project)
label.toggle_subscription(admin, project)
reset_delivered_emails!
@ -568,11 +568,11 @@ describe NotificationService, services: true do
let(:group_label_2) { create(:group_label, group: group, title: 'Group Label 2') }
let(:label_1) { create(:label, project: project, title: 'Label 1', issues: [issue]) }
let(:label_2) { create(:label, project: project, title: 'Label 2') }
let!(:subscriber_to_group_label_1) { create(:user).tap { |u| group_label_1.toggle_subscription(u, project) } }
let!(:subscriber_to_group_label_2) { create(:user).tap { |u| group_label_2.toggle_subscription(u, project) } }
let!(:subscriber_to_group_label_2_on_another_project) { create(:user).tap { |u| group_label_2.toggle_subscription(u, another_project) } }
let!(:subscriber_to_label_1) { create(:user).tap { |u| label_1.toggle_subscription(u) } }
let!(:subscriber_to_label_2) { create(:user).tap { |u| label_2.toggle_subscription(u) } }
let!(:subscriber_to_group_label_1) { create(:user) { |u| group_label_1.toggle_subscription(u, project) } }
let!(:subscriber_to_group_label_2) { create(:user) { |u| group_label_2.toggle_subscription(u, project) } }
let!(:subscriber_to_group_label_2_on_another_project) { create(:user) { |u| group_label_2.toggle_subscription(u, another_project) } }
let!(:subscriber_to_label_1) { create(:user) { |u| label_1.toggle_subscription(u, project) } }
let!(:subscriber_to_label_2) { create(:user) { |u| label_2.toggle_subscription(u, project) } }
it "emails subscribers of the issue's added labels only" do
notification.relabeled_issue(issue, [group_label_2, label_2], @u_disabled)
@ -618,12 +618,12 @@ describe NotificationService, services: true do
project.team << [member, :developer]
project.team << [guest, :guest]
label_2.toggle_subscription(non_member)
label_2.toggle_subscription(author)
label_2.toggle_subscription(assignee)
label_2.toggle_subscription(member)
label_2.toggle_subscription(guest)
label_2.toggle_subscription(admin)
label_2.toggle_subscription(non_member, project)
label_2.toggle_subscription(author, project)
label_2.toggle_subscription(assignee, project)
label_2.toggle_subscription(member, project)
label_2.toggle_subscription(guest, project)
label_2.toggle_subscription(admin, project)
reset_delivered_emails!
@ -786,7 +786,7 @@ describe NotificationService, services: true do
user_3 = create(:user)
label = create(:label, project: project, merge_requests: [merge_request])
group_label = create(:group_label, group: group, merge_requests: [merge_request])
label.toggle_subscription(user_1)
label.toggle_subscription(user_1, project)
group_label.toggle_subscription(user_2, project)
group_label.toggle_subscription(user_3, another_project)
@ -892,11 +892,11 @@ describe NotificationService, services: true do
let(:group_label_2) { create(:group_label, group: group, title: 'Group Label 2') }
let(:label_1) { create(:label, project: project, title: 'Label 1', merge_requests: [merge_request]) }
let(:label_2) { create(:label, project: project, title: 'Label 2') }
let!(:subscriber_to_group_label_1) { create(:user).tap { |u| group_label_1.toggle_subscription(u, project) } }
let!(:subscriber_to_group_label_2) { create(:user).tap { |u| group_label_2.toggle_subscription(u, project) } }
let!(:subscriber_to_group_label_2_on_another_project) { create(:user).tap { |u| group_label_2.toggle_subscription(u, another_project) } }
let!(:subscriber_to_label_1) { create(:user).tap { |u| label_1.toggle_subscription(u) } }
let!(:subscriber_to_label_2) { create(:user).tap { |u| label_2.toggle_subscription(u) } }
let!(:subscriber_to_group_label_1) { create(:user) { |u| group_label_1.toggle_subscription(u, project) } }
let!(:subscriber_to_group_label_2) { create(:user) { |u| group_label_2.toggle_subscription(u, project) } }
let!(:subscriber_to_group_label_2_on_another_project) { create(:user) { |u| group_label_2.toggle_subscription(u, another_project) } }
let!(:subscriber_to_label_1) { create(:user) { |u| label_1.toggle_subscription(u, project) } }
let!(:subscriber_to_label_2) { create(:user) { |u| label_2.toggle_subscription(u, project) } }
it "emails subscribers of the merge request's added labels only" do
notification.relabeled_merge_request(merge_request, [group_label_2, label_2], @u_disabled)

View file

@ -169,7 +169,7 @@ describe SlashCommands::InterpretService, services: true do
shared_examples 'unsubscribe command' do
it 'populates subscription_event: "unsubscribe" if content contains /unsubscribe' do
issuable.subscribe(developer)
issuable.subscribe(developer, project)
_, updates = service.execute(content, issuable)
expect(updates).to eq(subscription_event: 'unsubscribe')
@ -321,7 +321,7 @@ describe SlashCommands::InterpretService, services: true do
it_behaves_like 'multiple label with same argument' do
let(:content) { %(/label ~"#{inprogress.title}" \n/label ~#{inprogress.title}) }
let(:issuable) { issue }
end
end
it_behaves_like 'unlabel command' do
let(:content) { %(/unlabel ~"#{inprogress.title}") }

View file

@ -230,31 +230,31 @@ shared_examples 'issuable record that supports slash commands in its description
context "with a note subscribing to the #{issuable_type}" do
it "creates a new todo for the #{issuable_type}" do
expect(issuable.subscribed?(master)).to be_falsy
expect(issuable.subscribed?(master, project)).to be_falsy
write_note("/subscribe")
expect(page).not_to have_content '/subscribe'
expect(page).to have_content 'Your commands have been executed!'
expect(issuable.subscribed?(master)).to be_truthy
expect(issuable.subscribed?(master, project)).to be_truthy
end
end
context "with a note unsubscribing to the #{issuable_type} as done" do
before do
issuable.subscribe(master)
issuable.subscribe(master, project)
end
it "creates a new todo for the #{issuable_type}" do
expect(issuable.subscribed?(master)).to be_truthy
expect(issuable.subscribed?(master, project)).to be_truthy
write_note("/unsubscribe")
expect(page).not_to have_content '/unsubscribe'
expect(page).to have_content 'Your commands have been executed!'
expect(issuable.subscribed?(master)).to be_falsy
expect(issuable.subscribed?(master, project)).to be_falsy
end
end
end