Merge branch 'master' into 4009-external-users4009-external-users

7 files changed, 54 insertions, 40 deletions

diff --git a/app/controllers/concerns/toggle_subscription_action.rb b/app/controllers/concerns/toggle_subscription_action.rb
new file mode 100644
index 00000000000..8a43c0b93c4
--- /dev/null
+++ b/app/controllers/concerns/toggle_subscription_action.rb
@@ -0,0 +1,17 @@
+module ToggleSubscriptionAction
+  extend ActiveSupport::Concern
+
+  def toggle_subscription
+    return unless current_user
+
+    subscribable_resource.toggle_subscription(current_user)
+
+    render nothing: true
+  end
+
+  private
+
+  def subscribable_resource
+    raise NotImplementedError
+  end
+end
diff --git a/app/controllers/oauth/applications_controller.rb b/app/controllers/oauth/applications_controller.rb
index dc22101cd5e..d1e4ac10f6c 100644
--- a/app/controllers/oauth/applications_controller.rb
+++ b/app/controllers/oauth/applications_controller.rb
@@ -8,7 +8,7 @@ class Oauth::ApplicationsController < Doorkeeper::ApplicationsController
   layout 'profile'
 
   def index
-    head :forbidden and return
+    set_index_vars
   end
 
   def create
@@ -20,18 +20,11 @@ class Oauth::ApplicationsController < Doorkeeper::ApplicationsController
       flash[:notice] = I18n.t(:notice, scope: [:doorkeeper, :flash, :applications, :create])
       redirect_to oauth_application_url(@application)
     else
-      render :new
+      set_index_vars
+      render :index
     end
   end
 
-  def destroy
-    if @application.destroy
-      flash[:notice] = I18n.t(:notice, scope: [:doorkeeper, :flash, :applications, :destroy])
-    end
-
-    redirect_to applications_profile_url
-  end
-
   private
 
   def verify_user_oauth_applications_enabled
@@ -40,6 +33,17 @@ class Oauth::ApplicationsController < Doorkeeper::ApplicationsController
     redirect_to applications_profile_url
   end
 
+  def set_index_vars
+    @applications = current_user.oauth_applications
+    @authorized_tokens = current_user.oauth_authorized_tokens
+    @authorized_anonymous_tokens = @authorized_tokens.reject(&:application)
+    @authorized_apps = @authorized_tokens.map(&:application).uniq.reject(&:nil?)
+
+    # Don't overwrite a value possibly set by `create`
+    @application ||= Doorkeeper::Application.new
+  end
+
+  # Override Doorkeeper to scope to the current user
   def set_application
     @application = current_user.oauth_applications.find(params[:id])
   end
diff --git a/app/controllers/profiles_controller.rb b/app/controllers/profiles_controller.rb
index fa7a1148961..32fca6b838e 100644
--- a/app/controllers/profiles_controller.rb
+++ b/app/controllers/profiles_controller.rb
@@ -8,13 +8,6 @@ class ProfilesController < Profiles::ApplicationController
   def show
   end
 
-  def applications
-    @applications = current_user.oauth_applications
-    @authorized_tokens = current_user.oauth_authorized_tokens
-    @authorized_anonymous_tokens = @authorized_tokens.reject(&:application)
-    @authorized_apps = @authorized_tokens.map(&:application).uniq - [nil]
-  end
-
   def update
     user_params.except!(:email) if @user.ldap_user?
 
