Merge branch 'rs-form_errors' into 'master'

Standardize the way we check for and display form errors

- Some views had a "Close" button. We've removed this, because we don't
  want users accidentally hiding the validation errors and not knowing
  what needs to be fixed.
- Some views used `li`, some used `p`, some used `span`. We've
  standardized on `li`.
- Some views only showed the first error. We've standardized on showing
  all of them.
- Some views added an `#error_explanation` div, which we've made
  standard.

See merge request !3531

app/helpers/form_helper.rb

@@ -0,0 +1,18 @@
+module FormHelper
+  def form_errors(model)
+    return unless model.errors.any?
+
+    pluralized = 'error'.pluralize(model.errors.count)
+    headline   = "The form contains the following #{pluralized}:"
+
+    content_tag(:div, class: 'alert alert-danger', id: 'error_explanation') do
+      content_tag(:h4, headline) <<
+      content_tag(:ul) do
+        model.errors.full_messages.
+          map { |msg| content_tag(:li, msg) }.
+          join.
+          html_safe
+      end
+    end
+  end
+end

app/views/abuse_reports/new.html.haml

@@ -3,11 +3,9 @@
 %p Please use this form to report users who create spam issues, comments or behave inappropriately.
 %hr
 = form_for @abuse_report, html: { class: 'form-horizontal js-quick-submit js-requires-input'} do |f|
+  = form_errors(@abuse_report)
+
   = f.hidden_field :user_id
-  - if @abuse_report.errors.any?
-    .alert.alert-danger
-      - @abuse_report.errors.full_messages.each do |msg|
-        %p= msg
   .form-group
     = f.label :user_id, class: 'control-label'
     .col-sm-10

app/views/admin/appearances/_form.html.haml

@@ -1,8 +1,5 @@
 = form_for @appearance, url: admin_appearances_path, html: { class: 'form-horizontal'} do |f|
-  - if @appearance.errors.any?
-    .alert.alert-danger
-      - @appearance.errors.full_messages.each do |msg|
-        %p= msg
+  = form_errors(@appearance)
 
   %fieldset.sign-in
     %legend

app/views/admin/application_settings/_form.html.haml

@@ -1,9 +1,5 @@
 = form_for @application_setting, url: admin_application_settings_path, html: { class: 'form-horizontal fieldset-form' } do |f|
-  - if @application_setting.errors.any?
-    #error_explanation
-      .alert.alert-danger
-        - @application_setting.errors.full_messages.each do |msg|
-          %p= msg
+  = form_errors(@application_setting)
 
   %fieldset
     %legend Visibility and Access Controls

app/views/admin/applications/_form.html.haml

@@ -1,9 +1,6 @@
 = form_for [:admin, @application], url: @url, html: {class: 'form-horizontal', role: 'form'} do |f|
-  - if application.errors.any?
-    .alert.alert-danger
-      %button{ type: "button", class: "close", "data-dismiss" => "alert"} ×
-      - application.errors.full_messages.each do |msg|
-        %p= msg
+  = form_errors(application)
+
   = content_tag :div, class: 'form-group' do
     = f.label :name, class: 'col-sm-2 control-label'
     .col-sm-10

app/views/admin/broadcast_messages/_form.html.haml

@@ -4,10 +4,8 @@
     = render_broadcast_message(@broadcast_message.message.presence || "Your message here")
 
 = form_for [:admin, @broadcast_message], html: { class: 'broadcast-message-form form-horizontal js-quick-submit js-requires-input'} do |f|
-  -if @broadcast_message.errors.any?
-    .alert.alert-danger
-      - @broadcast_message.errors.full_messages.each do |msg|
-        %p= msg
+  = form_errors(@broadcast_message)
+
   .form-group
     = f.label :message, class: 'control-label'
     .col-sm-10

app/views/admin/deploy_keys/new.html.haml

@@ -4,11 +4,7 @@
 
 %div
   = form_for [:admin, @deploy_key], html: { class: 'deploy-key-form form-horizontal' } do |f|
-    -if @deploy_key.errors.any?
-      .alert.alert-danger
-        %ul
-          - @deploy_key.errors.full_messages.each do |msg|
-            %li= msg
+    = form_errors(@deploy_key)
 
     .form-group
       = f.label :title, class: "control-label"

app/views/admin/groups/_form.html.haml

@@ -1,8 +1,5 @@
 = form_for [:admin, @group], html: { class: "form-horizontal" } do |f|
-  - if @group.errors.any?
-    .alert.alert-danger
-      %span= @group.errors.full_messages.first
-
+  = form_errors(@group)
   = render 'shared/group_form', f: f
 
   .form-group.group-description-holder

app/views/admin/hooks/index.html.haml

@@ -10,10 +10,8 @@
 
 
 = form_for @hook, as: :hook, url: admin_hooks_path, html: { class: 'form-horizontal' } do |f|
-  -if @hook.errors.any?
-    .alert.alert-danger
-      - @hook.errors.full_messages.each do |msg|
-        %p= msg
+  = form_errors(@hook)
+
   .form-group
     = f.label :url, "URL:", class: 'control-label'
     .col-sm-10

app/views/admin/identities/_form.html.haml

@@ -1,9 +1,5 @@
 = form_for [:admin, @user, @identity], html: { class: 'form-horizontal fieldset-form' } do |f|
-  - if @identity.errors.any?
-    #error_explanation
-      .alert.alert-danger
-        - @identity.errors.full_messages.each do |msg|
-          %p= msg
+  = form_errors(@identity)
 
   .form-group
     = f.label :provider, class: 'control-label'

app/views/admin/labels/_form.html.haml

@@ -1,11 +1,5 @@
 = form_for [:admin, @label], html: { class: 'form-horizontal label-form js-requires-input' } do |f|
-  -if @label.errors.any?
-    .row
-      .col-sm-offset-2.col-sm-10
-        .alert.alert-danger
-          - @label.errors.full_messages.each do |msg|
-            %span= msg
-            %br
+  = form_errors(@label)
 
   .form-group
     = f.label :title, class: 'control-label'

app/views/admin/users/_form.html.haml

@@ -1,10 +1,6 @@
 .user_new
   = form_for [:admin, @user], html: { class: 'form-horizontal fieldset-form' } do |f|
-    -if @user.errors.any?
-      #error_explanation
-        .alert.alert-danger
-          - @user.errors.full_messages.each do |msg|
-            %p= msg
+    = form_errors(@user)
 
     %fieldset
       %legend Account

app/views/doorkeeper/applications/_form.html.haml

@@ -1,9 +1,5 @@
 = form_for application, url: doorkeeper_submit_path(application), html: {role: 'form'} do |f|
-  - if application.errors.any?
-    .alert.alert-danger
-      %ul
-        - application.errors.full_messages.each do |msg|
-          %li= msg
+  = form_errors(application)
 
   .form-group
     = f.label :name, class: 'label-light'

app/views/groups/edit.html.haml

@@ -5,9 +5,7 @@
     Group settings
   .panel-body
     = form_for @group, html: { multipart: true, class: "form-horizontal" }, authenticity_token: true do |f|
-      - if @group.errors.any?
-        .alert.alert-danger
-          %span= @group.errors.full_messages.first
+      = form_errors(@group)
       = render 'shared/group_form', f: f
 
       .form-group

app/views/groups/new.html.haml

@@ -6,10 +6,7 @@
 %hr
 
 = form_for @group, html: { class: 'group-form form-horizontal' } do |f|
-  - if @group.errors.any?
-    .alert.alert-danger
-      %span= @group.errors.full_messages.first
-
+  = form_errors(@group)
   = render 'shared/group_form', f: f, autofocus: true
 
   .form-group.group-description-holder

app/views/profiles/keys/_form.html.haml

@@ -1,10 +1,6 @@
 %div
   = form_for [:profile, @key], html: { class: 'js-requires-input' } do |f|
-    - if @key.errors.any?
-      .alert.alert-danger
-        %ul
-          - @key.errors.full_messages.each do |msg|
-            %li= msg
+    = form_errors(@key)
 
     .form-group
       = f.label :key, class: 'label-light'

app/views/profiles/notifications/show.html.haml

@@ -2,11 +2,7 @@
 - header_title page_title, profile_notifications_path
 
 = form_for @user, url: profile_notifications_path, method: :put, html: { class: 'update-notifications prepend-top-default' } do |f|
-  -if @user.errors.any?
-    %div.alert.alert-danger
-      %ul
-        - @user.errors.full_messages.each do |msg|
-          %li= msg
+  = form_errors(@user)
 
   = hidden_field_tag :notification_type, 'global'
   .row

app/views/profiles/passwords/edit.html.haml

