From c9b0ebb0708d17273b3a0dcd0fa12a1d42f3b495 Mon Sep 17 00:00:00 2001 From: Graeme Porteous Date: Fri, 22 May 2020 12:52:40 +0100 Subject: [PATCH 1/6] Fix extract form redirecting on error Should redisplay form instead of loading new request to extract. --- app/controllers/projects/extracts_controller.rb | 4 +++- .../projects/extracts_controller_spec.rb | 16 +++++++++++----- 2 files changed, 14 insertions(+), 6 deletions(-) diff --git a/app/controllers/projects/extracts_controller.rb b/app/controllers/projects/extracts_controller.rb index 9a6805af08..dc8bad42c4 100644 --- a/app/controllers/projects/extracts_controller.rb +++ b/app/controllers/projects/extracts_controller.rb @@ -15,7 +15,9 @@ def show def create authorize! :read, @project - if @project.submissions.create(submission_params) + submission = @project.submissions.new(submission_params) + + if submission.save redirect_to project_extract_path else render :show diff --git a/spec/controllers/projects/extracts_controller_spec.rb b/spec/controllers/projects/extracts_controller_spec.rb index c0be50c47f..74ed507895 100644 --- a/spec/controllers/projects/extracts_controller_spec.rb +++ b/spec/controllers/projects/extracts_controller_spec.rb @@ -165,13 +165,18 @@ def post_extract(extract = nil) include_context 'with a logged in user who can read the project' let(:submissions) { double(:submissions_collection) } - before { allow(project).to receive(:submissions).and_return(submissions) } + let(:submission) { instance_double(Project::Submission) } + + before do + allow(project).to receive(:submissions).and_return(submissions) + allow(submissions).to receive(:new).and_return(submission) + end end context 'submission created' do include_context 'extraction can be submitted' - before { allow(submissions).to receive(:create).and_return(true) } + before { allow(submission).to receive(:save).and_return(true) } it 'initialises new value set with request' do params = { @@ -192,9 +197,10 @@ def post_extract(extract = nil) it 'creates project submission' do value_set = instance_double(Dataset::ValueSet) allow(Dataset::ValueSet).to receive(:new).and_return(value_set) - expect(submissions).to receive(:create).with( + expect(submissions).to receive(:new).with( user: user, info_request: info_request, resource: value_set - ) + ).and_return(submission) + expect(submission).to receive(:save) post_extract end @@ -207,7 +213,7 @@ def post_extract(extract = nil) context 'submission validation fails' do include_context 'extraction can be submitted' - before { expect(submissions).to receive(:create).and_return(false) } + before { expect(submission).to receive(:save).and_return(false) } it 'assigns the project' do post_extract From 9e51265bca6ced063503bd7bd8e6a39f0034e284 Mon Sep 17 00:00:00 2001 From: Graeme Porteous Date: Fri, 22 May 2020 12:53:38 +0100 Subject: [PATCH 2/6] Show extract error message --- app/controllers/projects/extracts_controller.rb | 1 + spec/controllers/projects/extracts_controller_spec.rb | 5 +++++ 2 files changed, 6 insertions(+) diff --git a/app/controllers/projects/extracts_controller.rb b/app/controllers/projects/extracts_controller.rb index dc8bad42c4..9a17ad9d37 100644 --- a/app/controllers/projects/extracts_controller.rb +++ b/app/controllers/projects/extracts_controller.rb @@ -20,6 +20,7 @@ def create if submission.save redirect_to project_extract_path else + flash.now[:error] = _("Extraction couldn't be saved.") render :show end end diff --git a/spec/controllers/projects/extracts_controller_spec.rb b/spec/controllers/projects/extracts_controller_spec.rb index 74ed507895..1430e4dfba 100644 --- a/spec/controllers/projects/extracts_controller_spec.rb +++ b/spec/controllers/projects/extracts_controller_spec.rb @@ -225,6 +225,11 @@ def post_extract(extract = nil) expect(assigns[:info_request]).to eq(info_request) end + it 'sets flash now error' do + post_extract + expect(flash.now[:error]).to eq("Extraction couldn't be saved.") + end + it 'renders show template' do post_extract expect(response).to render_template('show') From eca5d17593e5fd06acad163644c16a2b07df6f47 Mon Sep 17 00:00:00 2001 From: Graeme Porteous Date: Fri, 22 May 2020 12:54:44 +0100 Subject: [PATCH 3/6] Keep extract form values on errors Reuse the ValueSet and Value instances which keeps the form values and shows form errors inline. --- app/controllers/projects/extracts_controller.rb | 5 ++++- app/views/projects/dataset/keys/_key.html.erb | 3 ++- app/views/projects/extracts/_form.html.erb | 5 +---- app/views/projects/extracts/_sidebar.html.erb | 3 ++- app/views/projects/extracts/show.html.erb | 3 ++- spec/controllers/projects/extracts_controller_spec.rb | 9 +++++++++ 6 files changed, 20 insertions(+), 8 deletions(-) diff --git a/app/controllers/projects/extracts_controller.rb b/app/controllers/projects/extracts_controller.rb index 9a17ad9d37..0ef8449d97 100644 --- a/app/controllers/projects/extracts_controller.rb +++ b/app/controllers/projects/extracts_controller.rb @@ -10,11 +10,14 @@ def show redirect_to @project, notice: msg return end + + @value_set = Dataset::ValueSet.new end def create authorize! :read, @project + @value_set = Dataset::ValueSet.new(extract_params) submission = @project.submissions.new(submission_params) if submission.save @@ -56,7 +59,7 @@ def submission_params { user: current_user, info_request: @info_request, - resource: Dataset::ValueSet.new(extract_params) + resource: @value_set } end end diff --git a/app/views/projects/dataset/keys/_key.html.erb b/app/views/projects/dataset/keys/_key.html.erb index 35fc4c3ca7..509170d6a1 100644 --- a/app/views/projects/dataset/keys/_key.html.erb +++ b/app/views/projects/dataset/keys/_key.html.erb @@ -1,4 +1,5 @@ -<%= f.fields_for :values, Dataset::Value.new do |nested_form| %> +<% value = f.object.values.find { |v| v.key.order == key.order } %> +<%= f.fields_for :values, value || Dataset::Value.new do |nested_form| %> <%= render partial: "projects/dataset/keys/#{key.format}_key", locals: { f: nested_form, key: key } %> <%= nested_form.hidden_field :dataset_key_id, value: key.to_param %> diff --git a/app/views/projects/extracts/_form.html.erb b/app/views/projects/extracts/_form.html.erb index e17fdeffd6..4846e76a7d 100644 --- a/app/views/projects/extracts/_form.html.erb +++ b/app/views/projects/extracts/_form.html.erb @@ -1,8 +1,5 @@
- <%= form_for Dataset::ValueSet.new, - url: project_extract_path, - as: :extract do |f| %> - + <%= form_for value_set, url: project_extract_path, as: :extract do |f| %> <%= hidden_field_tag :url_title, info_request.url_title %> <%= render project.key_set, f: f %> diff --git a/app/views/projects/extracts/_sidebar.html.erb b/app/views/projects/extracts/_sidebar.html.erb index c606cc96e3..4f66830042 100644 --- a/app/views/projects/extracts/_sidebar.html.erb +++ b/app/views/projects/extracts/_sidebar.html.erb @@ -3,6 +3,7 @@ <%= js_correspondence_navigation %> <%= render partial: 'form', locals: { project: project, - info_request: info_request } %> + info_request: info_request, + value_set: value_set } %>
diff --git a/app/views/projects/extracts/show.html.erb b/app/views/projects/extracts/show.html.erb index 34aa47a16f..c7aeec3113 100644 --- a/app/views/projects/extracts/show.html.erb +++ b/app/views/projects/extracts/show.html.erb @@ -29,5 +29,6 @@ <%= render partial: 'sidebar', locals: { project: @project, - info_request: @info_request } %> + info_request: @info_request, + value_set: @value_set } %> diff --git a/spec/controllers/projects/extracts_controller_spec.rb b/spec/controllers/projects/extracts_controller_spec.rb index 1430e4dfba..370bb4d159 100644 --- a/spec/controllers/projects/extracts_controller_spec.rb +++ b/spec/controllers/projects/extracts_controller_spec.rb @@ -32,6 +32,10 @@ expect(assigns[:project]).to eq(project) end + it 'assigns the value set' do + expect(assigns[:value_set]).to be_a(Dataset::ValueSet) + end + it 'renders the project template' do expect(response).to render_template('projects/extracts/show') end @@ -225,6 +229,11 @@ def post_extract(extract = nil) expect(assigns[:info_request]).to eq(info_request) end + it 'assigns the value set' do + post_extract + expect(assigns[:value_set]).to be_a(Dataset::ValueSet) + end + it 'sets flash now error' do post_extract expect(flash.now[:error]).to eq("Extraction couldn't be saved.") From c1da68c9ec8689433d8773ec187cf5a633cb53e5 Mon Sep 17 00:00:00 2001 From: Graeme Porteous Date: Fri, 22 May 2020 13:01:13 +0100 Subject: [PATCH 4/6] Change boolean field to select box When using checkbox we'll always get a value submitted even if the project member wants to skip this extraction. Using a select box allows as to have a blank value which can be ignored. --- app/views/projects/dataset/keys/_boolean_key.html.erb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/views/projects/dataset/keys/_boolean_key.html.erb b/app/views/projects/dataset/keys/_boolean_key.html.erb index a536f3d5d3..c3f2cee89d 100644 --- a/app/views/projects/dataset/keys/_boolean_key.html.erb +++ b/app/views/projects/dataset/keys/_boolean_key.html.erb @@ -1,4 +1,4 @@ -
- <%= f.check_box :value %> +
<%= f.label :value, key.title %> + <%= f.select :value, [[_('No'), 0], [_('Yes'), 1]], include_blank: true %>
From db4d4274d0c5557ac81c83e819c6145fd249043b Mon Sep 17 00:00:00 2001 From: Graeme Porteous Date: Fri, 22 May 2020 13:02:45 +0100 Subject: [PATCH 5/6] Ensure dataset values are present Don't allow value sets or submissions to be saved if the project member skips the form by clicking submit without entering any values. --- app/models/dataset/value_set.rb | 14 ++++++++++---- spec/factories/dataset_value_sets.rb | 9 ++++++--- spec/models/dataset/value_set_spec.rb | 25 +++++++++++++++++++++++++ 3 files changed, 41 insertions(+), 7 deletions(-) diff --git a/app/models/dataset/value_set.rb b/app/models/dataset/value_set.rb index ff0092dfeb..0244fc6f31 100644 --- a/app/models/dataset/value_set.rb +++ b/app/models/dataset/value_set.rb @@ -19,9 +19,7 @@ class Dataset::ValueSet < ApplicationRecord belongs_to :key_set, foreign_key: 'dataset_key_set_id' has_many :values, foreign_key: 'dataset_value_set_id', inverse_of: :value_set - accepts_nested_attributes_for :values, reject_if: proc { |attributes| - attributes['dataset_key_id'].blank? - } + accepts_nested_attributes_for :values RESOURCE_TYPES = %w[ InfoRequest @@ -29,7 +27,15 @@ class Dataset::ValueSet < ApplicationRecord FoiAttachment ].freeze - validates :key_set, presence: true + validates :key_set, :values, presence: true validates :resource_type, inclusion: { in: RESOURCE_TYPES }, if: :resource validates_associated :values + validate :check_at_least_one_value_is_present + + private + + def check_at_least_one_value_is_present + return unless values.map(&:value).all?(&:blank?) + errors.add :values, :emtpy + end end diff --git a/spec/factories/dataset_value_sets.rb b/spec/factories/dataset_value_sets.rb index ee78fff1e1..27954919db 100644 --- a/spec/factories/dataset_value_sets.rb +++ b/spec/factories/dataset_value_sets.rb @@ -30,11 +30,14 @@ end transient do - value_count { 0 } + value_count { 1 } end - after(:create) do |value_set, evaluator| - create_list(:dataset_value, evaluator.value_count, value_set: value_set) + after(:build) do |value_set, evaluator| + next if value_set.values.count > 0 || evaluator.value_count.zero? + value_set.values = build_list( + :dataset_value, evaluator.value_count, value_set: value_set + ) end end end diff --git a/spec/models/dataset/value_set_spec.rb b/spec/models/dataset/value_set_spec.rb index 5d95e2cf99..97a8e0c81f 100644 --- a/spec/models/dataset/value_set_spec.rb +++ b/spec/models/dataset/value_set_spec.rb @@ -48,6 +48,21 @@ end end + describe 'nested attibutes' do + it 'accpets attributes for values' do + key = FactoryBot.create(:dataset_key) + value_set = FactoryBot.create( + :dataset_value_set, + value_count: 0, + values_attributes: [{ dataset_key_id: key.id, value: '1' }] + ) + value = value_set.values.first + expect(value).to be_a Dataset::Value + expect(value.key).to eq key + expect(value.value).to eq '1' + end + end + describe 'validations' do it { is_expected.to be_valid } @@ -71,5 +86,15 @@ value_set.key_set = nil is_expected.not_to be_valid end + + it 'requires values' do + value_set.values = [] + is_expected.not_to be_valid + end + + it 'requires at least one value to be present' do + value_set.values.each { |v| v.value = '' } + is_expected.to_not be_valid + end end end From 7caf9709587d546b19614165af8417d12d73ad8e Mon Sep 17 00:00:00 2001 From: Graeme Porteous Date: Fri, 22 May 2020 15:16:40 +0100 Subject: [PATCH 6/6] Remove value require validation This is now not needed as value set checks that at least one value is present, if not a validation error is added to the value set instance. --- app/models/dataset/value.rb | 6 ++++-- spec/models/dataset/value_spec.rb | 3 +++ 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/app/models/dataset/value.rb b/app/models/dataset/value.rb index 5b6a2aa48a..823f92b8be 100644 --- a/app/models/dataset/value.rb +++ b/app/models/dataset/value.rb @@ -19,6 +19,8 @@ class Dataset::Value < ApplicationRecord belongs_to :value_set, foreign_key: 'dataset_value_set_id' belongs_to :key, foreign_key: 'dataset_key_id' - validates :value_set, :key, :value, presence: true - validates :value, format: { with: -> (value) { value.key&.format_regexp } } + validates :value_set, :key, presence: true + validates :value, + format: { with: -> (value) { value.key&.format_regexp } }, + allow_blank: true end diff --git a/spec/models/dataset/value_spec.rb b/spec/models/dataset/value_spec.rb index 47c9bb180e..1d8abbafe1 100644 --- a/spec/models/dataset/value_spec.rb +++ b/spec/models/dataset/value_spec.rb @@ -39,6 +39,7 @@ def invalid(value_to_test) it 'checks text values' do value.key = FactoryBot.build(:dataset_key, :text) + valid('') valid('A string') valid('1234') valid('1') @@ -47,6 +48,7 @@ def invalid(value_to_test) it 'checks numeric values' do value.key = FactoryBot.build(:dataset_key, :numeric) + valid('') invalid('A string') valid('1234') valid('1') @@ -55,6 +57,7 @@ def invalid(value_to_test) it 'checks boolean values' do value.key = FactoryBot.build(:dataset_key, :boolean) + valid('') invalid('A string') invalid('1234') valid('1')