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 hasMatchingEvent implementation #31

Merged
merged 5 commits into from
Aug 8, 2018
Merged

Conversation

sielver
Copy link
Contributor

@sielver sielver commented Feb 18, 2018

See issue #30.

Hope I didn't mess up anything--seems to do the job in my case at least.

@codecov
Copy link

codecov bot commented Feb 18, 2018

Codecov Report

Merging #31 into master will decrease coverage by 0.88%.
The diff coverage is 0%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master      #31      +/-   ##
============================================
- Coverage     48.22%   47.33%   -0.89%     
- Complexity      154      158       +4     
============================================
  Files             8        8              
  Lines           479      488       +9     
============================================
  Hits            231      231              
- Misses          248      257       +9
Impacted Files Coverage Δ Complexity Δ
src/SlackDriver.php 74.73% <0%> (-3.72%) 68 <4> (+4)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6f4abc5...f9e3f78. Read the comment docs.

@mpociot
Copy link
Member

mpociot commented Feb 21, 2018

Thank you! Can you add a test for this too?

@sielver
Copy link
Contributor Author

sielver commented Feb 21, 2018

Sure, I'll get to it soon.

@mpociot mpociot merged commit a832671 into botman:master Aug 8, 2018
@lukewaite
Copy link

@mpociot This was merged but never tagged. Any chance of getting a tagged release?

Thank you

@RobinHoutevelts
Copy link

Not sure if this is the place to discuss this. I've just started working on a very crude slackbot that we could use to do some basic actions. I'm mostly using slash commands.

Since this merge I've noticed that my Questions aren't working anymore.

I think it has to do with this:

         // If the event type isn't 'message' (which should go through BotMan::hears),
         // build a GenericEvent and return it

And in Botman we have this listen method:

    /**
     * Try to match messages with the ones we should
     * listen to.
     */
    public function listen()
    {
        try {
            $isVerificationRequest = $this->verifyServices();

            if (! $isVerificationRequest) {
                $this->fireDriverEvents();

                if ($this->firedDriverEvents === false) {
                    $this->loadActiveConversation();

                    if ($this->loadedConversation === false) {
                        $this->callMatchingMessages();
                    }

                    /*
                     * If the driver has a  "messagesHandled" method, call it.
                     * This method can be used to trigger driver methods
                     * once the messages are handles.
                     */
                    if (method_exists($this->getDriver(), 'messagesHandled')) {
                        $this->getDriver()->messagesHandled();
                    }
                }

                $this->firedDriverEvents = false;
                $this->message = new IncomingMessage('', '', '');
            }
        } catch (\Throwable $e) {
            $this->exceptionHandler->handleException($e, $this);
        }
    }

So first it calls $this->fireDriverEvents(); and if that fired any event. It'll ignore the rest.

I have an action and that returns with type dialog_submission so I get a GenericEvent fired and that's it.

https://api.slack.com/dialogs#response


This might be because I don't actually use $botman->hears() but $botman->fallback() where I do my own routing. So just letting you know this could break things.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants