Fix specs and tighten conditional handling

controllers/custom_wizard/steps.rb

@@ -8,7 +8,7 @@ class CustomWizard::StepsController < ::ApplicationController
 
     update[:fields] = {}
     if params[:fields]
-      field_ids = @step_template['fields'].map { |f| f['id'] }
+      field_ids = @builder.wizard.field_ids
       params[:fields].each do |k, v|
         update[:fields][k] = v if field_ids.include? k
       end
@@ -36,11 +36,15 @@ class CustomWizard::StepsController < ::ApplicationController
       if current_step.final?
         builder.template.actions.each do |action_template|
           if action_template['run_after'] === 'wizard_completion'
-            CustomWizard::Action.new(
+            action_result = CustomWizard::Action.new(
               action: action_template,
               wizard: @wizard,
-              data: current_submission
+              submission: current_submission
             ).perform
+
+            if action_result.success?
+              current_submission = action_result.submission
+            end
           end
         end
 
@@ -54,6 +58,8 @@ class CustomWizard::StepsController < ::ApplicationController
 
         result[:final] = true
       else
+        current_submission.save
+
         result[:final] = false
         result[:next_step_id] = current_step.next.id
       end

controllers/custom_wizard/wizard.rb

@@ -64,7 +64,7 @@ class CustomWizard::WizardController < ::ApplicationController
     if user && wizard.can_access?
       submission = wizard.current_submission
 
-      if submission && submission.redirect_to
+      if submission.present? && submission.redirect_to
         result.merge!(redirect_to: submission.redirect_to)
       end
 

lib/custom_wizard/action.rb

@@ -30,6 +30,9 @@ class CustomWizard::Action
     end
 
     save_log
+
+    @result.submission = @submission
+    @result
   end
 
   def mapper_data

lib/custom_wizard/action_result.rb

@@ -1,6 +1,6 @@
 # frozen_string_literal: true
 class CustomWizard::ActionResult
-  attr_accessor :success, :handler, :output
+  attr_accessor :success, :handler, :output, :submission
 
   def initialize
     @success = false

lib/custom_wizard/builder.rb

@@ -47,8 +47,8 @@ class CustomWizard::Builder
 
         step.on_update do |updater|
           @updater = updater
-          @submission = @wizard.current_submission || CustomWizard::Submission.new(@wizard)
-          @submission.fields.merge(@updater.submission)
+          @submission = @wizard.current_submission
+          @submission.fields.merge!(@updater.submission)
 
           @updater.validate
           next if @updater.errors.any?
@@ -63,7 +63,9 @@ class CustomWizard::Builder
             @submission.route_to = nil
             @submission.save
 
+            @wizard.update!
             @updater.result[:redirect_on_next] = route_to if route_to
+
             true
           else
             false
@@ -72,7 +74,7 @@ class CustomWizard::Builder
       end
     end
 
-    @wizard.update_step_order!
+    @wizard.update!
     @wizard
   end
 
@@ -89,7 +91,7 @@ class CustomWizard::Builder
 
     params[:value] = prefill_field(field_template, step_template)
 
-    if !build_opts[:reset] && (submission = @wizard.current_submission)
+    if !build_opts[:reset] && (submission = @wizard.current_submission).present?
       params[:value] = submission.fields[field_template['id']] if submission.fields[field_template['id']]
     end
 
@@ -272,14 +274,15 @@ class CustomWizard::Builder
     permitted_data = {}
     submission_key = nil
     params_key = nil
-    submission = @wizard.current_submission || CustomWizard::Submission.new(@wizard)
+    submission = @wizard.current_submission
 
     permitted_params.each do |pp|
       pair = pp['pairs'].first
       params_key = pair['key'].to_sym
       submission_key = pair['value'].to_sym
 
-      if submission_key && params_key
+      if submission_key && params_key && params[params_key].present?
+        submission.permitted_param_keys << submission_key.to_s
         submission.fields[submission_key] = params[params_key]
       end
     end
@@ -293,7 +296,7 @@ class CustomWizard::Builder
         pair['key'].present? && pair['value'].present?
       end
 
-      if pairs.any? && !@wizard.current_submission
+      if pairs.any? && !@wizard.current_submission.present?
         step.permitted = false
         break
       end
@@ -323,11 +326,15 @@ class CustomWizard::Builder
     if @template.actions.present?
       @template.actions.each do |action_template|
         if action_template['run_after'] === updater.step.id
-          CustomWizard::Action.new(
+          result = CustomWizard::Action.new(
             action: action_template,
             wizard: @wizard,
             submission: @submission
           ).perform
+
+          if result.success?
+            @submission = result.submission
+          end
         end
       end
     end

lib/custom_wizard/submission.rb

@@ -2,14 +2,15 @@ class CustomWizard::Submission
   include ActiveModel::SerializerSupport
 
   KEY ||= "submissions"
-  ACTION_KEY ||= "action"
   META ||= %w(submitted_at route_to redirect_on_complete redirect_to)
 
   attr_reader :id,
               :user,
+              :user_id,
               :wizard
 
-  attr_accessor :fields
+  attr_accessor :fields,
+                :permitted_param_keys
 
   META.each do |attr|
     class_eval { attr_accessor attr }
@@ -17,9 +18,10 @@ class CustomWizard::Submission
 
   def initialize(wizard, data = {}, user_id = nil)
     @wizard = wizard
+    @user_id = user_id
 
     if user_id
-      @user = User.find_by(id: user_id) || OpenStruct.new(deleted: true, id: user_id)
+      @user = User.find_by(id: user_id)
     else
       @user = wizard.user 
     end
@@ -31,23 +33,30 @@ class CustomWizard::Submission
     META.each do |attr|
       send("#{attr}=", data[attr]) if data[attr]
     end
+
+    @permitted_param_keys = data['permitted_param_keys'] || []
   end
 
   def save
     return nil unless wizard.save_submissions
-    validate_fields
+    validate
 
-    submissions = self.class.list(wizard, user_id: user.id).select { |s| s.id != self.id }
+    submission_list = self.class.list(wizard, user_id: user.id)
+    submissions = submission_list.select { |submission| submission.id != self.id }
     submissions.push(self)
 
-    submission_data = submissions.map { |s| s.fields_and_meta }
+    submission_data = submissions.map { |submission| data_to_save(submission)  }
     PluginStore.set("#{wizard.id}_#{KEY}", user.id, submission_data)
   end
 
-  def validate_fields
-    self.fields = fields.select do |key, value|
-      wizard.field_ids.include?(key) || key.include?(ACTION_KEY)
-    end
+  def validate
+    self.fields = fields.select { |key, value| validate_field_key(key) }
+  end
+
+  def validate_field_key(key)
+    wizard.field_ids.include?(key) ||
+    wizard.action_ids.include?(key) ||
+    permitted_param_keys.include?(key)
   end
 
   def fields_and_meta
@@ -62,6 +71,24 @@ class CustomWizard::Submission
     result
   end
 
+  def present?
+    fields_and_meta.present?
+  end
+
+  def data_to_save(submission)
+    data = {
+      id: submission.id
+    }
+
+    data.merge!(submission.fields_and_meta)
+
+    if submission.permitted_param_keys.present?
+      data[:permitted_param_keys] = submission.permitted_param_keys
+    end
+
+    data
+  end
+
   def self.get(wizard, user_id)
     data = PluginStore.get("#{wizard.id}_#{KEY}", user_id).first
     new(wizard, data, user_id)

lib/custom_wizard/wizard.rb

@@ -26,12 +26,16 @@ class CustomWizard::Wizard
                 :needs_groups,
                 :steps,
                 :step_ids,
+                :field_ids,
                 :first_step,
                 :start,
                 :actions,
+                :action_ids,
                 :user,
                 :submissions
 
+  attr_reader   :all_step_ids
+
   def initialize(attrs = {}, user = nil)
     @user = user
     attrs = attrs.with_indifferent_access
@@ -59,11 +63,22 @@ class CustomWizard::Wizard
 
     @first_step = nil
     @steps = []
+
     if attrs['steps'].present?
-      @step_ids = attrs['steps'].map { |s| s['id'] }
+      @step_ids = @all_step_ids = attrs['steps'].map { |s| s['id'] }
+
+      @field_ids = []
+      attrs['steps'].each do |step|
+        if step['fields'].present?
+          step['fields'].each do |field|
+            @field_ids << field['id']
+          end
+        end
+      end
     end
 
-    @actions = []
+    @actions = attrs['actions'] || []
+    @action_ids = @actions.map { |a| a['id'] }
   end
 
   def cast_bool(val)
@@ -84,7 +99,19 @@ class CustomWizard::Wizard
     step.index = (steps.size == 1 ? 0 : steps.size) if step.index.nil?
   end
 
-  def update_step_order!
+  def update!
+    update_step_order
+    update_step_ids
+    update_field_ids
+    update_action_ids
+
+    @submissions = nil
+    @current_submission = nil
+
+    true
+  end
+
+  def update_step_order
     steps.sort_by!(&:index)
 
     steps.each_with_index do |step, index|
@@ -103,7 +130,7 @@ class CustomWizard::Wizard
         step.conditional_final_step = true
       end
 
-      if index === (step_ids.length - 1)
+      if index === (all_step_ids.length - 1)
         step.last_step = true
       end
 
@@ -118,7 +145,7 @@ class CustomWizard::Wizard
         acting_user_id: user.id,
         action: ::UserHistory.actions[:custom_wizard_step],
         context: id,
-        subject: step_ids
+        subject: all_step_ids
       ).order("created_at").last
 
       last_completed_step.subject
@@ -222,8 +249,25 @@ class CustomWizard::Wizard
     @groups ||= ::Site.new(Guardian.new(user)).groups
   end
 
-  def field_ids
-    steps.map { |step| step.fields.map { |field| field.id } }.flatten
+  def update_step_ids
+    @step_ids = steps.map(&:id)
+  end
+
+  def update_field_ids
+    @field_ids = steps.map { |step| step.fields.map { |field| field.id } }.flatten
+  end
+
+  def update_action_ids
+    @action_ids = []
+
+    @actions.each do |action|
+      if action['run_after'].blank? ||
+         action['run_after'] === 'wizard_completion' ||
+         step_ids.include?(action['run_after'])
+
+        @action_ids << action['id']
+      end
+    end
   end
 
   def submissions
@@ -235,9 +279,9 @@ class CustomWizard::Wizard
     @current_submission ||= begin
       if submissions.present?
         unsubmitted = submissions.select { |submission| !submission.submitted_at }
-        unsubmitted.present? ? unsubmitted.first : nil
+        unsubmitted.present? ? unsubmitted.first : CustomWizard::Submission.new(self)
       else
-        nil
+        CustomWizard::Submission.new(self)
       end
     end
   end
@@ -252,6 +296,8 @@ class CustomWizard::Wizard
       current_submission.submitted_at = Time.now.iso8601
       current_submission.save
     end
+
+    update!
   end
 
   def self.create(wizard_id, user = nil)
@@ -321,7 +367,7 @@ class CustomWizard::Wizard
     wizard = self.create(wizard_id, user)
 
     if wizard.permitted?
-      submission = wizard.current_submission || CustomWizard::Submission.new(wizard)
+      submission = wizard.current_submission
       submission.redirect_to = url
       submission.save
     else

plugin.rb

@@ -86,6 +86,7 @@ after_initialize do
     ../serializers/custom_wizard/wizard_step_serializer.rb
     ../serializers/custom_wizard/wizard_serializer.rb
     ../serializers/custom_wizard/log_serializer.rb
+    ../serializers/custom_wizard/submission_serializer.rb
     ../serializers/custom_wizard/realtime_validation/similar_topics_serializer.rb
     ../extensions/extra_locales_controller.rb
     ../extensions/invites_controller.rb

serializers/custom_wizard/submission.rb

@@ -1,13 +0,0 @@
-class CustomWizard::SubmissionSerializer
-  attributes :id,
-             :username,
-             :fields,
-             :redirect_to,
-             :submitted_at
-
-  def username
-    object.user.deleted ?
-    I18n.t('admin.wizard.submission.no_user', user_id: object.user.id) :
-    object.user.username
-  end
-end

serializers/custom_wizard/submission_serializer.rb

@@ -0,0 +1,16 @@
+# frozen_string_literal: true
+class CustomWizard::SubmissionSerializer < ApplicationSerializer
+  attributes :id,
+             :username,
+             :fields,
+             :submitted_at,
+             :route_to,
+             :redirect_on_complete,
+             :redirect_to
+
+  def username
+    object.user.present? ?
+    object.user.username :
+    I18n.t('admin.wizard.submission.no_user', user_id: object.user_id)
+  end
+end

spec/components/custom_wizard/action_spec.rb

@@ -194,11 +194,10 @@ describe CustomWizard::Action do
       open_composer['post_template'] = "Body & more body & more body".dup
 
       wizard = CustomWizard::Wizard.new(@template, user)
-
       action = CustomWizard::Action.new(
         wizard: wizard,
         action: open_composer,
-        data: {}
+        submission: wizard.current_submission
       )
       action.perform
 

spec/components/custom_wizard/builder_spec.rb

@@ -257,10 +257,7 @@ describe CustomWizard::Builder do
 
         it 'is permitted if required data is present' do
           wizard = CustomWizard::Wizard.create('super_mega_fun_wizard', user)
-          data = {
-            step_1_field_1: 'I am a user submission'
-          }
-          CustomWizard::Submission.new(wizard, data).save
+          CustomWizard::Submission.new(wizard, step_1_field_1: "required").save
 
           expect(
             CustomWizard::Builder.new(@template[:id], user).build
@@ -280,7 +277,7 @@ describe CustomWizard::Builder do
           wizard = CustomWizard::Builder.new(@template[:id], user).build({},
             param: 'param_value'
           )
-          expect(wizard.current_submission['saved_param']).to eq('param_value')
+          expect(wizard.current_submission.fields['saved_param']).to eq('param_value')
         end
       end
 
@@ -362,7 +359,7 @@ describe CustomWizard::Builder do
 
         it "does not save submissions" do
           perform_update('step_1', step_1_field_1: 'Text input')
-          expect(@wizard.current_submission).to eq(nil)
+          expect(@wizard.current_submission.present?).to eq(false)
         end
       end
     end

spec/components/custom_wizard/submission_spec.rb

@@ -29,7 +29,7 @@ describe CustomWizard::Submission do
 
   it "saves a user's submission" do
     expect(
-      described_class.get(template_json["id"], user.id).fields["step_1_field_1"]
+      described_class.get(@wizard, user.id).fields["step_1_field_1"]
     ).to eq("I am a user submission")
   end
 
@@ -38,6 +38,6 @@ describe CustomWizard::Submission do
   end
 
   it "list submissions by wizard and user" do
-    expect(described_class.list(@wizard, user).size).to eq(1)
+    expect(described_class.list(@wizard, user_id: user.id).size).to eq(1)
   end
 end

spec/components/custom_wizard/wizard_spec.rb

@@ -34,7 +34,7 @@ describe CustomWizard::Wizard do
     template_json['steps'].each do |step_template|
       @wizard.append_step(step_template['id'])
     end
-    @wizard.update_step_order!
+    @wizard.update!
   end
 
   def progress_step(step_id, acting_user: user, wizard: @wizard)
@@ -44,7 +44,7 @@ describe CustomWizard::Wizard do
       context: wizard.id,
       subject: step_id
     )
-    @wizard.update_step_order!
+    @wizard.update!
   end
 
   it "appends steps" do
@@ -72,7 +72,7 @@ describe CustomWizard::Wizard do
     expect(@wizard.steps.first.index).to eq(2)
     expect(@wizard.steps.last.index).to eq(0)
 
-    @wizard.update_step_order!
+    @wizard.update!
 
     expect(@wizard.steps.first.id).to eq("step_3")
     expect(@wizard.steps.last.id).to eq("step_1")
@@ -204,7 +204,7 @@ describe CustomWizard::Wizard do
 
   context "submissions" do
     before do
-      CustomWizard::Submission.new(@wizard, step_1_field_1: "I am a user submission")
+      CustomWizard::Submission.new(@wizard, step_1_field_1: "I am a user submission").save
     end
 
     it "lists the user's submissions" do

spec/fixtures/step/required_data.json

@@ -6,9 +6,9 @@
       "pairs": [
         {
           "index": 0,
-          "key": "required_data",
+          "key": "step_1_field_1",
           "key_type": "text",
-          "value": "required_value",
+          "value": "required",
           "value_type": "text",
           "connector": "equal"
         }

spec/requests/custom_wizard/admin/submissions_controller_spec.rb

@@ -13,11 +13,14 @@ describe CustomWizard::AdminSubmissionsController do
     ).read)
   }
 
+  let(:template_2) {
+    temp = template.dup
+    temp["id"] = "super_mega_fun_wizard_2"
+    temp
+  }
+
   before do
     CustomWizard::Template.save(template, skip_jobs: true)
-
-    template_2 = template.dup
-    template_2["id"] = "super_mega_fun_wizard_2"
     CustomWizard::Template.save(template_2, skip_jobs: true)
 
     wizard1 = CustomWizard::Wizard.create(template["id"], user1)
@@ -33,7 +36,7 @@ describe CustomWizard::AdminSubmissionsController do
 
   it "returns a list of wizards" do
     get "/admin/wizards/submissions.json"
-    expect(response.parsed_body.length).to eq(3)
+    expect(response.parsed_body.length).to eq(2)
     expect(response.parsed_body.first['id']).to eq(template['id'])
   end
 

spec/requests/custom_wizard/application_controller_spec.rb

@@ -39,8 +39,8 @@ describe ApplicationController do
       it "saves original destination of user" do
         get '/', headers: { 'REFERER' => "/t/2" }
         expect(
-          CustomWizard::Wizard.submissions(@template['id'], user)
-            .first['redirect_to']
+          CustomWizard::Wizard.create(@template['id'], user).submissions
+            .first.fields['redirect_to']
         ).to eq("/t/2")
       end
 

spec/requests/custom_wizard/steps_controller_spec.rb

@@ -175,8 +175,11 @@ describe CustomWizard::StepsController do
 
     wizard_id = response.parsed_body['wizard']['id']
     wizard = CustomWizard::Wizard.create(wizard_id, user)
-    group_name = wizard.current_submission.fields['action_9']
+
+    group_name = wizard.submissions.first.fields['action_9']
     group = Group.find_by(name: group_name)
+
+    expect(group.present?).to eq(true)
     expect(group.full_name).to eq("My cool group")
   end
 

spec/requests/custom_wizard/wizard_controller_spec.rb

@@ -72,7 +72,7 @@ describe CustomWizard::WizardController do
 
   it 'skip response contains a redirect_to if in users submissions' do
     @wizard = CustomWizard::Wizard.create(@template["id"], user)
-    CustomWizard::Submission.new(@wizard, step_1_field_1: "I am a user submission").save
+    CustomWizard::Submission.new(@wizard, redirect_to: "/t/2").save
     put '/w/super-mega-fun-wizard/skip.json'
     expect(response.parsed_body['redirect_to']).to eq('/t/2')
   end