Skip to content
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
wants to merge 21 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
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 Dec 20, 2024
a353c4e
[TAN-3444] Rubocop fixes
jamesspeake Dec 20, 2024
804664c
[TAN-3444] Test fix
jamesspeake Dec 23, 2024
338465c
[TAN-3444] Added extra form editor metadata to track when opened etc
jamesspeake Dec 23, 2024
5d9e1c5
[TAN-3444] Added missing serializer
jamesspeake Dec 23, 2024
ac5cbb7
[TAN-3444] Fixed tests
jamesspeake Jan 2, 2025
f18fb00
[TAN-3444] Test fixes
jamesspeake Jan 2, 2025
bf6ce68
[TAN-3444] Updated date format
jamesspeake Jan 2, 2025
4a71f63
[TAN-3444] Added endpoint for custom_form
jamesspeake Jan 2, 2025
74403f8
[TAN-3444] Added useCustomForm and use last updated to restrict form …
jamesspeake Jan 2, 2025
5e2f1b7
[TAN-3444] Added specific error message for stale data
jamesspeake Jan 2, 2025
bc2f10c
Translations updated by CI (extract-intl)
Jan 2, 2025
5520dba
[TAN-3444] Refresh custom form data when successfully saved
jamesspeake Jan 3, 2025
0a27604
Merge remote-tracking branch 'origin/TAN-3444-improve-form-save-loggi…
jamesspeake Jan 3, 2025
b40409b
Merge branch 'master' into TAN-3444-improve-form-save-logging
jamesspeake Jan 3, 2025
fbc6a4f
Translations updated by CI (extract-intl)
Jan 3, 2025
dd289ad
[TAN-3444] Test and lint fixes
jamesspeake Jan 3, 2025
b55e2fa
[TAN-3444] Ensure that open at only gets set the first time the compo…
jamesspeake Jan 3, 2025
8bb06e3
[TAN-3444] Code tidy & test fixes
jamesspeake Jan 3, 2025
2f4bc0e
[TAN-3444] Linting fix
jamesspeake Jan 3, 2025
4e09a4d
[TAN-3444] Removed commented code
jamesspeake Jan 3, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions back/app/serializers/web_api/v1/custom_field_serializer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ class WebApi::V1::CustomFieldSerializer < WebApi::V1::BaseSerializer
}

has_many :options, record_type: :custom_field_option, serializer: ::WebApi::V1::CustomFieldOptionSerializer
has_one :resource, record_type: :custom_form, serializer: ::WebApi::V1::CustomFormSerializer
end

WebApi::V1::CustomFieldSerializer.include(CustomMaps::Extensions::WebApi::V1::CustomFieldSerializer)
8 changes: 8 additions & 0 deletions back/app/serializers/web_api/v1/custom_form_serializer.rb
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
9 changes: 9 additions & 0 deletions back/app/services/side_fx_custom_form_service.rb
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
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ module V1
module Admin
module IdeaCustomFieldsController
def include_in_index_response
%i[options options.image map_config]
super + %i[map_config]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just tidying up this

end

def relate_map_config_to_field(field, field_params, errors, index)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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

Expand All @@ -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
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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),
Expand All @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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,
Expand Down Expand Up @@ -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

Expand Down
2 changes: 2 additions & 0 deletions back/engines/commercial/idea_custom_fields/config/routes.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
defaults: { container_type: 'Project' }
) do
patch 'update_all', on: :collection
get 'custom_form', on: :collection
resources :custom_field_options, controller: '/web_api/v1/custom_field_options', only: %i[show]
end
end
Expand All @@ -23,6 +24,7 @@
defaults: { container_type: 'Phase' }
) do
patch 'update_all', on: :collection
get 'custom_form', on: :collection
get :as_geojson, on: :member, action: 'as_geojson'
resources :custom_field_options, controller: '/web_api/v1/custom_field_options', only: %i[show]
end
Expand Down
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
Loading