@@ -13,11 +13,8 @@
       - unless @user.password_automatically_set?
         or recover your current one
     = form_for @user, url: profile_password_path, method: :put, html: {class: "update-password"} do |f|
-      -if @user.errors.any?
-        .alert.alert-danger
-          %ul
-            - @user.errors.full_messages.each do |msg|
-              %li= msg
+      = form_errors(@user)
+
       - unless @user.password_automatically_set?
         .form-group
           = f.label :current_password, class: 'label-light'

app/views/profiles/passwords/new.html.haml

@@ -7,11 +7,8 @@
     Please set a new password before proceeding.
     %br
     After a successful password update you will be redirected to login screen.
-  -if @user.errors.any?
-    .alert.alert-danger
-      %ul
-        - @user.errors.full_messages.each do |msg|
-          %li= msg
+
+  = form_errors(@user)
 
   - unless @user.password_automatically_set?
     .form-group

app/views/profiles/show.html.haml

@@ -1,9 +1,6 @@
 = form_for @user, url: profile_path, method: :put, html: { multipart: true, class: "edit-user prepend-top-default" }, authenticity_token: true do |f|
-  -if @user.errors.any?
-    %div.alert.alert-danger
-      %ul
-        - @user.errors.full_messages.each do |msg|
-          %li= msg
+  = form_errors(@user)
+
   .row
     .col-lg-3.profile-settings-sidebar
       %h4.prepend-top-0

app/views/projects/_errors.html.haml

@@ -1,4 +1 @@
-- if @project.errors.any?
-  .alert.alert-danger
-    %button{ type: "button", class: "close", "data-dismiss" => "alert"} ×
-    = @project.errors.full_messages.first
+= form_errors(@project)

app/views/projects/deploy_keys/_form.html.haml

@@ -1,10 +1,6 @@
 %div
   = form_for [@project.namespace.becomes(Namespace), @project, @key], url: namespace_project_deploy_keys_path, html: { class: 'deploy-key-form form-horizontal js-requires-input' } do |f|
-    -if @key.errors.any?
-      .alert.alert-danger
-        %ul
-          - @key.errors.full_messages.each do |msg|
-            %li= msg
+    = form_errors(@key)
 
     .form-group
       = f.label :title, class: "control-label"

app/views/projects/hooks/index.html.haml

@@ -9,10 +9,8 @@
 %hr.clearfix
 
 = form_for [@project.namespace.becomes(Namespace), @project, @hook], as: :hook, url: namespace_project_hooks_path(@project.namespace, @project), html: { class: 'form-horizontal' } do |f|
-  -if @hook.errors.any?
-    .alert.alert-danger
-      - @hook.errors.full_messages.each do |msg|
-        %p= msg
+  = form_errors(@hook)
+
   .form-group
     = f.label :url, "URL", class: 'control-label'
     .col-sm-10

app/views/projects/labels/_form.html.haml

@@ -1,11 +1,5 @@
 = form_for [@project.namespace.becomes(Namespace), @project, @label], html: { class: 'form-horizontal label-form js-quick-submit js-requires-input' } do |f|
-  -if @label.errors.any?
-    .row
-      .col-sm-offset-2.col-sm-10
-        .alert.alert-danger
-          - @label.errors.full_messages.each do |msg|
-            %span= msg
-            %br
+  = form_errors(@label)
 
   .form-group
     = f.label :title, class: 'control-label'

app/views/projects/merge_requests/_new_compare.html.haml

@@ -72,10 +72,7 @@
           %ul.list-unstyled.mr_target_commit
 
   - if @merge_request.errors.any?
-    .alert.alert-danger
-      - @merge_request.errors.full_messages.each do |msg|
-        %div= msg
-
+    = form_errors(@merge_request)
   - elsif @merge_request.source_branch.present? && @merge_request.target_branch.present?
     .light-well.append-bottom-default
       .center

app/views/projects/milestones/_form.html.haml

@@ -1,9 +1,6 @@
 = form_for [@project.namespace.becomes(Namespace), @project, @milestone], html: {class: 'form-horizontal milestone-form gfm-form js-quick-submit js-requires-input'}  do |f|
-  -if @milestone.errors.any?
-    .alert.alert-danger
-      %ul
-        - @milestone.errors.full_messages.each do |msg|
-          %li= msg
+  = form_errors(@milestone)
+
   .row
     .col-md-6
       .form-group

app/views/projects/protected_branches/index.html.haml

