Merge pull request #9512 from danidoni/in-place-editing-for-basic-pac…

…kage-information

Implement the in-place package editing

src/api/.rubocop_todo.yml

@@ -196,6 +196,8 @@ Metrics/BlockNesting:
 # Configuration parameters: CountComments.
 Metrics/ClassLength:
   Max: 885
+  Exclude:
+    - 'app/controllers/webui/package_controller.rb'
 
 # Offense count: 222
 # Configuration parameters: IgnoredMethods.

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

@@ -5,13 +5,13 @@ class Webui::PackageController < Webui::WebuiController
   include Webui::ManageRelationships
   include BuildLogSupport
 
-  before_action :set_project, only: [:show, :index, :users, :dependency, :binary, :binaries, :requests, :statistics, :revisions,
+  before_action :set_project, only: [:show, :edit, :update, :index, :users, :dependency, :binary, :binaries, :requests, :statistics, :revisions,
                                      :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]
 
-  before_action :require_package, only: [:show, :dependency, :binary, :binaries, :requests, :statistics, :revisions,
+  before_action :require_package, only: [:edit, :update, :show, :dependency, :binary, :binaries, :requests, :statistics, :revisions,
                                          :branch_diff_info, :rdiff, :save, :save_meta, :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, :files, :users,
@@ -42,6 +42,32 @@ def index
     render json: PackageDatatable.new(params, view_context: view_context, project: @project)
   end
 
+  def edit
+    authorize @package, :update?
+    respond_to do |format|
+      format.js
+    end
+  end
+
+  def update
+    authorize @package, :update?
+    respond_to do |format|
+      if @package.update(package_details_params)
+        format.html do
+          flash[:success] = 'Package was successfully updated.'
+          redirect_to package_show_path(@package)
+        end
+        format.js { flash.now[:success] = 'Package was successfully updated.' }
+      else
+        format.html do
+          flash[:error] = 'Failed to update package'
+          redirect_to package_show_path(@package)
+        end
+        format.js
+      end
+    end
+  end
+
   def show
     if request.bot?
       params.delete(:rev)
@@ -80,6 +106,12 @@ def show
     @comments = @package.comments.includes(:user)
     @comment = Comment.new
     @services = @files.any? { |file| file[:name] == '_service' }
+
+    respond_to do |format|
+      format.html
+      format.js
+      format.json { render template: 'webui/package/show.html.haml' }
+    end
   end
 
   def main_object
@@ -770,6 +802,19 @@ def package_params
     params.require(:package).permit(:name, :title, :description)
   end
 
+  def package_details_params
+    # We use :package_details instead of the canonical :package param key
+    # because :package is already used in the Webui::WebuiController#require_package
+    # filter.
+    # TODO: rename the usage of :package in #require_package to :package_name to unlock
+    # the proper use of defaults.
+    params
+      .require(:package_details)
+      .permit(:title,
+              :description,
+              :url)
+  end
+
   def validate_xml
     Suse::Validator.validate('package', params[:meta])
     @meta_xml = Xmlhash.parse(params[:meta])

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

@@ -0,0 +1,10 @@
+.basic-info.mb-3{ class: flipper_responsive? ? 'in-place-editing' : '' }
+  %h3#package-title
+    = package.title.presence || package.name
+  - if package.url.present?
+    = link_to(package.url, package.url, class: 'mb-3 d-block')
+  #description-text
+    - if package.description.blank?
+      %i No description set
+    - else
+      = render partial: 'webui/shared/collapsible_text', locals: { text: package.description }

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

@@ -10,7 +10,8 @@
       = render partial: 'webui/package/bottom_actions/submit_package', locals: { package: package, project: project,
                                                                                  revision: revision }
     - if User.possibly_nobody.can_modify?(package)
-      = render partial: 'webui/package/bottom_actions/edit_description'
+      - unless flipper_responsive?
+        = render partial: 'webui/package/bottom_actions/edit_description'
       = render partial: 'webui/package/bottom_actions/delete_package'
 
       - if package.kiwi_image? && policy(package).update?

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

@@ -0,0 +1,17 @@
+= form_for([project, package], as: :package_details, url: package_update_url, method: :patch, remote: true) do |form|
+  %h5 Edit Package #{package}
+  = hidden_field_tag(:id, package.id)
+  = hidden_field_tag(:project, project)
+  = hidden_field_tag(:package, package.name)
+  .form-group
+    = form.label(:title, 'Title:')
+    = form.text_field(:title, class: 'form-control', autofocus: true)
+  .form-group
+    = form.label(:url, 'URL:')
+    = form.text_field(:url, class: 'form-control')
+  .form-group
+    = form.label(:description, 'Description:')
+    = form.text_area(:description, rows: 8, class: 'form-control')
+  .form-group.text-right
+    = link_to 'Cancel', package_show_path(project, package), class: 'cancel btn btn-outline-danger px-4', remote: true
+    = submit_tag('Update', class: 'btn btn-primary px-4')

src/api/app/views/webui/package/edit.js.erb

@@ -0,0 +1,10 @@
+$('#flash').empty();
+$('.in-place-editing').animate({
+  opacity: 0.25
+}, 400, function() {
+  scrollToInPlace();
+  $('.in-place-editing').html("<%= escape_javascript(render(partial: 'webui/package/edit', locals: { project: @project, package: @package })) %>");
+  $('.in-place-editing').animate({ opacity: 1 }, 400, function() {
+    $("form *[autofocus='autofocus']").focus();
+  });
+});

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

@@ -9,7 +9,7 @@
       = render partial: 'webui/package/responsive_ux/show_actions/submit_package', locals: { package: package, project: project,
                                                                                              revision: revision }
     - if User.possibly_nobody.can_modify?(package)
-      = render partial: 'webui/package/responsive_ux/show_actions/edit_description'
+      = render partial: 'webui/package/responsive_ux/actions/edit_package', locals: { project: project, package: package }
       = render partial: 'webui/package/responsive_ux/show_actions/delete_package'
 
       - if package.kiwi_image? && policy(package).update?

src/api/app/views/webui/package/responsive_ux/actions/_edit_package.html.haml

@@ -0,0 +1,4 @@
+%li.nav-item
+  = link_to(edit_package_path(project, package), class: 'nav-link', remote: true) do
+    %i.fas.fa-edit.fa-lg.mr-1
+    Edit Package

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

@@ -17,20 +17,13 @@
                                                     package: @package,
                                                     is_working: @forced_unexpand.blank? }
   end
+
 .card.mb-3
   = render partial: 'tabs', locals: { project: @project, package: @package }
   .card-body
     .row
       .col-md-8
-        %h3#package-title
-          = @package.title.presence || @package.name
-        - if @package.url.present?
-          = link_to(@package.url, @package.url, class: 'mb-3 d-block')
-        #description-text
-          - if @package.description.blank?
-            %i No description set
-          - else
-            = render partial: 'webui/shared/collapsible_text', locals: { text: @package.description }
+        = render partial: 'basic_info', locals: { package: @package }
       .col-md-4
         = render partial: 'side_links', locals: { devel_package: devel_package,
                                                   failures: @failures,

src/api/app/views/webui/package/show.js.erb

@@ -0,0 +1,8 @@
+$('.in-place-editing').animate({
+  opacity: 0.25
+}, 400, function() {
+  scrollToInPlace();
+  $('.in-place-editing').html("<%= escape_javascript(render(partial: 'webui/package/basic_info', locals: { package: @package })) %>");
+  setCollapsible();
+  $('.in-place-editing').animate({ opacity: 1 }, 400);
+});

src/api/app/views/webui/package/update.js.haml

@@ -0,0 +1,17 @@
+resetFormValidation();
+- if @package.errors.any?
+  - @package.errors.messages.each do |field, messages|
+    element = $("##{@package.class.name.underscore}_details_#{field}"); // Create strings like "package_title"
+    setFormValidation(element, "#{messages.to_sentence}");
+- else
+  :plain
+    $('.in-place-editing').animate({
+      opacity: 0.25
+    }, 400, function() {
+      scrollToInPlace();
+      $('.in-place-editing').html("#{escape_javascript(render(partial: 'webui/package/basic_info', locals: { package: @package }))}");
+      setCollapsible();
+      $('.in-place-editing').animate({ opacity: 1 }, 400, function() {
+        $('#flash').html("#{escape_javascript(render(layout: false, partial: 'layouts/webui/flash', object: flash))}");
+      });
+    });

src/api/config/routes/webui_routes.rb

@@ -51,9 +51,14 @@
 
     resources :package, only: [:index], controller: 'webui/package', constraints: cons
 
+    controller 'webui/package' do
+      get 'package/edit/:project/:package' => :edit, constraints: cons, as: 'edit_package'
+      patch 'package/update' => :update, constraints: cons
+      get 'package/show/:project/:package' => :show, as: 'package_show', constraints: cons
+    end
+
     defaults format: 'html' do
       controller 'webui/package' do
-        get 'package/show/:project/:package' => :show, as: 'package_show', constraints: cons
         get 'package/branch_diff_info/:project/:package' => :branch_diff_info, as: 'package_branch_diff_info', constraints: cons
         get 'package/dependency/:project/:package' => :dependency, constraints: cons, as: 'package_dependency'
         get 'package/binary/:project/:package/:repository/:arch/:filename' => :binary, constraints: cons, as: 'package_binary'

src/api/spec/browser_helper.rb

@@ -8,6 +8,7 @@
 require 'support/features/features_authentication'
 require 'support/features/features_attribute'
 require 'support/features/features_beta'
+require 'support/wait_for_ajax'
 
 # Shared examples. Per recommendation of RSpec,
 # https://www.relishapp.com/rspec/rspec-core/v/2-12/docs/example-groups/shared-examples

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

@@ -1349,4 +1349,86 @@ def do_request(params)
       end
     end
   end
+
+  describe 'GET #edit' do
+    context 'when the user is authorized to edit the package' do
+      before do
+        login(user)
+        get :edit, xhr: true, params: { project: source_project, package: source_package }, format: :js
+      end
+
+      it { expect(response).to have_http_status(:success) }
+      it { expect(assigns[:project]).to eql(source_project) }
+      it { expect(assigns[:package]).to eql(source_package) }
+    end
+
+    context 'when the user is NOT authorized to edit the package' do
+      let(:admins_home_project) { admin.home_project }
+      let(:package_from_admin) do
+        create(:package, name: 'admins_package', project: admins_home_project)
+      end
+
+      before do
+        login(user)
+        get :edit, params: { project: admins_home_project, package: package_from_admin }, format: :js
+      end
+
+      it { expect(response).to redirect_to(root_path) }
+    end
+  end
+
+  describe 'PATCH #update' do
+    context 'when the user is authorized to change the package' do
+      let(:package_params) do
+        {
+          title: 'Updated title',
+          url: 'https://updated.url',
+          description: 'Updated description.'
+        }
+      end
+
+      before do
+        login(user)
+        patch :update,
+              params: {
+                project: source_project,
+                package_details: package_params,
+                package: source_package.name
+              },
+              format: :js
+      end
+
+      it { expect(response).to have_http_status(:success) }
+      it { expect(assigns(:package).title).to eql(package_params[:title]) }
+      it { expect(assigns(:package).url).to eql(package_params[:url]) }
+      it { expect(assigns(:package).description).to eql(package_params[:description]) }
+    end
+  end
+
+  context 'when the user is NOT authorized to change the package' do
+    let(:package_params) do
+      {
+        title: 'Updated title',
+        url: 'https://updated.url',
+        description: 'Updated description.'
+      }
+    end
+    let(:admins_home_project) { admin.home_project }
+    let(:package_from_admin) do
+      create(:package, name: 'admins_package', project: admins_home_project)
+    end
+
+    before do
+      login(user)
+      patch :update,
+            params: {
+              project: admins_home_project,
+              package_details: package_params,
+              package: package_from_admin.name
+            },
+            format: :js
+    end
+
+    it { expect(response).to redirect_to(root_path) }
+  end
 end

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

@@ -281,18 +281,20 @@
   scenario 'editing a package' do
     login user
     visit package_show_path(package: package, project: user.home_project)
-    click_menu_link('Actions', 'Edit description')
-    sleep 1 # FIXME: Needed to avoid a flickering test.
+    click_menu_link('Actions', 'Edit Package')
+    wait_for_ajax
 
-    within('#edit-modal') do
-      fill_in('title', with: 'test title')
-      fill_in('description', with: 'test description')
+    within('#edit_package_details') do
+      fill_in('package_details[title]', with: 'test title')
+      fill_in('package_details[description]', with: 'test description')
+      fill_in('package_details[url]', with: 'https://test.url')
       click_button('Update')
     end
 
-    expect(find('#flash')).to have_text("Package data for '#{package}' was saved successfully")
+    expect(find('#flash')).to have_text('Package was successfully updated.')
     expect(page).to have_text('test title')
     expect(page).to have_text('test description')
+    expect(page).to have_text('https://test.url')
   end
 
   context 'meta configuration' do

src/api/spec/support/wait_for_ajax.rb

@@ -0,0 +1,16 @@
+# See https://thoughtbot.com/blog/automatically-wait-for-ajax-with-capybara
+module WaitForAjax
+  def wait_for_ajax
+    Timeout.timeout(Capybara.default_max_wait_time) do
+      loop until finished_all_ajax_requests?
+    end
+  end
+
+  def finished_all_ajax_requests?
+    page.evaluate_script('jQuery.active').zero?
+  end
+end
+
+RSpec.configure do |config|
+  config.include WaitForAjax, type: :feature
+end