Merge branch 'ee-refactor-boards-actions' into 'master'

EE-port for refactoring groups and projects boards actions

See merge request gitlab-org/gitlab-ee!9811

app/controllers/concerns/boards_actions.rb

+# frozen_string_literal: true
+
+module BoardsActions
+  include Gitlab::Utils::StrongMemoize
+  extend ActiveSupport::Concern
+
+  included do
+    include BoardsResponses
+
+    before_action :boards, only: :index
+    before_action :board, only: :show
+  end
+
+  def index
+    respond_with_boards
+  end
+
+  def show
+    # Add / update the board in the recent visits table
+    Boards::Visits::CreateService.new(parent, current_user).execute(board) if request.format.html?
+
+    respond_with_board
+  end
+
+  private
+
+  def boards
+    strong_memoize(:boards) do
+      Boards::ListService.new(parent, current_user).execute
+    end
+  end
+
+  def board
+    strong_memoize(:board) do
+      boards.find(params[:id])
+    end
+  end
+end
+
+BoardsActions.prepend(EE::BoardsActions)

app/controllers/groups/boards_controller.rb

 # frozen_string_literal: true
 
 class Groups::BoardsController < Groups::ApplicationController
-  include BoardsResponses
+  include BoardsActions
   include RecordUserLastActivity
 
   before_action :assign_endpoint_vars
-  before_action :boards, only: :index
-  before_action :redirect_to_recent_board, only: :index
-
-  def index
-    respond_with_boards
-  end
-
-  def show
-    @board = boards.find(params[:id])
-
-    # add/update the board in the recent visited table
-    Boards::Visits::CreateService.new(@board.group, current_user).execute(@board) if request.format.html?
-
-    respond_with_board
-  end
 
   private
 
-  def boards
-    @boards ||= Boards::ListService.new(group, current_user).execute
-  end
-
   def assign_endpoint_vars
     @boards_endpoint = group_boards_url(group)
     @namespace_path = group.to_param
     @labels_endpoint = group_labels_url(group)
   end
-
-  def serialize_as_json(resource)
-    resource.as_json(only: [:id])
-  end
-
-  def includes_board?(board_id)
-    boards.any? { |board| board.id == board_id }
-  end
-
-  def redirect_to_recent_board
-    return if request.format.json?
-
-    recently_visited = Boards::Visits::LatestService.new(group, current_user).execute
-
-    if recently_visited && includes_board?(recently_visited.board_id)
-      redirect_to(group_board_path(id: recently_visited.board_id), status: :found)
-    end
-  end
 end
-
-Groups::BoardsController.prepend(EE::Boards::BoardsController)

app/controllers/projects/boards_controller.rb

 # frozen_string_literal: true
 
 class Projects::BoardsController < Projects::ApplicationController
-  include BoardsResponses
+  include BoardsActions
   include IssuableCollections
 
   before_action :check_issues_available!
   before_action :authorize_read_board!, only: [:index, :show]
-  before_action :boards, only: :index
   before_action :assign_endpoint_vars
-  before_action :redirect_to_recent_board, only: :index
-
-  def index
-    respond_with_boards
-  end
-
-  def show
-    @board = boards.find(params[:id])
-
-    # add/update the board in the recent visited table
-    Boards::Visits::CreateService.new(@board.project, current_user).execute(@board) if request.format.html?
-
-    respond_with_board
-  end
 
   private
 
-  def boards
-    @boards ||= Boards::ListService.new(project, current_user).execute
-  end
-
   def assign_endpoint_vars
     @boards_endpoint = project_boards_path(project)
     @bulk_issues_path = bulk_update_project_issues_path(project)
@@ -39,24 +20,4 @@ class Projects::BoardsController < Projects::ApplicationController
   def authorize_read_board!
     access_denied! unless can?(current_user, :read_board, project)
   end
-
-  def serialize_as_json(resource)
-    resource.as_json(only: [:id])
-  end
-
-  def includes_board?(board_id)
-    boards.any? { |board| board.id == board_id }
-  end
-
-  def redirect_to_recent_board
-    return if request.format.json?
-
-    recently_visited = Boards::Visits::LatestService.new(project, current_user).execute
-
-    if recently_visited && includes_board?(recently_visited.board_id)
-      redirect_to(namespace_project_board_path(id: recently_visited.board_id), status: :found)
-    end
-  end
 end
-
-Projects::BoardsController.prepend(EE::Boards::BoardsController)

app/models/project.rb

@@ -38,7 +38,6 @@ class Project < ActiveRecord::Base
   BoardLimitExceeded = Class.new(StandardError)
 
   STATISTICS_ATTRIBUTE = 'repositories_count'.freeze
-  NUMBER_OF_PERMITTED_BOARDS = 1
   UNKNOWN_IMPORT_URL = 'http://unknown.git'.freeze
   # Hashed Storage versions handle rolling out new storage to project and dependents models:
   # nil: legacy
@@ -137,7 +136,7 @@ class Project < ActiveRecord::Base
   alias_attribute :parent_id, :namespace_id
 
   has_one :last_event, -> {order 'events.created_at DESC'}, class_name: 'Event'
-  has_many :boards, before_add: :validate_board_limit
+  has_many :boards
 
   # Project services
   has_one :campfire_service
@@ -2192,17 +2191,6 @@ class Project < ActiveRecord::Base
     "projects/#{id}/pushes_since_gc"
   end
 
-  # Similar to the normal callbacks that hook into the life cycle of an
-  # Active Record object, you can also define callbacks that get triggered
-  # when you add an object to an association collection. If any of these
-  # callbacks throw an exception, the object will not be added to the
-  # collection. Before you add a new board to the boards collection if you
-  # already have 1, 2, or n it will fail, but it if you have 0 that is lower
-  # than the number of permitted boards per project it won't fail.
-  def validate_board_limit(board)
-    raise BoardLimitExceeded, 'Number of permitted boards exceeded' if boards.size >= NUMBER_OF_PERMITTED_BOARDS
-  end
-
   def update_project_statistics
     stats = statistics || build_statistics
     stats.update(namespace_id: namespace_id)

ee/app/controllers/concerns/ee/boards_actions.rb

+# frozen_string_literal: true
+
+module EE
+  module BoardsActions
+    extend ActiveSupport::Concern
+
+    prepended do
+      # We need to include the filters as a separate concern since multiple `included` blocks are not allowed
+      include Filters
+    end
+
+    module Filters
+      extend ActiveSupport::Concern
+
+      included do
+        before_action :redirect_to_recent_board, only: :index
+        before_action :authenticate_user!, only: [:recent]
+        before_action :authorize_create_board!, only: [:create]
+        before_action :authorize_admin_board!, only: [:create, :update, :destroy]
+      end
+    end
+
+    def recent
+      recent_visits = ::Boards::Visits::LatestService.new(parent, current_user, count: 4).execute
+      recent_boards = recent_visits.map(&:board)
+
+      render json: serialize_as_json(recent_boards)
+    end
+
+    def create
+      board = ::Boards::CreateService.new(parent, current_user, board_params).execute
+
+      respond_to do |format|
+        format.json do
+          if board.valid?
+            extra_json = { board_path: board_path(board) }
+            render json: serialize_as_json(board).merge(extra_json)
+          else
+            render json: board.errors, status: :unprocessable_entity
+          end
+        end
+      end
+    end
+
+    def update
+      service = ::Boards::UpdateService.new(parent, current_user, board_params)
+
+      service.execute(board)
+
+      respond_to do |format|
+        format.json do
+          if board.valid?
+            extra_json = { board_path: board_path(board) }
+            render json: serialize_as_json(board).merge(extra_json)
+          else
+            render json: board.errors, status: :unprocessable_entity
+          end
+        end
+      end
+    end
+
+    def destroy
+      service = ::Boards::DestroyService.new(parent, current_user)
+      service.execute(board)
+
+      respond_to do |format|
+        format.json { head :ok }
+        format.html { redirect_to boards_path, status: :found }
+      end
+    end
+
+    private
+
+    def redirect_to_recent_board
+      return if request.format.json? || !parent.multiple_issue_boards_available?
+
+      if recently_visited = ::Boards::Visits::LatestService.new(parent, current_user).execute
+        redirect_to board_path(recently_visited.board)
+      end
+    end
+
+    def authorize_create_board!
+      if group?
+        check_multiple_group_issue_boards_available!
+      else
+        check_multiple_project_issue_boards_available!
+      end
+    end
+
+    def authorize_admin_board!
+      return render_404 unless can?(current_user, :admin_board, parent)
+    end
+  end
+end

ee/app/controllers/concerns/ee/boards_responses.rb

@@ -3,6 +3,7 @@
 module EE
   module BoardsResponses
     extend ActiveSupport::Concern
+    extend ::Gitlab::Utils::Override
 
     def authorize_read_parent
       authorize_action_for!(board, :read_parent)
@@ -11,5 +12,15 @@ module EE
     def authorize_read_milestone
       authorize_action_for!(board, :read_milestone)
     end
+
+    override :serialize_as_json
+    def serialize_as_json(resource)
+      resource.as_json(
+        only: [:id, :name],
+        include: {
+          milestone: { only: [:id, :title] }
+        }
+      )
+    end
   end
 end

ee/app/controllers/ee/boards/boards_controller.rb

-# frozen_string_literal: true
-
-# Shared actions between Groups::BoardsController and Projects::BoardsController
-module EE
-  module Boards
-    module BoardsController
-      include ::Gitlab::Utils::StrongMemoize
-      extend ActiveSupport::Concern
-
-      prepended do
-        before_action :authenticate_user!, only: [:recent]
-        before_action :authorize_create_board!, only: [:create]
-        before_action :authorize_admin_board!, only: [:create, :update, :destroy]
-      end
-
-      def recent
-        recent_visits = ::Boards::Visits::LatestService.new(parent, current_user, count: 4).execute
-        recent_boards = recent_visits.map(&:board)
-
-        render json: serialize_as_json(recent_boards)
-      end
-
-      def create
-        board = ::Boards::CreateService.new(parent, current_user, board_params).execute
-
-        respond_to do |format|
-          format.json do
-            if board.valid?
-              extra_json = { board_path: board_path(board) }
-              render json: serialize_as_json(board).merge(extra_json)
-            else
-              render json: board.errors, status: :unprocessable_entity
-            end
-          end
-        end
-      end
-
-      def update
-        service = ::Boards::UpdateService.new(parent, current_user, board_params)
-
-        service.execute(board)
-
-        respond_to do |format|
-          format.json do
-            if board.valid?
-              extra_json = { board_path: board_path(board) }
-              render json: serialize_as_json(board).merge(extra_json)
-            else
-              render json: board.errors, status: :unprocessable_entity
-            end
-          end
-        end
-      end
-
-      def destroy
-        service = ::Boards::DestroyService.new(parent, current_user)
-        service.execute(board)
-
-        respond_to do |format|
-          format.json { head :ok }
-          format.html { redirect_to boards_path, status: :found }
-        end
-      end
-
-      private
-
-      def board
-        strong_memoize(:board) do
-          parent.boards.find(params[:id])
-        end
-      end
-
-      def authorize_create_board!
-        if group?
-          check_multiple_group_issue_boards_available!
-        else
-          check_multiple_project_issue_boards_available!
-        end
-      end
-
-      def authorize_admin_board!
-        return render_404 unless can?(current_user, :admin_board, parent)
-      end
-
-      def serialize_as_json(resource)
-        resource.as_json(
-          only: [:id, :name],
-          include: {
-            milestone: { only: [:id, :title] }
-          }
-        )
-      end
-    end
-  end
-end

ee/spec/controllers/groups/boards_controller_spec.rb

@@ -47,6 +47,10 @@ describe Groups::BoardsController do
       subject { list_boards }
     end
 
+    it_behaves_like 'redirects to last visited board' do
+      let(:parent) { group }
+    end
+
     def list_boards(format: :html)
       get :index, params: { group_id: group }, format: format
     end

ee/spec/controllers/projects/boards_controller_spec.rb

@@ -29,6 +29,10 @@ describe Projects::BoardsController do
       subject { list_boards }
     end
 
