From 9698b36c1cd0808adb006593c0e8649cb42f3571 Mon Sep 17 00:00:00 2001 From: Valery Sizov Date: Sun, 15 Mar 2015 18:17:12 +0200 Subject: [PATCH 01/12] Subscription --- app/assets/javascripts/subscription.js.coffee | 18 ++++++++++++++++++ app/controllers/projects/issues_controller.rb | 11 ++++++++++- .../projects/merge_requests_controller.rb | 11 ++++++++++- app/models/concerns/issuable.rb | 10 ++++++++++ app/models/subscribe.rb | 3 +++ app/services/notification_service.rb | 18 ++++++++++++++++++ .../projects/issues/_issue_context.html.haml | 16 ++++++++++++++++ .../merge_requests/show/_context.html.haml | 16 ++++++++++++++++ config/routes.rb | 4 ++++ .../20150313012111_create_subscribes_table.rb | 12 ++++++++++++ db/schema.rb | 11 ++++++++++- 11 files changed, 127 insertions(+), 3 deletions(-) create mode 100644 app/assets/javascripts/subscription.js.coffee create mode 100644 app/models/subscribe.rb create mode 100644 db/migrate/20150313012111_create_subscribes_table.rb diff --git a/app/assets/javascripts/subscription.js.coffee b/app/assets/javascripts/subscription.js.coffee new file mode 100644 index 0000000000..f457622fc3 --- /dev/null +++ b/app/assets/javascripts/subscription.js.coffee @@ -0,0 +1,18 @@ +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, => + $(".fa-spinner.subscription").addClass("hidden") + status = if current_status == "subscribed" then "unsubscribed" else "subscribed" + $(".sub_status").text(status) + action = if status == "subscribed" then "Unsubscribe" else "Subscribe" + btn.prop("value", action) + + diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb index 4266bcaef1..4eb5092b9d 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] + before_filter :issue, only: [:edit, :update, :show, :set_subscription] # Allow read any issue before_filter :authorize_read_issue! @@ -97,6 +97,15 @@ class Projects::IssuesController < Projects::ApplicationController redirect_to :back, notice: "#{result[:count]} issues updated" end + def set_subscription + subscribed = params[:subscription] == "Subscribe" + + sub = @issue.subscribes.find_or_create_by(user_id: current_user.id) + sub.update(subscribed: subscribed) + + render nothing: true + end + protected def issue diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index 93d79d8166..5613eee35c 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] + before_filter :merge_request, only: [:edit, :update, :show, :diffs, :automerge, :automerge_check, :ci_status, :set_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,6 +174,15 @@ class Projects::MergeRequestsController < Projects::ApplicationController render json: response end + def set_subscription + subscribed = params[:subscription] == "Subscribe" + + sub = @merge_request.subscribes.find_or_create_by(user_id: current_user.id) + sub.update(subscribed: subscribed) + + render nothing: true + end + protected def selected_target_project diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb index f5e23e9dc2..e89dcbf9ac 100644 --- a/app/models/concerns/issuable.rb +++ b/app/models/concerns/issuable.rb @@ -15,6 +15,7 @@ module Issuable has_many :notes, as: :noteable, dependent: :destroy has_many :label_links, as: :target, dependent: :destroy has_many :labels, through: :label_links + has_many :subscribes, dependent: :destroy validates :author, presence: true validates :title, presence: true, length: { within: 0..255 } @@ -132,6 +133,15 @@ module Issuable users.concat(mentions.reduce([], :|)).uniq end + def subscribe_status(user) + sub = subscribes.find_by_user_id(user.id) + if sub + return sub.subscribed + end + + participants.include?(user) + end + def to_hook_data(user) { object_kind: self.class.name.underscore, diff --git a/app/models/subscribe.rb b/app/models/subscribe.rb new file mode 100644 index 0000000000..a68546667f --- /dev/null +++ b/app/models/subscribe.rb @@ -0,0 +1,3 @@ +class Subscribe < ActiveRecord::Base + belongs_to :user +end diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb index 0063b7ce40..4fa775a28c 100644 --- a/app/services/notification_service.rb +++ b/app/services/notification_service.rb @@ -314,6 +314,13 @@ class NotificationService end end + def reject_unsubscribed_users(recipients, target) + recipients.reject do |user| + subscribe = target.subscribes.find_by_user_id(user.id) + subscribe && !subscribe.subscribed + end + end + def new_resource_email(target, project, method) recipients = build_recipients(target, project) recipients.delete(target.author) @@ -361,10 +368,21 @@ class NotificationService recipients = reject_muted_users(recipients, project) recipients = reject_mention_users(recipients, project) + recipients = add_subscribers(recipients, project) recipients = recipients.concat(project_watchers(project)).uniq + recipients = reject_unsubscribed_users(recipients, target) recipients end + def add_subscribers(recipients, target) + subs = target.subscribes + if subs.any? + recipients.merge(subs.where("subscribed is true").map(&:user)) + else + recipients + end + end + def mailer Notify.delay end diff --git a/app/views/projects/issues/_issue_context.html.haml b/app/views/projects/issues/_issue_context.html.haml index 4c7654354f..09c531ac7f 100644 --- a/app/views/projects/issues/_issue_context.html.haml +++ b/app/views/projects/issues/_issue_context.html.haml @@ -26,3 +26,19 @@ = f.select(:milestone_id, milestone_options(@issue), { include_blank: "Select milestone" }, {class: 'select2 select2-compact js-select2 js-milestone'}) = hidden_field_tag :issue_context = f.submit class: 'btn' + + %div.prepend-top-20.clearfix + .issuable-context-title + %label + Subscription: + %i.fa.fa-spinner.fa-spin.hidden.subscription + %span.sub_status + = @issue.subscribe_status(current_user) ? "subscribed" : "unsubscribed" + - subscribe_action = @issue.subscribe_status(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)}") + + \ 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 a74f3fb24e..aae0aa24ed 100644 --- a/app/views/projects/merge_requests/show/_context.html.haml +++ b/app/views/projects/merge_requests/show/_context.html.haml @@ -28,3 +28,19 @@ = f.select(:milestone_id, milestone_options(@merge_request), { include_blank: "Select milestone" }, {class: 'select2 select2-compact js-select2 js-milestone'}) = hidden_field_tag :merge_request_context = f.submit class: 'btn' + + %div.prepend-top-20.clearfix + .issuable-context-title + %label + Subscription: + %i.fa.fa-spinner.fa-spin.hidden.subscription + %span.sub_status + = @merge_request.subscribe_status(current_user) ? "subscribed" : "unsubscribed" + - subscribe_action = @merge_request.subscribe_status(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)}") + + \ No newline at end of file diff --git a/config/routes.rb b/config/routes.rb index 889995e92a..a976ba9d59 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -406,6 +406,7 @@ Gitlab::Application.routes.draw do post :automerge get :automerge_check get :ci_status + post :set_subscription end collection do @@ -440,6 +441,9 @@ Gitlab::Application.routes.draw do end resources :issues, constraints: { id: /\d+/ }, except: [:destroy] do + member do + post :set_subscription + end collection do post :bulk_update end diff --git a/db/migrate/20150313012111_create_subscribes_table.rb b/db/migrate/20150313012111_create_subscribes_table.rb new file mode 100644 index 0000000000..706cf77118 --- /dev/null +++ b/db/migrate/20150313012111_create_subscribes_table.rb @@ -0,0 +1,12 @@ +class CreateSubscribesTable < ActiveRecord::Migration + def change + create_table :subscribes do |t| + t.integer :user_id + t.integer :merge_request_id + t.integer :issue_id + t.boolean :subscribed + + t.timestamps + end + end +end diff --git a/db/schema.rb b/db/schema.rb index 3afbc082b7..6afb79069e 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20150306023112) do +ActiveRecord::Schema.define(version: 20150313012111) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -397,6 +397,15 @@ ActiveRecord::Schema.define(version: 20150306023112) do add_index "snippets", ["project_id"], name: "index_snippets_on_project_id", using: :btree add_index "snippets", ["visibility_level"], name: "index_snippets_on_visibility_level", using: :btree + create_table "subscribes", force: true do |t| + t.integer "user_id" + t.integer "merge_request_id" + t.integer "issue_id" + t.boolean "subscribed" + t.datetime "created_at" + t.datetime "updated_at" + end + create_table "taggings", force: true do |t| t.integer "tag_id" t.integer "taggable_id" From 09ef69b7c8e534cedff2ca1a42b58f534e349107 Mon Sep 17 00:00:00 2001 From: Valery Sizov Date: Mon, 16 Mar 2015 13:52:45 +0200 Subject: [PATCH 02/12] code folding fix --- app/models/concerns/issuable.rb | 6 +++--- app/models/subscribe.rb | 3 +++ app/services/notification_service.rb | 10 +++++----- db/migrate/20150313012111_create_subscribes_table.rb | 4 ++++ db/schema.rb | 4 ++++ 5 files changed, 19 insertions(+), 8 deletions(-) diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb index e89dcbf9ac..c74d9cb991 100644 --- a/app/models/concerns/issuable.rb +++ b/app/models/concerns/issuable.rb @@ -134,9 +134,9 @@ module Issuable end def subscribe_status(user) - sub = subscribes.find_by_user_id(user.id) - if sub - return sub.subscribed + subscribe = subscribes.find_by_user_id(user.id) + if subscribe + return subscribe.subscribed end participants.include?(user) diff --git a/app/models/subscribe.rb b/app/models/subscribe.rb index a68546667f..be8b9e7605 100644 --- a/app/models/subscribe.rb +++ b/app/models/subscribe.rb @@ -1,3 +1,6 @@ class Subscribe < ActiveRecord::Base belongs_to :user + + validates :issue_id, uniqueness: { scope: :user_id, allow_nil: true } + validates :merge_request_id, uniqueness: { scope: :user_id, allow_nil: true } end diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb index 4fa775a28c..edfb62a4b1 100644 --- a/app/services/notification_service.rb +++ b/app/services/notification_service.rb @@ -368,16 +368,16 @@ class NotificationService recipients = reject_muted_users(recipients, project) recipients = reject_mention_users(recipients, project) - recipients = add_subscribers(recipients, project) + recipients = add_subscribed_users(recipients, project) recipients = recipients.concat(project_watchers(project)).uniq recipients = reject_unsubscribed_users(recipients, target) recipients end - def add_subscribers(recipients, target) - subs = target.subscribes - if subs.any? - recipients.merge(subs.where("subscribed is true").map(&:user)) + def add_subscribed_users(recipients, target) + subscribes = target.subscribes + if subscribes.any? + recipients.merge(subscribes.where("subscribed is true").map(&:user)) else recipients end diff --git a/db/migrate/20150313012111_create_subscribes_table.rb b/db/migrate/20150313012111_create_subscribes_table.rb index 706cf77118..ab0e9a2a5b 100644 --- a/db/migrate/20150313012111_create_subscribes_table.rb +++ b/db/migrate/20150313012111_create_subscribes_table.rb @@ -8,5 +8,9 @@ class CreateSubscribesTable < ActiveRecord::Migration t.timestamps end + + add_index :subscribes, :user_id + add_index :subscribes, :issue_id + add_index :subscribes, :merge_request_id end end diff --git a/db/schema.rb b/db/schema.rb index 6afb79069e..46663ad495 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -406,6 +406,10 @@ ActiveRecord::Schema.define(version: 20150313012111) do t.datetime "updated_at" end + add_index "subscribes", ["issue_id"], name: "index_subscribes_on_issue_id", using: :btree + add_index "subscribes", ["merge_request_id"], name: "index_subscribes_on_merge_request_id", using: :btree + add_index "subscribes", ["user_id"], name: "index_subscribes_on_user_id", using: :btree + create_table "taggings", force: true do |t| t.integer "tag_id" t.integer "taggable_id" From 0e20dc910f25db3b3f71867d54367db36334ff45 Mon Sep 17 00:00:00 2001 From: Valery Sizov Date: Mon, 16 Mar 2015 13:58:34 +0200 Subject: [PATCH 03/12] update changelog --- CHANGELOG | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG b/CHANGELOG index 97376c85ec..6c5ba28f0b 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -61,6 +61,7 @@ v 7.9.0 (unreleased) - Allow smb:// links in Markdown text. - Filter merge request by title or description at Merge Requests page - Block user if he/she was blocked in Active Directory + - Ability to unsubscribe/subscribe to issue or merge request v 7.8.4 - Fix issue_tracker_id substitution in custom issue trackers From 410d25c8ca8afabb25e5f89b36e3cfd09ffe6f87 Mon Sep 17 00:00:00 2001 From: Valery Sizov Date: Mon, 16 Mar 2015 15:22:50 +0200 Subject: [PATCH 04/12] rename table subscribe; make it polymorfic --- app/controllers/projects/issues_controller.rb | 2 +- .../projects/merge_requests_controller.rb | 2 +- app/models/concerns/issuable.rb | 11 ++++++----- app/models/subscribe.rb | 6 ------ app/models/subscription.rb | 7 +++++++ app/services/notification_service.rb | 10 +++++----- .../projects/issues/_issue_context.html.haml | 4 ++-- .../merge_requests/show/_context.html.haml | 4 ++-- .../20150313012111_create_subscribes_table.rb | 16 ---------------- .../20150313012111_create_subscriptions_table.rb | 13 +++++++++++++ db/schema.rb | 10 ++++------ 11 files changed, 41 insertions(+), 44 deletions(-) delete mode 100644 app/models/subscribe.rb create mode 100644 app/models/subscription.rb delete mode 100644 db/migrate/20150313012111_create_subscribes_table.rb create mode 100644 db/migrate/20150313012111_create_subscriptions_table.rb diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb index 4eb5092b9d..903b7a68dc 100644 --- a/app/controllers/projects/issues_controller.rb +++ b/app/controllers/projects/issues_controller.rb @@ -100,7 +100,7 @@ class Projects::IssuesController < Projects::ApplicationController def set_subscription subscribed = params[:subscription] == "Subscribe" - sub = @issue.subscribes.find_or_create_by(user_id: current_user.id) + sub = @issue.subscriptions.find_or_create_by(user_id: current_user.id) sub.update(subscribed: subscribed) render nothing: true diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index 5613eee35c..51ac61c327 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -177,7 +177,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController def set_subscription subscribed = params[:subscription] == "Subscribe" - sub = @merge_request.subscribes.find_or_create_by(user_id: current_user.id) + sub = @merge_request.subscriptions.find_or_create_by(user_id: current_user.id) sub.update(subscribed: subscribed) render nothing: true diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb index c74d9cb991..d1a35ca529 100644 --- a/app/models/concerns/issuable.rb +++ b/app/models/concerns/issuable.rb @@ -15,7 +15,7 @@ module Issuable has_many :notes, as: :noteable, dependent: :destroy has_many :label_links, as: :target, dependent: :destroy has_many :labels, through: :label_links - has_many :subscribes, dependent: :destroy + has_many :subscriptions, dependent: :destroy, as: :subscribable validates :author, presence: true validates :title, presence: true, length: { within: 0..255 } @@ -133,10 +133,11 @@ module Issuable users.concat(mentions.reduce([], :|)).uniq end - def subscribe_status(user) - subscribe = subscribes.find_by_user_id(user.id) - if subscribe - return subscribe.subscribed + def subscription_status(user) + subscription = subscriptions.find_by_user_id(user.id) + + if subscription + return subscription.subscribed end participants.include?(user) diff --git a/app/models/subscribe.rb b/app/models/subscribe.rb deleted file mode 100644 index be8b9e7605..0000000000 --- a/app/models/subscribe.rb +++ /dev/null @@ -1,6 +0,0 @@ -class Subscribe < ActiveRecord::Base - belongs_to :user - - validates :issue_id, uniqueness: { scope: :user_id, allow_nil: true } - validates :merge_request_id, uniqueness: { scope: :user_id, allow_nil: true } -end diff --git a/app/models/subscription.rb b/app/models/subscription.rb new file mode 100644 index 0000000000..7e57a8570e --- /dev/null +++ b/app/models/subscription.rb @@ -0,0 +1,7 @@ +class Subscription < ActiveRecord::Base + belongs_to :subscribable, polymorphic: true + + validates :user_id, + uniqueness: { scope: [:subscribable_id, :subscribable_type]}, + presence: true +end diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb index edfb62a4b1..e02418b724 100644 --- a/app/services/notification_service.rb +++ b/app/services/notification_service.rb @@ -316,8 +316,8 @@ class NotificationService def reject_unsubscribed_users(recipients, target) recipients.reject do |user| - subscribe = target.subscribes.find_by_user_id(user.id) - subscribe && !subscribe.subscribed + subscription = target.subscriptions.find_by_user_id(user.id) + subscription && !subscription.subscribed end end @@ -375,9 +375,9 @@ class NotificationService end def add_subscribed_users(recipients, target) - subscribes = target.subscribes - if subscribes.any? - recipients.merge(subscribes.where("subscribed is true").map(&:user)) + subscriptions = target.subscriptions + if subscriptions.any? + recipients.merge(subscriptions.where("subscribed is 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 09c531ac7f..24bfbdd4c5 100644 --- a/app/views/projects/issues/_issue_context.html.haml +++ b/app/views/projects/issues/_issue_context.html.haml @@ -33,8 +33,8 @@ Subscription: %i.fa.fa-spinner.fa-spin.hidden.subscription %span.sub_status - = @issue.subscribe_status(current_user) ? "subscribed" : "unsubscribed" - - subscribe_action = @issue.subscribe_status(current_user) ? "Unsubscribe" : "Subscribe" + = @issue.subscription_status(current_user) ? "subscribed" : "unsubscribed" + - subscribe_action = @issue.subscription_status(current_user) ? "Unsubscribe" : "Subscribe" %input.btn.subscribe-button{:type => "button", :value => subscribe_action} :coffeescript diff --git a/app/views/projects/merge_requests/show/_context.html.haml b/app/views/projects/merge_requests/show/_context.html.haml index aae0aa24ed..d0c00c1aea 100644 --- a/app/views/projects/merge_requests/show/_context.html.haml +++ b/app/views/projects/merge_requests/show/_context.html.haml @@ -35,8 +35,8 @@ Subscription: %i.fa.fa-spinner.fa-spin.hidden.subscription %span.sub_status - = @merge_request.subscribe_status(current_user) ? "subscribed" : "unsubscribed" - - subscribe_action = @merge_request.subscribe_status(current_user) ? "Unsubscribe" : "Subscribe" + = @merge_request.subscription_status(current_user) ? "subscribed" : "unsubscribed" + - subscribe_action = @merge_request.subscription_status(current_user) ? "Unsubscribe" : "Subscribe" %input.btn.subscribe-button{:type => "button", :value => subscribe_action} :coffeescript diff --git a/db/migrate/20150313012111_create_subscribes_table.rb b/db/migrate/20150313012111_create_subscribes_table.rb deleted file mode 100644 index ab0e9a2a5b..0000000000 --- a/db/migrate/20150313012111_create_subscribes_table.rb +++ /dev/null @@ -1,16 +0,0 @@ -class CreateSubscribesTable < ActiveRecord::Migration - def change - create_table :subscribes do |t| - t.integer :user_id - t.integer :merge_request_id - t.integer :issue_id - t.boolean :subscribed - - t.timestamps - end - - add_index :subscribes, :user_id - add_index :subscribes, :issue_id - add_index :subscribes, :merge_request_id - end -end diff --git a/db/migrate/20150313012111_create_subscriptions_table.rb b/db/migrate/20150313012111_create_subscriptions_table.rb new file mode 100644 index 0000000000..78f7aeeaf7 --- /dev/null +++ b/db/migrate/20150313012111_create_subscriptions_table.rb @@ -0,0 +1,13 @@ +class CreateSubscriptionsTable < ActiveRecord::Migration + def change + create_table :subscriptions do |t| + t.integer :user_id + t.references :subscribable, polymorphic: true + t.boolean :subscribed + + t.timestamps + end + + add_index :subscriptions, [:subscribable_id, :subscribable_type, :user_id], unique: true, name: 'subscriptions_user_id_and_ref_fields' + end +end diff --git a/db/schema.rb b/db/schema.rb index 46663ad495..3f808d5ac3 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -397,18 +397,16 @@ ActiveRecord::Schema.define(version: 20150313012111) do add_index "snippets", ["project_id"], name: "index_snippets_on_project_id", using: :btree add_index "snippets", ["visibility_level"], name: "index_snippets_on_visibility_level", using: :btree - create_table "subscribes", force: true do |t| + create_table "subscriptions", force: true do |t| t.integer "user_id" - t.integer "merge_request_id" - t.integer "issue_id" + t.integer "subscribable_id" + t.string "subscribable_type" t.boolean "subscribed" t.datetime "created_at" t.datetime "updated_at" end - add_index "subscribes", ["issue_id"], name: "index_subscribes_on_issue_id", using: :btree - add_index "subscribes", ["merge_request_id"], name: "index_subscribes_on_merge_request_id", using: :btree - add_index "subscribes", ["user_id"], name: "index_subscribes_on_user_id", using: :btree + add_index "subscriptions", ["subscribable_id", "subscribable_type", "user_id"], name: "subscriptions_user_id_and_ref_fields", unique: true, using: :btree create_table "taggings", force: true do |t| t.integer "tag_id" From f53683e67fa0db7b13d0dee977bc21206af7e0fd Mon Sep 17 00:00:00 2001 From: Valery Sizov Date: Mon, 16 Mar 2015 15:35:48 +0200 Subject: [PATCH 05/12] fix specs --- app/models/subscription.rb | 3 ++- app/services/notification_service.rb | 29 ++++++++++++++++++---------- db/schema.rb | 2 +- 3 files changed, 22 insertions(+), 12 deletions(-) diff --git a/app/models/subscription.rb b/app/models/subscription.rb index 7e57a8570e..276cf0e946 100644 --- a/app/models/subscription.rb +++ b/app/models/subscription.rb @@ -1,7 +1,8 @@ class Subscription < ActiveRecord::Base + belongs_to :user belongs_to :subscribable, polymorphic: true validates :user_id, - uniqueness: { scope: [:subscribable_id, :subscribable_type]}, + uniqueness: { scope: [:subscribable_id, :subscribable_type] }, presence: true end diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb index e02418b724..5ebde8fea8 100644 --- a/app/services/notification_service.rb +++ b/app/services/notification_service.rb @@ -151,6 +151,10 @@ class NotificationService # Reject mutes users recipients = reject_muted_users(recipients, note.project) + recipients = add_subscribed_users(recipients, note.noteable) + + recipients = reject_unsubscribed_users(recipients, note.noteable) + # Reject author recipients.delete(note.author) @@ -315,12 +319,26 @@ class NotificationService end def reject_unsubscribed_users(recipients, target) + return recipients unless target.respond_to? :subscriptions + recipients.reject do |user| subscription = target.subscriptions.find_by_user_id(user.id) subscription && !subscription.subscribed end end + def add_subscribed_users(recipients, target) + return recipients unless target.respond_to? :subscriptions + + subscriptions = target.subscriptions + + if subscriptions.any? + recipients + subscriptions.where("subscribed is true").map(&:user) + else + recipients + end + end + def new_resource_email(target, project, method) recipients = build_recipients(target, project) recipients.delete(target.author) @@ -368,21 +386,12 @@ class NotificationService recipients = reject_muted_users(recipients, project) recipients = reject_mention_users(recipients, project) - recipients = add_subscribed_users(recipients, project) + recipients = add_subscribed_users(recipients, target) recipients = recipients.concat(project_watchers(project)).uniq recipients = reject_unsubscribed_users(recipients, target) recipients end - def add_subscribed_users(recipients, target) - subscriptions = target.subscriptions - if subscriptions.any? - recipients.merge(subscriptions.where("subscribed is true").map(&:user)) - else - recipients - end - end - def mailer Notify.delay end diff --git a/db/schema.rb b/db/schema.rb index 3f808d5ac3..ebbeb2beab 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -334,12 +334,12 @@ ActiveRecord::Schema.define(version: 20150313012111) do t.string "import_url" t.integer "visibility_level", default: 0, null: false t.boolean "archived", default: false, null: false + t.string "avatar" t.string "import_status" t.float "repository_size", default: 0.0 t.integer "star_count", default: 0, null: false t.string "import_type" t.string "import_source" - t.string "avatar" end add_index "projects", ["created_at", "id"], name: "index_projects_on_created_at_and_id", using: :btree From 1b437ec3498bc544dbd1b252f5c755e9073407fd Mon Sep 17 00:00:00 2001 From: Valery Sizov Date: Mon, 16 Mar 2015 17:20:17 +0200 Subject: [PATCH 06/12] 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 f457622fc3..a009969e4d 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 903b7a68dc..88302276b5 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 51ac61c327..c63a9b0cd4 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 d1a35ca529..88ac83744d 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 276cf0e946..dd75d3ab8b 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 5ebde8fea8..3e1f4e62f1 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 24bfbdd4c5..85937e7bf4 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 d0c00c1aea..79b0e7799a 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 a976ba9d59..ad5f2c10f6 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 78f7aeeaf7..a1d4d9dedc 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 283979204d..b9031f6f32 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 adad100e56..91dc576f8b 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 6d72c93ad1..cc0d6033a2 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 b67b2e58ca..5a35d70376 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 2074f8e7f7..5badb63532 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 From 90aa870c3607c170091b6034c0150f119697b0b9 Mon Sep 17 00:00:00 2001 From: Christian Walther Date: Sat, 21 Feb 2015 22:12:13 +0100 Subject: [PATCH 07/12] Fix invalid Atom feeds when using emoji, horizontal rules, or images. Fixes issues #880, #723, #1113: Markdown must be rendered to XHTML, not HTML, when generating summary content for Atom feeds. Otherwise, content-less tags like and
, generated when issue descriptions, merge request descriptions, comments, or commit messages use emoji, horizontal rules, or images, are not terminated and make the Atom XML invalid. --- CHANGELOG | 1 + app/views/events/_event_issue.atom.haml | 2 +- .../events/_event_merge_request.atom.haml | 2 +- app/views/events/_event_note.atom.haml | 2 +- app/views/events/_event_push.atom.haml | 2 +- lib/gitlab/markdown.rb | 32 +++++++++++++------ lib/redcarpet/render/gitlab_html.rb | 6 +--- spec/features/atom/users_spec.rb | 27 ++++++++++++++-- 8 files changed, 54 insertions(+), 20 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index c4b5a847e1..92aadc0584 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -2,6 +2,7 @@ v 7.9.0 (unreleased) - Move labels/milestones tabs to sidebar - Improve UI for commits, issues and merge request lists - Fix commit comments on first line of diff not rendering in Merge Request Discussion view. + - Fix invalid Atom feeds when using emoji, horizontal rules, or images (Christian Walther) v 7.8.0 (unreleased) - Fix access control and protection against XSS for note attachments and other uploads. diff --git a/app/views/events/_event_issue.atom.haml b/app/views/events/_event_issue.atom.haml index eba2b63797..0edb61ea24 100644 --- a/app/views/events/_event_issue.atom.haml +++ b/app/views/events/_event_issue.atom.haml @@ -1,3 +1,3 @@ %div{xmlns: "http://www.w3.org/1999/xhtml"} - if issue.description.present? - = markdown issue.description + = markdown(issue.description, xhtml: true) diff --git a/app/views/events/_event_merge_request.atom.haml b/app/views/events/_event_merge_request.atom.haml index 0aea2d17d6..1a8b62abea 100644 --- a/app/views/events/_event_merge_request.atom.haml +++ b/app/views/events/_event_merge_request.atom.haml @@ -1,3 +1,3 @@ %div{xmlns: "http://www.w3.org/1999/xhtml"} - if merge_request.description.present? - = markdown merge_request.description + = markdown(merge_request.description, xhtml: true) diff --git a/app/views/events/_event_note.atom.haml b/app/views/events/_event_note.atom.haml index be0e05481e..b49c331ccf 100644 --- a/app/views/events/_event_note.atom.haml +++ b/app/views/events/_event_note.atom.haml @@ -1,2 +1,2 @@ %div{xmlns: "http://www.w3.org/1999/xhtml"} - = markdown note.note + = markdown(note.note, xhtml: true) diff --git a/app/views/events/_event_push.atom.haml b/app/views/events/_event_push.atom.haml index 2b63519eda..2fb9f7ec24 100644 --- a/app/views/events/_event_push.atom.haml +++ b/app/views/events/_event_push.atom.haml @@ -6,7 +6,7 @@ %i at = commit[:timestamp].to_time.to_s(:short) - %blockquote= markdown(escape_once(commit[:message])) + %blockquote= markdown(escape_once(commit[:message]), xhtml: true) - if event.commits_count > 15 %p %i diff --git a/lib/gitlab/markdown.rb b/lib/gitlab/markdown.rb index fb0218a277..dceb2bc71f 100644 --- a/lib/gitlab/markdown.rb +++ b/lib/gitlab/markdown.rb @@ -33,17 +33,23 @@ module Gitlab attr_reader :html_options - def gfm_with_tasks(text, project = @project, html_options = {}) - text = gfm(text, project, html_options) - parse_tasks(text) - end - # Public: Parse the provided text with GitLab-Flavored Markdown # # text - the source text # project - extra options for the reference links as given to link_to # html_options - extra options for the reference links as given to link_to def gfm(text, project = @project, html_options = {}) + gfm_with_options(text, {}, project, html_options) + end + + # Public: Parse the provided text with GitLab-Flavored Markdown + # + # text - the source text + # options - parse_tasks: true - render tasks + # - xhtml: true - output XHTML instead of HTML + # project - extra options for the reference links as given to link_to + # html_options - extra options for the reference links as given to link_to + def gfm_with_options(text, options = {}, project = @project, html_options = {}) return text if text.nil? # Duplicate the string so we don't alter the original, then call to_str @@ -86,14 +92,22 @@ module Gitlab markdown_pipeline = HTML::Pipeline::Gitlab.new(filters).pipeline result = markdown_pipeline.call(text, markdown_context) - text = result[:output].to_html(save_with: 0) + saveoptions = 0 + if options[:xhtml] + saveoptions |= Nokogiri::XML::Node::SaveOptions::AS_XHTML + end + text = result[:output].to_html(save_with: saveoptions) allowed_attributes = ActionView::Base.sanitized_allowed_attributes allowed_tags = ActionView::Base.sanitized_allowed_tags - sanitize text.html_safe, - attributes: allowed_attributes + %w(id class style), - tags: allowed_tags + %w(table tr td th) + text = sanitize text.html_safe, + attributes: allowed_attributes + %w(id class style), + tags: allowed_tags + %w(table tr td th) + if options[:parse_tasks] + text = parse_tasks(text) + end + text end private diff --git a/lib/redcarpet/render/gitlab_html.rb b/lib/redcarpet/render/gitlab_html.rb index 714261f815..8b0c193f3d 100644 --- a/lib/redcarpet/render/gitlab_html.rb +++ b/lib/redcarpet/render/gitlab_html.rb @@ -58,10 +58,6 @@ class Redcarpet::Render::GitlabHTML < Redcarpet::Render::HTML unless @template.instance_variable_get("@project_wiki") || @project.nil? full_document = h.create_relative_links(full_document) end - if @options[:parse_tasks] - h.gfm_with_tasks(full_document) - else - h.gfm(full_document) - end + h.gfm_with_options(full_document, @options) end end diff --git a/spec/features/atom/users_spec.rb b/spec/features/atom/users_spec.rb index c0316b073a..770ac04c2c 100644 --- a/spec/features/atom/users_spec.rb +++ b/spec/features/atom/users_spec.rb @@ -15,17 +15,24 @@ describe "User Feed", feature: true do let(:project) { create(:project) } let(:issue) do create(:issue, project: project, - author: user, description: '') + author: user, description: "Houston, we have a bug!\n\n***\n\nI guess.") end let(:note) do create(:note, noteable: issue, author: user, - note: 'Bug confirmed', project: project) + note: 'Bug confirmed :+1:', project: project) + end + let(:merge_request) do + create(:merge_request, + title: 'Fix bug', author: user, + source_project: project, target_project: project, + description: "Here is the fix: ![an image](image.png)") end before do project.team << [user, :master] issue_event(issue, user) note_event(note, user) + merge_request_event(merge_request, user) visit user_path(user, :atom, private_token: user.private_token) end @@ -37,6 +44,18 @@ describe "User Feed", feature: true do expect(body). to have_content("#{safe_name} commented on issue ##{issue.iid}") end + + it 'should have XHTML summaries in issue descriptions' do + expect(body).to match /we have a bug!<\/p>\n\n
\n\n

