Page MenuHomePhabricator

PhabricatorBotFeedNotificationHandler is completely broken and unusable
Closed, WontfixPublic

Description

I can't seem to figure out how to specify the types that should be posted to our IRC channels. Right now it's not posting anything. Here is my config:

"notification.channels" : ["#dev-activity"],
"notification.types":     ["DREV", "TASK"],
"notification.verbosity": 2

Not sure if I'm just not specifying the types correctly or this is an actual bug. This definitely should be documented in the diviner docs as well. I also realize that notification.channels does not work according to T6911 (which is apparent in the source code as well).

Event Timeline

gerfuls updated the task description. (Show Details)
gerfuls raised the priority of this task from to Needs Triage.
gerfuls added a project: Phabot.
gerfuls added a subscriber: gerfuls.
epriestley moved this task from Backlog to Future Bot/API Update on the Conpherence board.
epriestley renamed this task from Cannot get PhabricatorBotFeedNotificationHandler configured to work correctly to PhabricatorBotFeedNotificationHandler is completely broken and unusable.
epriestley triaged this task as Wishlist priority.
epriestley added subscribers: eadler, matelich.
epriestley added a subscriber: rmmh.

Here's the general outline of how this should move forward:

  • We should move to giving the bot a list of channels to join, and a list of handlers to run in each channel -- not a list of handlers to run at top level, and then other top-level config for each handler about channel behaviors. This also resolves issues like T7800.
  • notification.channels is misdocumentation from D9479. It should be removed. It should be obsoleted by channel-based config anyway.
  • notification.types and notification.verbosity are arbitrary and we should find a path forward which moves away from them.
  • Stuff like throttling should be implemented in a nonblocking way in the protocol adapter, not a random handler.

Bot stuff is generally a very low priority and I don't expect to review or merge any of it for a long time (roughly, around the Bot/API iteration of Conpherence, which is months/years away).

staticshock added a comment.EditedJun 11 2015, 7:35 PM

Short of an overhaul that won't be available "for a long time", here are the minimal local changes one could make to get notifications working:

diff --git a/src/infrastructure/daemon/bot/handler/PhabricatorBotFeedNotificationHandler.php b/src/infrastructure/daemon/bot/handler/PhabricatorBotFeedNotificationHandler.php
index 21eadf1..1215f39 100644
--- a/src/infrastructure/daemon/bot/handler/PhabricatorBotFeedNotificationHandler.php
+++ b/src/infrastructure/daemon/bot/handler/PhabricatorBotFeedNotificationHandler.php
@@ -125,12 +125,12 @@ final class PhabricatorBotFeedNotificationHandler
         'feed.query',
         array(
           'limit' => $config_page_size,
-          'after' => $chrono_key_cursor,
+          'before' => $chrono_key_cursor,
           'view' => 'text',
         ));
 
       foreach ($stories as $story) {
-        if ($story['chronologicalKey'] == $last_seen_chrono_key) {
+        if ($story['chronologicalKey'] <= $last_seen_chrono_key) {
           // Caught up on feed
           return;
         }
@@ -161,7 +161,7 @@ final class PhabricatorBotFeedNotificationHandler
           $message .= ' '.$objects[$story['objectPHID']]['uri'];
         }
 
-        $channels = $this->getConfig('join');
+        $channels = $this->getConfig('notification.channels');
         foreach ($channels as $channel_name) {
 
           $channel = id(new PhabricatorBotChannel())
  • I assume 'before' has just been broken forever.
  • I think there are some lingering order issues with feed events about tokens (maybe this is limited to x86 systems, don't know.) D13250 didn't fully resolve them, but they can be addressed by changing the $last_seen_chrono_key comparison to`<=`.
  • notification.channels is "wrong" in the grand plan, but good-enough for plenty of use cases.
  • We should move to giving the bot a list of channels to join, and a list of handlers to run in each channel -- not a list of handlers to run at top level, and then other top-level config for each handler about channel behaviors. This also resolves issues like T7800.

In general this sounds way better. We are stuck with slack which is mostly hipster IRC but does have a few additions. People tend to invite phabot to ad-hoc channels because they like the T123 mentions. It would be nice if a default set of handlers could be specified.

srijan added a subscriber: srijan.Aug 4 2015, 6:42 PM

@staticshock, did you work out what the underlying problem was? I tried you patch but it didn't make any difference.

@epriestley, do you have any suggestions on how we might work around this or fix it? The fact that we no longer have a working IRC bot is really painful for us. We don't have any php devs but if you were to pointed us in the right direction I'm sure we could make it work.

@filterfish, I'm using that patch as-is on an x86 system, and it works for me. I suspect that the real solution is not to run phabricator on systems that the core team doesn't test on, so we're planning to move our install to x64 in the near future.

@staticshock. Yeah, fair enough. I'm running phabricator on a 64bit machine so that'll explain why your patch made no difference. It looks like I'll have to dig a bit deeper!

Any thoughts @epriestley?

@filterfish, @staticshock's patch works fine for me on a 64bit machine.

Could you share what problem you are facing? What's not working?

@epriestley, do you have any suggestions on how we might work around this or fix it? The fact that we no longer have a working IRC bot is really painful for us. We don't have any php devs but if you were to pointed us in the right direction I'm sure we could make it work.

Any thoughts @epriestley?

Please see my response earlier on this task:

Bot stuff is generally a very low priority and I don't expect to review or merge any of it for a long time (roughly, around the Bot/API iteration of Conpherence, which is months/years away).

In lieu of waiting for a fix to the official phabot, if you're feeling adventurous, I wrote a Phabricator plugin for my open-source IRC bot. My team uses it 24/7 and it meets our needs.

Bot core: https://github.com/unionstreetmedia/minion
Phabricator plugin: https://github.com/unionstreetmedia/minion-phabricator-plugin

Like most open-source projects, the documentation is sorely lacking, but feel free to email me or open issues on the projects, etc. (Please don't discuss in this task's comment thread, as it's essentially unrelated.)

revi added a subscriber: revi.Oct 3 2015, 5:10 AM