Commit 7a2370f7 authored by Robert Speicher's avatar Robert Speicher

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.
parent 2ed6cd9e
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
......@@ -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
......
= 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
......
= 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
......
= 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"} &times;
- 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
......
......@@ -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
......
......@@ -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"
......
= 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
......
......@@ -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
......
= 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'
......
= 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'
......
.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
......
= 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'
......
......@@ -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
......
......@@ -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
......
%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'
......
......@@ -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
......
......@@ -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'
......
......@@ -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
......
= 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
......
- if @project.errors.any?
.alert.alert-danger
%button{ type: "button", class: "close", "data-dismiss" => "alert"} &times;
= @project.errors.full_messages.first
= form_errors(@project)
%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"
......
......@@ -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
......
= 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'
......
......@@ -28,10 +28,7 @@
.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
......
= 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
......
......@@ -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'
......
......@@ -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
......
= 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
......
- 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
......
- 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
......
.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'
......
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
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment