Merge pull request #9605 from dmarcoux/page-request-deletion

Move 'Request Deletion' modal to its own page

src/api/app/controllers/webui/request_controller.rb

@@ -4,13 +4,29 @@ class Webui::RequestController < Webui::WebuiController
   before_action :require_login, except: [:show, :sourcediff, :diff]
   # requests do not really add much value for our page rank :)
   before_action :lockout_spiders
-
   before_action :require_request, only: [:changerequest, :show]
-
   before_action :set_superseded_request, only: :show
-
   before_action :check_ajax, only: :sourcediff
 
+  after_action :verify_authorized, only: [:create]
+
+  def create
+    request = BsRequest.new(bs_request_params)
+    authorize request, :create?
+    begin
+      request.save!
+    rescue APIError => e
+      flash[:error] = e.message
+      if params.key?(:package_name)
+        redirect_to(controller: :package, action: :show, package: params[:package_name], project: params[:project_name])
+      else
+        redirect_to(controller: :project, action: :show, project: params[:project_name])
+      end
+      return
+    end
+    redirect_to request_show_path(request.number)
+  end
+
   def add_reviewer
     begin
       opts = {}
@@ -147,26 +163,6 @@ def list_small
     render partial: 'requests_small', locals: { requests: requests }
   end
 
-  def delete_request
-    request = nil
-    begin
-      request = BsRequest.create!(
-        description: params[:delete_description], bs_request_actions: [BsRequestAction.new(request_action_attributes(:delete))]
-      )
-      request_link = ActionController::Base.helpers.link_to("delete request #{request.number}", request_show_path(request.number))
-      flash[:success] = "Created #{request_link}"
-    rescue APIError => e
-      flash[:error] = e.message
-      if params[:package]
-        redirect_to package_show_path(project: params[:project], package: params[:package])
-      else
-        redirect_to project_show_path(project: params[:project])
-      end
-      return
-    end
-    redirect_to request_show_path(number: request.number)
-  end
-
   def set_bugowner_request
     required_parameters :project
     request = nil
@@ -319,6 +315,17 @@ def forward_request_to(fwd)
     flash[:success] += " and forwarded to #{target_link} (#{request_link})"
   end
 
+  def set_package
+    return unless params.key?(:package_name)
+
+    @package = Package.find_by(name: params[:package_name])
+  end
+
+  # @abstract Subcontroller is expected to implement #bs_request_params
+  # @!method bs_request_params
+  #    Strong parameters for BsRequest with nested attributes for its bs_request_actions association
+
+  # FIXME: We should rely on strong parameters, so implement `bs_request_params` in subcontrollers as explained above
   def request_action_attributes(type)
     opt = {}
     opt['target_project'] = params[:project]

src/api/app/controllers/webui/requests/deletions_controller.rb

@@ -0,0 +1,23 @@
+module Webui
+  module Requests
+    class DeletionsController < Webui::RequestController
+      before_action :require_login
+      before_action :set_package
+      before_action :set_project
+
+      after_action :verify_authorized
+
+      def new
+        bs_request_action = BsRequestAction.new(target_package: @package, target_project: @project, type: 'delete')
+        @bs_request = BsRequest.new(bs_request_actions: [bs_request_action])
+        authorize @bs_request, :create?
+      end
+
+      private
+
+      def bs_request_params
+        params.require(:bs_request).permit(:description, bs_request_actions_attributes: [:target_project, :target_package, :type])
+      end
+    end
+  end
+end

src/api/app/controllers/webui/requests/role_additions_controller.rb

@@ -1,10 +1,9 @@
 module Webui
   module Requests
-    class RoleAdditionsController < WebuiController
+    class RoleAdditionsController < Webui::RequestController
       before_action :require_login
-      before_action :lockout_spiders
-      before_action :set_package, only: [:new]
-      before_action :set_project, only: [:new]
+      before_action :set_package
+      before_action :set_project
 
       after_action :verify_authorized
 
@@ -14,30 +13,11 @@ def new
         authorize @bs_request, :create?
       end
 
-      def create
-        request = BsRequest.new(bs_request_params)
-        authorize request, :create?
-        begin
-          request.save!
-        rescue APIError => e
-          flash[:error] = e.message
-          redirect_to(controller: :package, action: :show, package: params[:package_name], project: params[:project_name]) && return if @package
-          redirect_to(controller: :project, action: :show, project: params[:project_name]) && return
-        end
-        redirect_to request_show_path(request.number)
-      end
-
       private
 
       def bs_request_params
         params.require(:bs_request).permit(:description, bs_request_actions_attributes: [:group_name, :person_name, :role, :target_project, :target_package, :type])
       end
-
-      def set_package
-        return unless params.key?(:package_name)
-
-        @package = Package.find_by(name: params[:package_name])
-      end
     end
   end
 end

src/api/app/views/webui/package/_bottom_actions.html.haml

@@ -25,7 +25,7 @@
 
     - else
       = render partial: 'webui/package/bottom_actions/request_role_addition', locals: { package: package, project: project }
