Move 'Create Package' modal to its own page

Refactor the controllers/routes to make this RESTful and follow Rails
conventions. The routes aren't using resources since it's too much work
for now with the huge amount of routes in the package_controller.

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

@@ -6,7 +6,7 @@ class Webui::PackageController < Webui::WebuiController
   include BuildLogSupport
 
   before_action :set_project, only: [:show, :index, :users, :dependency, :binary, :binaries, :requests, :statistics, :revisions,
-                                     :branch_diff_info, :rdiff, :save_new, :save, :remove, :add_file, :save_file,
+                                     :new, :branch_diff_info, :rdiff, :create, :save, :remove, :add_file, :save_file,
                                      :remove_file, :save_person, :save_group, :remove_role, :view_file, :abort_build, :trigger_rebuild,
                                      :trigger_services, :wipe_binaries, :buildresult, :rpmlint_result, :rpmlint_log, :meta, :save_meta, :files,
                                      :binary_download]
@@ -29,13 +29,14 @@ class Webui::PackageController < Webui::WebuiController
 
   before_action :check_build_log_access, only: [:live_build_log, :update_build_log]
 
-  before_action :check_package_name_for_new, only: [:save_new]
+  # FIXME: Remove this before_action, it's doing validation and authorization at the same time
+  before_action :check_package_name_for_new, only: [:create]
 
   before_action :handle_parameters_for_rpmlint_log, only: [:rpmlint_log]
 
   prepend_before_action :lockout_spiders, only: [:revisions, :dependency, :rdiff, :binary, :binaries, :requests, :binary_download]
 
-  after_action :verify_authorized, only: [:remove_file, :remove, :save_file, :abort_build, :trigger_rebuild, :wipe_binaries, :save_meta, :save, :abort_build]
+  after_action :verify_authorized, only: [:new, :create, :remove_file, :remove, :save_file, :abort_build, :trigger_rebuild, :wipe_binaries, :save_meta, :save, :abort_build]
 
   def index
     render json: PackageDatatable.new(params, view_context: view_context, project: @project)
@@ -337,19 +338,23 @@ def rdiff
     end
   end
 
-  def save_new
-    @package = @project.packages.build(name: @package_name)
-    @package.title = params[:title]
-    @package.description = params[:description]
-    if params[:source_protection]
-      @package.flags.build(flag: :sourceaccess, status: :disable)
-    end
-    if params[:disable_publishing]
-      @package.flags.build(flag: :publish, status: :disable)
-    end
+  def new
+    # FIXME: Use the package policy for this
+    authorize @project, :update?
+  end
+
+  def create
+    @package = @project.packages.build(package_params)
+
+    # FIXME: Use the package policy for this
+    authorize @project, :update?
+
+    @package.flags.build(flag: :sourceaccess, status: :disable) if params[:source_protection]
+    @package.flags.build(flag: :publish, status: :disable) if params[:disable_publishing]
+
     if @package.save
-      flash[:success] = "Package '#{@package.name}' was created successfully"
-      redirect_to action: :show, project: params[:project], package: @package_name
+      flash[:success] = "Package '#{@package}' was created successfully"
+      redirect_to action: :show, project: params[:project], package: @package.name
     else
       flash[:error] = "Failed to create package '#{@package}'"
       redirect_to controller: :project, action: :show, project: params[:project]
@@ -840,6 +845,10 @@ def binary_download
 
   private
 
+  def package_params
+    params.require(:package).permit(:name, :title, :description)
+  end
+
   def validate_xml
     Suse::Validator.validate('package', params[:meta])
     @meta_xml = Xmlhash.parse(params[:meta])
@@ -991,23 +1000,24 @@ def set_remote_linkinfo
   end
 
   def check_package_name_for_new
-    @package_name = params[:name]
-    @package_title = params[:title]
-    @package_description = params[:description]
+    package_name = params[:package][:name]
 
-    unless Package.valid_name?(@package_name)
-      flash[:error] = "Invalid package name: '#{@package_name}'"
-      redirect_to controller: :project, action: :new_package, project: @project
+    # FIXME: This should be a validation in the Package model
+    unless Package.valid_name?(package_name)
+      flash[:error] = "Invalid package name: '#{package_name}'"
+      redirect_to action: :new, project: @project
       return false
     end
-    if Package.exists_by_project_and_name(@project.name, @package_name)
-      flash[:error] = "Package '#{@package_name}' already exists in project '#{@project}'"
-      redirect_to controller: :project, action: :new_package, project: @project
+    # FIXME: This should be a validation in the Package model
+    if Package.exists_by_project_and_name(@project.name, package_name)
+      flash[:error] = "Package '#{package_name}' already exists in project '#{@project}'"
+      redirect_to action: :new, project: @project
       return false
     end
+    # FIXME: This should be a Pundit policy
     unless User.possibly_nobody.can_create_package_in?(@project)
       flash[:error] = "You can't create packages in #{@project.name}"
-      redirect_to controller: :project, action: :new_package, project: @project
+      redirect_to action: :new, project: @project
       return false
     end
     true

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

@@ -7,10 +7,10 @@ class Webui::ProjectController < Webui::WebuiController
   before_action :lockout_spiders, only: [:requests, :buildresults]
 
   before_action :require_login, only: [:create, :toggle_watch, :destroy, :new, :release_request,
-                                       :new_release_request, :new_package, :edit_comment]
+                                       :new_release_request, :edit_comment]
 
   before_action :set_project, only: [:autocomplete_repositories, :users, :subprojects,
-                                     :new_package, :edit, :release_request,
+                                     :edit, :release_request,
                                      :show, :buildresult,
                                      :destroy, :remove_path_from_target,
                                      :requests, :save, :monitor, :toggle_watch,
@@ -85,10 +85,6 @@ def new
     @show_restore_message = params[:restore_option] && Project.deleted?(params[:name])
   end
 
-  def new_package
-    authorize @project, :update?
-  end
-
   def release_request
     authorize @project, :update?
   end

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

@@ -1,46 +1,51 @@
 = render partial: 'webui/project/breadcrumb_items'
 
-%li.breadcrumb-item.text-word-break-all
-  - if flipper_responsive?
-    %i.fa.fa-archive
-    = link_to(@package, package_show_path(@project, @package))
-  - else
-    = link_to truncate(@package.to_s, length: 50), package_show_path(@project, @package)
-- if current_page?(package_add_file_path)
-  %li.breadcrumb-item.active{ 'aria-current' => 'page' }
-    Add File
-- if current_page?(package_view_revisions_path)
-  %li.breadcrumb-item.active{ 'aria-current' => 'page' }
-    Revisions
-- if current_page?(package_requests_path)
-  %li.breadcrumb-item.active{ 'aria-current' => 'page' }
-    Requests
-- if current_page?(package_users_path)
-  %li.breadcrumb-item.active{ 'aria-current' => 'page' }
-    Users
-- if controller_name == 'package' && action_name == 'binaries'
-  %li.breadcrumb-item.active{ 'aria-current' => 'page' }
-    Repository State
-- if controller_name == 'package' && action_name == 'binary'
-  %li.breadcrumb-item.active{ 'aria-current' => 'page' }
-    Binary
-- if current_page?(package_dependency_path)
-  %li.breadcrumb-item
-    = link_to('Binary', package_binary_path(project: @dependant_project, package: @package_name, repository: @dependant_repository,
-                                            arch: @arch, filename: @filename))
-  %li.breadcrumb-item.active{ 'aria-current' => 'page' }
-    Dependency
-- if current_page?(package_show_path)
-  %li.breadcrumb-item.active{ 'aria-current' => 'page' }
-    Overview
-- if controller_name == 'package' && action_name == 'live_build_log'
-  %li.breadcrumb-item.active{ 'aria-current' => 'page' }
-    Build Log
-- if controller_name == 'package' && action_name == 'statistics'
-  %li.breadcrumb-item
-    = link_to 'Repository State', package_binaries_path(@project, @package_name, @repository)
-  %li.breadcrumb-item.active{ 'aria-current' => 'page' }
-    Statistics
-- if current_page?(package_view_file_path)
-  %li.breadcrumb-item.active.text-word-break-all{ 'aria-current' => 'page' }
-    = truncate(@filename, length: 50)
+-# The package doesn't exist in the context of package#new, so we don't check other paths below to avoid exceptions
+- if current_page?(new_package_path(@project))
+  %li.breadcrumb-item.active{ 'aria-current' => 'page' }
+    Create Package
+- else
+  %li.breadcrumb-item.text-word-break-all
+    - if flipper_responsive?
+      %i.fa.fa-archive
+      = link_to(@package, package_show_path(@project, @package))
+    - else
+      = link_to truncate(@package.to_s, length: 50), package_show_path(@project, @package)
+  - if current_page?(package_add_file_path)
+    %li.breadcrumb-item.active{ 'aria-current' => 'page' }
+      Add File
+  - if current_page?(package_view_revisions_path)
+    %li.breadcrumb-item.active{ 'aria-current' => 'page' }
+      Revisions
+  - if current_page?(package_requests_path)
+    %li.breadcrumb-item.active{ 'aria-current' => 'page' }
+      Requests
+  - if current_page?(package_users_path)
+    %li.breadcrumb-item.active{ 'aria-current' => 'page' }
+      Users
+  - if controller_name == 'package' && action_name == 'binaries'
+    %li.breadcrumb-item.active{ 'aria-current' => 'page' }
+      Repository State
+  - if controller_name == 'package' && action_name == 'binary'
+    %li.breadcrumb-item.active{ 'aria-current' => 'page' }
+      Binary
+  - if current_page?(package_dependency_path)
+    %li.breadcrumb-item
+      = link_to('Binary', package_binary_path(project: @dependant_project, package: @package_name, repository: @dependant_repository,
+                                              arch: @arch, filename: @filename))
+    %li.breadcrumb-item.active{ 'aria-current' => 'page' }
+      Dependency
+  - if current_page?(package_show_path)
+    %li.breadcrumb-item.active{ 'aria-current' => 'page' }
+      Overview
+  - if controller_name == 'package' && action_name == 'live_build_log'
+    %li.breadcrumb-item.active{ 'aria-current' => 'page' }
+      Build Log
+  - if controller_name == 'package' && action_name == 'statistics'
+    %li.breadcrumb-item
+      = link_to 'Repository State', package_binaries_path(@project, @package_name, @repository)
+    %li.breadcrumb-item.active{ 'aria-current' => 'page' }
+      Statistics
+  - if current_page?(package_view_file_path)
+    %li.breadcrumb-item.active.text-word-break-all{ 'aria-current' => 'page' }
+      = truncate(@filename, length: 50)

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

@@ -0,0 +1,31 @@
+- @pagetitle = "Create Package for #{@project}"
+
+.row
+  .col
+    .card
+      -# We display the project tabs since we're creating a package in the context of the project `@project`
+      = render partial: 'webui/project/tabs', locals: { project: @project }
+      .card-body
+        .row
+          .col-12
+            %h3= @pagetitle
+          .col-12.col-md-9.col-lg-6
+            = form_for(:package, url: packages_url(@project)) do |f|
+              .form-group
+                = f.label(:name)
+                %abbr.text-danger{ title: 'required' } *
+                = f.text_field(:name, size: 80, required: true, class: 'form-control', placeholder: 'Enter Name')
+              .form-group
+                = f.label(:title)
+                = f.text_field(:title, size: 80, class: 'form-control', placeholder: 'Enter Title')
+              .form-group
+                = f.label(:description)
+                = f.text_area(:description, cols: 80, rows: 10, class: 'form-control')
+              - unless @configuration['hide_private_options']
+                .form-group.custom-control.custom-checkbox
+                  = check_box_tag :source_protection, 1, false, class: 'custom-control-input', type: 'checkbox'
+                  %label.custom-control-label{ for: 'source_protection' } Deny access to source of package
+              .form-group.custom-control.custom-checkbox
+                = check_box_tag :disable_publishing, 1, false, class: 'custom-control-input', type: 'checkbox'
+                %label.custom-control-label{ for: 'disable_publishing' } Disable build results publishing
+              = f.submit('Create', class: 'btn btn-primary px-4')

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

@@ -15,7 +15,7 @@
       .pt-4
         %ul.list-inline
           %li.list-inline-item
-            = link_to('#', class: 'nav-link', data: { toggle: 'modal', target: '#new-package-modal' }) do
+            = link_to(new_package_path(project), class: 'nav-link') do
               %i.fas.fa-plus-circle.text-primary
               Create Package
           %li.list-inline-item

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

@@ -23,7 +23,7 @@
       New Image
   - if User.possibly_nobody.can_modify?(project) && show_package_actions?
     %li.nav-item
-      = link_to('#', class: 'nav-link', data: { toggle: 'modal', target: '#new-package-modal' }) do
+      = link_to(new_package_path(project), class: 'nav-link') do
         %i.fas.fa-plus-circle.fa-lg.mr-2
         Create Package
     %li.nav-item

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

@@ -82,7 +82,6 @@
     = render partial: 'delete_project_dialog', locals: { project: @project }
 
     - if show_package_actions?
-      = render partial: 'create_package_modal', locals: { project: @project }
       = render partial: 'new_package_branch_modal', locals: { project: @project, remote_projects: @remote_projects }
   - elsif !@project.is_locked?
     = render partial: 'webui/request/add_role_request_dialog', locals: { project: @project }

src/api/config/routes/webui_routes.rb

@@ -66,7 +66,8 @@
         get 'package/revisions/:project/:package' => :revisions, constraints: cons, as: 'package_view_revisions'
         post 'package/submit_request/:project/:package' => :submit_request, constraints: cons
         get 'package/rdiff/:project/:package' => :rdiff, constraints: cons, as: 'package_rdiff'
-        post 'package/save_new/:project' => :save_new, constraints: cons, as: 'save_new_package'
+        post 'package/create/:project' => :create, constraints: cons, as: 'packages'
+        get 'package/new/:project' => :new, constraints: cons, as: 'new_package'
         post 'package/branch' => :branch, constraints: cons
         post 'package/save/:project/:package' => :save, constraints: cons, as: 'package_save'
         post 'package/remove/:project/:package' => :remove, constraints: cons
@@ -183,7 +184,6 @@
       get 'project/users/:project' => :users, constraints: cons, as: 'project_users'
       get 'project/subprojects/:project' => :subprojects, constraints: cons, as: 'project_subprojects'
       get 'project/attributes/:project', to: redirect('/attribs/%{project}'), constraints: cons
-      get 'project/new_package/:project' => :new_package, constraints: cons, as: 'project_new_package'
       get 'project/release_request/(:project)' => :release_request, constraints: cons, as: :project_release_request
       post 'project/new_release_request/(:project)' => :new_release_request, constraints: cons, as: :project_new_release_request
       get 'project/show/:project' => :show, constraints: cons, as: 'project_show'