@@ -65,9 +58,6 @@ class ProfilesController < Profiles::ApplicationController
 
   def user_params
     params.require(:user).permit(
-      :avatar_crop_x,
-      :avatar_crop_y,
-      :avatar_crop_size,
       :avatar,
       :bio,
       :email,
diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb
index 67faa1e4437..b0a03ee45cc 100644
--- a/app/controllers/projects/issues_controller.rb
+++ b/app/controllers/projects/issues_controller.rb
@@ -1,6 +1,8 @@
 class Projects::IssuesController < Projects::ApplicationController
+  include ToggleSubscriptionAction
+
   before_action :module_enabled
-  before_action :issue, only: [:edit, :update, :show, :toggle_subscription]
+  before_action :issue, only: [:edit, :update, :show]
 
   # Allow read any issue
   before_action :authorize_read_issue!
@@ -110,12 +112,6 @@ class Projects::IssuesController < Projects::ApplicationController
     redirect_back_or_default(default: { action: 'index' }, options: { notice: "#{result[:count]} issues updated" })
   end
 
-  def toggle_subscription
-    @issue.toggle_subscription(current_user)
-
-    render nothing: true
-  end
-
   def closed_by_merge_requests
     @closed_by_merge_requests ||= @issue.closed_by_merge_requests(current_user)
   end
@@ -129,6 +125,7 @@ class Projects::IssuesController < Projects::ApplicationController
                  redirect_old
                end
   end
+  alias_method :subscribable_resource, :issue
 
   def authorize_update_issue!
     return render_404 unless can?(current_user, :update_issue, @issue)
diff --git a/app/controllers/projects/labels_controller.rb b/app/controllers/projects/labels_controller.rb
index ecac3c395ec..40d8098690a 100644
--- a/app/controllers/projects/labels_controller.rb
+++ b/app/controllers/projects/labels_controller.rb
@@ -1,8 +1,12 @@
 class Projects::LabelsController < Projects::ApplicationController
+  include ToggleSubscriptionAction
+
   before_action :module_enabled
   before_action :label, only: [:edit, :update, :destroy]
   before_action :authorize_read_label!
-  before_action :authorize_admin_labels!, except: [:index]
+  before_action :authorize_admin_labels!, only: [
+    :new, :create, :edit, :update, :generate, :destroy
+  ]
 
   respond_to :js, :html
 
@@ -73,8 +77,9 @@ class Projects::LabelsController < Projects::ApplicationController
   end
 
   def label
-    @label = @project.labels.find(params[:id])
+    @label ||= @project.labels.find(params[:id])
   end
+  alias_method :subscribable_resource, :label
 
   def authorize_admin_labels!
     return render_404 unless can?(current_user, :admin_label, @project)
diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb
index 03ba289eb94..61b82c9db46 100644
--- a/app/controllers/projects/merge_requests_controller.rb
+++ b/app/controllers/projects/merge_requests_controller.rb
@@ -1,10 +1,11 @@
 class Projects::MergeRequestsController < Projects::ApplicationController
+  include ToggleSubscriptionAction
   include DiffHelper
 
   before_action :module_enabled
   before_action :merge_request, only: [
     :edit, :update, :show, :diffs, :commits, :builds, :merge, :merge_check,
-    :ci_status, :toggle_subscription, :cancel_merge_when_build_succeeds
+    :ci_status, :cancel_merge_when_build_succeeds
   ]
   before_action :closes_issues, only: [:edit, :update, :show, :diffs, :commits, :builds]
   before_action :validates_merge_request, only: [:show, :diffs, :commits, :builds]
@@ -233,12 +234,6 @@ class Projects::MergeRequestsController < Projects::ApplicationController
     render json: response
   end
 
-  def toggle_subscription
-    @merge_request.toggle_subscription(current_user)
-
-    render nothing: true
-  end
-
   protected
 
   def selected_target_project
@@ -252,6 +247,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
   def merge_request
     @merge_request ||= @project.merge_requests.find_by!(iid: params[:id])
   end
+  alias_method :subscribable_resource, :merge_request
 
   def closes_issues
     @closes_issues ||= @merge_request.closes_issues
diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb
index c70add86a20..36f37221c58 100644
--- a/app/controllers/projects_controller.rb
+++ b/app/controllers/projects_controller.rb
@@ -172,10 +172,15 @@ class ProjectsController < ApplicationController
   def housekeeping
     ::Projects::HousekeepingService.new(@project).execute
 
-    respond_to do |format|
-      flash[:notice] = "Housekeeping successfully started."
-      format.html { redirect_to project_path(@project) }
-    end
+    redirect_to(
+      project_path(@project),
+      notice: "Housekeeping successfully started"
+    )
+  rescue ::Projects::HousekeepingService::LeaseTaken => ex
+    redirect_to(
+      edit_project_path(@project),
+      alert: ex.to_s
+    )
   end
 
   def toggle_star