@@ -13,11 +13,7 @@
 
 - if can? current_user, :admin_project, @project
   = form_for [@project.namespace.becomes(Namespace), @project, @protected_branch], html: { class: 'form-horizontal' } do |f|
-    -if @protected_branch.errors.any?
-      .alert.alert-danger
-        %ul
-          - @protected_branch.errors.full_messages.each do |msg|
-            %li= msg
+    = form_errors(@protected_branch)
 
     .form-group
       = f.label :name, "Branch", class: 'control-label'

app/views/projects/variables/show.html.haml

@@ -13,13 +13,7 @@
 
 
 = nested_form_for @project, url: url_for(controller: 'projects/variables', action: 'update'), html: { class: 'form-horizontal' }  do |f|
-  - if @project.errors.any?
-    #error_explanation
-      %p.lead= "#{pluralize(@project.errors.count, "error")} prohibited this project from being saved:"
-      .alert.alert-error
-        %ul
-          - @project.errors.full_messages.each do |msg|
-            %li= msg
+  = form_errors(@project)
 
   = f.fields_for :variables do |variable_form|
     .form-group

app/views/projects/wikis/_form.html.haml

@@ -1,9 +1,5 @@
 = form_for [@project.namespace.becomes(Namespace), @project, @page], method: @page.persisted? ? :put : :post, html: { class: 'form-horizontal wiki-form gfm-form prepend-top-default js-quick-submit' } do |f|
-  -if @page.errors.any?
-    #error_explanation
-      .alert.alert-danger
-        - @page.errors.full_messages.each do |msg|
-          %p= msg
+  = form_errors(@page)
 
   = f.hidden_field :title, value: @page.title
   .form-group

app/views/shared/_service_settings.html.haml

@@ -1,9 +1,4 @@
-- if @service.errors.any?
-  #error_explanation
-    .alert.alert-danger
-      %ul
-        - @service.errors.full_messages.each do |msg|
-          %li= msg
+= form_errors(@service)
 
 - if @service.help.present?
   .well

app/views/shared/issuable/_form.html.haml

@@ -1,10 +1,5 @@
-- if issuable.errors.any?
-  .row
-    .col-sm-offset-2.col-sm-10
-      .alert.alert-danger
-        - issuable.errors.full_messages.each do |msg|
-          %span= msg
-          %br
+= form_errors(issuable)
+
 .form-group
   = f.label :title, class: 'control-label'
   .col-sm-10

app/views/shared/snippets/_form.html.haml

@@ -1,10 +1,6 @@
 .snippet-form-holder
   = form_for @snippet, url: url, html: { class: "form-horizontal snippet-form js-requires-input" } do |f|
-    - if @snippet.errors.any?
-      .alert.alert-danger
-        %ul
-          - @snippet.errors.full_messages.each do |msg|
-            %li= msg
+    = form_errors(@snippet)
 
     .form-group
       = f.label :title, class: 'control-label'

spec/helpers/form_helper_spec.rb

@@ -0,0 +1,46 @@
+require 'rails_helper'
+
+describe FormHelper do
+  describe 'form_errors' do
+    it 'returns nil when model has no errors' do
+      model = double(errors: [])
+
+      expect(helper.form_errors(model)).to be_nil
+    end
+
+    it 'renders an alert div' do
+      model = double(errors: errors_stub('Error 1'))
+
+      expect(helper.form_errors(model)).
+        to include('<div class="alert alert-danger" id="error_explanation">')
+    end
+
+    it 'contains a summary message' do
+      single_error = double(errors: errors_stub('A'))
+      multi_errors = double(errors: errors_stub('A', 'B', 'C'))
+
+      expect(helper.form_errors(single_error)).
+        to include('<h4>The form contains the following error:')
+      expect(helper.form_errors(multi_errors)).
+        to include('<h4>The form contains the following errors:')
+    end
+
+    it 'renders each message' do
+      model = double(errors: errors_stub('Error 1', 'Error 2', 'Error 3'))
+
+      errors = helper.form_errors(model)
+
+      aggregate_failures do
+        expect(errors).to include('<li>Error 1</li>')
+        expect(errors).to include('<li>Error 2</li>')
+        expect(errors).to include('<li>Error 3</li>')
+      end
+    end
+
+    def errors_stub(*messages)
+      ActiveModel::Errors.new(double).tap do |errors|
+        messages.each { |msg| errors.add(:base, msg) }
+      end
+    end
+  end
+end