From 5e5b5e67ee521ee38dd87df1b51368ad74384f00 Mon Sep 17 00:00:00 2001 From: Angus McLeod Date: Mon, 31 Jan 2022 17:18:04 +0800 Subject: [PATCH] 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 | 4 +- controllers/custom_wizard/steps.rb | 2 +- controllers/custom_wizard/wizard.rb | 12 +-- coverage/.last_run.json | 2 +- extensions/invites_controller.rb | 2 +- jobs/set_after_time_wizard.rb | 2 + lib/custom_wizard/template.rb | 47 ++++++++++-- lib/custom_wizard/validators/template.rb | 27 +++++-- lib/custom_wizard/wizard.rb | 24 ++++-- plugin.rb | 30 +++++--- .../custom_wizard/template_validator_spec.rb | 32 ++++++++ .../admin/manager_controller_spec.rb | 3 - .../application_controller_spec.rb | 73 +++++++++++++++---- .../custom_wizard/wizard_controller_spec.rb | 9 +++ 14 files changed, 212 insertions(+), 57 deletions(-) diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 7e507450..88e945f9 100644 --- a/config/locales/server.en.yml +++ b/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." diff --git a/controllers/custom_wizard/steps.rb b/controllers/custom_wizard/steps.rb index 66ec2da9..df3c2cb3 100644 --- a/controllers/custom_wizard/steps.rb +++ b/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 diff --git a/controllers/custom_wizard/wizard.rb b/controllers/custom_wizard/wizard.rb index 99add54d..854d1e39 100644 --- a/controllers/custom_wizard/wizard.rb +++ b/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? - submission = wizard.current_submission - - if submission.present? && submission.redirect_to - result.merge!(redirect_to: submission.redirect_to) + if current_user && wizard.can_access? + if redirect_to = wizard.current_submission&.redirect_to + result.merge!(redirect_to: redirect_to) end - submission.remove if submission.present? - wizard.reset + wizard.cleanup_on_skip! end render json: result diff --git a/coverage/.last_run.json b/coverage/.last_run.json index 2d4d0378..a2dfb49e 100644 --- a/coverage/.last_run.json +++ b/coverage/.last_run.json @@ -1,5 +1,5 @@ { "result": { - "line": 91.83 + "line": 92.52 } } diff --git a/extensions/invites_controller.rb b/extensions/invites_controller.rb index 5e0094da..e23cf265 100644 --- a/extensions/invites_controller.rb +++ b/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) diff --git a/jobs/set_after_time_wizard.rb b/jobs/set_after_time_wizard.rb index 7a5b86c6..a8935c8a 100644 --- a/jobs/set_after_time_wizard.rb +++ b/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 diff --git a/lib/custom_wizard/template.rb b/lib/custom_wizard/template.rb index 8e944dca..b57ebbd8 100644 --- a/lib/custom_wizard/template.rb +++ b/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) - self.class.clear_user_wizard_redirect(wizard_id) + clear_user_wizard_redirect(wizard_id, after_time: true) end end end diff --git a/lib/custom_wizard/validators/template.rb b/lib/custom_wizard/validators/template.rb index d4a8367f..5c3f5218 100644 --- a/lib/custom_wizard/validators/template.rb +++ b/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? - false - else - true - end + !errors.any? 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 diff --git a/lib/custom_wizard/wizard.rb b/lib/custom_wizard/wizard.rb index ebae615e..90177b82 100644 --- a/lib/custom_wizard/wizard.rb +++ b/lib/custom_wizard/wizard.rb @@ -288,11 +288,8 @@ class CustomWizard::Wizard end end - def final_cleanup! - if id == user.custom_fields['redirect_to_wizard'] - user.custom_fields.delete('redirect_to_wizard') - user.save_custom_fields(true) - end + def cleanup_on_complete! + remove_user_redirect 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) diff --git a/plugin.rb b/plugin.rb index 94f9059b..87050d25 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.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 not_api && !excluded_route + wizard_id = current_user.redirect_to_wizard + + 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 request.format === 'text/html' && !@excluded_routes.any? { |str| /#{str}/ =~ url } && wizard_id - if request.referer !~ /\/w\// && request.referer !~ /\/invites\// - CustomWizard::Wizard.set_wizard_redirect(current_user, wizard_id, request.referer) - end - if CustomWizard::Template.exists?(wizard_id) redirect_to "/w/#{wizard_id.dasherize}" end end diff --git a/spec/components/custom_wizard/template_validator_spec.rb b/spec/components/custom_wizard/template_validator_spec.rb index 015228a3..d8706307 100644 --- a/spec/components/custom_wizard/template_validator_spec.rb +++ b/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 diff --git a/spec/requests/custom_wizard/admin/manager_controller_spec.rb b/spec/requests/custom_wizard/admin/manager_controller_spec.rb index 87c980f2..7d087e3e 100644 --- a/spec/requests/custom_wizard/admin/manager_controller_spec.rb +++ b/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) diff --git a/spec/requests/custom_wizard/application_controller_spec.rb b/spec/requests/custom_wizard/application_controller_spec.rb index 0835f246..0b5513aa 100644 --- a/spec/requests/custom_wizard/application_controller_spec.rb +++ b/spec/requests/custom_wizard/application_controller_spec.rb @@ -31,24 +31,67 @@ describe ApplicationController do user.save_custom_fields(true) 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 "saves original destination of user" do - get '/', headers: { 'REFERER' => "/t/2" } - expect( - CustomWizard::Wizard.create(@template['id'], user).submissions - .first.redirect_to - ).to eq("/t/2") - end - - it "does not redirect if wizard does not exist" do - CustomWizard::Template.remove('super_mega_fun_wizard') + 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( + CustomWizard::Wizard.create(@template['id'], user).submissions + .first.redirect_to + ).to eq("/t/2") + end + end + + context "after time enabled" do + 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 diff --git a/spec/requests/custom_wizard/wizard_controller_spec.rb b/spec/requests/custom_wizard/wizard_controller_spec.rb index 87ff7efe..f5bcd5ac 100644 --- a/spec/requests/custom_wizard/wizard_controller_spec.rb +++ b/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