Merge branch 'ee-proj-settings-ok-leftovers' into 'master'

Improve project settings page (leftovers)

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

app/assets/javascripts/dirty_submit/dirty_submit_form.js

 import _ from 'underscore';
+import $ from 'jquery';
 
 class DirtySubmitForm {
   constructor(form) {
@@ -26,6 +27,7 @@ class DirtySubmitForm {
     );
     this.form.addEventListener('input', throttledUpdateDirtyInput);
     this.form.addEventListener('change', throttledUpdateDirtyInput);
+    $(this.form).on('change.select2', throttledUpdateDirtyInput);
     this.form.addEventListener('submit', event => this.formSubmit(event));
   }
 

app/assets/javascripts/pages/projects/edit/index.js

@@ -3,17 +3,24 @@ import initSettingsPanels from '~/settings_panels';
 import setupProjectEdit from '~/project_edit';
 import initConfirmDangerModal from '~/confirm_danger_modal';
 import mountBadgeSettings from '~/pages/shared/mount_badge_settings';
+import dirtySubmitFactory from '~/dirty_submit/dirty_submit_factory';
 import initAvatarPicker from '~/avatar_picker';
 import initProjectLoadingSpinner from '../shared/save_project_loader';
 import initProjectPermissionsSettings from '../shared/permissions';
 
 document.addEventListener('DOMContentLoaded', () => {
-  initProjectLoadingSpinner();
-  setupProjectEdit();
-  // Initialize expandable settings panels
-  initSettingsPanels();
   initAvatarPicker();
-  initProjectPermissionsSettings();
   initConfirmDangerModal();
+  initSettingsPanels();
   mountBadgeSettings(PROJECT_BADGE);
+
+  initProjectLoadingSpinner();
+  initProjectPermissionsSettings();
+  setupProjectEdit();
+
+  dirtySubmitFactory(
+    document.querySelectorAll(
+      '.js-general-settings-form, .js-mr-settings-form, .js-mr-approvals-form',
+    ),
+  );
 });

app/assets/stylesheets/framework/forms.scss

@@ -157,6 +157,10 @@ label {
   padding-left: 10px;
   padding-right: 10px;
   appearance: none;
+  /* stylelint-disable property-no-vendor-prefix */
+  -webkit-appearance: none;
+  -moz-appearance: none;
+  /* stylelint-enable property-no-vendor-prefix */
 
   &::-ms-expand {
     display: none;

app/assets/stylesheets/pages/settings.scss

@@ -39,7 +39,7 @@
 
 .settings-header {
   position: relative;
-  padding: 20px 110px 10px 0;
+  padding: 20px 110px 0 0;
 
   h4 {
     margin-top: 0;

app/views/projects/_classification_policy_settings.html.haml

 - if ::Gitlab::ExternalAuthorization.enabled?
-  .form-group
-    = f.label :external_authorization_classification_label, class: 'label-bold' do
-      = s_('ExternalAuthorizationService|Classification Label')
-      %span.light (optional)
+  .form-group.col-md-9
+    = f.label :external_authorization_classification_label, _('Classification Label (optional)'), class: 'label-bold'
     = f.text_field :external_authorization_classification_label, class: "form-control"
     %span.form-text.text-muted
       = external_classification_label_help_message

app/views/projects/_export.html.haml

 - return unless Gitlab::CurrentSettings.project_export_enabled?
 
 - project = local_assigns.fetch(:project)
-- expanded = Rails.env.test?
 
-%section.settings.no-animate#js-export-project{ class: ('expanded' if expanded) }
-  .settings-header
-    %h4
-      Export project
-    %button.btn.js-settings-toggle{ type: 'button' }
-      = expanded ? 'Collapse' : 'Expand'
-    %p
-      Export this project with all its related data in order to move your project to a new GitLab instance. Once the export is finished, you can import the file from the "New Project" page.
-  .settings-content
-    .bs-callout.bs-callout-info
-      %p.append-bottom-0
-        %p
-          The following items will be exported:
-        %ul
-          %li Project and wiki repositories
-          %li Project uploads
-          %li Project configuration, including services
-          %li Issues with comments, merge requests with diffs and comments, labels, milestones, snippets, and other project entities
-          %li LFS objects
-        %p
-          The following items will NOT be exported:
-        %ul
-          %li Job traces and artifacts
-          %li Container registry images
-          %li CI variables
-          %li Webhooks
-          %li Any encrypted tokens
-    %p
-      Once the exported file is ready, you will receive a notification email with a download link, or you can download it from this page.
-    - if project.export_status == :finished
-      = link_to 'Download export', download_export_project_path(project),
-              rel: 'nofollow', download: '', method: :get, class: "btn btn-default"
-      = link_to 'Generate new export', generate_new_export_project_path(project),
-              method: :post, class: "btn btn-default"
-    - else
-      = link_to 'Export project', export_project_path(project),
-              method: :post, class: "btn btn-default"
+.sub-section
+  %h4= _('Export project')
+  %p= _('Export this project with all its related data in order to move your project to a new GitLab instance. Once the export is finished, you can import the file from the "New Project" page.')
+
+  .bs-callout.bs-callout-info
+    %p.append-bottom-0
+      %p= _('The following items will be exported:')
+      %ul
+        %li= _('Project and wiki repositories')
+        %li= _('Project uploads')
+        %li= _('Project configuration, including services')
+        %li= _('Issues with comments, merge requests with diffs and comments, labels, milestones, snippets, and other project entities')
+        %li= _('LFS objects')
+      %p= _('The following items will NOT be exported:')
+      %ul
+        %li= _('Job traces and artifacts')
+        %li= _('Container registry images')
+        %li= _('CI variables')
+        %li= _('Webhooks')
+        %li= _('Any encrypted tokens')
+  %p= _('Once the exported file is ready, you will receive a notification email with a download link, or you can download it from this page.')
+  - if project.export_status == :finished
+    = link_to _('Download export'), download_export_project_path(project),
+            rel: 'nofollow', download: '', method: :get, class: "btn btn-default"
+    = link_to _('Generate new export'), generate_new_export_project_path(project),
+            method: :post, class: "btn btn-default"
+  - else
+    = link_to _('Export project'), export_project_path(project),
+            method: :post, class: "btn btn-default"

app/views/projects/settings/_general.html.haml

+= form_for [@project.namespace.becomes(Namespace), @project], remote: true, html: { multipart: true, class: "edit-project js-general-settings-form" }, authenticity_token: true do |f|
+  %input{ name: 'update_section', type: 'hidden', value: 'js-general-settings' }
+  = form_errors(@project)
+
+  %fieldset
+    .row
+      .form-group.col-md-5
+        = f.label :name, class: 'label-bold', for: 'project_name_edit' do
+          = _('Project name')
+        = f.text_field :name, class: 'form-control qa-project-name-field', id: "project_name_edit"
+
+      .form-group.col-md-7
+        = f.label :id, class: 'label-bold' do
+          = _('Project ID')
+        = f.text_field :id, class: 'form-control w-auto', readonly: true
+
+    .row
+      .form-group.col-md-9
+        = f.label :tag_list, _('Topics'), class: 'label-bold'
+        = f.text_field :tag_list, value: @project.tag_list.join(', '), maxlength: 2000, class: "form-control"
+        %p.form-text.text-muted= _('Separate topics with commas.')
+
+    .row
+      .form-group.col-md-9
+        = f.label :description, _('Project description (optional)'), class: 'label-bold'
+        = f.text_area :description, class: 'form-control', rows: 3, maxlength: 250
+
+  .row= render_if_exists 'projects/classification_policy_settings', f: f
+
+  .row= render_if_exists 'shared/repository_size_limit_setting', form: f, type: :project
+
+  .form-group.prepend-top-default.append-bottom-20
+    .avatar-container.s90
+      = project_icon(@project, alt: _('Project avatar'), class: 'avatar project-avatar s90')
+    = f.label :avatar, _('Project avatar'), class: 'label-bold d-block'
+    = render 'shared/choose_avatar_button', f: f
+    - if @project.avatar?
+      %hr
+      = link_to _('Remove avatar'), project_avatar_path(@project), data: { confirm: _('Avatar will be removed. Are you sure?')}, method: :delete, class: 'btn btn-link'
+
+
+  = f.submit _('Save changes'), class: "btn btn-success mt-4 qa-save-naming-topics-avatar-button"

app/views/shared/_confirm_modal.html.haml

@@ -2,8 +2,7 @@
   .modal-dialog
     .modal-content
       .modal-header
-        %h3.page-title
-          Confirmation required
+        %h3.page-title= _('Confirmation required')
         %button.close{ type: "button", "data-dismiss": "modal", "aria-label" => _('Close') }
           %span{ "aria-hidden": true } ×
 
@@ -11,9 +10,7 @@
         %p.text-danger.js-confirm-text
 
         %p
-          %span.js-warning-text
-            This action can lead to data loss.
-            To prevent accidental actions we ask you to confirm your intention.
+          %span.js-warning-text= _('This action can lead to data loss. To prevent accidental actions we ask you to confirm your intention.')
           %br
           Please type
           %code.js-confirm-danger-match= phrase
@@ -22,4 +19,4 @@
         .form-group
           = text_field_tag 'confirm_name_input', '', class: 'form-control js-confirm-danger-input'
         .form-actions
-          = submit_tag 'Confirm', class: "btn btn-danger js-confirm-danger-submit"
+          = submit_tag _('Confirm'), class: "btn btn-danger js-confirm-danger-submit"

ee/app/views/projects/_merge_request_approvals_settings.html.haml

@@ -9,7 +9,7 @@
       = link_to _("Learn more about approvals."), help_page_path("user/project/merge_requests/merge_request_approvals"), target: '_blank'
 
   .settings-content
-    = form_for [@project.namespace.becomes(Namespace), @project], remote: true, html: { class: "merge-request-approval-settings-form" }, authenticity_token: true do |f|
+    = form_for [@project.namespace.becomes(Namespace), @project], remote: true, html: { class: "merge-request-approval-settings-form js-mr-approvals-form" }, authenticity_token: true do |f|
       %input{ name: 'update_section', type: 'hidden', value: 'js-merge-request-approval-settings' }
       = render 'projects/merge_request_approvals_settings_form', form: f, project: @project
       = f.submit _("Save changes"), class: "btn btn-success"

ee/app/views/projects/_issues_settings.html.haml → ee/app/views/projects/settings/_default_issue_template.html.haml

@@ -2,18 +2,20 @@
   - expanded = Rails.env.test?
   %section.settings.issues-feature.no-animate#js-issue-settings{ class: [('expanded' if expanded), ('hidden' if @project.project_feature.send(:issues_access_level) == 0)] }
     .settings-header
-      %h4.settings-title.js-settings-toggle.js-settings-toggle-trigger-only= _('Issue settings')
-      %button.btn.js-settings-toggle= expanded ? _('Collapse') : _('Expand')
-      %p= _('Customize your issue restrictions.')
+      %h4.settings-title.js-settings-toggle.js-settings-toggle-trigger-only= _('Default issue template')
+      %button.btn.btn-default.js-settings-toggle= expanded ? _('Collapse') : _('Expand')
+      %p= _('Set a default template for issue descriptions.')
 
     .settings-content
       = form_for [@project.namespace.becomes(Namespace), @project], remote: true, html: { multipart: true, class: "issue-settings-form" }, authenticity_token: true do |f|
         %input{ type: 'hidden', name: 'update_section', value: 'js-issue-settings' }
-        .form-group
-          = f.label :issues_template, class: 'label-bold' do
-            Default description template for issues
-            = link_to icon('question-circle'), help_page_path('user/project/description_templates', anchor: 'setting-a-default-template-for-issues-and-merge-requests'), target: '_blank'
-          = f.text_area :issues_template, class: "form-control", rows: 3
-          .hint
-            Description parsed with #{link_to "GitLab Flavored Markdown", help_page_path('user/markdown'), target: '_blank'}.
-        = f.submit 'Save changes', class: "btn btn-success"
+        .row
+          .form-group.col-md-9
+            = f.label :issues_template, class: 'label-bold' do
+              = _('Default description template for issues')
+              = link_to icon('question-circle'), help_page_path('user/project/description_templates', anchor: 'setting-a-default-template-for-issues-and-merge-requests'), target: '_blank'
+            = f.text_area :issues_template, class: "form-control", rows: 3
+            .text-secondary
+              - link_start = '<a href="%{url}" target="_blank" rel="noopener noreferrer">'.html_safe % { url: help_page_path('user/markdown') }
+              = _('Description parsed with %{link_start}GitLab Flavored Markdown%{link_end}').html_safe % { link_start: link_start, link_end: '</a>'.html_safe }
+        = f.submit _('Save changes'), class: "btn btn-success"

ee/app/views/shared/_repository_size_limit_setting.html.haml

 - return unless current_user.admin? && License.feature_available?(:repository_size_limit)
 
 - form = local_assigns.fetch(:form)
-- type = local_assigns.fetch(:type)
-- form_group_class = type === :group ? 'col-md-9' : ''
+- is_project = local_assigns.fetch(:type) == :project
 
-.form-group{ class: form_group_class }
+.form-group.col-md-9
   = form.label :repository_size_limit, class: 'label-bold' do
     Repository size limit (MB)
   = form.number_field :repository_size_limit, value: form.object.repository_size_limit.try(:to_mb), class: 'form-control', min: 0
   %span.form-text.text-muted#repository_size_limit_help_block
-    = type === :project ? size_limit_message(@project) : size_limit_message_for_group(@group)
+    = is_project ? size_limit_message(@project) : size_limit_message_for_group(@group)

ee/changelogs/unreleased/proj-settings-ok-leftovers.yml

+---
+title: Improve project settings page layout and UX
+merge_request: 10388
+author:
+type: other

ee/spec/features/projects/settings/issues_settings_spec.rb

@@ -17,14 +17,14 @@ describe 'Project settings > Issues', :js do
       end
 
       it 'shows the Issues settings' do
-        expect(page).to have_content('Customize your issue restrictions')
+        expect(page).to have_content('Set a default template for issue descriptions.')
 
         within('.sharing-permissions-form') do
           find('.project-feature-controls[data-for="project[project_feature_attributes][issues_access_level]"] .project-feature-toggle').click
           click_on('Save changes')
         end
 
-        expect(page).not_to have_content('Customize your issue restrictions')
+        expect(page).not_to have_content('Set a default template for issue descriptions.')
       end
     end
   end
@@ -36,14 +36,14 @@ describe 'Project settings > Issues', :js do
     end
 
     it 'does not show the Issues settings' do
-      expect(page).not_to have_content('Customize your issue restrictions')
+      expect(page).not_to have_content('Set a default template for issue descriptions.')
 
       within('.sharing-permissions-form') do
         find('.project-feature-controls[data-for="project[project_feature_attributes][issues_access_level]"] .project-feature-toggle').click
         click_on('Save changes')
       end
 
-      expect(page).to have_content('Customize your issue restrictions')
+      expect(page).to have_content('Set a default template for issue descriptions.')
     end
   end
 

ee/spec/features/projects/settings/user_manages_approval_settings_spec.rb

@@ -3,16 +3,19 @@ require 'spec_helper'
 describe 'EE > Projects > Settings > User manages approval rule settings' do
   let(:project) { create(:project) }
   let(:user) { project.owner }
+  let(:path) { edit_project_path(project) }
 
   before do
     sign_in(user)
     stub_licensed_features(licensed_features)
-    visit edit_project_path(project)
+    visit path
   end
 
   context 'when `code_owner_approval_required` is available' do
     let(:licensed_features) { { code_owner_approval_required: true } }
 
+    it_behaves_like 'dirty submit form', [{ form: '#js-merge-request-approval-settings', input: '#project_merge_requests_author_approval' }]
+
     it 'allows the user to enforce code owner approval' do
       within('.require-code-owner-approval') do
         check('Require approval from code owners')

qa/qa/page/project/settings/advanced.rb

@@ -4,27 +4,21 @@ module QA
       module Settings
         class Advanced < Page::Base
           view 'app/views/projects/edit.html.haml' do
-            element :project_path_field, 'text_field :path' # rubocop:disable QA/ElementWithPattern
-            element :project_name_field, 'text_field :name' # rubocop:disable QA/ElementWithPattern
-            element :rename_project_button, "submit 'Rename project'" # rubocop:disable QA/ElementWithPattern
+            element :project_path_field
+            element :change_path_button
           end
 
-          def rename_to(path)
-            fill_project_name(path)
+          def update_project_path_to(path)
             fill_project_path(path)
-            rename_project!
+            click_change_path_button
           end
 
           def fill_project_path(path)
-            fill_in :project_path, with: path
+            fill_element :project_path_field, path
           end
 
-          def fill_project_name(name)
-            fill_in :project_name, with: name
-          end
-
-          def rename_project!
-            click_on 'Rename project'
+          def click_change_path_button
+            click_element :change_path_button
           end
         end
       end

qa/qa/page/project/settings/common.rb

@@ -4,14 +4,6 @@ module QA
       module Settings
         module Common
           include QA::Page::Settings::Common
-
-          def self.included(base)
-            base.class_eval do
-              view 'app/views/projects/edit.html.haml' do
-                element :advanced_settings_expand, "= expanded ? 'Collapse' : 'Expand'" # rubocop:disable QA/ElementWithPattern
-              end
-            end
-          end
         end
       end
     end

qa/qa/page/project/settings/main.rb

@@ -9,6 +9,24 @@ module QA
             element :advanced_settings
           end
 
+          view 'app/views/projects/settings/_general.html.haml' do
+            element :project_name_field
+            element :save_naming_topics_avatar_button
+          end
+
+          def rename_project_to(name)
+            fill_project_name(name)
+            click_save_changes
+          end
+
+          def fill_project_name(name)
+            fill_element :project_name_field, name
+          end
+
+          def click_save_changes
+            click_element :save_naming_topics_avatar_button
+          end
+
           def expand_advanced_settings(&block)
             expand_section(:advanced_settings) do
               Advanced.perform(&block)

qa/qa/specs/features/browser_ui/ee_geo/rename_replication_spec.rb

@@ -34,8 +34,11 @@ module QA
 
           geo_project_renamed = "geo-after-rename-#{SecureRandom.hex(8)}"
           Page::Project::Settings::Main.perform do |settings|
+            settings.rename_project_to(geo_project_renamed)
+            expect(page).to have_content "Project '#{geo_project_renamed}' was successfully updated."
+
             settings.expand_advanced_settings do |page|
-              page.rename_to(geo_project_renamed)
+              page.update_project_path_to(geo_project_renamed)
             end
           end
 

spec/features/projects/settings/user_renames_a_project_spec.rb

@@ -9,24 +9,33 @@ describe 'Projects > Settings > User renames a project' do
     visit edit_project_path(project)
   end
 
-  def rename_project(project, name: nil, path: nil)
-    fill_in('project_name', with: name) if name
-    fill_in('Path', with: path) if path
-    click_button('Rename project')
+  def change_path(project, path)
+    within('.advanced-settings') do
+      fill_in('Path', with: path)
+      click_button('Change path')
+    end
+    project.reload
     wait_for_edit_project_page_reload
+  end
+
+  def change_name(project, name)
+    within('.general-settings') do
+      fill_in('Project name', with: name)
+      click_button('Save changes')
+    end
     project.reload
+    wait_for_edit_project_page_reload
   end
 
   def wait_for_edit_project_page_reload
-    expect(find('.project-edit-container')).to have_content('Rename repository')
+    expect(find('.advanced-settings')).to have_content('Change path')
   end
 
   context 'with invalid characters' do
-    it 'shows errors for invalid project path/name' do
-      rename_project(project, name: 'foo&bar', path: 'foo&bar')
-      expect(page).to have_field 'Project name', with: 'foo&bar'
+    it 'shows errors for invalid project path' do
+      change_path(project, 'foo&bar')
+
       expect(page).to have_field 'Path', with: 'foo&bar'
-      expect(page).to have_content "Name can contain only letters, digits, emojis, '_', '.', dash, space. It must start with letter, digit, emoji or '_'."
       expect(page).to have_content "Path can contain only letters, digits, '_', '-' and '.'. Cannot start with '-', end in '.git' or end in '.atom'"
     end
   end
@@ -42,13 +51,13 @@ describe 'Projects > Settings > User renames a project' do
 
   context 'when changing project name' do
     it 'renames the repository' do
-      rename_project(project, name: 'bar')
+      change_name(project, 'bar')
       expect(find('.breadcrumbs')).to have_content(project.name)
     end
 
     context 'with emojis' do
       it 'shows error for invalid project name' do
-        rename_project(project, name: '🚀 foo bar ☁️')
+        change_name(project, '🚀 foo bar ☁️')
         expect(page).to have_field 'Project name', with: '🚀 foo bar ☁️'
         expect(page).not_to have_content "Name can contain only letters, digits, emojis '_', '.', dash and space. It must start with letter, digit, emoji or '_'."
       end
@@ -67,7 +76,7 @@ describe 'Projects > Settings > User renames a project' do
     end
 
     it 'the project is accessible via the new path' do
-      rename_project(project, path: 'bar')
+      change_path(project, 'bar')
       new_path = namespace_project_path(project.namespace, 'bar')
       visit new_path
 
@@ -77,7 +86,7 @@ describe 'Projects > Settings > User renames a project' do
 
     it 'the project is accessible via a redirect from the old path' do
       old_path = project_path(project)
-      rename_project(project, path: 'bar')
+      change_path(project, 'bar')
       new_path = namespace_project_path(project.namespace, 'bar')
       visit old_path
 
@@ -88,7 +97,7 @@ describe 'Projects > Settings > User renames a project' do
     context 'and a new project is added with the same path' do
       it 'overrides the redirect' do
         old_path = project_path(project)
-        rename_project(project, path: 'bar')
+        change_path(project, 'bar')
         new_project = create(:project, namespace: user.namespace, path: 'gitlabhq', name: 'quz')
         visit old_path
 

spec/features/projects_spec.rb

@@ -373,6 +373,21 @@ describe 'Project' do
     end
   end
 
+  describe 'edit' do
+    let(:user) { create(:user) }
+    let(:project) { create(:project, :public) }
+    let(:path) { edit_project_path(project) }
+
+    before do
+      project.add_maintainer(user)
+      sign_in(user)
+      visit path
+    end
+
+    it_behaves_like 'dirty submit form', [{ form: '.js-general-settings-form', input: 'input[name="project[name]"]' },
+                                          { form: '.qa-merge-request-settings', input: '#project_printing_merge_request_link_enabled' }]
+  end
+
   def remove_with_confirm(button_text, confirm_with)
     click_button button_text
     fill_in 'confirm_name_input', with: confirm_with

spec/support/shared_examples/dirty_submit_form_shared_examples.rb

 shared_examples 'dirty submit form' do |selector_args|
   selectors = selector_args.is_a?(Array) ? selector_args : [selector_args]
 
-  def expect_disabled_state(form, submit, is_disabled = true)
+  def expect_disabled_state(form, submit_selector, is_disabled = true)
     disabled_selector = is_disabled == true ? '[disabled]' : ':not([disabled])'
 
-    form.find(".js-dirty-submit#{disabled_selector}", match: :first)
-
-    expect(submit.disabled?).to be is_disabled
+    form.find("#{submit_selector}#{disabled_selector}")
   end
 
   selectors.each do |selector|
     it "disables #{selector[:form]} submit until there are changes on #{selector[:input]}", :js do
       form = find(selector[:form])
-      submit = form.first('.js-dirty-submit')
+      submit_selector = selector[:submit] || 'input[type="submit"]'
+      submit = form.first(submit_selector)
       input = form.first(selector[:input])
       is_radio = input[:type] == 'radio'
       is_checkbox = input[:type] == 'checkbox'
@@ -22,15 +21,14 @@ shared_examples 'dirty submit form' do |selector_args|
       original_checkable = input if is_checkbox
 
       expect(submit.disabled?).to be true
-      expect(input.checked?).to be false
 
       is_checkable ? input.click : input.set("#{original_value} changes")
 
-      expect_disabled_state(form, submit, false)
+      expect_disabled_state(form, submit_selector, false)
 
       is_checkable ? original_checkable.click : input.set(original_value)
 
-      expect_disabled_state(form, submit)
+      expect_disabled_state(form, submit_selector)
     end
   end
 end