I guess/ + end + + it 'should have XHTML summaries in notes' do + expect(body).to match /Bug confirmed ]*\/>/ + end + + it 'should have XHTML summaries in merge request descriptions' do + expect(body).to match /Here is the fix: ]*\/>/ + end end end @@ -48,6 +67,10 @@ describe "User Feed", feature: true do EventCreateService.new.leave_note(note, user) end + def merge_request_event(request, user) + EventCreateService.new.open_mr(request, user) + end + def safe_name html_escape(user.name) end From 409097bd7e0f5857cf0bc5462bd47484980ec787 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Tue, 17 Mar 2015 00:06:25 -0700 Subject: [PATCH 08/12] Properly align save user profile button --- app/views/profiles/show.html.haml | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/app/views/profiles/show.html.haml b/app/views/profiles/show.html.haml index e6b204451c..409b6b5a19 100644 --- a/app/views/profiles/show.html.haml +++ b/app/views/profiles/show.html.haml @@ -96,5 +96,7 @@ .row .col-md-7 - .col-sm-2 - = f.submit 'Save changes', class: "btn btn-success" + .form-group + .col-sm-2   + .col-sm-10 + = f.submit 'Save changes', class: "btn btn-success" From df91781a346e6b70c43195f2f4f550b097ac9d2e Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Tue, 17 Mar 2015 10:09:49 +0100 Subject: [PATCH 09/12] Fix changelog. --- CHANGELOG | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index ec30b09b90..15e220b483 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -74,6 +74,7 @@ v 7.9.0 (unreleased) - Raise recommended number of unicorn workers from 2 to 3 - Use same layout and interactivity for project members as group members. - Prevent gitlab-shell character encoding issues by receiving its changes as raw data. + - Fix invalid Atom feeds when using emoji, horizontal rules, or images (Christian Walther) v 7.8.4 - Fix issue_tracker_id substitution in custom issue trackers @@ -101,9 +102,6 @@ v 7.8.1 - Fix urls for the issues when relative url was enabled v 7.8.0 - - Fix invalid Atom feeds when using emoji, horizontal rules, or images (Christian Walther) - -v 7.8.0 (unreleased) - Fix access control and protection against XSS for note attachments and other uploads. - Replace highlight.js with rouge-fork rugments (Stefan Tatschner) - Make project search case insensitive (Hannes Rosenögger) From 9c7fffb6559facdcf8bbda680795f70d836293bf Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Tue, 17 Mar 2015 14:55:43 +0100 Subject: [PATCH 10/12] Delete deploy key when last connection to a project is destroyed. --- CHANGELOG | 1 + .../projects/deploy_keys_controller.rb | 5 +-- app/models/deploy_keys_project.rb | 8 +++++ spec/models/deploy_keys_project_spec.rb | 33 +++++++++++++++++++ 4 files changed, 45 insertions(+), 2 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index bd66a92933..23744c0405 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -74,6 +74,7 @@ v 7.9.0 (unreleased) - Raise recommended number of unicorn workers from 2 to 3 - Use same layout and interactivity for project members as group members. - Prevent gitlab-shell character encoding issues by receiving its changes as raw data. + - Delete deploy key when last connection to a project is destroyed. v 7.8.4 - Fix issue_tracker_id substitution in custom issue trackers diff --git a/app/controllers/projects/deploy_keys_controller.rb b/app/controllers/projects/deploy_keys_controller.rb index b7cc305899..2ecde8381e 100644 --- a/app/controllers/projects/deploy_keys_controller.rb +++ b/app/controllers/projects/deploy_keys_controller.rb @@ -37,7 +37,8 @@ class Projects::DeployKeysController < Projects::ApplicationController @key.destroy respond_to do |format| - format.html { redirect_to project_deploy_keys_url } + format.html { redirect_to namespace_project_deploy_keys_path(@project.namespace, + @project) } format.js { render nothing: true } end end @@ -50,7 +51,7 @@ class Projects::DeployKeysController < Projects::ApplicationController end def disable - @project.deploy_keys_projects.where(deploy_key_id: params[:id]).last.destroy + @project.deploy_keys_projects.find_by(deploy_key_id: params[:id]).destroy redirect_to namespace_project_deploy_keys_path(@project.namespace, @project) diff --git a/app/models/deploy_keys_project.rb b/app/models/deploy_keys_project.rb index f23d8205dd..7e88903b9a 100644 --- a/app/models/deploy_keys_project.rb +++ b/app/models/deploy_keys_project.rb @@ -16,4 +16,12 @@ class DeployKeysProject < ActiveRecord::Base validates :deploy_key_id, presence: true validates :deploy_key_id, uniqueness: { scope: [:project_id], message: "already exists in project" } validates :project_id, presence: true + + after_destroy :destroy_orphaned_deploy_key + + private + + def destroy_orphaned_deploy_key + self.deploy_key.destroy if self.deploy_key.deploy_keys_projects.length == 0 + end end diff --git a/spec/models/deploy_keys_project_spec.rb b/spec/models/deploy_keys_project_spec.rb index aacd9bf38b..f351aab923 100644 --- a/spec/models/deploy_keys_project_spec.rb +++ b/spec/models/deploy_keys_project_spec.rb @@ -21,4 +21,37 @@ describe DeployKeysProject do it { is_expected.to validate_presence_of(:project_id) } it { is_expected.to validate_presence_of(:deploy_key_id) } end + + describe "Destroying" do + let(:project) { create(:project) } + subject { create(:deploy_keys_project, project: project) } + let(:deploy_key) { subject.deploy_key } + + context "when the deploy key is only used by this project" do + it "destroys the deploy key" do + subject.destroy + + expect { + deploy_key.reload + }.to raise_error(ActiveRecord::RecordNotFound) + end + end + + context "when the deploy key is used by more than one project" do + + let!(:other_project) { create(:project) } + + before do + other_project.deploy_keys << deploy_key + end + + it "doesn't destroy the deploy key" do + subject.destroy + + expect { + deploy_key.reload + }.not_to raise_error(ActiveRecord::RecordNotFound) + end + end + end end From 7d2b34bd61df9722ac2461e87ce595228eecef21 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Tue, 17 Mar 2015 16:00:32 +0100 Subject: [PATCH 11/12] Satisfy Rubocop. --- app/controllers/projects/deploy_keys_controller.rb | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/app/controllers/projects/deploy_keys_controller.rb b/app/controllers/projects/deploy_keys_controller.rb index 2ecde8381e..679a5d76ec 100644 --- a/app/controllers/projects/deploy_keys_controller.rb +++ b/app/controllers/projects/deploy_keys_controller.rb @@ -37,8 +37,7 @@ class Projects::DeployKeysController < Projects::ApplicationController @key.destroy respond_to do |format| - format.html { redirect_to namespace_project_deploy_keys_path(@project.namespace, - @project) } + format.html { redirect_to namespace_project_deploy_keys_path(@project.namespace, @project) } format.js { render nothing: true } end end From 22fcb2f418ed6a2c7e68c0cd3ec2d414510ad4ec Mon Sep 17 00:00:00 2001 From: Valery Sizov Date: Tue, 17 Mar 2015 15:04:25 +0200 Subject: [PATCH 12/12] improve UI --- app/assets/javascripts/subscription.js.coffee | 16 ++++++++-------- .../projects/issues/_issue_context.html.haml | 14 +++++++++----- .../merge_requests/show/_context.html.haml | 14 +++++++++----- features/steps/project/issues/issues.rb | 4 ++-- features/steps/project/merge_requests.rb | 4 ++-- 5 files changed, 30 insertions(+), 22 deletions(-) diff --git a/app/assets/javascripts/subscription.js.coffee b/app/assets/javascripts/subscription.js.coffee index a009969e4d..7f41616d4e 100644 --- a/app/assets/javascripts/subscription.js.coffee +++ b/app/assets/javascripts/subscription.js.coffee @@ -1,17 +1,17 @@ class @Subscription constructor: (url) -> - $(".subscribe-button").click (event)=> + $(".subscribe-button").unbind("click").click (event)=> btn = $(event.currentTarget) - action = btn.prop("value") - current_status = $(".sub_status").text().trim() - $(".fa-spinner.subscription").removeClass("hidden") - $(".sub_status").empty() + action = btn.find("span").text() + current_status = $(".subscription-status").attr("data-status") + btn.prop("disabled", true) $.post url, => - $(".fa-spinner.subscription").addClass("hidden") + btn.prop("disabled", false) status = if current_status == "subscribed" then "unsubscribed" else "subscribed" - $(".sub_status").text(status) + $(".subscription-status").attr("data-status", status) action = if status == "subscribed" then "Unsubscribe" else "Subscribe" - btn.prop("value", action) + btn.find("span").text(action) + $(".subscription-status>div").toggleClass("hidden") diff --git a/app/views/projects/issues/_issue_context.html.haml b/app/views/projects/issues/_issue_context.html.haml index 85937e7bf4..cb4846a41d 100644 --- a/app/views/projects/issues/_issue_context.html.haml +++ b/app/views/projects/issues/_issue_context.html.haml @@ -31,11 +31,15 @@ .issuable-context-title %label Subscription: - %i.fa.fa-spinner.fa-spin.hidden.subscription - %span.sub_status - = @issue.subscribed?(current_user) ? "subscribed" : "unsubscribed" - - subscribe_action = @issue.subscribed?(current_user) ? "Unsubscribe" : "Subscribe" - %input.btn.subscribe-button{:type => "button", :value => subscribe_action} + %button.btn.btn-block.subscribe-button + %i.fa.fa-eye + %span= @issue.subscribed?(current_user) ? "Unsubscribe" : "Subscribe" + - subscribtion_status = @issue.subscribed?(current_user) ? "subscribed" : "unsubscribed" + .subscription-status{"data-status" => subscribtion_status} + .description-block.unsubscribed{class: ( "hidden" if @issue.subscribed?(current_user) )} + You're not receiving notifications from this thread. + .description-block.subscribed{class: ( "hidden" unless @issue.subscribed?(current_user) )} + You're receiving notifications because you're subscribed to this thread. :coffeescript $ -> diff --git a/app/views/projects/merge_requests/show/_context.html.haml b/app/views/projects/merge_requests/show/_context.html.haml index 79b0e7799a..753c7e0e61 100644 --- a/app/views/projects/merge_requests/show/_context.html.haml +++ b/app/views/projects/merge_requests/show/_context.html.haml @@ -33,11 +33,15 @@ .issuable-context-title %label Subscription: - %i.fa.fa-spinner.fa-spin.hidden.subscription - %span.sub_status - = @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} + %button.btn.btn-block.subscribe-button + %i.fa.fa-eye + %span= @merge_request.subscribed?(current_user) ? "Unsubscribe" : "Subscribe" + - subscribtion_status = @merge_request.subscribed?(current_user) ? "subscribed" : "unsubscribed" + .subscription-status{"data-status" => subscribtion_status} + .description-block.unsubscribed{class: ( "hidden" if @merge_request.subscribed?(current_user) )} + You're not receiving notifications from this thread. + .description-block.subscribed{class: ( "hidden" unless @merge_request.subscribed?(current_user) )} + You're receiving notifications because you're subscribed to this thread. :coffeescript $ -> diff --git a/features/steps/project/issues/issues.rb b/features/steps/project/issues/issues.rb index cc0d6033a2..e8ca3f7c17 100644 --- a/features/steps/project/issues/issues.rb +++ b/features/steps/project/issues/issues.rb @@ -19,12 +19,12 @@ class Spinach::Features::ProjectIssues < Spinach::FeatureSteps end step 'I should see that I am subscribed' do - find(".sub_status").text.should == "subscribed" + find(".subscribe-button span").text.should == "Unsubscribe" end step 'I should see that I am unsubscribed' do sleep 0.2 - find(".sub_status").text.should == "unsubscribed" + find(".subscribe-button span").text.should == "Subscribe" end step 'I click link "Closed"' do diff --git a/features/steps/project/merge_requests.rb b/features/steps/project/merge_requests.rb index 5a35d70376..6e2f60972b 100644 --- a/features/steps/project/merge_requests.rb +++ b/features/steps/project/merge_requests.rb @@ -57,12 +57,12 @@ class Spinach::Features::ProjectMergeRequests < Spinach::FeatureSteps end step 'I should see that I am subscribed' do - find(".sub_status").text.should == "subscribed" + find(".subscribe-button span").text.should == "Unsubscribe" end step 'I should see that I am unsubscribed' do sleep 0.2 - find(".sub_status").text.should == "unsubscribed" + find(".subscribe-button span").text.should == "Subscribe" end step 'I click button "Unsubscribe"' do