Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Check for update.effective_chat explicitly in ConversationHandler.check_update #959

Merged
merged 2 commits into from Feb 12, 2018

Conversation

Projects
None yet
2 participants
@nmlorg
Copy link
Contributor

nmlorg commented Jan 1, 2018

Fixes #927

nmlorg added some commits Jan 1, 2018

I think this part of check_update was just trying to exclude updates …
…without an effective_chat, so just go ahead and check for that explicitly. This should be pretty future proof.

(I'm not sure what `update.callback_query and self.per_chat and not update.callback_query.message` is doing, though. _get_key only directly touches update.callback_query if self.per_message (not self.per_chat) is set, and only requires update.callback_query.message if not update.callback_query.inline_message_id. I'm not sure what the use case is, so just leaving it alone for now.)

Add a new test_all_update_types which just runs through a quick list of update types. This is not future proof. Maybe add a list of all possible update types as Update.TYPES in a followup?
@tsnoam
Copy link
Member

tsnoam left a comment

Hi @nmlorg
Thanks for your contribution.
Would you be so kind and look at the questions I left on the code itself and make the appropriate changes?

update.channel_post or
self.per_chat and not update.effective_chat or
self.per_message and not update.callback_query or
update.callback_query and self.per_chat and not update.callback_query.message):

This comment has been minimized.

@tsnoam

tsnoam Jan 9, 2018

Member

Honestly I'm confused form the logic here (it doesn't necessarily make it wrong though) so I have some questions:

  1. Is anything "buggy" with the current logic? If yes, please explain what.
  2. What is the new logic you're trying to introduce?
  3. Was and (update.inline_query or update.chosen_inline_result) removed intentionally or is it by mistake? If intentionally please explain why.
  4. When mixing and, and not & or conditionals it makes it hard to read (even if its correct). Can you be so kind and add parentheses to make the logic easier to read?

This comment has been minimized.

@tsnoam

tsnoam Jan 9, 2018

Member

btw, this piece of code was barely readable before (at least to me) so this change made it much more difficult to undestand...

This comment has been minimized.

@nmlorg

nmlorg Jan 9, 2018

Author Contributor

The original code was:

if (not isinstance(update, Update) or update.channel_post or self.per_chat
        and (update.inline_query or update.chosen_inline_result) or self.per_message
        and not update.callback_query or update.callback_query and self.per_chat
        and not update.callback_query.message):

and I first changed it to group related conditions by line:

if (not isinstance(update, Update) or
        update.channel_post or
        self.per_chat and (update.inline_query or update.chosen_inline_result) or
        self.per_message and not update.callback_query or
        update.callback_query and self.per_chat and not update.callback_query.message):

i.e. each line contains conditions that are joined by and, and the lines are separated by or. Due to order of operations rules, this means each line is now the same as if it had been enclosed in parentheses:

if ((not isinstance(update, Update)) or
        (update.channel_post) or
        (self.per_chat and (update.inline_query or update.chosen_inline_result)) or
        (self.per_message and (not update.callback_query)) or
        (update.callback_query and self.per_chat and (not update.callback_query.message))):

The actual logic change (the second commit) was to remove the (update.inline_query or update.chosen_inline_result) check and replace it with a check for not update.effective_chat. It seemed pretty clear the intent was to exclude updates that don't have a chat assigned, which includes inline_query and chosen_inline_result (as explicitly tested in the original logic) as well as callback_query from inline messages, shipping_query, and pre_checkout_query (as documented in Update.effective_chat). So I could have changed the logic to exclude those additional types explicitly, but ultimately the update gets passed into _get_key which blindly dereferences update.effective_chat, so either that would need to be modified to be if self.per_chat and chat is not None: (like the per_user check) or go ahead and just check for effective_chat before calling _get_key, which is what I did.

The one remaining issue is that last line, which excludes callback_query updates that don't have a message attached when per_chat is set, which doesn't seem to make sense to me. The only time update.callback_query.message is dereferenced is again in _get_key when per_message is set and update.callback_query.inline_message_id is not set, neither of which seem related to what's being tested. It's possible the original logic's author meant to include that logic tacked onto the previous per_message test:

if (not isinstance(update, Update) or
        update.channel_post or
        self.per_chat and not update.effective_chat or
        self.per_message and (not update.callback_query or
                              update.callback_query and self.per_chat and not update.callback_query.message)):

i.e. if per_message and either it's not a callback_query or it is a callback_query but callback_query.message is not set (though that's not strictly necessary, and still not sure what the per_chat check in the middle is for).

(Personally, if I was confident that _get_key's logic was perfect, I would just get rid of that whole block and have _get_key return None or something itself if the update isn't relevant, and do:

if not isinstance(update, Update):
    return False
key = self._get_key(update)
if not key:
    return False

but the smallest necessary change is to explicitly check for update.effective_chat before dereferencing it, and either exclude all updates without one (like I did) or include them even if per_chat is set ( if self.per_chat and chat is not None:).)

(Do you still want me to add the extra explicit parens?)

@tsnoam tsnoam merged commit 063704c into python-telegram-bot:master Feb 12, 2018

1 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
codeclimate 1 issue to fix
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@tsnoam

This comment has been minimized.

Copy link
Member

tsnoam commented Feb 12, 2018

@nmlorg
Thank you for your contribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.