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

Trigger :message_dispatched when confirmed command is dispatched #9

Closed

Conversation

t33chong
Copy link

I'm using lita-confirmation in conjunction with lita-metrics, and I
discovered that commands requiring confirmation were not being logged
properly as valid commands. With this change, the :message_dispatched
event is triggered explicitly so that both the command requiring
confirmation and the confirmation itself are logged.

I'm using lita-confirmation in conjunction with lita-metrics, and I
discovered that commands requiring confirmation were not being logged
properly as valid commands. With this change, the :message_dispatched
event is triggered explicitly so that both the command requiring
confirmation and the confirmation itself are logged.
@coveralls
Copy link

coveralls commented May 16, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling 3c0b5ef on tristaneuan:trigger-message-dispatched into 0c1f991 on jimmycuadra:master.

@jimmycuadra
Copy link
Owner

I think the right way to fix this might be to change where that event is triggered in Lita. Instead of triggering it in Lita::Handler::ChatRouter#dispatch, it should probably be triggered in Lita::Handler::ChatRouter#dispatch_to_route, since that is called by #dispatch anyway. Can you think of any problems with that? I can't remember if there was a reason I didn't do it that way in the first place.

@t33chong
Copy link
Author

Haha, probably because I was the one who introduced the trigger in the first place 😆 : litaio/lita#134

I've opened a new PR that moves the :message_dispatched trigger from Lita::Handler::ChatRouter#dispatch to Lita::Handler::ChatRouter#dispatch_to_route like you suggested:
litaio/lita#186

@t33chong t33chong closed this May 23, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants