From 6dd705fe89d1daef0f54558caf3c607f15cbf82c Mon Sep 17 00:00:00 2001 From: Max Melentiev Date: Mon, 28 May 2018 18:41:40 +0300 Subject: [PATCH] Drop `.context_handler`, `.context_to_action!` methods --- CHANGELOG.md | 3 + README.md | 11 +- lib/telegram/bot/updates_controller.rb | 8 +- .../bot/updates_controller/message_context.rb | 100 +++++++----------- .../message_context_spec.rb | 87 +++++---------- 5 files changed, 77 insertions(+), 132 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e7ca600..8f250dc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,9 @@ - `:telegram_bot` rspec tag is replaced with `telegram_bot: :rails`. - __Breaking change__. Use bang-methods as actions for commands. This prevents calling context contextual actions and payload specific actions with commands. +- __Breaking change__. Drop `.context_handler`, `.context_to_action!` methods. + Use pass action name directly to `#save_context`. + It's the same as `.context_to_action!` is enabled by default. # 0.13.1 diff --git a/README.md b/README.md index d319ed9..3483295 100644 --- a/README.md +++ b/README.md @@ -288,12 +288,12 @@ class Telegram::WebhookController < Telegram::Bot::UpdatesController end # register context handlers to handle this context - context_handler :rename do |*words| + def rename(*words) update_name words[0] respond_with :message, text: 'Renamed!' end - # You can do it in other way: + # You can use same action name as context name: def rename!(name = nil, *) if name update_name name @@ -303,13 +303,6 @@ class Telegram::WebhookController < Telegram::Bot::UpdatesController respond_with :message, text: 'What name do you like?' end end - - # This will call #rename! like if it is called with message '/rename %text%' - context_handler :rename! - - # If you have a lot of such methods you can call this method - # to use context value as action name for all contexts which miss handlers: - context_to_action! end ``` diff --git a/lib/telegram/bot/updates_controller.rb b/lib/telegram/bot/updates_controller.rb index 0aa8c3c..742378f 100644 --- a/lib/telegram/bot/updates_controller.rb +++ b/lib/telegram/bot/updates_controller.rb @@ -206,9 +206,11 @@ module Telegram [payload_type, [payload['data']]] end - # Silently ignore unsupported messages. - # Params are `action, *args`. - def action_missing(*) + # Silently ignore unsupported messages to not fail when user crafts + # an update with usupported command, callback query context, etc. + def action_missing(action, *_args) + logger.debug { "The action '#{action}' is not defined in #{self.class.name}" } if logger + nil end PAYLOAD_TYPES.each do |type| diff --git a/lib/telegram/bot/updates_controller/message_context.rb b/lib/telegram/bot/updates_controller/message_context.rb index 01146ed..b1b3496 100644 --- a/lib/telegram/bot/updates_controller/message_context.rb +++ b/lib/telegram/bot/updates_controller/message_context.rb @@ -2,51 +2,35 @@ module Telegram module Bot class UpdatesController # Allows to store context in session and treat next message according to this context. + # + # It provides `save_context` method to store method name + # to be used as action for next update: + # + # def set_location!(*) + # save_context(:set_location_from_message) + # respond_with :message, text: 'Where are you?' + # end + # + # def set_location_from_messge(city = nil, *) + # # update + # end + # + # # OR + # # This will support both `/set_location city_name`, and `/set_location` + # # with subsequent refinement. + # def set_location!(city = nil, *) + # if city + # # update + # else + # save_context(:set_location!) + # respond_with :message, text: 'Where are you?' + # end + # end module MessageContext extend ActiveSupport::Concern include Session - module ClassMethods - def context_handlers - @_context_handlers ||= {} - end - - # Registers handler for context. - # - # context_handler :rename do |*| - # resource.update!(name: payload['text']) - # end - # - # # To run other action with all the callbacks: - # context_handler :rename do |*words| - # process(:rename, *words) - # end - # - # # Or just - # context_handler :rename, :your_action_to_call - # context_handler :rename # to call :rename - # - def context_handler(context = nil, action = nil, &block) - context &&= context.to_sym - if block - action = "_context_handler_#{context}" - define_method(action, &block) - end - context_handlers[context] = action || context - end - - attr_reader :context_to_action - - # Use it to use context value as action name for all contexts - # which miss handlers. - # For security reasons it supports only action methods and will - # raise AbstractController::ActionNotFound if context is invalid. - def context_to_action! - @context_to_action = true - end - end - # Action to clear context. def cancel! # Context is already cleared in action_for_message @@ -54,10 +38,6 @@ module Telegram private - # Context is read from the session to treat messages - # according to previous request. - attr_reader :context - # Controller may have multiple sessions, let it be possible # to select session for message context. def message_context_session @@ -68,13 +48,11 @@ module Telegram # it has higher priority than contextual action. def action_for_message val = message_context_session.delete(:context) - @context = val && val.to_sym + context = val && val.to_s super || context && begin - handler = handler_for_context - if handler - action_options = {type: :message_context, context: context} - [[handler, action_options], payload['text'].try!(:split) || []] - end + args = payload['text'].try!(:split) || [] + action = action_for_message_context(context) + [[action, type: :message_context, context: context], args] end end @@ -83,18 +61,16 @@ module Telegram message_context_session[:context] = context end - def handler_for_context - self.class.context_handlers[context] || self.class.context_to_action && begin - action_name = context.to_s - unless action_method?(action_name) - raise AbstractController::ActionNotFound, - "The action '#{action_name}' could not be set from context " \ - "for #{self.class.name}. " \ - 'context_to_action! supports only action methods for security reasons. ' \ - 'If you need to call this action use context_handler for it.' - end - action_name - end + # Returns action name for message context. By default it's the same as context name. + # Raises AbstractController::ActionNotFound if action is not available. + # This differs from other cases where invalid actions are silently ignored, + # because message context is controlled by developer, and users are not able + # to construct update to run any specific context. + def action_for_message_context(context) + action = context.to_s + return action if action_method?(action) + raise AbstractController::ActionNotFound, + "The context action '#{action}' is not found in #{self.class.name}" end end end diff --git a/spec/telegram/bot/updates_controller/message_context_spec.rb b/spec/telegram/bot/updates_controller/message_context_spec.rb index 0aaef81..9ab4952 100644 --- a/spec/telegram/bot/updates_controller/message_context_spec.rb +++ b/spec/telegram/bot/updates_controller/message_context_spec.rb @@ -6,7 +6,7 @@ RSpec.describe Telegram::Bot::UpdatesController::MessageContext do include described_class attr_accessor :filter_done - before_action only: :redirect do + before_action only: :context_with_filter do self.filter_done = true end @@ -17,19 +17,16 @@ RSpec.describe Telegram::Bot::UpdatesController::MessageContext do [:no_context, *args] end - context_handler :block do |*args| - [:block_result, *args] + def handler_method(*args) + [:method_result_1, *args] end - context_handler :redirect - context_handler :other_redirect, :redirect - - def redirect(*args) - [:method_result, *args] + def context_with_filter(*args) + [:method_result_2, *args] end def action!(*args) - [:action_result, *args] + [:command_result, *args] end private @@ -52,85 +49,59 @@ RSpec.describe Telegram::Bot::UpdatesController::MessageContext do it { should_not change { session[:context] } } end - context 'when context is handled by block' do - before { session[:context] = :block } - its(:call) { should eq [:block_result, *text.split] } + context 'when context is handled by handler_method' do + before { session[:context] = :handler_method } + its(:call) { should eq [:method_result_1, *text.split] } it { should_not change(controller, :filter_done) } it { should change { session[:context] }.to nil } context 'when message has no text' do let(:payload) { {'audio' => {'file_id' => 123}} } - its(:call) { should eq [:block_result] } + its(:call) { should eq [:method_result_1] } end context 'when message has new command' do let(:text) { '/action a s d' } - its(:call) { should eq [:action_result, 'a', 's', 'd'] } + its(:call) { should eq [:command_result, 'a', 's', 'd'] } it { should change { session[:context] }.to nil } end end - context 'when context is handled by short redirect' do - before { session[:context] = :redirect } - its(:call) { should eq [:method_result, *text.split] } + context 'when context is handled by short context_with_filter' do + before { session[:context] = :context_with_filter } + its(:call) { should eq [:method_result_2, *text.split] } it { should change(controller, :filter_done).to true } it { should change { session[:context] }.to nil } it { should change(controller, :callbacks_runs).to 1 } context 'when message has no text' do let(:payload) { {'audio' => {'file_id' => 123}} } - its(:call) { should eq [:method_result] } + its(:call) { should eq [:method_result_2] } it { should change(controller, :filter_done).to true } it { should change { session[:context] }.to nil } end end - context 'when context is handled by custom redirect' do - before { session[:context] = :other_redirect } - its(:call) { should eq [:method_result, *text.split] } - it { should change(controller, :filter_done).to true } - it { should change { session[:context] }.to nil } - end - - context 'when context is action`s name but not mapped' do - before { session[:context] = :action } - its(:call) { should eq [:no_context, payload] } + context 'when context is command-action`s name' do + before { session[:context] = :action! } + its(:call) { should eq [:command_result, *text.split] } it { should_not change(controller, :filter_done) } it { should change { session[:context] }.to nil } end - context 'when context_to_action is true' do - before { controller_class.context_to_action! } - - context 'when context is not set' do - its(:call) { should eq [:no_context, payload] } - it { should_not change(controller, :filter_done) } - it { should_not change { session[:context] } } + context 'when context is not an action`s name' do + before { session[:context] = :not_action } + it do + should raise_error(AbstractController::ActionNotFound). + and change { session[:context] }.to nil end + end - context 'when context is action`s name but not mapped' do - before { session[:context] = :action! } - its(:call) { should eq [:action_result, *text.split] } - it { should_not change(controller, :filter_done) } - it { should change { session[:context] }.to nil } - end - - context 'when context is invalid' do - before { session[:context] = :invalid } - it 'raises error and clears context' do - expect do - should raise_error AbstractController::ActionNotFound - end.to change { session[:context] }.to nil - end - end - - context 'when context is private method`s name' do - before { session[:context] = :not_action } - it 'raises error and clears context' do - expect do - should raise_error AbstractController::ActionNotFound - end.to change { session[:context] }.to nil - end + context 'when context is invalid name' do + before { session[:context] = :invalid } + it do + should raise_error(AbstractController::ActionNotFound). + and change { session[:context] }.to nil end end end