Fix for afterLastStep function call in Dialog/proceed. #26
Conversation
…mpToIndex took into account = afterLastStep should be called if flow jumps from the last step. PassiveTestDialog updated to test this.
lptn
left a comment
There was a problem hiding this comment.
@dimaodnokoz
looks great! Thank for the fix 💪 🚀
I have some questions, can you please take a look and reply to them before the merge?
I see how this package finally becoming usable - great work!
I also contributed a bit to it and make example dialog more advanced: https://github.com/koot-labs/telegram-bot-dialogs/blob/master/src/Dialogs/HelloExampleDialog.php
I hope it will make it easy to use this package and write other Dialogs for new comers.
| } | ||
|
|
||
| /** Check if Dialog on the last step */ | ||
| final public function isLastStep(): bool |
There was a problem hiding this comment.
BTW: do you have any meaningful use-cases for this method? I usually try to minimize creation of public methods to avoid BC breaks in the future.
There was a problem hiding this comment.
I use it just for fix, because isEnd is not useful there. We may just use the check $this->next == count($this->steps) - 1; at that place instead of function, but it seems not good and clear. Probably it's better to make this function private and that's all.
There was a problem hiding this comment.
let's start from private. If someone will need it public - they can create a PR.
|
|
||
| if ($this->isLastStep()) { | ||
| $this->afterLastStep($update); | ||
| } |
|
|
||
| public function step3(Update $update): void | ||
| { | ||
| $this->jump("step2"); |
There was a problem hiding this comment.
Honestly, I'm not really happy about making this test class specific for this case. It will make this test Tet class full of magic and not really reusable for further changes. is it safe to remove this line?
There was a problem hiding this comment.
This line is for test of afterLastStep call, when dialog jumps from the last step. In this case afterLastStep should be also called, i think (BTW: I do not imagine the case, where beforeFirstStep and/or afterLastStep functions may be used, so this call is still a question). I agree with your conclusion. But, we can remove this line only with make of another test. What may it be?
- Ideally to do like in DialogTest.php, where you initiate dialog instance with steps at a place. I tried this approach, but failed. Error: can not serialize object by anonymous user (smth like that) when "activate" function called. I didn't find solution for that.
- Also it is the way to make FakeDialog file like FakeBot for similar small tests (and add cases there for different tests), but i didn't understand how to add function there.
- Making another test dialog just for this test seems bad.
BTW: I build telegram bots from 2018. It is like my main job. But i do it with some no code service (Pipe.bot - like Manychat or Chatfuel) with adding of php scripts for advanced tasks. Now i'd like to work without any no code service. So i'm here and very interested in some packages like this and sdk) I want to say, i have a lot of experience about dialog cases and tg api, but with notice, that it was partially with nocode service. As about my php knowledge. I integrated almost all Ukrainian payment systems to my chatbots, using plain php and webhooks. I'm like juniour, but with strong university dev background of 2001-2007 ))) Probably it will be more quick and comfortable to communicate in tg? It will be for me for sure. My contact: @dimaodnokoz there.
There was a problem hiding this comment.
ok, let's merge is as it, I'll reply later. Green CI is more important than clean tests
Closes #17
Fix for afterLastStep function call in Dialog/proceed. afterProceedJumpToIndex took into account = afterLastStep should be called if flow jumps from the last step. PassiveTestDialog updated to test this. isLastStep added because isEnd can not be used in this case.