-
Notifications
You must be signed in to change notification settings - Fork 33
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
TAN-3444 - Prevent custom form saving if updated more recently in another browser window #9888
Open
jamesspeake
wants to merge
21
commits into
master
Choose a base branch
from
TAN-3444-improve-form-save-logging
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
21 commits
Select commit
Hold shift + click to select a range
aa287c7
[TAN-3444] Added additional logging to custom form editing
jamesspeake a353c4e
[TAN-3444] Rubocop fixes
jamesspeake 804664c
[TAN-3444] Test fix
jamesspeake 338465c
[TAN-3444] Added extra form editor metadata to track when opened etc
jamesspeake 5d9e1c5
[TAN-3444] Added missing serializer
jamesspeake ac5cbb7
[TAN-3444] Fixed tests
jamesspeake f18fb00
[TAN-3444] Test fixes
jamesspeake bf6ce68
[TAN-3444] Updated date format
jamesspeake 4a71f63
[TAN-3444] Added endpoint for custom_form
jamesspeake 74403f8
[TAN-3444] Added useCustomForm and use last updated to restrict form …
jamesspeake 5e2f1b7
[TAN-3444] Added specific error message for stale data
jamesspeake bc2f10c
Translations updated by CI (extract-intl)
5520dba
[TAN-3444] Refresh custom form data when successfully saved
jamesspeake 0a27604
Merge remote-tracking branch 'origin/TAN-3444-improve-form-save-loggi…
jamesspeake b40409b
Merge branch 'master' into TAN-3444-improve-form-save-logging
jamesspeake fbc6a4f
Translations updated by CI (extract-intl)
dd289ad
[TAN-3444] Test and lint fixes
jamesspeake b55e2fa
[TAN-3444] Ensure that open at only gets set the first time the compo…
jamesspeake 8bb06e3
[TAN-3444] Code tidy & test fixes
jamesspeake 2f4bc0e
[TAN-3444] Linting fix
jamesspeake 4e09a4d
[TAN-3444] Removed commented code
jamesspeake File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
# frozen_string_literal: true | ||
|
||
class WebApi::V1::CustomFormSerializer < WebApi::V1::BaseSerializer | ||
attributes :updated_at | ||
attribute :opened_at do |_object| | ||
Time.zone.now.strftime('%Y-%m-%dT%H:%M:%S.%LZ') # Same format as updated_at | ||
end | ||
end |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
# frozen_string_literal: true | ||
|
||
class SideFxCustomFormService | ||
include SideFxHelper | ||
|
||
def after_update(custom_form, current_user, payload) | ||
LogActivityJob.perform_later(custom_form, 'changed', current_user, custom_form.updated_at.to_i, payload: payload) | ||
end | ||
end |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -24,7 +24,7 @@ class WebApi::V1::Admin::IdeaCustomFieldsController < ApplicationController | |
} | ||
|
||
before_action :set_custom_field, only: %i[show as_geojson] | ||
before_action :set_custom_form, only: %i[index update_all] | ||
before_action :set_custom_form, only: %i[index update_all custom_form] | ||
skip_after_action :verify_policy_scoped | ||
rescue_from UpdatingFormWithInputError, with: :render_updating_form_with_input_error | ||
|
||
|
@@ -46,7 +46,7 @@ def show | |
render json: ::WebApi::V1::CustomFieldSerializer.new( | ||
@custom_field, | ||
params: jsonapi_serializer_params, | ||
include: %i[options options.image] | ||
include: include_in_index_response | ||
).serializable_hash | ||
end | ||
|
||
|
@@ -63,14 +63,15 @@ def as_geojson | |
|
||
def update_all | ||
authorize CustomField.new(resource: @custom_form), :update_all?, policy_class: IdeaCustomFieldPolicy | ||
@participation_method = @custom_form.participation_context.pmethod | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Did not need setting as a global variable |
||
raise_error_if_stale_form_data | ||
|
||
page_temp_ids_to_ids_mapping = {} | ||
option_temp_ids_to_ids_mapping = {} | ||
errors = {} | ||
update_fields! page_temp_ids_to_ids_mapping, option_temp_ids_to_ids_mapping, errors | ||
@custom_form.reload if @custom_form.persisted? | ||
update_logic! page_temp_ids_to_ids_mapping, option_temp_ids_to_ids_mapping, errors | ||
update_form! | ||
render json: ::WebApi::V1::CustomFieldSerializer.new( | ||
IdeaCustomFieldsService.new(@custom_form).all_fields, | ||
params: serializer_params(@custom_form), | ||
|
@@ -80,11 +81,20 @@ def update_all | |
render json: { errors: e.errors }, status: :unprocessable_entity | ||
end | ||
|
||
def custom_form | ||
authorize CustomField.new(resource: @custom_form), :index?, policy_class: IdeaCustomFieldPolicy | ||
render json: ::WebApi::V1::CustomFormSerializer.new( | ||
@custom_form, | ||
params: jsonapi_serializer_params, | ||
include: [] | ||
).serializable_hash | ||
end | ||
|
||
private | ||
|
||
# Overriden from CustomMaps::Patches::IdeaCustomFields::WebApi::V1::Admin::IdeaCustomFieldsController | ||
# Extended by CustomMaps::Patches::IdeaCustomFields::WebApi::V1::Admin::IdeaCustomFieldsController | ||
def include_in_index_response | ||
%i[options options.image] | ||
%i[options options.image resource] | ||
end | ||
|
||
def raise_error_if_not_geographic_field | ||
|
@@ -93,6 +103,16 @@ def raise_error_if_not_geographic_field | |
raise "Custom field with input_type: '#{@custom_field.input_type}' is not a geographic type" | ||
end | ||
|
||
# To try and avoid forms being overwritten with stale data, we check if the form has been updated since the form editor last loaded it | ||
# But ONLY if the FE sends the form_last_updated_at param | ||
def raise_error_if_stale_form_data | ||
return unless update_all_params[:form_last_updated_at].present? && | ||
@custom_form.persisted? && | ||
@custom_form.updated_at.to_i > update_all_params[:form_last_updated_at].to_datetime.to_i | ||
|
||
raise UpdateAllFailedError, { form: [{ error: 'stale_data' }] } | ||
end | ||
|
||
def update_fields!(page_temp_ids_to_ids_mapping, option_temp_ids_to_ids_mapping, errors) | ||
idea_custom_fields_service = IdeaCustomFieldsService.new(@custom_form) | ||
fields = idea_custom_fields_service.all_fields | ||
|
@@ -121,20 +141,22 @@ def update_fields!(page_temp_ids_to_ids_mapping, option_temp_ids_to_ids_mapping, | |
end | ||
relate_map_config_to_field(field, field_params, errors, index) | ||
field.set_list_position(index) | ||
count_fields(field) | ||
end | ||
raise UpdateAllFailedError, errors if errors.present? | ||
end | ||
end | ||
|
||
def create_field!(field_params, errors, page_temp_ids_to_ids_mapping, index) | ||
if field_params['code'].nil? && @participation_method.allowed_extra_field_input_types.exclude?(field_params['input_type']) | ||
participation_method = @custom_form.participation_context.pmethod | ||
if field_params['code'].nil? && participation_method.allowed_extra_field_input_types.exclude?(field_params['input_type']) | ||
errors[index.to_s] = { input_type: [{ error: 'inclusion', value: field_params['input_type'] }] } | ||
return false | ||
end | ||
|
||
create_params = field_params.except('temp_id').to_h | ||
if create_params.key?('code') && !create_params['code'].nil? | ||
default_field = @participation_method.default_fields(@custom_form).find do |field| | ||
default_field = participation_method.default_fields(@custom_form).find do |field| | ||
field.code == create_params['code'] | ||
end | ||
create_params['key'] = default_field.key | ||
|
@@ -263,7 +285,6 @@ def update_option!(option, option_params, errors, field_index, option_index) | |
end | ||
|
||
def delete_option!(option) | ||
SideFxCustomFieldOptionService.new.before_destroy option, current_user | ||
option.destroy! | ||
SideFxCustomFieldOptionService.new.after_destroy option, current_user | ||
option | ||
|
@@ -281,14 +302,45 @@ def update_logic!(page_temp_ids_to_ids_mapping, option_temp_ids_to_ids_mapping, | |
raise UpdateAllFailedError, errors if errors.present? | ||
end | ||
|
||
# Update the timestamp of the form and log the activity | ||
def update_form! | ||
return unless @custom_form.persisted? | ||
|
||
@custom_form.touch | ||
update_payload = { | ||
save_type: update_all_params[:form_save_type], | ||
pages: @page_count, | ||
sections: @section_count, | ||
fields: @field_count, | ||
params_size: params.to_s.size, | ||
form_opened_at: update_all_params[:form_opened_at]&.to_datetime, | ||
form_updated_at: @custom_form.updated_at&.to_datetime | ||
} | ||
SideFxCustomFormService.new.after_update @custom_form, current_user, update_payload | ||
end | ||
|
||
def count_fields(field) | ||
@page_count ||= 0 | ||
@section_count ||= 0 | ||
@field_count ||= 0 | ||
case field.input_type | ||
when 'section' | ||
@section_count += 1 | ||
when 'page' | ||
@page_count += 1 | ||
else | ||
@field_count += 1 | ||
end | ||
end | ||
|
||
def add_options_errors(options_errors, errors, field_index, option_index) | ||
errors[field_index.to_s] ||= {} | ||
errors[field_index.to_s][:options] ||= {} | ||
errors[field_index.to_s][:options][option_index.to_s] = options_errors | ||
end | ||
|
||
def update_all_params | ||
params.permit(custom_fields: [ | ||
params.permit(:form_last_updated_at, :form_opened_at, :form_save_type, custom_fields: [ | ||
:id, | ||
:temp_id, | ||
:code, | ||
|
@@ -330,6 +382,7 @@ def update_all_params | |
def set_custom_form | ||
container_id = params[secure_constantize(:container_id)] | ||
@container = secure_constantize(:container_class).find container_id | ||
|
||
@custom_form = CustomForm.find_or_initialize_by participation_context: @container | ||
end | ||
|
||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
25 changes: 25 additions & 0 deletions
25
...l/idea_custom_fields/spec/acceptance/idea_custom_fields/phase_context/custom_form_spec.rb
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,25 @@ | ||
# frozen_string_literal: true | ||
|
||
require 'rails_helper' | ||
require 'rspec_api_documentation/dsl' | ||
|
||
resource 'Custom Form' do | ||
before { header 'Content-Type', 'application/json' } | ||
|
||
get 'web_api/v1/admin/phases/:phase_id/custom_fields/custom_form' do | ||
let(:context) { create(:native_survey_phase) } | ||
let!(:form) { create(:custom_form, participation_context: context) } | ||
let(:phase_id) { context.id } | ||
|
||
context 'when admin' do | ||
before { admin_header_token } | ||
|
||
example_request 'Return the custom form for a phase' do | ||
assert_status 200 | ||
expect(response_data[:id]).to eq form.id | ||
expect(response_data[:type]).to eq 'custom_form' | ||
expect(response_data[:attributes].keys).to match_array %i[updated_at opened_at] | ||
end | ||
end | ||
end | ||
end |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just tidying up this