-      = render partial: 'webui/package/bottom_actions/request_deletion'
+      = render partial: 'webui/package/bottom_actions/request_deletion', locals: { package: package, project: project }
 
       //TODO: only users w/o rights should see this, maintainers should get a different dialog:
     - if package.develpackage

src/api/app/views/webui/package/bottom_actions/_request_deletion.html.haml

@@ -1,4 +1,4 @@
 %li.list-inline-item
-  = link_to('#', class: 'nav-link', data: { toggle: 'modal', target: '#delete-request-modal' }) do
-    %i.fas.fa-list-alt.text-danger
-    Request deletion
+  = link_to(new_project_package_deletion_path(project, package), class: 'nav-link') do
+    %i.fas.fa-times-circle.text-danger
+    Request Deletion

src/api/app/views/webui/package/responsive_ux/_show_actions.html.haml

@@ -22,7 +22,7 @@
         = render partial: 'webui/package/responsive_ux/show_actions/trigger_services', locals: { package: package, project: project }
     - else
       = render partial: 'webui/package/responsive_ux/show_actions/request_role_addition', locals: { package: package, project: project }
-      = render partial: 'webui/package/responsive_ux/show_actions/request_deletion'
+      = render partial: 'webui/package/responsive_ux/show_actions/request_deletion', locals: { package: package, project: project }
 
       //TODO: only users w/o rights should see this, maintainers should get a different dialog:
     - if package.develpackage

src/api/app/views/webui/package/responsive_ux/show_actions/_request_deletion.html.haml

@@ -1,4 +1,4 @@
 %li.nav-item
-  = link_to('#', class: 'nav-link', data: { toggle: 'modal', target: '#delete-request-modal' }) do
-    %i.fas.fa-list-alt.fa-lg.mr-2
-    Request deletion
+  = link_to(new_project_package_deletion_path(project, package), class: 'nav-link') do
+    %i.fas.fa-times-circle.fa-lg.mr-2
+    Request Deletion

src/api/app/views/webui/package/show.html.haml

@@ -92,8 +92,6 @@
   - if User.possibly_nobody.can_modify?(@package)
     = render partial: 'edit_dialog', locals: { project: @project, package: @package }
     = render partial: 'delete_dialog', locals: { project: @project, package: @package, cleanup_source: @cleanup_source }
-  - else
-    = render partial: 'webui/request/delete_request_dialog', locals: { project: @project, package: @package }
   - if @package.develpackage
     = render partial: 'webui/request/change_devel_request_dialog', locals: { project: @project, package: @package }
 -# haml-lint:enable ViewLength

src/api/app/views/webui/project/bottom_actions/_request_role_addition_and_deletion.html.haml

@@ -3,6 +3,6 @@
     %i.fas.fa-plus-circle.text-primary
     Request Role Addition
 %li.list-inline-item
-  = link_to('#', class: 'nav-link', data: { toggle: 'modal', target: '#delete-request-modal' }) do
+  = link_to(new_project_deletion_path(project), class: 'nav-link') do
     %i.fas.fa-times-circle.text-danger
     Request Deletion

src/api/app/views/webui/project/responsive_ux/show_actions/_request_role_addition_and_deletion.html.haml

@@ -3,6 +3,6 @@
     %i.fas.fa-plus-circle.fa-lg.mr-2
     Request Role Addition
 %li.nav-item
-  = link_to('#', class: 'nav-link', data: { toggle: 'modal', target: '#delete-request-modal' }) do
+  = link_to(new_project_deletion_path(project), class: 'nav-link') do
     %i.fas.fa-times-circle.fa-lg.mr-2
     Request Deletion

src/api/app/views/webui/project/show.html.haml

@@ -80,8 +80,6 @@
   - elsif User.session!.can_modify?(@project)
     = render partial: 'edit_project_dialog', locals: { project: @project }
     = render partial: 'delete_project_dialog', locals: { project: @project }
-  - elsif !@project.is_locked?
-    = render partial: 'webui/request/delete_request_dialog', locals: { project: @project }
 
 - content_for :ready_function do
   initializeDataTable('#inherited-packages-table');

src/api/app/views/webui/requests/deletions/_breadcrumb_items.html.haml

@@ -0,0 +1,7 @@
+- if @package
+  = render partial: 'webui/package/breadcrumb_items'
+- else
+  = render partial: 'webui/project/breadcrumb_items'
+
+%li.breadcrumb-item.active{ 'aria-current' => 'page' }
+  Request Deletion

src/api/app/views/webui/requests/deletions/new.html.haml

