From 51553bc71db952689fb27424b6a458fb90eae7d3 Mon Sep 17 00:00:00 2001 From: Faizaan Gagan Date: Mon, 31 Jan 2022 15:11:14 +0530 Subject: [PATCH] FEATURE: validate liquid templates on wizard save (#156) * DEV: validate liquid templates on wizard save * minor fix * code improvements and spec * version bump * fixed failing specs * FIX: handle displaying backend validation errors on frontend * fixed linting * improved error display * validate raw description for steps * refactor conditional * Identify attribute with liquid template error and pass syntax error Co-authored-by: angusmcleod Co-authored-by: Angus McLeod --- .../admin-wizards-wizard-show.js.es6 | 28 +++--- .../discourse/models/custom-wizard.js.es6 | 2 +- config/locales/server.en.yml | 1 + controllers/custom_wizard/admin/wizard.rb | 2 +- lib/custom_wizard/validators/template.rb | 34 ++++++++ plugin.rb | 4 +- spec/components/custom_wizard/builder_spec.rb | 2 +- .../custom_wizard/template_validator_spec.rb | 87 +++++++++++++++++++ spec/fixtures/wizard.json | 9 +- .../wizard_field_serializer_spec.rb | 2 +- .../wizard_step_serializer_spec.rb | 3 +- 11 files changed, 155 insertions(+), 19 deletions(-) diff --git a/assets/javascripts/discourse/controllers/admin-wizards-wizard-show.js.es6 b/assets/javascripts/discourse/controllers/admin-wizards-wizard-show.js.es6 index 332efedd..1eeb62e6 100644 --- a/assets/javascripts/discourse/controllers/admin-wizards-wizard-show.js.es6 +++ b/assets/javascripts/discourse/controllers/admin-wizards-wizard-show.js.es6 @@ -58,6 +58,21 @@ export default Controller.extend({ } return wizardFieldList(steps); }, + getErrorMessage(result) { + if (result.backend_validation_error) { + return result.backend_validation_error; + } + + let errorType = "failed"; + let errorParams = {}; + + if (result.error) { + errorType = result.error.type; + errorParams = result.error.params; + } + + return I18n.t(`admin.wizard.error.${errorType}`, errorParams); + }, actions: { save() { @@ -80,18 +95,7 @@ export default Controller.extend({ this.send("afterSave", result.wizard_id); }) .catch((result) => { - let errorType = "failed"; - let errorParams = {}; - - if (result.error) { - errorType = result.error.type; - errorParams = result.error.params; - } - - this.set( - "error", - I18n.t(`admin.wizard.error.${errorType}`, errorParams) - ); + this.set("error", this.getErrorMessage(result)); later(() => this.set("error", null), 10000); }) diff --git a/assets/javascripts/discourse/models/custom-wizard.js.es6 b/assets/javascripts/discourse/models/custom-wizard.js.es6 index e6a8408d..2fa6bc4c 100644 --- a/assets/javascripts/discourse/models/custom-wizard.js.es6 +++ b/assets/javascripts/discourse/models/custom-wizard.js.es6 @@ -28,7 +28,7 @@ const CustomWizard = EmberObject.extend({ contentType: "application/json", data: JSON.stringify(data), }).then((result) => { - if (result.error) { + if (result.backend_validation_error) { reject(result); } else { resolve(result); diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 88e945f9..81697e43 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -51,6 +51,7 @@ en: after_signup: "You can only have one 'after signup' wizard at a time. %{wizard_id} has 'after signup' enabled." after_signup_after_time: "You can't use 'after time' and 'after signup' on the same wizard." after_time: "After time setting is invalid." + liquid_syntax_error: "Liquid syntax error in %{attribute}: %{message}" site_settings: custom_wizard_enabled: "Enable custom wizards." diff --git a/controllers/custom_wizard/admin/wizard.rb b/controllers/custom_wizard/admin/wizard.rb index e2edfba0..09471680 100644 --- a/controllers/custom_wizard/admin/wizard.rb +++ b/controllers/custom_wizard/admin/wizard.rb @@ -37,7 +37,7 @@ class CustomWizard::AdminWizardController < CustomWizard::AdminController wizard_id = template.save(create: params[:create]) if template.errors.any? - render json: failed_json.merge(errors: template.errors.full_messages) + render json: failed_json.merge(backend_validation_error: template.errors.full_messages.join("\n\n")) else render json: success_json.merge(wizard_id: wizard_id) end diff --git a/lib/custom_wizard/validators/template.rb b/lib/custom_wizard/validators/template.rb index 5c3f5218..e2820c97 100644 --- a/lib/custom_wizard/validators/template.rb +++ b/lib/custom_wizard/validators/template.rb @@ -20,10 +20,12 @@ class CustomWizard::TemplateValidator data[:steps].each do |step| check_required(step, :step) + validate_liquid_template(step, :step) if step[:fields].present? step[:fields].each do |field| check_required(field, :field) + validate_liquid_template(field, :field) end end end @@ -31,6 +33,7 @@ class CustomWizard::TemplateValidator if data[:actions].present? data[:actions].each do |action| check_required(action, :action) + validate_liquid_template(action, :action) end end @@ -95,4 +98,35 @@ class CustomWizard::TemplateValidator errors.add :base, I18n.t("wizard.validation.after_time") end end + + def validate_liquid_template(object, type) + %w[ + description + raw_description + placeholder + preview_template + post_template + ].each do |field| + if template = object[field] + result = is_liquid_template_valid?(template) + + unless "valid" == result + error = I18n.t("wizard.validation.liquid_syntax_error", + attribute: "#{object[:id]}.#{field}", + message: result + ) + errors.add :base, error + end + end + end + end + + def is_liquid_template_valid?(template) + begin + Liquid::Template.parse(template) + 'valid' + rescue Liquid::SyntaxError => error + error.message + end + end end diff --git a/plugin.rb b/plugin.rb index 87050d25..a0094420 100644 --- a/plugin.rb +++ b/plugin.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true # name: discourse-custom-wizard # about: Create custom wizards -# version: 1.16.5 +# version: 1.17.0 # authors: Angus McLeod # url: https://github.com/paviliondev/discourse-custom-wizard # contact emails: angus@thepavilion.io @@ -117,6 +117,8 @@ after_initialize do load File.expand_path(path, __FILE__) end + Liquid::Template.error_mode = :strict + # preloaded category custom fields %w[ create_topic_wizard diff --git a/spec/components/custom_wizard/builder_spec.rb b/spec/components/custom_wizard/builder_spec.rb index def54fc4..099d8681 100644 --- a/spec/components/custom_wizard/builder_spec.rb +++ b/spec/components/custom_wizard/builder_spec.rb @@ -324,7 +324,7 @@ describe CustomWizard::Builder do .build .steps.first .fields.length - ).to eq(4) + ).to eq(@template[:steps][0][:fields].length) end context "with condition" do diff --git a/spec/components/custom_wizard/template_validator_spec.rb b/spec/components/custom_wizard/template_validator_spec.rb index d8706307..0ff0d1e7 100644 --- a/spec/components/custom_wizard/template_validator_spec.rb +++ b/spec/components/custom_wizard/template_validator_spec.rb @@ -9,6 +9,33 @@ describe CustomWizard::TemplateValidator do "#{Rails.root}/plugins/discourse-custom-wizard/spec/fixtures/wizard.json" ).read).with_indifferent_access } + let(:valid_liquid_template) { + <<-LIQUID.strip + {%- assign hello = "Topic Form 1" %} + LIQUID + } + + let(:invalid_liquid_template) { + <<-LIQUID.strip + {%- assign hello = "Topic Form 1" % + LIQUID + } + + let(:liquid_syntax_error) { + "Liquid syntax error: Tag '{%' was not properly terminated with regexp: /\\%\\}/" + } + + def expect_validation_success + expect( + CustomWizard::TemplateValidator.new(template).perform + ).to eq(true) + end + + def expect_validation_failure(object_id, message) + validator = CustomWizard::TemplateValidator.new(template) + expect(validator.perform).to eq(false) + expect(validator.errors.first.message).to eq("Liquid syntax error in #{object_id}: #{message}") + end it "validates valid templates" do expect( @@ -110,4 +137,64 @@ describe CustomWizard::TemplateValidator do end end end + + context "liquid templates" do + it "validates if no liquid syntax in use" do + expect_validation_success + end + + it "validates if liquid syntax in use is correct" do + template[:steps][0][:raw_description] = valid_liquid_template + expect_validation_success + end + + it "doesn't validate if liquid syntax in use is incorrect" do + template[:steps][0][:raw_description] = invalid_liquid_template + expect_validation_failure("step_1.raw_description", liquid_syntax_error) + end + + context "validation targets" do + context "fields" do + it "validates descriptions" do + template[:steps][0][:fields][0][:description] = invalid_liquid_template + expect_validation_failure("step_1_field_1.description", liquid_syntax_error) + end + + it "validates placeholders" do + template[:steps][0][:fields][0][:placeholder] = invalid_liquid_template + expect_validation_failure("step_1_field_1.placeholder", liquid_syntax_error) + end + + it "validates preview templates" do + template[:steps][0][:fields][4][:preview_template] = invalid_liquid_template + expect_validation_failure("step_1_field_5.preview_template", liquid_syntax_error) + end + end + + context "steps" do + it "validates descriptions" do + template[:steps][0][:raw_description] = invalid_liquid_template + expect_validation_failure("step_1.raw_description", liquid_syntax_error) + end + end + + context "actions" do + it "validates post builder" do + action = nil + action_index = nil + + template[:actions].each_with_index do |a, i| + if a["post_builder"] + action = a + action_index = i + break + end + end + template[:actions][action_index][:post_template] = invalid_liquid_template + + expect_validation_failure("#{action[:id]}.post_template", liquid_syntax_error) + end + end + end + end end diff --git a/spec/fixtures/wizard.json b/spec/fixtures/wizard.json index 4727c7a8..01421c7f 100644 --- a/spec/fixtures/wizard.json +++ b/spec/fixtures/wizard.json @@ -43,6 +43,13 @@ "label": "I'm only text", "description": "", "type": "text_only" + }, + { + "id": "step_1_field_5", + "label": "I'm a preview", + "description": "", + "type": "composer_preview", + "preview_template": "w{step_1_field_1}" } ], "description": "Text inputs!" @@ -576,4 +583,4 @@ ] } ] -} \ No newline at end of file +} diff --git a/spec/serializers/custom_wizard/wizard_field_serializer_spec.rb b/spec/serializers/custom_wizard/wizard_field_serializer_spec.rb index 1fa9671c..a5a5e721 100644 --- a/spec/serializers/custom_wizard/wizard_field_serializer_spec.rb +++ b/spec/serializers/custom_wizard/wizard_field_serializer_spec.rb @@ -21,7 +21,7 @@ describe CustomWizard::FieldSerializer do scope: Guardian.new(user) ).as_json - expect(json_array.size).to eq(4) + expect(json_array.size).to eq(@wizard.steps.first.fields.size) expect(json_array[0][:label]).to eq("

Text

") expect(json_array[0][:description]).to eq("Text field description.") expect(json_array[3][:index]).to eq(3) diff --git a/spec/serializers/custom_wizard/wizard_step_serializer_spec.rb b/spec/serializers/custom_wizard/wizard_step_serializer_spec.rb index 35ce0fd2..21345352 100644 --- a/spec/serializers/custom_wizard/wizard_step_serializer_spec.rb +++ b/spec/serializers/custom_wizard/wizard_step_serializer_spec.rb @@ -43,7 +43,8 @@ describe CustomWizard::StepSerializer do each_serializer: described_class, scope: Guardian.new(user) ).as_json - expect(json_array[0][:fields].length).to eq(4) + + expect(json_array[0][:fields].length).to eq(@wizard.steps[0].fields.length) end context 'with required data' do