1289977 - reviewbot incorrectly reports replies as reviews

Status: RESOLVED FIXED

(MozReview Graveyard :: reviewbot, defect)

Description

[:glob ✱]

i replied to a review issue:

https://reviewboard.mozilla.org/r/56506/
https://bugzilla.mozilla.org/show_bug.cgi?id=1276843#c13

reviewbot announced this in channel:

[03:43] <reviewbot> glob: review submitted (2 issues to address) on https://reviewboard.mozilla.org/r/56506 (mozreview: add automatic python code formatting (bug 1276843); r?smacleod)
[03:43] <firebot> https://bugzil.la/1276843 — NEW, [e-mail] — automatically format python code using yapf

this isn't a review and shouldn't have been reported.


imho the bot should only report when the review flag state is changed.

Comment 1

[Gregory Szorc [:gps]]

I agree it should not have been reported in channel. Maybe we allow people to subscribe to these events (eventually). But it isn't appropriate to broadcast this event type to the channel.

A goal of reviewbot is to encourage things to move faster - lessen the time between review submission and landing. I think limiting channel notifications to /meaningful/ changes is best. Now, defining /meaningful/ is difficult. But I think "consisting of a review flag change" is a good start.

Comment 2

[:glob ✱]

another slightly different example:

[06:46] <reviewbot> glob: r+ granted on https://reviewboard.mozilla.org/r/66402 (mozreview: Don't overwrite valid ldap associations (bug 1287988) r?smacleod)

triggered by https://bugzilla.mozilla.org/show_bug.cgi?id=1287988#c12

Comment 3

[Mark Côté [:mcote]]

Internally, as has been mentioned in IRC, it is technically a "review"; however, it is also flagged as a "reply" (https://github.com/reviewboard/reviewboard/blob/release-2.5.x/reviewboard/reviews/models/review.py#L159).  At minimum, the IRC message should reflect that, e.g., 

[03:43] <reviewbot> glob: reply submitted: https://reviewboard.mozilla.org/r/56506/#review64652 (mozreview: add automatic python code formatting (bug 1276843); r?smacleod)

Also probably we should strip out the reviewer, at least for reviews & replies, as we do in the commit table and such.

Comment 4

[Mark Côté [:mcote]]

Oh, I see my last point has already been filed as bug 1290000.

Comment 5

[:glob ✱]

(In reply to Mark Côté [:mcote] from comment #3)
> Internally, as has been mentioned in IRC, it is technically a "review";
> however, it is also flagged as a "reply"
> (https://github.com/reviewboard/reviewboard/blob/release-2.5.x/reviewboard/
> reviews/models/review.py#L159).  At minimum, the IRC message should reflect
> that, e.g., 
> 
> [03:43] <reviewbot> glob: reply submitted:
> https://reviewboard.mozilla.org/r/56506/#review64652 (mozreview: add
> automatic python code formatting (bug 1276843); r?smacleod)

please not to a public channel, that will make the irc channel very noisy.

Comment 6

[Gregory Szorc [:gps]]

I think I [hackily] fixed this with https://github.com/mozilla/reviewbot/commit/b945d061bc8375fa2c92736cc7ad356630348809