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

Added conversation timeout in ConversationHandler #895

Merged
merged 4 commits into from Mar 1, 2018

Conversation

Projects
None yet
5 participants
@evgfilim1
Copy link
Contributor

evgfilim1 commented Oct 31, 2017

Closes #648

Short usage:

updater = Updater(TOKEN)
conversation = ConversationHandler(
    [CommandHandler('start', start)], 
    {0: MessageHandler(Filters.text, continue_)},
    conversation_timeout=60
)
updater.dispatcher.add_handler(conversation)
@codecov

This comment has been minimized.

Copy link

codecov bot commented Nov 1, 2017

Codecov Report

Merging #895 into master will decrease coverage by 0.42%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #895      +/-   ##
==========================================
- Coverage   92.25%   91.82%   -0.43%     
==========================================
  Files         103      103              
  Lines        4169     4060     -109     
  Branches      669      641      -28     
==========================================
- Hits         3846     3728     -118     
- Misses        188      193       +5     
- Partials      135      139       +4
Impacted Files Coverage Δ
telegram/ext/conversationhandler.py 81.81% <100%> (+0.73%) ⬆️
telegram/files/file.py 74.07% <0%> (-14.82%) ⬇️
telegram/utils/helpers.py 82.35% <0%> (-3.7%) ⬇️
telegram/update.py 97.61% <0%> (-2.39%) ⬇️
telegram/user.py 86.36% <0%> (-1.97%) ⬇️
telegram/utils/request.py 67.85% <0%> (-1.79%) ⬇️
telegram/ext/updater.py 77.17% <0%> (-1.16%) ⬇️
telegram/ext/jobqueue.py 91.97% <0%> (-0.88%) ⬇️
telegram/files/inputmediavideo.py 95.23% <0%> (-0.77%) ⬇️
... and 34 more
@Eldinnie
Copy link
Member

Eldinnie left a comment

Looks good to me!

@Eldinnie

This comment has been minimized.

Copy link
Member

Eldinnie commented Nov 9, 2017

Looks good to me.

@Eldinnie

This comment has been minimized.

Copy link
Member

Eldinnie commented Dec 5, 2017

Anyone else wants a look on this? @tsnoam or @jh0ker ?
Or I'll add pending_merge

@Eldinnie Eldinnie requested a review from jh0ker Dec 7, 2017

@@ -294,6 +303,14 @@ def handle_update(self, update, dispatcher):
"""
new_state = self.current_handler.handle_update(update, dispatcher)

if self.timeout_job is not None:
self.timeout_job.schedule_removal()

This comment has been minimized.

@jh0ker

jh0ker Dec 7, 2017

Member

Do I understand correctly that there is only a single timeout job which gets removed whenever any conversation is triggered?

Wouldn't you want to use a different job for each conversation?

This comment has been minimized.

@evgfilim1

evgfilim1 Jan 15, 2018

Author Contributor

This timeout job is triggered in any conversation state, if you mean this. For different ConversationHandler instances the job is different too.

UPD. Yes, you're right, I have to use a different job.

@@ -124,7 +130,8 @@ def __init__(self,
timed_out_behavior=None,
per_chat=True,
per_user=True,
per_message=False):
per_message=False,
conversation_timeout=0):

This comment has been minimized.

@jh0ker

jh0ker Dec 7, 2017

Member

I prefer None as the default value for no timeout

This comment has been minimized.

@evgfilim1

evgfilim1 Jan 14, 2018

Author Contributor

I'll change it, but there is no difference, I think.

@@ -277,3 +277,27 @@ def test_channel_message_without_chat(self, bot):
message = Message(0, None, None, Chat(0, Chat.CHANNEL, 'Misses Test'), bot=bot)
update = Update(0, message=message)
assert not handler.check_update(update)

def test_conversation_timeout(self, dp, bot, user1):

This comment has been minimized.

@jh0ker

jh0ker Dec 7, 2017

Member

Related to the comment above, perhaps adding a test for 2 users would be prudent.

@evgfilim1

This comment has been minimized.

Copy link
Contributor Author

evgfilim1 commented Jan 15, 2018

@jh0ker Done

UPD. This branch is based on an old revision of master branch, so some checks can fail. I can re-open PR to follow latest revision of master, if it is necessary or important.

@Eldinnie

This comment has been minimized.

Copy link
Member

Eldinnie commented Jan 18, 2018

Hi, because you're latest commit has [ci skip] the latest version is not run on CI's and codecov. Could rebase on current master and commit so it will run a full CI?

Thanks

@evgfilim1

This comment has been minimized.

Copy link
Contributor Author

evgfilim1 commented Jan 19, 2018

@Eldinnie I know, I marked it because there are no useful changes (I removed a line with comment that is not needed at all)

@evgfilim1

This comment has been minimized.

Copy link
Contributor Author

evgfilim1 commented Jan 20, 2018

Maybe we remove "pending reply" label?

@tsnoam

This comment has been minimized.

Copy link
Member

tsnoam commented Jan 20, 2018

@evgfilim1
the last commit was marked "ci skip" and the commit before it failed unitests.
would you please be so kind to rebase and remove the "ci skip" from the commit message?

@evgfilim1

This comment has been minimized.

Copy link
Contributor Author

evgfilim1 commented Jan 21, 2018

@tsnoam done.
CI failed because of test_official[sendInvoice]

@evgfilim1

This comment has been minimized.

Copy link
Contributor Author

evgfilim1 commented Jan 29, 2018

test_official fail status should be solved when #986 is merged.

@tsnoam tsnoam merged commit 811369d into python-telegram-bot:master Mar 1, 2018

5 checks passed

codecov/patch 100% of diff hit (target 92.25%)
Details
codecov/project Absolute coverage decreased by -0.02% but relative coverage increased by +7.74% compared to b67ea7a
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
hound No violations found. Woof!
@tsnoam

This comment has been minimized.

Copy link
Member

tsnoam commented Mar 1, 2018

@evgfilim1 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.