Skip to content

Commit

Permalink
Lock Application During Update_And_Advance (#996)
Browse files Browse the repository at this point in the history
## Description of change

[Provider: Fast Submit Clicks Can Wrongly Create Multiple
Records](https://dsdmoj.atlassian.net/browse/CRM457-1746)

## Notes for reviewer

- Lock current_application when update_and_advance method in
BaseStepController is handling attempting to update records
- Disable all submit buttons associated with a form when the form is
being submitted
  • Loading branch information
ivanELEC authored Jul 16, 2024
1 parent 2ff8f77 commit f25739b
Show file tree
Hide file tree
Showing 8 changed files with 51 additions and 9 deletions.
1 change: 1 addition & 0 deletions app/javascript/application.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// https://frontend.design-system.service.gov.uk/importing-css-assets-and-javascript/#javascript
import { initAll } from 'govuk-frontend'
import accessibleAutocomplete from 'accessible-autocomplete'
import './handle-forms'
import '@hotwired/turbo-rails'
import $ from 'jquery'

Expand Down
26 changes: 26 additions & 0 deletions app/javascript/handle-forms.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
import $ from 'jquery'

window.$ = $

$(function() {
$('button[type="submit"]').on("click", function(e) {
e.preventDefault()
let form = $(this).closest("form")
let formButtons = form.find('button[type="submit"]')
formButtons.each(function(){
$(this).attr("disabled", true)
})

/*
this code ensures the button's submission type is appended to params
so that the BaseStepController still has this available to know
whether the form should commit_draft/save_and_refresh/reload
*/
let buttonAction = $(this).attr("name")
if(buttonAction){
form.append(`<input type="hidden" name="${buttonAction}" value="" >`)
}

form.trigger('submit.rails')
})
})
2 changes: 1 addition & 1 deletion config/locales/en/errors.yml
Original file line number Diff line number Diff line change
Expand Up @@ -428,7 +428,7 @@ en:
work_before_date: *shared_date_errors
work_after_date: *shared_date_errors
work_completed_date:
blank: Enter the date work completeed
blank: Enter the date work completed
invalid: Enter a valid date work completed
invalid_day: Enter a valid day work completed
invalid_month: Enter a valid month work completed
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,30 +29,36 @@ def decision_tree_class
raise 'implement this action, in subclasses'
end

# rubocop:disable Metrics/MethodLength, Metrics/AbcSize
def update_and_advance(form_class, opts = {})
hash = permitted_params(form_class).to_h
record = opts.fetch(:record, current_application)
@form_object = form_class.new(
hash.merge(application: current_application, record: record)
)

current_application.with_lock do
process_form(@form_object, opts)
end
end

# rubocop:disable Metrics/AbcSize, Metrics/MethodLength
def process_form(form_object, opts)
if params.key?(:commit_draft)
# Validations will not be run when saving a draft
@form_object.save!
form_object.save!
redirect_to opts[:after_commit_redirect_path] || nsm_after_commit_path(id: current_application.id)
elsif params.key?(:reload)
reload
elsif params.key?(:save_and_refresh)
@form_object.save!
form_object.save!
redirect_to_current_object
elsif @form_object.save
redirect_to decision_tree_class.new(@form_object, as: opts.fetch(:as)).destination, flash: opts[:flash]
elsif form_object.save
redirect_to decision_tree_class.new(form_object, as: opts.fetch(:as)).destination, flash: opts[:flash]
else
render opts.fetch(:render, :edit)
end
end
# rubocop:enable Metrics/MethodLength, Metrics/AbcSize
# rubocop:enable Metrics/AbcSize, Metrics/MethodLength

# This deals with the case when it is called from the NEW_RECORD endpoint
# to avoid creating a new record on each click
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,10 @@ class FakeApp < Steps::BaseFormObject
def save!(*)
true
end

def with_lock
true
end
end

RSpec.describe DummyStepController, type: :controller do
Expand All @@ -31,6 +35,7 @@ def save!(*)
let(:destination) { { action: :show, id: application.id, controller: :dummy_step } }

before do
allow(application).to receive(:with_lock).and_yield
allow(DummyStepImplementation).to receive_messages(form_class: form_class, current_application: application,
options: options, decision_tree_class: decision_tree_class)
end
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
# Needed because some specs that include these examples stub current_application,
# which is undesirable for this particular test
allow(controller).to receive(:current_application).and_return(current_application)
allow(current_application).to receive(:with_lock).and_yield
end

describe '#edit' do
Expand Down Expand Up @@ -108,7 +109,7 @@

context 'when the form saves successfully' do
before do
expect(form_object).to receive(:save).and_return(true)
allow(form_object).to receive(:save).and_return(true)
end

let(:decision_tree) { instance_double(Decisions::DecisionTree, destination: '/expected_destination') }
Expand All @@ -123,7 +124,7 @@

context 'when the form fails to save' do
before do
expect(form_object).to receive(:save).and_return(false)
allow(form_object).to receive(:save).and_return(false)
end

it 'renders the question page again' do
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
# Needed because some specs that include these examples stub current_application,
# which is undesirable for this particular test
allow(controller).to receive(:current_application).and_return(current_application)
allow(current_application).to receive(:with_lock).and_yield
end

describe '#edit' do
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
# Needed because some specs that include these examples stub current_application,
# which is undesirable for this particular test
allow(controller).to receive(:current_application).and_return(current_application)
allow(current_application).to receive(:with_lock).and_yield
end

describe '#edit' do
Expand Down Expand Up @@ -111,6 +112,7 @@

it 'asks the decision tree for the next destination and redirects there' do
expect(Decisions::DecisionTree).to receive(:new).and_return(decision_tree)

put :update, params: expected_params
expect(response).to have_http_status(:redirect)
expect(subject).to redirect_to('/expected_destination')
Expand Down

0 comments on commit f25739b

Please sign in to comment.