293181 - [BUGZILLA] Move bugsHistory checking into FetchBug

Status: RESOLVED FIXED

(Webtools Graveyard :: Mozbot, enhancement)

Description

[Max Kanat-Alexander]

The Bugzilla.bm module checks every time somebody mentions a bug, to make sure
that it hasn't reported it too recently. It does this in CheckForBugs, which
calls FetchBug.

However, with the new bzbot-into-mozbot code (bug 292492), we call FetchBug
directly when we want to mention a bug. So the bzbot-into-mozbot will report the
same bug over and over, if many changes to the same bug come in a few seconds apart.

Instead, the check should happen in FetchBug, and there should be a parateter to
override it.

Also, we have so many parameters to the Bugzilla.bm GetURI, now, that I think we
should instead pass the final args as a hash of params, since so many of them
are optional.

Comment 1

[Max Kanat-Alexander]

*** Bug 302117 has been marked as a duplicate of this bug. ***

Comment 2

[Colin Ogilvie [:cso]]

Attachment: Patch v1

Comment 3

[Max Kanat-Alexander]

Comment on attachment 190570 [details] [diff] [review]
Patch v1

It looks generally good.

Only two comments:

needToFetchBug is both an accessor and a mutator. That is, it gives us
information, and as a side effect it silently changes the bugsHistory. I'd
prefer to have two different functions for doing it.

Also, we might as well make bugsToFetch an array and pass it around as an
arrayref, while we're in that code anyway.

Oh, and you don't need to do "return unless scalar(@ids) > 0" -- you can just
do "return unless @ids" (that's a nit).

Comment 4

[Colin Ogilvie [:cso]]

Attachment: Patch v2

(In reply to comment #3)
> needToFetchBug is both an accessor and a mutator. That is, it gives us
> information, and as a side effect it silently changes the bugsHistory. I'd
> prefer to have two different functions for doing it.

Fixed by moving the change out of needToFetchBug, not moving it to another
function though as it doesn't seem to need one for one line of code...

> Also, we might as well make bugsToFetch an array and pass it around as an
> arrayref, while we're in that code anyway.

That's a bigger change than this bug deals with IMO.

> Oh, and you don't need to do "return unless scalar(@ids) > 0" -- you can just

> do "return unless @ids" (that's a nit).

Changed...

Comment 5

[Max Kanat-Alexander]

Comment on attachment 190605 [details] [diff] [review]
Patch v2

Yeah, this looks right. I can't yet test it on ssdbot, because it seems to
conflict with the "DUPLICATE of" patch, but I'll get it into ssdbot soon.

Comment 6

[Colin Ogilvie [:cso]]

Perhaps checking things in would help... I can never remember if the Bugzilla
module needs anything more than r+ from someone though otherwise I would.

Comment 7

[Max Kanat-Alexander]

The Bugzilla module only needs r+. You can check in anything with an r+ on it.

Comment 8

[Colin Ogilvie [:cso]]

(In reply to comment #5)
> Yeah, this looks right. I can't yet test it on ssdbot, because it seems to
> conflict with the "DUPLICATE of" patch, but I'll get it into ssdbot soon.

Interesting - it didn't conflict when doing the checkin :)

Checking in BotModules/Bugzilla.bm;
/cvsroot/mozilla/webtools/mozbot/BotModules/Bugzilla.bm,v  <--  Bugzilla.bm
new revision: 2.16; previous revision: 2.15
done