@@ -0,0 +1,36 @@
+- @pagetitle = 'Request Deletion'
+
+.row
+  .col
+    .card
+      - if @package
+        = render partial: 'webui/package/tabs', locals: { project: @project, package: @package }
+      - else
+        = render partial: 'webui/project/tabs', locals: { project: @project }
+      .card-body
+        .row
+          .col-12
+            %h3= @pagetitle
+          .col-12.col-md-9.col-lg-8
+            %p
+              -# FIXME: get rid of the helper project_or_package_link
+              Do you really want to request the deletion of
+              = project_or_package_link(project: @project.name, package: @package.try(:name))
+              ?
+          .col-12.col-md-9.col-lg-6
+            - form_path = @package ? project_package_role_additions_path(@project, @package) : project_role_additions_path(@project)
+            = form_for(@bs_request, url: form_path) do |f|
+              = f.fields_for(:bs_request_actions) do |bs_request_actions|
+                = bs_request_actions.hidden_field(:target_project)
+                = bs_request_actions.hidden_field(:type)
+                - if @package
+                  .form-group
+                    = bs_request_actions.hidden_field(:target_package)
+                    = bs_request_actions.label(:target_project, 'In target project:')
+                    = bs_request_actions.text_field(:target_project, disabled: true, class: 'form-control')
+              .form-group
+                = f.label(:description, "Please describe your reasons to request the deletion of this #{@package ? 'package' : 'project'}:")
+                %abbr.text-danger{ title: 'required' } *
+                = f.text_area(:description, row: 3, class: 'form-control', required: true)
+
+              = submit_tag('Request', class: 'btn btn-primary px-4')

src/api/config/routes/webui_routes.rb

@@ -266,9 +266,11 @@
       resources :maintenance_incident_requests, controller: 'webui/projects/maintenance_incident_requests', only: [:new, :create], constraints: cons
       resources :packages, only: [], param: :name do
         resources :role_additions, controller: 'webui/requests/role_additions', only: [:new, :create], constraints: cons
+        resources :deletions, controller: 'webui/requests/deletions', only: [:new, :create], constraints: cons
       end
 
       resources :role_additions, controller: 'webui/requests/role_additions', only: [:new, :create], constraints: cons
+      resources :deletions, controller: 'webui/requests/deletions', only: [:new, :create], constraints: cons
     end
 
     controller 'webui/request' do
@@ -279,7 +281,6 @@
       post 'request/changerequest' => :changerequest
       get 'request/diff/:number' => :diff
       get 'request/list_small' => :list_small, as: 'request_list_small'
-      post 'request/delete_request/:project' => :delete_request, constraints: cons, as: 'delete_request'
       post 'request/set_bugowner_request' => :set_bugowner_request
       post 'request/change_devel_request/:project/:package' => :change_devel_request, constraints: cons, as: 'change_devel_request'
     end

src/api/spec/controllers/webui/request_controller_spec.rb

@@ -195,42 +195,6 @@
     end
   end
 
-  describe 'POST #delete_request' do
-    before do
-      login(submitter)
-    end
-
-    context 'a valid request' do
-      before do
-        post :delete_request, params: { project: target_project, package: target_package, delete_description: 'delete it!' }
-      end
-
-      subject do
-        BsRequest.joins(:bs_request_actions).
-          where('bs_request_actions.target_project=? AND bs_request_actions.target_package=? AND type=?',
-                target_project.to_s, target_package.to_s, 'delete').first
-      end
-
-      it { expect(response).to redirect_to(request_show_path(number: subject)) }
-      it { expect(flash[:success]).to match("Created .+delete request #{subject.number}") }
-      it { expect(subject.description).to eq('delete it!') }
-    end
-
-    context 'a request causing a APIError' do
-      before do
-        allow_any_instance_of(BsRequest).to receive(:save!).and_raise(APIError, 'something happened')
-        post :delete_request, params: { project: target_project, package: target_package, description: 'delete it!' }
-      end
-
-      it { expect(flash[:error]).to eq('something happened') }
-      it { expect(response).to redirect_to(package_show_path(project: target_project, package: target_package)) }
-
-      it 'does not create a delete request' do
-        expect(BsRequest.count).to eq(0)
-      end
-    end
-  end
-
   describe 'POST #modify_review' do
     RSpec.shared_examples 'a valid review' do |new_state|
       let(:params_hash) do

src/api/spec/features/beta/webui/packages_spec.rb

@@ -246,17 +246,16 @@
   scenario 'requesting package deletion' do
     login user
     visit package_show_path(package: other_users_package, project: other_user.home_project)
-    click_menu_link('Actions', 'Request deletion')
+    click_menu_link('Actions', 'Request Deletion')
 
     expect(page).to have_text('Do you really want to request the deletion of package ')
-    within('#delete-request-modal') do
-      fill_in('delete_description', with: 'Hey, why not?')
-      click_button('Create')
-    end
+    fill_in('bs_request_description', with: 'Hey, why not?')
+    expect { click_button('Request') }.to change(BsRequest, :count).by(1)
 
-    expect(page).to have_text('Created delete request')
-    find('a', text: /delete request \d+/).click
-    expect(page).to have_current_path(/\/request\/show\/\d+/)
+    # The project name can be ellipsed when it's too long, so this explains why it's hardcoded in the spec
+    expect(page).to have_text("Delete package home:othe...test_user / #{other_users_package}")
+    expect(page).to have_css('#description-text', text: 'Hey, why not?')
+    expect(page).to have_text('In state new')
   end
 
   scenario "changing the package's devel project" do