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 <angus@mcleod.org.au>
Co-authored-by: Angus McLeod <angusmcleod@users.noreply.github.com>

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);
         })

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);

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."

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

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

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

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

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

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!"

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("<p>Text</p>")
     expect(json_array[0][:description]).to eq("Text field description.")
     expect(json_array[3][:index]).to eq(3)

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