FIX: Cache valid directs and only allow one type in a template (#176)

* Cache valid directs and only allow one type in a template

* Add spec

* Bump version

* Bump version

* Exclude current wizard from other_after_signup

config/locales/server.en.yml

@@ -48,7 +48,9 @@ en:
     validation:
       required: "%{property} is required"
       conflict: "Wizard with id '%{wizard_id}' already exists"
-      after_time: "After time setting is invalid"
+      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."
 
   site_settings:
     custom_wizard_enabled: "Enable custom wizards."

controllers/custom_wizard/steps.rb

@@ -54,7 +54,7 @@ class CustomWizard::StepsController < ::ApplicationController
           updater.result[:redirect_on_complete] = redirect
         end
 
-        @wizard.final_cleanup!
+        @wizard.cleanup_on_complete!
 
         result[:final] = true
       else

controllers/custom_wizard/wizard.rb

@@ -60,17 +60,13 @@ class CustomWizard::WizardController < ::ApplicationController
     end
 
     result = success_json
-    user = current_user
 
-    if user && wizard.can_access?
+    if current_user && wizard.can_access?
-      submission = wizard.current_submission
+      if redirect_to = wizard.current_submission&.redirect_to
-
+        result.merge!(redirect_to: redirect_to)
-      if submission.present? && submission.redirect_to
-        result.merge!(redirect_to: submission.redirect_to)
       end
 
-      submission.remove if submission.present?
+      wizard.cleanup_on_skip!
-      wizard.reset
     end
 
     render json: result

coverage/.last_run.json

@@ -1,5 +1,5 @@
 {
   "result": {
-    "line": 91.83
+    "line": 92.52
   }
 }

extensions/invites_controller.rb

@@ -2,7 +2,7 @@
 module InvitesControllerCustomWizard
   def path(url)
     if ::Wizard.user_requires_completion?(@user)
-      wizard_id = @user.custom_fields['redirect_to_wizard']
+      wizard_id = @user.redirect_to_wizard
 
       if wizard_id && url != '/'
         CustomWizard::Wizard.set_wizard_redirect(@user, wizard_id, url)

jobs/set_after_time_wizard.rb

@@ -14,6 +14,8 @@ module Jobs
             end
           end
 
+          CustomWizard::Template.clear_cache_keys
+
           MessageBus.publish "/redirect_to_wizard", wizard.id, user_ids: user_ids
         end
       end

lib/custom_wizard/template.rb

@@ -3,6 +3,9 @@
 class CustomWizard::Template
   include HasErrors
 
+  AFTER_SIGNUP_CACHE_KEY ||= "after_signup_wizard_ids"
+  AFTER_TIME_CACHE_KEY ||= "after_time_wizard_ids"
+
   attr_reader :data,
               :opts,
               :steps,
@@ -28,6 +31,8 @@ class CustomWizard::Template
       PluginStore.set(CustomWizard::PLUGIN_NAME, @data[:id], @data)
     end
 
+    self.class.clear_cache_keys
+
     @data[:id]
   end
 
@@ -53,10 +58,10 @@ class CustomWizard::Template
 
     ActiveRecord::Base.transaction do
       PluginStore.remove(CustomWizard::PLUGIN_NAME, wizard.id)
-      clear_user_wizard_redirect(wizard_id)
+      clear_user_wizard_redirect(wizard_id, after_time: !!wizard.after_time)
     end
 
-    Jobs.cancel_scheduled_job(:set_after_time_wizard) if wizard.after_time
+    clear_cache_keys
 
     true
   end
@@ -65,9 +70,10 @@ class CustomWizard::Template
     PluginStoreRow.exists?(plugin_name: 'custom_wizard', key: wizard_id)
   end
 
-  def self.list(setting: nil, order: :id)
+  def self.list(setting: nil, query_str: nil, order: :id)
     query = "plugin_name = 'custom_wizard'"
-    query += "AND (value::json ->> '#{setting}')::boolean IS TRUE" if setting
+    query += " AND (value::json ->> '#{setting}')::boolean IS TRUE" if setting
+    query += " #{query_str}" if query_str
 
     PluginStoreRow.where(query).order(order)
       .reduce([]) do |result, record|
@@ -85,8 +91,36 @@ class CustomWizard::Template
       end
   end
 
-  def self.clear_user_wizard_redirect(wizard_id)
+  def self.clear_user_wizard_redirect(wizard_id, after_time: false)
     UserCustomField.where(name: 'redirect_to_wizard', value: wizard_id).destroy_all
+
+    if after_time
+      Jobs.cancel_scheduled_job(:set_after_time_wizard, wizard_id: wizard_id)
+    end
+  end
+
+  def self.after_signup_ids
+    ::CustomWizard::Cache.wrap(AFTER_SIGNUP_CACHE_KEY) do
+      list(setting: 'after_signup').map { |t| t['id'] }
+    end
+  end
+
+  def self.after_time_ids
+    ::CustomWizard::Cache.wrap(AFTER_TIME_CACHE_KEY) do
+      list(
+        setting: 'after_time',
+        query_str: "AND (value::json ->> 'after_time_scheduled')::timestamp < CURRENT_TIMESTAMP"
+      ).map { |t| t['id'] }
+    end
+  end
+
+  def self.can_redirect_users?(wizard_id)
+    after_signup_ids.include?(wizard_id) || after_time_ids.include?(wizard_id)
+  end
+
+  def self.clear_cache_keys
+    CustomWizard::Cache.new(AFTER_SIGNUP_CACHE_KEY).delete
+    CustomWizard::Cache.new(AFTER_TIME_CACHE_KEY).delete
   end
 
   private
@@ -132,8 +166,7 @@ class CustomWizard::Template
         Jobs.cancel_scheduled_job(:set_after_time_wizard, wizard_id: wizard_id)
         Jobs.enqueue_at(enqueue_wizard_at, :set_after_time_wizard, wizard_id: wizard_id)
       elsif old_data && old_data[:after_time]
-        Jobs.cancel_scheduled_job(:set_after_time_wizard, wizard_id: wizard_id)
+        clear_user_wizard_redirect(wizard_id, after_time: true)
-        self.class.clear_user_wizard_redirect(wizard_id)
       end
     end
   end

lib/custom_wizard/validators/template.rb

@@ -13,8 +13,11 @@ class CustomWizard::TemplateValidator
 
     check_id(data, :wizard)
     check_required(data, :wizard)
+    validate_after_signup
     validate_after_time
 
+    return false if errors.any?
+
     data[:steps].each do |step|
       check_required(step, :step)
 
@@ -31,11 +34,7 @@ class CustomWizard::TemplateValidator
       end
     end
 
-    if errors.any?
+    !errors.any?
-      false
-    else
-      true
-    end
   end
 
   def self.required
@@ -63,8 +62,24 @@ class CustomWizard::TemplateValidator
     end
   end
 
+  def validate_after_signup
+    return unless ActiveRecord::Type::Boolean.new.cast(@data[:after_signup])
+
+    other_after_signup = CustomWizard::Template.list(setting: 'after_signup')
+      .select { |template| template['id'] != @data[:id] }
+
+    if other_after_signup.any?
+      errors.add :base, I18n.t("wizard.validation.after_signup", wizard_id: other_after_signup.first['id'])
+    end
+  end
+
   def validate_after_time
-    return unless @data[:after_time]
+    return unless ActiveRecord::Type::Boolean.new.cast(@data[:after_time])
+
+    if ActiveRecord::Type::Boolean.new.cast(@data[:after_signup])
+      errors.add :base, I18n.t("wizard.validation.after_signup_after_time")
+      return
+    end
 
     wizard = CustomWizard::Wizard.create(@data[:id]) if !@opts[:create]
     current_time = wizard.present? ? wizard.after_time_scheduled : nil

lib/custom_wizard/wizard.rb

@@ -288,11 +288,8 @@ class CustomWizard::Wizard
     end
   end
 
-  def final_cleanup!
+  def cleanup_on_complete!
-    if id == user.custom_fields['redirect_to_wizard']
+    remove_user_redirect
-      user.custom_fields.delete('redirect_to_wizard')
-      user.save_custom_fields(true)
-    end
 
     if current_submission.present?
       current_submission.submitted_at = Time.now.iso8601
@@ -302,6 +299,23 @@ class CustomWizard::Wizard
     update!
   end
 
+  def cleanup_on_skip!
+    remove_user_redirect
+
+    if current_submission.present?
+      current_submission.remove
+    end
+
+    reset
+  end
+
+  def remove_user_redirect
+    if id == user.redirect_to_wizard
+      user.custom_fields.delete('redirect_to_wizard')
+      user.save_custom_fields(true)
+    end
+  end
+
   def self.create(wizard_id, user = nil)
     if template = CustomWizard::Template.find(wizard_id)
       new(template.to_h, user)

plugin.rb

@@ -1,7 +1,7 @@
 # frozen_string_literal: true
 # name: discourse-custom-wizard
 # about: Create custom wizards
-# version: 1.16.4
+# version: 1.16.5
 # authors: Angus McLeod
 # url: https://github.com/paviliondev/discourse-custom-wizard
 # contact emails: angus@thepavilion.io
@@ -149,8 +149,16 @@ after_initialize do
     !!custom_redirect
   end
 
+  add_to_class(:user, :redirect_to_wizard) do
+    if custom_fields['redirect_to_wizard'].present?
+      custom_fields['redirect_to_wizard']
+    else
+      nil
+    end
+  end
+
   add_to_class(:users_controller, :wizard_path) do
-    if custom_wizard_redirect = current_user.custom_fields['redirect_to_wizard']
+    if custom_wizard_redirect = current_user.redirect_to_wizard
       "#{Discourse.base_url}/w/#{custom_wizard_redirect.dasherize}"
     else
       "#{Discourse.base_url}/wizard"
@@ -158,7 +166,7 @@ after_initialize do
   end
 
   add_to_serializer(:current_user, :redirect_to_wizard) do
-    object.custom_fields['redirect_to_wizard']
+    object.redirect_to_wizard
   end
 
   on(:user_approved) do |user|
@@ -168,15 +176,19 @@ after_initialize do
   end
 
   add_to_class(:application_controller, :redirect_to_wizard_if_required) do
-    wizard_id = current_user.custom_fields['redirect_to_wizard']
     @excluded_routes ||= SiteSetting.wizard_redirect_exclude_paths.split('|') + ['/w/']
     url = request.referer || request.original_url
+    excluded_route = @excluded_routes.any? { |str| /#{str}/ =~ url }
+    not_api = request.format === 'text/html'
 
-    if request.format === 'text/html' && !@excluded_routes.any? { |str| /#{str}/ =~ url } && wizard_id
+    if not_api && !excluded_route
-      if request.referer !~ /\/w\// && request.referer !~ /\/invites\//
+      wizard_id = current_user.redirect_to_wizard
-        CustomWizard::Wizard.set_wizard_redirect(current_user, wizard_id, request.referer)
+
+      if CustomWizard::Template.can_redirect_users?(wizard_id)
+        if url !~ /\/w\// && url !~ /\/invites\//
+          CustomWizard::Wizard.set_wizard_redirect(current_user, wizard_id, url)
         end
-      if CustomWizard::Template.exists?(wizard_id)
+
         redirect_to "/w/#{wizard_id.dasherize}"
       end
     end

spec/components/custom_wizard/template_validator_spec.rb

@@ -30,6 +30,38 @@ describe CustomWizard::TemplateValidator do
     ).to eq(false)
   end
 
+  it "only allows one after signup wizard at a time" do
+    wizard_id = template[:id]
+    template[:after_signup] = true
+    CustomWizard::Template.save(template)
+
+    template[:id] = "wizard_2"
+    template[:after_signup] = true
+
+    validator = CustomWizard::TemplateValidator.new(template)
+    expect(validator.perform).to eq(false)
+    expect(validator.errors.first.type).to eq(
+      I18n.t("wizard.validation.after_signup", wizard_id: wizard_id)
+    )
+  end
+
+  it "only allows a wizard with after signup to be validated twice" do
+    template[:after_signup] = true
+    CustomWizard::Template.save(template)
+    expect(CustomWizard::TemplateValidator.new(template).perform).to eq(true)
+  end
+
+  it "only allows one after _ setting per wizard" do
+    template[:after_signup] = true
+    template[:after_time] = true
+
+    validator = CustomWizard::TemplateValidator.new(template)
+    expect(validator.perform).to eq(false)
+    expect(validator.errors.first.type).to eq(
+      I18n.t("wizard.validation.after_signup_after_time")
+    )
+  end
+
   it "validates after time settings" do
     template[:after_time] = true
     template[:after_time_scheduled] = (Time.now + 3.hours).iso8601

spec/requests/custom_wizard/admin/manager_controller_spec.rb

@@ -15,11 +15,8 @@ describe CustomWizard::AdminManagerController do
 
     template_2 = template.dup
     template_2["id"] = 'super_mega_fun_wizard_2'
-
     template_3 = template.dup
     template_3["id"] = 'super_mega_fun_wizard_3'
-    template_3["after_signup"] = true
-
     @template_array = [template, template_2, template_3]
 
     FileUtils.mkdir_p(file_from_fixtures_tmp_folder) unless Dir.exists?(file_from_fixtures_tmp_folder)

spec/requests/custom_wizard/application_controller_spec.rb

@@ -31,11 +31,39 @@ describe ApplicationController do
         user.save_custom_fields(true)
       end
 
+      it "does not redirect if wizard if no after setting is enabled" do
+        get "/"
+        expect(response.status).to eq(200)
+      end
+
+      context "after signup enabled" do
+        before do
+          @template["after_signup"] = true
+          CustomWizard::Template.save(@template)
+        end
+
+        it "does not redirect if wizard does not exist" do
+          CustomWizard::Template.remove(@template[:id])
+          get "/"
+          expect(response.status).to eq(200)
+        end
+
         it "redirects if user is required to complete a wizard" do
           get "/"
           expect(response).to redirect_to("/w/super-mega-fun-wizard")
         end
 
+        it "does not redirect if wizard is subsequently disabled" do
+          get "/"
+          expect(response).to redirect_to("/w/super-mega-fun-wizard")
+
+          @template["after_signup"] = false
+          CustomWizard::Template.save(@template)
+
+          get "/"
+          expect(response.status).to eq(200)
+        end
+
         it "saves original destination of user" do
           get '/', headers: { 'REFERER' => "/t/2" }
           expect(
@@ -43,12 +71,27 @@ describe ApplicationController do
               .first.redirect_to
           ).to eq("/t/2")
         end
+      end
 
-      it "does not redirect if wizard does not exist" do
+      context "after time enabled" do
-        CustomWizard::Template.remove('super_mega_fun_wizard')
+        before do
+          @template["after_time"] = true
+          @template["after_time_scheduled"] = (Time.now + 3.hours).iso8601
+          CustomWizard::Template.save(@template)
+        end
+
+        it "does not redirect if time hasn't passed" do
           get "/"
           expect(response.status).to eq(200)
         end
+
+        it "redirects if time has passed" do
+          @template["after_time_scheduled"] = (Time.now - 1.hours).iso8601
+          CustomWizard::Template.save(@template)
+          get "/"
+          expect(response.status).to eq(200)
+        end
+      end
     end
 
     context "who is not required to complete wizard" do

spec/requests/custom_wizard/wizard_controller_spec.rb

@@ -79,6 +79,15 @@ describe CustomWizard::WizardController do
       expect(response.parsed_body['redirect_to']).to eq('/t/2')
     end
 
+    it 'deletes the users redirect_to_wizard if present' do
+      user.custom_fields['redirect_to_wizard'] = @template["id"]
+      user.save_custom_fields(true)
+      @wizard = CustomWizard::Wizard.create(@template["id"], user)
+      put '/w/super-mega-fun-wizard/skip.json'
+      expect(response.status).to eq(200)
+      expect(user.reload.redirect_to_wizard).to eq(nil)
+    end
+
     it "deletes the submission if user has filled up some data" do
       @wizard = CustomWizard::Wizard.create(@template["id"], user)
       CustomWizard::Submission.new(@wizard, step_1_field_1: "Hello World").save