+    it_behaves_like 'redirects to last visited board' do
+      let(:parent) { project }
+    end
+
     def list_boards(format: :html)
       get :index, params: {
                     namespace_id: project.namespace,

ee/spec/support/shared_examples/controllers/multiple_issue_board_show.rb

@@ -5,13 +5,18 @@ shared_examples 'multiple issue boards show' do
   let!(:board2) { create(:board, parent: parent, name: 'a') }
 
   context 'when multiple issue boards is enabled' do
-    it 'let user view any board from parent' do
-      [board1, board2].each do |board|
-        show(board)
+    it 'lets user view board1' do
+      show(board1)
+
+      expect(response).to have_gitlab_http_status(200)
+      expect(assigns(:board)).to eq(board1)
+    end
 
-        expect(response).to have_gitlab_http_status(200)
-        expect(assigns(:board)).to eq(board)
-      end
+    it 'lets user view board2' do
+      show(board2)
+
+      expect(response).to have_gitlab_http_status(200)
+      expect(assigns(:board)).to eq(board2)
     end
   end
 

ee/spec/support/shared_examples/controllers/recent_boards.rb

@@ -9,7 +9,7 @@ shared_examples 'returns recently visited boards' do
     it 'returns a 401' do
       sign_out(user)
 
-      get_recent_boards
+      list_boards(recent: true)
 
       expect(response).to have_gitlab_http_status(401)
     end
@@ -20,27 +20,69 @@ shared_examples 'returns recently visited boards' do
       visit_board(boards[board_index], Time.now + i.minutes)
     end
 
-    get_recent_boards
+    list_boards(recent: true)
 
     expect(json_response.length).to eq(4)
     expect(json_response.map { |b| b['id'] }).to eq([1, 7, 3, 5].map { |i| boards[i].id })
   end
+end
+
+shared_examples 'redirects to last visited board' do
+  let(:boards) { create_list(:board, 3, parent: parent) }
 
-  def visit_board(board, time)
-    if parent.is_a?(Group)
-      create(:board_group_recent_visit, group: parent, board: board, user: user, updated_at: time)
-    else
-      create(:board_project_recent_visit, project: parent, board: board, user: user, updated_at: time)
+  before do
+    visit_board(boards[2], Time.now + 1.minute)
+    visit_board(boards[0], Time.now + 2.minutes)
+    visit_board(boards[1], Time.now + 5.minutes)
+  end
+
+  context 'when multiple boards are disabled' do
+    before do
+      stub_licensed_features(multiple_project_issue_boards: false, multiple_group_issue_boards: false)
+    end
+
+    it 'renders first board' do
+      list_boards
+
+      expect(response).to render_template :index
+      expect(response.content_type).to eq 'text/html'
     end
   end
 
-  def get_recent_boards
-    params = if parent.is_a?(Group)
-               { group_id: parent }
-             else
-               { namespace_id: parent.namespace, project_id: parent }
-             end
+  context 'when multiple boards are enabled' do
+    before do
+      stub_licensed_features(multiple_project_issue_boards: true, multiple_group_issue_boards: true)
+    end
+
+    it 'redirects to latest visited board' do
+      list_boards
+
+      board_path = if parent.is_a?(Group)
+                     group_board_path(group_id: parent, id: boards[1].id)
+                   else
+                     namespace_project_board_path(namespace_id: parent.namespace, project_id: parent, id: boards[1].id)
+                   end
+
+      expect(response).to redirect_to(board_path)
+    end
+  end
+end
+
+def list_boards(recent: false)
+  action = recent ? :recent : :index
+  params = if parent.is_a?(Group)
+             { group_id: parent }
+           else
+             { namespace_id: parent.namespace, project_id: parent }
+           end
+
+  get action, params: params, format: :json
+end
 
-    get :recent, params: params, format: :json
+def visit_board(board, time)
+  if parent.is_a?(Group)
+    create(:board_group_recent_visit, group: parent, board: board, user: user, updated_at: time)
+  else
+    create(:board_project_recent_visit, project: parent, board: board, user: user, updated_at: time)
   end
 end

spec/controllers/groups/boards_controller_spec.rb

@@ -22,28 +22,6 @@ describe Groups::BoardsController do
         expect(response.content_type).to eq 'text/html'
       end
 
-      it 'redirects to latest visited board' do
-        board = create(:board, group: group)
-        create(:board_group_recent_visit, group: board.group, board: board, user: user)
-
-        list_boards
-
-        expect(response).to redirect_to(group_board_path(id: board.id))
-      end
-
-      it 'renders template if visited board is not found' do
-        temporary_board = create(:board, group: group)
-        visited = create(:board_group_recent_visit, group: temporary_board.group, board: temporary_board, user: user)
-        temporary_board.delete
-
-        allow_any_instance_of(Boards::Visits::LatestService).to receive(:execute).and_return(visited)
-
-        list_boards
-
-        expect(response).to render_template :index
-        expect(response.content_type).to eq 'text/html'
-      end
-
       context 'with unauthorized user' do
         before do
           allow(Ability).to receive(:allowed?).with(user, :read_cross_project, :global).and_return(true)

spec/controllers/projects/boards_controller_spec.rb

@@ -28,28 +28,6 @@ describe Projects::BoardsController do
         expect(response.content_type).to eq 'text/html'
       end
 
-      it 'redirects to latest visited board' do
-        board = create(:board, project: project)
-        create(:board_project_recent_visit, project: board.project, board: board, user: user)
-
-        list_boards
-
-        expect(response).to redirect_to(namespace_project_board_path(id: board.id))
-      end
-
-      it 'renders template if visited board is not found' do
-        temporary_board = create(:board, project: project)
-        visited = create(:board_project_recent_visit, project: temporary_board.project, board: temporary_board, user: user)
-        temporary_board.delete
-
-        allow_any_instance_of(Boards::Visits::LatestService).to receive(:execute).and_return(visited)
-
-        list_boards
-
-        expect(response).to render_template :index
-        expect(response.content_type).to eq 'text/html'
-      end
-
       context 'with unauthorized user' do
         before do
           allow(Ability).to receive(:allowed?).with(user, :read_project, project).and_return(true)