From 1b437ec3498bc544dbd1b252f5c755e9073407fd Mon Sep 17 00:00:00 2001 From: Valery Sizov Date: Mon, 16 Mar 2015 17:20:17 +0200 Subject: [PATCH] tests --- app/assets/javascripts/subscription.js.coffee | 3 +- app/controllers/projects/issues_controller.rb | 9 ++---- .../projects/merge_requests_controller.rb | 9 ++---- app/models/concerns/issuable.rb | 8 ++++- app/models/subscription.rb | 13 ++++++++ app/services/notification_service.rb | 4 ++- .../projects/issues/_issue_context.html.haml | 6 ++-- .../merge_requests/show/_context.html.haml | 6 ++-- config/routes.rb | 4 +-- ...150313012111_create_subscriptions_table.rb | 5 ++- features/project/issues/issues.feature | 8 +++++ features/project/merge_requests.feature | 7 ++++ features/steps/project/issues/issues.rb | 13 ++++++++ features/steps/project/merge_requests.rb | 13 ++++++++ spec/services/notification_service_spec.rb | 32 +++++++++++++++++++ 15 files changed, 115 insertions(+), 25 deletions(-) diff --git a/app/assets/javascripts/subscription.js.coffee b/app/assets/javascripts/subscription.js.coffee index f457622fc3a..a009969e4dc 100644 --- a/app/assets/javascripts/subscription.js.coffee +++ b/app/assets/javascripts/subscription.js.coffee @@ -1,14 +1,13 @@ class @Subscription constructor: (url) -> $(".subscribe-button").click (event)=> - self = @ btn = $(event.currentTarget) action = btn.prop("value") current_status = $(".sub_status").text().trim() $(".fa-spinner.subscription").removeClass("hidden") $(".sub_status").empty() - $.post url, subscription: action, => + $.post url, => $(".fa-spinner.subscription").addClass("hidden") status = if current_status == "subscribed" then "unsubscribed" else "subscribed" $(".sub_status").text(status) diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb index 903b7a68dc9..88302276b5e 100644 --- a/app/controllers/projects/issues_controller.rb +++ b/app/controllers/projects/issues_controller.rb @@ -1,6 +1,6 @@ class Projects::IssuesController < Projects::ApplicationController before_filter :module_enabled - before_filter :issue, only: [:edit, :update, :show, :set_subscription] + before_filter :issue, only: [:edit, :update, :show, :toggle_subscription] # Allow read any issue before_filter :authorize_read_issue! @@ -97,11 +97,8 @@ class Projects::IssuesController < Projects::ApplicationController redirect_to :back, notice: "#{result[:count]} issues updated" end - def set_subscription - subscribed = params[:subscription] == "Subscribe" - - sub = @issue.subscriptions.find_or_create_by(user_id: current_user.id) - sub.update(subscribed: subscribed) + def toggle_subscription + @issue.toggle_subscription(current_user) render nothing: true end diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index 51ac61c3273..c63a9b0cd44 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -2,7 +2,7 @@ require 'gitlab/satellite/satellite' class Projects::MergeRequestsController < Projects::ApplicationController before_filter :module_enabled - before_filter :merge_request, only: [:edit, :update, :show, :diffs, :automerge, :automerge_check, :ci_status, :set_subscription] + before_filter :merge_request, only: [:edit, :update, :show, :diffs, :automerge, :automerge_check, :ci_status, :toggle_subscription] before_filter :closes_issues, only: [:edit, :update, :show, :diffs] before_filter :validates_merge_request, only: [:show, :diffs] before_filter :define_show_vars, only: [:show, :diffs] @@ -174,11 +174,8 @@ class Projects::MergeRequestsController < Projects::ApplicationController render json: response end - def set_subscription - subscribed = params[:subscription] == "Subscribe" - - sub = @merge_request.subscriptions.find_or_create_by(user_id: current_user.id) - sub.update(subscribed: subscribed) + def toggle_subscription + @merge_request.toggle_subscription(current_user) render nothing: true end diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb index d1a35ca5294..88ac83744df 100644 --- a/app/models/concerns/issuable.rb +++ b/app/models/concerns/issuable.rb @@ -133,7 +133,7 @@ module Issuable users.concat(mentions.reduce([], :|)).uniq end - def subscription_status(user) + def subscribed?(user) subscription = subscriptions.find_by_user_id(user.id) if subscription @@ -143,6 +143,12 @@ module Issuable participants.include?(user) end + def toggle_subscription(user) + subscriptions. + find_or_initialize_by(user_id: user.id). + update(subscribed: !subscribed?(user)) + end + def to_hook_data(user) { object_kind: self.class.name.underscore, diff --git a/app/models/subscription.rb b/app/models/subscription.rb index 276cf0e9465..dd75d3ab8ba 100644 --- a/app/models/subscription.rb +++ b/app/models/subscription.rb @@ -1,3 +1,16 @@ +# == Schema Information +# +# Table name: subscriptions +# +# id :integer not null, primary key +# user_id :integer +# subscribable_id :integer +# subscribable_type :string(255) +# subscribed :boolean +# created_at :datetime +# updated_at :datetime +# + class Subscription < ActiveRecord::Base belongs_to :user belongs_to :subscribable, polymorphic: true diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb index 5ebde8fea84..3e1f4e62f10 100644 --- a/app/services/notification_service.rb +++ b/app/services/notification_service.rb @@ -92,6 +92,8 @@ class NotificationService # def merge_mr(merge_request, current_user) recipients = reject_muted_users([merge_request.author, merge_request.assignee], merge_request.target_project) + recipients = add_subscribed_users(recipients, merge_request) + recipients = reject_unsubscribed_users(recipients, merge_request) recipients = recipients.concat(project_watchers(merge_request.target_project)).uniq recipients.delete(current_user) @@ -333,7 +335,7 @@ class NotificationService subscriptions = target.subscriptions if subscriptions.any? - recipients + subscriptions.where("subscribed is true").map(&:user) + recipients + subscriptions.where(subscribed: true).map(&:user) else recipients end diff --git a/app/views/projects/issues/_issue_context.html.haml b/app/views/projects/issues/_issue_context.html.haml index 24bfbdd4c58..85937e7bf42 100644 --- a/app/views/projects/issues/_issue_context.html.haml +++ b/app/views/projects/issues/_issue_context.html.haml @@ -33,12 +33,12 @@ Subscription: %i.fa.fa-spinner.fa-spin.hidden.subscription %span.sub_status - = @issue.subscription_status(current_user) ? "subscribed" : "unsubscribed" - - subscribe_action = @issue.subscription_status(current_user) ? "Unsubscribe" : "Subscribe" + = @issue.subscribed?(current_user) ? "subscribed" : "unsubscribed" + - subscribe_action = @issue.subscribed?(current_user) ? "Unsubscribe" : "Subscribe" %input.btn.subscribe-button{:type => "button", :value => subscribe_action} :coffeescript $ -> - new Subscription("#{set_subscription_namespace_project_issue_path(@issue.project.namespace, @project, @issue)}") + new Subscription("#{toggle_subscription_namespace_project_issue_path(@issue.project.namespace, @project, @issue)}") \ No newline at end of file diff --git a/app/views/projects/merge_requests/show/_context.html.haml b/app/views/projects/merge_requests/show/_context.html.haml index d0c00c1aeaf..79b0e7799a9 100644 --- a/app/views/projects/merge_requests/show/_context.html.haml +++ b/app/views/projects/merge_requests/show/_context.html.haml @@ -35,12 +35,12 @@ Subscription: %i.fa.fa-spinner.fa-spin.hidden.subscription %span.sub_status - = @merge_request.subscription_status(current_user) ? "subscribed" : "unsubscribed" - - subscribe_action = @merge_request.subscription_status(current_user) ? "Unsubscribe" : "Subscribe" + = @merge_request.subscribed?(current_user) ? "subscribed" : "unsubscribed" + - subscribe_action = @merge_request.subscribed?(current_user) ? "Unsubscribe" : "Subscribe" %input.btn.subscribe-button{:type => "button", :value => subscribe_action} :coffeescript $ -> - new Subscription("#{set_subscription_namespace_project_issue_path(@merge_request.project.namespace, @project, @merge_request)}") + new Subscription("#{toggle_subscription_namespace_project_merge_request_path(@merge_request.project.namespace, @project, @merge_request)}") \ No newline at end of file diff --git a/config/routes.rb b/config/routes.rb index a976ba9d593..ad5f2c10f63 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -406,7 +406,7 @@ Gitlab::Application.routes.draw do post :automerge get :automerge_check get :ci_status - post :set_subscription + post :toggle_subscription end collection do @@ -442,7 +442,7 @@ Gitlab::Application.routes.draw do resources :issues, constraints: { id: /\d+/ }, except: [:destroy] do member do - post :set_subscription + post :toggle_subscription end collection do post :bulk_update diff --git a/db/migrate/20150313012111_create_subscriptions_table.rb b/db/migrate/20150313012111_create_subscriptions_table.rb index 78f7aeeaf7c..a1d4d9dedc5 100644 --- a/db/migrate/20150313012111_create_subscriptions_table.rb +++ b/db/migrate/20150313012111_create_subscriptions_table.rb @@ -8,6 +8,9 @@ class CreateSubscriptionsTable < ActiveRecord::Migration t.timestamps end - add_index :subscriptions, [:subscribable_id, :subscribable_type, :user_id], unique: true, name: 'subscriptions_user_id_and_ref_fields' + add_index :subscriptions, + [:subscribable_id, :subscribable_type, :user_id], + unique: true, + name: 'subscriptions_user_id_and_ref_fields' end end diff --git a/features/project/issues/issues.feature b/features/project/issues/issues.feature index 283979204db..b9031f6f32b 100644 --- a/features/project/issues/issues.feature +++ b/features/project/issues/issues.feature @@ -202,3 +202,11 @@ Feature: Project Issues And I click link "Edit" for the issue And I preview a description text like "Bug fixed :smile:" Then I should see the Markdown write tab + + @javascript + Scenario: I can unsubscribe from issue + Given project "Shop" has "Tasks-open" open issue with task markdown + When I visit issue page "Tasks-open" + Then I should see that I am subscribed + When I click button "Unsubscribe" + Then I should see that I am unsubscribed diff --git a/features/project/merge_requests.feature b/features/project/merge_requests.feature index adad100e56c..91dc576f8b4 100644 --- a/features/project/merge_requests.feature +++ b/features/project/merge_requests.feature @@ -225,3 +225,10 @@ Feature: Project Merge Requests When I fill in merge request search with "Fe" Then I should see "Feature NS-03" in merge requests And I should not see "Bug NS-04" in merge requests + + @javascript + Scenario: I can unsubscribe from merge request + Given I visit merge request page "Bug NS-04" + Then I should see that I am subscribed + When I click button "Unsubscribe" + Then I should see that I am unsubscribed diff --git a/features/steps/project/issues/issues.rb b/features/steps/project/issues/issues.rb index 6d72c93ad13..cc0d6033a2b 100644 --- a/features/steps/project/issues/issues.rb +++ b/features/steps/project/issues/issues.rb @@ -18,10 +18,23 @@ class Spinach::Features::ProjectIssues < Spinach::FeatureSteps page.should_not have_content "Tweet control" end + step 'I should see that I am subscribed' do + find(".sub_status").text.should == "subscribed" + end + + step 'I should see that I am unsubscribed' do + sleep 0.2 + find(".sub_status").text.should == "unsubscribed" + end + step 'I click link "Closed"' do click_link "Closed" end + step 'I click button "Unsubscribe"' do + click_on "Unsubscribe" + end + step 'I should see "Release 0.3" in issues' do page.should have_content "Release 0.3" end diff --git a/features/steps/project/merge_requests.rb b/features/steps/project/merge_requests.rb index b67b2e58caf..5a35d703765 100644 --- a/features/steps/project/merge_requests.rb +++ b/features/steps/project/merge_requests.rb @@ -56,6 +56,19 @@ class Spinach::Features::ProjectMergeRequests < Spinach::FeatureSteps page.should_not have_content "Bug NS-04" end + step 'I should see that I am subscribed' do + find(".sub_status").text.should == "subscribed" + end + + step 'I should see that I am unsubscribed' do + sleep 0.2 + find(".sub_status").text.should == "unsubscribed" + end + + step 'I click button "Unsubscribe"' do + click_on "Unsubscribe" + end + step 'I click link "Close"' do first(:css, '.close-mr-link').click end diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb index 2074f8e7f78..5badb635325 100644 --- a/spec/services/notification_service_spec.rb +++ b/spec/services/notification_service_spec.rb @@ -41,13 +41,18 @@ describe NotificationService do describe :new_note do it do + add_users_with_subscription(note.project, issue) + should_email(@u_watcher.id) should_email(note.noteable.author_id) should_email(note.noteable.assignee_id) should_email(@u_mentioned.id) + should_email(@subscriber.id) should_not_email(note.author_id) should_not_email(@u_participating.id) should_not_email(@u_disabled.id) + should_not_email(@unsubscriber.id) + notification.new_note(note) end @@ -191,6 +196,7 @@ describe NotificationService do before do build_team(issue.project) + add_users_with_subscription(issue.project, issue) end describe :new_issue do @@ -224,6 +230,8 @@ describe NotificationService do should_email(issue.assignee_id) should_email(@u_watcher.id) should_email(@u_participant_mentioned.id) + should_email(@subscriber.id) + should_not_email(@unsubscriber.id) should_not_email(@u_participating.id) should_not_email(@u_disabled.id) @@ -245,6 +253,8 @@ describe NotificationService do should_email(issue.author_id) should_email(@u_watcher.id) should_email(@u_participant_mentioned.id) + should_email(@subscriber.id) + should_not_email(@unsubscriber.id) should_not_email(@u_participating.id) should_not_email(@u_disabled.id) @@ -266,6 +276,8 @@ describe NotificationService do should_email(issue.author_id) should_email(@u_watcher.id) should_email(@u_participant_mentioned.id) + should_email(@subscriber.id) + should_not_email(@unsubscriber.id) should_not_email(@u_participating.id) should_not_email(@u_disabled.id) @@ -287,6 +299,7 @@ describe NotificationService do before do build_team(merge_request.target_project) + add_users_with_subscription(merge_request.target_project, merge_request) end describe :new_merge_request do @@ -311,6 +324,8 @@ describe NotificationService do it do should_email(merge_request.assignee_id) should_email(@u_watcher.id) + should_email(@subscriber.id) + should_not_email(@unsubscriber.id) should_not_email(@u_participating.id) should_not_email(@u_disabled.id) notification.reassigned_merge_request(merge_request, merge_request.author) @@ -329,6 +344,8 @@ describe NotificationService do it do should_email(merge_request.assignee_id) should_email(@u_watcher.id) + should_email(@subscriber.id) + should_not_email(@unsubscriber.id) should_not_email(@u_participating.id) should_not_email(@u_disabled.id) notification.close_mr(merge_request, @u_disabled) @@ -347,6 +364,8 @@ describe NotificationService do it do should_email(merge_request.assignee_id) should_email(@u_watcher.id) + should_email(@subscriber.id) + should_not_email(@unsubscriber.id) should_not_email(@u_participating.id) should_not_email(@u_disabled.id) notification.merge_mr(merge_request, @u_disabled) @@ -365,6 +384,8 @@ describe NotificationService do it do should_email(merge_request.assignee_id) should_email(@u_watcher.id) + should_email(@subscriber.id) + should_not_email(@unsubscriber.id) should_not_email(@u_participating.id) should_not_email(@u_disabled.id) notification.reopen_mr(merge_request, @u_disabled) @@ -420,4 +441,15 @@ describe NotificationService do project.team << [@u_mentioned, :master] project.team << [@u_committer, :master] end + + def add_users_with_subscription(project, issuable) + @subscriber = create :user + @unsubscriber = create :user + + project.team << [@subscriber, :master] + project.team << [@unsubscriber, :master] + + issuable.subscriptions.create(user: @subscriber, subscribed: true) + issuable.subscriptions.create(user: @unsubscriber, subscribed: false) + end end