557445 - Provide tools for assisting in starring builds

Status: RESOLVED FIXED

(Tree Management Graveyard :: TBPL, defect)

Description

[(no longer active)]

I've hacked tbpl to provide a set of tools for assisting the user in starring oranges.

The goal here is to make the daunting task of starring oranges as easy/quick as possible.  This mainly consists of adding two things to tbpl:

1. A UI for suggesting possible bugs as candidates for starring against.
2. A UI for selecting from those candidates, and automating the task of leaving a comment in the orange bug about the starring operation.

I have a set of five patches which I will attach to this bug which made this possible.

I've made a copy of tbpl with these patches available on my own server, which can be accessed from here:

http://ehsanakhgari.org/tinderboxpushlog/index.html

Please note that once I land the patches in this bug, I'll remove that instance of tbpl, in order not to put up with the headache of maintaining this in the future.  The only purpose behind this instance is demonstrating the patches that I'll attach in this bug.

Comment 1

[(no longer active)]

Attachment: Part 1 - Make the linebreaks consistent

This is a trivial fix which will be used in other parts.

Comment 2

[(no longer active)]

Attachment: Part 2 - Support retrieving the bug suggestions for non-starred builds on server side

This patch implements all that is needed to generate bug suggestions for tinderbox logs on the server side.

Comment 3

[(no longer active)]

Attachment: Part 3 - Display suggestions in the UI

This patch shows the bug suggestions in the tbpl UI.

Comment 4

[(no longer active)]

Attachment: Part 4 - Show the starring suggestions inside the UI

This patch adds the suggestions for a log to the Add Comment UI.  With this patch, you can easily click on the bug # for a suggestion to add it to the comment text.

Comment 5

[(no longer active)]

Attachment: Part 5 - Comment on bugs automatically when starring builds

This is perhaps the most interesting part.  It allows automatic commenting on starred bugs.  The comments are all made by a dummy user (I've created a user named [e-mail] for this purpose.)

The file tbplbot.password is intentionally not included in the repository (because it's not smart to commit passwords in revision control systems!).  It should be manually created on the server.  It's a very simple PHP file which defines a single constant: TBPLBOT_PASSWORD.

I can email Markus the copy of the file which I'm using so that he can use it on the main tbpl server.

Comment 6

[(no longer active)]

Comment on attachment 437232 [details] [diff] [review]
Part 3 - Display suggestions in the UI

I forgot to add here that this patch doesn't show suggestions in the UI when the build is already starred.

I did that because I don't think that it provides much value if the build is already starred, and it can lead to confusion and mistakes.  However, if you think that having the suggestions all the time is a good idea, I can simply change this patch to bypass the starred flag to the fetch summary request.

Comment 7

[Markus Stange [:mstange]]

Awesome, thanks so much for doing this!

I'll look at the patches tomorrow.

Comment 9

[Phil Ringnalda (:philor)]

You know how when you are manually starring, you open the log, and find out that someone has starred it already but tbpl hasn't refreshed yet, or if there's no star in the log you open the bug, and see that the last comment is about the same log so you know someone's in the process of starring it, or if the last comment isn't the same log, you copy-paste into the bug and then get a mid-air warning telling you someone starred it while you were copy-pasting?

Since this gives you none of those warnings about what happened between refreshes, this is going to result (has already resulted) in a _lot_ more double-starring and double-commenting - might be good to have it not make a second comment in the same bug about the same log (which would also start to answer the first question you should always ask about doing anything accessible to the public, "how can this be spammed or crapflooded, and how can I stop that?").

Comment 10

[Phil Ringnalda (:philor)]

The likelihood that we'll do a lot more of http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1270685956.1270686830.23478.gz worries me, too.

Comment 11

[Markus Stange [:mstange]]

Comment on attachment 437230 [details] [diff] [review]
Part 2 - Support retrieving the bug suggestions for non-starred builds on server side

>@@ -44,20 +49,70 @@
>         if (preg_match_all("/Build Error Summary.*<PRE>(.*)$/i", $line, $m)) {
>           $foundSummaryStart = true;
>           $line = $m[1][0] . "\n";
>+	  $line = strip_tags($line);

wrong indent

>+        $bugs = getBugsForTestFailure($fileName);
>+        foreach ($bugs as $bug) {
>+          $bug->summary = htmlspecialchars($bug->summary);
>+          $lines[] =
>+            "<span data-bugid=\"$bug->id\" " .
>+                  "data-summary=\"$bug->summary\" " .
>+                  "data-status=\"$bug->status $bug->resolution\"" .
>+            ">Bug <span>$bug->id</span> - $bug->summary</span>\n";
>+        }

Since this is a list, can you use <ul> and <li> here? Not important, though.

Comment 12

[Markus Stange [:mstange]]

Comment on attachment 437232 [details] [diff] [review]
Part 3 - Display suggestions in the UI

>+.stars .summary [data-bugid] + br {
>+  display: none;
>+}

Where does this br come from? Can we block it in get.php?

Comment 13

[Markus Stange [:mstange]]

Comment on attachment 437233 [details] [diff] [review]
Part 4 - Show the starring suggestions inside the UI

>+      box.value = link.textContent;
>+      link.setAttribute("class", "added");
>+    } else {
>+      if (box.value.indexOf(link.textContent) >= 0) {
>+        box.value = box.value.replace(new RegExp("(, )?" + link.textContent), "");
>+        link.removeAttribute("class");
>+      } else {
>+        box.value += ", " + link.textContent;
>+	link.setAttribute("class", "added");

wrong indent

>+  _suggestionLink: function UserInterface__suggestionLink(machineResults) {
>+    var html = "";
>+    if (machineResults.suggestions) {
>+      for (var i = 0; i < machineResults.suggestions.length; ++i) {
>+        var item = machineResults.suggestions[i];
>+        html += '<a href="#" onclick="AddCommentUI.toggleSuggestion(&quot;' +
>+          item.id + "&quot;, &quot;" + escape(item.summary) + "&quot;, &quot;" +
>+          item.status + '&quot;, this); return false;" title="[' + item.status.trim() +
>+          "] " + this._sanitizeHTML(item.summary) + '">Bug ' + item.id + '</a>';

This is a little messy. Instead of constructing html, can you work on DOM node level and use addEventListener and setAttribute and friends? This would also remove the necessity of the _sanitizeHTML function.

Comment 14

[Markus Stange [:mstange]]

(In reply to comment #13)
> (From update of attachment 437233 [details] [diff] [review])
> >+      box.value = link.textContent;
> >+      link.setAttribute("class", "added");
> >+    } else {
> >+      if (box.value.indexOf(link.textContent) >= 0) {
> >+        box.value = box.value.replace(new RegExp("(, )?" + link.textContent), "");
> >+        link.removeAttribute("class");
> >+      } else {
> >+        box.value += ", " + link.textContent;
> >+	link.setAttribute("class", "added");

Actually, I think jQuery has addClass / removeClass helper functions which could be used here. Not 100% sure, though.

Comment 15

[Markus Stange [:mstange]]

Comment on attachment 437235 [details] [diff] [review]
Part 5 - Comment on bugs automatically when starring builds

I agree with philor that this is a little trigger-happy. I think we should ensure at least the following:
 - Don't comment without user intervention in order to prevent cluttering the
   wrong bugs.
 - Don't comment on bugs where this build log was already added.
 - Include that machine type + date + time line that we always paste in those
   comments.

If you can work out a UI where users can say which bugs should be commented on, that would be great.

Comment 16

[(no longer active)]

(In reply to comment #15)
> (From update of attachment 437235 [details] [diff] [review])
> I agree with philor that this is a little trigger-happy. I think we should
> ensure at least the following:
>  - Don't comment without user intervention in order to prevent cluttering the
>    wrong bugs.

I don't understand what you mean.  With this patch, you would only be commenting on a bug if you click on the suggestion button added to the Add Comment dialog.  That is considered user intervention, right?

>  - Don't comment on bugs where this build log was already added.

I definitely agree with that.  I'll update the patch to include this check.

>  - Include that machine type + date + time line that we always paste in those
>    comments.

It already does that.  See bug 557432 comment 23 for an example.

> If you can work out a UI where users can say which bugs should be commented on,
> that would be great.

We have that UI.  Maybe I should elaborate further.

If the log summary contains suggestions, we add bug #'s below the comment text box.  If you click on any of them, you would be including that bug # in the added comment, and tbpl would also submit a comment on the bug.  If you don't do that (for example, only type in the bug # inside the comment text box), you won't get the comment on the bug.

Comment 17

[(no longer active)]

(In reply to comment #9)
> You know how when you are manually starring, you open the log, and find out
> that someone has starred it already but tbpl hasn't refreshed yet, or if
> there's no star in the log you open the bug, and see that the last comment is
> about the same log so you know someone's in the process of starring it, or if
> the last comment isn't the same log, you copy-paste into the bug and then get a
> mid-air warning telling you someone starred it while you were copy-pasting?
> 
> Since this gives you none of those warnings about what happened between
> refreshes, this is going to result (has already resulted) in a _lot_ more
> double-starring and double-commenting - might be good to have it not make a
> second comment in the same bug about the same log (which would also start to
> answer the first question you should always ask about doing anything accessible
> to the public, "how can this be spammed or crapflooded, and how can I stop
> that?").

Like I said in comment 16, I agree that the double-commenting should be fixed.  Note that even after I fix that, it may happen again because of timing issues, and the fact that the Bugzilla API operations are not atomic, but it would cover 99% of the cases.

However, the double-starring problem isn't really specific to this set of patches.  It is still a problem in the Add Comment UI in tbpl (the work-around you mentioned above is for people who don't use tbpl for starring) even without this patch.  However, I think that this problem is a benign.  I prefer to track a fix for that in a separate bug.

(In reply to comment #10)
> The likelihood that we'll do a lot more of
> http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1270685956.1270686830.23478.gz
> worries me, too.

I don't think that this UI in tbpl makes people make more mistakes along the same lines.  I know that I have personally made quite a few such mistakes even without the UI in tbpl.

Currently, the suggestions in tbpl are very basic (they only look up the file name in sw:orange bugs).  It can definitely be improved, but the reason that I didn't make the entire process automatic is that human judgment is still needed here, and people make mistakes, no matter what types of tools they have.


I'll try to hack more on this later today to address the review comments.

Comment 18

[Markus Stange [:mstange]]

(In reply to comment #16)
> (In reply to comment #15)
> > (From update of attachment 437235 [details] [diff] [review] [details])
> > I agree with philor that this is a little trigger-happy. I think we should
> > ensure at least the following:
> >  - Don't comment without user intervention in order to prevent cluttering the
> >    wrong bugs.
> 
> I don't understand what you mean.  With this patch, you would only be
> commenting on a bug if you click on the suggestion button added to the Add
> Comment dialog.  That is considered user intervention, right?

Ah! Yes, indeed. Let me read the patch properly this time.

Comment 20

[Markus Stange [:mstange]]

Comment on attachment 437235 [details] [diff] [review]
Part 5 - Comment on bugs automatically when starring builds

OK, reading this patch again I like the approach. Also, feel free to land the _durationDisplay change before fixing up the rest.

Comment 21

[Markus Stange [:mstange]]

Comment on attachment 437233 [details] [diff] [review]
Part 4 - Show the starring suggestions inside the UI

>+  toggleSuggestion: function AddCommentUI_addSuggestion(id, status, summary, link) {

addSuggestion -> toggleSuggestion

Comment 22

[(no longer active)]

(In reply to comment #11)
> >+        $bugs = getBugsForTestFailure($fileName);
> >+        foreach ($bugs as $bug) {
> >+          $bug->summary = htmlspecialchars($bug->summary);
> >+          $lines[] =
> >+            "<span data-bugid=\"$bug->id\" " .
> >+                  "data-summary=\"$bug->summary\" " .
> >+                  "data-status=\"$bug->status $bug->resolution\"" .
> >+            ">Bug <span>$bug->id</span> - $bug->summary</span>\n";
> >+        }
> 
> Since this is a list, can you use <ul> and <li> here? Not important, though.

This will change a lot of code.  It's totally possible, but do you really want it?  If yes, are you OK with leaving it to a follow-up bug?

Comment 23

[(no longer active)]

(In reply to comment #12)
> (From update of attachment 437232 [details] [diff] [review])
> >+.stars .summary [data-bugid] + br {
> >+  display: none;
> >+}
> 
> Where does this br come from? Can we block it in get.php?

In the server-side code, I'm creating span elements for suggestions, and on the client, we do a html.replace("\n", "<br>"), which would mean that it complicates the logic if I want to remove the extra br's.  I can of course add something like html.replace("</span><br>", "</span>") on the client, if you really want this, but that's more messy than the css hack, IMO.  What do you think?

Comment 24

[(no longer active)]

(In reply to comment #20)
> (From update of attachment 437235 [details] [diff] [review])
> OK, reading this patch again I like the approach. Also, feel free to land the
> _durationDisplay change before fixing up the rest.

I prefer to land the entire thing in one chunk, because I'm afraid that I would miss something or mess things up otherwise!

Comment 25

[(no longer active)]

Attachment: Part 4 - Show the starring suggestions inside the UI

Review comments addressed.

Comment 26

[(no longer active)]

Attachment: Part 5 - Comment on bugs automatically when starring builds

This patch adds the duplicate detection check to the server side bugzilla commenter script.

Comment 27

[(no longer active)]

Attachment: Part 6 - Linkify the suggested bugs

Because otherwise it would be very hard to actually go and investigate those bugs.

Comment 28

[(no longer active)]

Attachment: Part 7 - Remove the background color of suggestions

because the linkified and indented suggestions seem distinct enough.

Comment 29

[(no longer active)]

Attachment: Part 8 - Add a tooltip to the suggestions with the status of the bug

I guess this is my last patch for now...

Comment 30

[Markus Stange [:mstange]]

(In reply to comment #22)
> (In reply to comment #11)
> > >+        $bugs = getBugsForTestFailure($fileName);
> > >+        foreach ($bugs as $bug) {
> > >+          $bug->summary = htmlspecialchars($bug->summary);
> > >+          $lines[] =
> > >+            "<span data-bugid=\"$bug->id\" " .
> > >+                  "data-summary=\"$bug->summary\" " .
> > >+                  "data-status=\"$bug->status $bug->resolution\"" .
> > >+            ">Bug <span>$bug->id</span> - $bug->summary</span>\n";
> > >+        }
> > 
> > Since this is a list, can you use <ul> and <li> here? Not important, though.
> 
> This will change a lot of code.  It's totally possible, but do you really want
> it?  If yes, are you OK with leaving it to a follow-up bug?

Follow-up bug is fine, sure.

(In reply to comment #23)
> (In reply to comment #12)
> > (From update of attachment 437232 [details] [diff] [review] [details])
> > >+.stars .summary [data-bugid] + br {
> > >+  display: none;
> > >+}
> > 
> > Where does this br come from? Can we block it in get.php?
> 
> In the server-side code, I'm creating span elements for suggestions, and on the
> client, we do a html.replace("\n", "<br>"),

Ah, there it is. OK then just leave it as it is.

Comment 31

[Markus Stange [:mstange]]

Comment on attachment 438019 [details] [diff] [review]
Part 6 - Linkify the suggested bugs

Maybe use .html() instead of .get(0).innerHTML?

Comment 32

[(no longer active)]

(In reply to comment #30)
> (In reply to comment #22)
> > (In reply to comment #11)
> > > >+        $bugs = getBugsForTestFailure($fileName);
> > > >+        foreach ($bugs as $bug) {
> > > >+          $bug->summary = htmlspecialchars($bug->summary);
> > > >+          $lines[] =
> > > >+            "<span data-bugid=\"$bug->id\" " .
> > > >+                  "data-summary=\"$bug->summary\" " .
> > > >+                  "data-status=\"$bug->status $bug->resolution\"" .
> > > >+            ">Bug <span>$bug->id</span> - $bug->summary</span>\n";
> > > >+        }
> > > 
> > > Since this is a list, can you use <ul> and <li> here? Not important, though.
> > 
> > This will change a lot of code.  It's totally possible, but do you really want
> > it?  If yes, are you OK with leaving it to a follow-up bug?
> 
> Follow-up bug is fine, sure.

Filed bug 558408.

Comment 33

[(no longer active)]

Landed all 8 parts as:

http://hg.mozilla.org/users/mstange_themasta.com/tinderboxpushlog/pushloghtml?changeset=cde5b07af93b

Comment 35

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Is the tbplbot working?  I starred http://tinderbox.mozilla.org/showlog.cgi?log=Firefox3.5/1270852726.1270853219.16380.gz earlier, and I waited a few minutes and saw no comment in bug 550963, so I added that comment manually.

Similarly, Ehsan starred http://tinderbox.mozilla.org/showlog.cgi?log=Firefox3.6/1270851025.1270853252.16507.gz at about the same time, and I see no comment in bug 515157.

Comment 36

[(no longer active)]

One of the bolts on tbplbot was loose.  I fastened it:

http://hg.mozilla.org/users/mstange_themasta.com/tinderboxpushlog/rev/fa63ebb799f3

Things should be back to normal now.

Comment 37

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #36)
> One of the bolts on tbplbot was loose.  I fastened it:
> 
> http://hg.mozilla.org/users/mstange_themasta.com/tinderboxpushlog/rev/fa63ebb799f3
> 
> Things should be back to normal now.

I don't see a comment on bug 523936 yet from you starring http://tinderbox.mozilla.org/showlog.cgi?log=Firefox3.6/1270854924.1270855872.25554.gz at 16:41 PDT, which was ~11 minutes after comment 36 :(

Comment 38

[(no longer active)]

(In reply to comment #37)
> (In reply to comment #36)
> > One of the bolts on tbplbot was loose.  I fastened it:
> > 
> > http://hg.mozilla.org/users/mstange_themasta.com/tinderboxpushlog/rev/fa63ebb799f3
> > 
> > Things should be back to normal now.
> 
> I don't see a comment on bug 523936 yet from you starring
> http://tinderbox.mozilla.org/showlog.cgi?log=Firefox3.6/1270854924.1270855872.25554.gz
> at 16:41 PDT, which was ~11 minutes after comment 36 :(

I verified the fix by looking at the network request, so I'm fairly sure that my script is working.  Gerv, is it possible that the BzAPI is failing on us?

Comment 39

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

It is apparently working some of the time, since Phil's starring of http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1270865109.1270866505.21978.gz showed up in Bug 555390, and tbplbot also commented in bug 556124 twice this afternoon (one was masayuki's starring of http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1270857048.1270858100.32251.gz).

Comment 40

[Phil Ringnalda (:philor)]

Did you look in other random bugs for the missing comments? I didn't notice the first time, but I just starred http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1270877782.1270880690.22867.gz as being bug 557432, and tbplbot commented in bug 507996, which is the other suggestion for that failure.

Comment 41

[Markus Stange [:mstange]]

Did you click on the bug 507996 suggestion and changed the text manually to bug 557432?

Comment 42

[Phil Ringnalda (:philor)]

No, while I don't know what bas and zwol did for their double miscomment, both of mine were the result of clicking on nothing but the bug 557432 suggestion and the Add Comment button.

Comment 43

[Phil Ringnalda (:philor)]

See also bug 523936, where there are no tbplbot comments after 2010-04-08 18:28:39, and bug 545323, the third suggestion for the constant "Get Bookmark Add-ons == Menu Link Before" failures, where there are 11 comments starting with 2010-04-09 16:08:16 PDT, the first two from ehsan, who certainly didn't misuse his toy.

With nothing to experiment with, I don't know whether it's off by one, or 2 == 3, or always the last of multiple suggestions, but the shiny pretty thing is utterly, horribly broken, and desperately needs to be backed out unless a fix is imminent, because right now it's teaching people that it is a source of random, completely wrong noise.

Comment 44

[Markus Stange [:mstange]]

I've backed out part 5:
http://hg.mozilla.org/users/mstange_themasta.com/tinderboxpushlog/rev/76c49610c114
http://hg.mozilla.org/users/mstange_themasta.com/tinderboxpushlog/rev/214172ec003a

Comment 45

[Markus Stange [:mstange]]

Ehsan, let me know how I can help you debug this.

Comment 46

[(no longer active)]

I'm starting to investigate the problem.

In order to do so, I updated my own instance of tbpl to tip, changed things so that suggestions are retrieved for starred builds as well, reapplied the backed out patch and also the bustage fix I landed for submitBugzillaComment.php, and changed AddCommentUI.js so that it just brings up alert windows instead of starring the builds and commenting in the bugs.

I'll post here when I know more about the problem.

Comment 47

[(no longer active)]

Attachment: Part 9 - Fix commenting in the wrong bug when there are multiple suggestions

So, the problem was a mistake I made when addressing comment 13.  I was using JS closures in my patch, which caused the last suggestion's bug ID to be passed to AddCommentUI.toggleSuggestion no matter which suggestion you actually clicked.  Therefore, for summaries which had multiple suggestions, we would end up commenting in the last bug in the suggestion list, which caused the broken behavior here.

In order to test my fix, go to http://ehsanakhgari.org/tinderboxpushlog2/index.html (which has a copy of tbpl without the patch I'm attaching), find one of the Xd test_browserGlue_distribution.js oranges (which has three suggestions), click Add a comment, select bug 523936, and press Add Comment.  In the second alert bug, you see that tbpl is trying to comment in bug 545323, which is the last suggestion.

Now, go to http://ehsanakhgari.org/tinderboxpushlog/index.html which is an instace with this patch, and repeat the same steps.  This time, the correct bug will be chosen for commenting.

I'm very sorry for missing this originally, and for all the wrong comments that resulted from this embarrassing mistake.

Comment 48

[(no longer active)]

Oh, and neither of the two test tbpl instances mentioned in comment 47 actually stars any logs or comment on any bugs, so you can use them as a sandbox to make sure that the problem has been fixed.

Comment 49

[Markus Stange [:mstange]]

Comment on attachment 438275 [details] [diff] [review]
Part 9 - Fix commenting in the wrong bug when there are multiple suggestions

Gah! I actually noticed this when I re-read your patches today ("I wonder if this works..."), but failed to mention it and then forgot about it...

Comment 50

[(no longer active)]

Relanded part 5 with the bustage fix:

http://hg.mozilla.org/users/mstange_themasta.com/tinderboxpushlog/rev/46488835ca28

Landed the fix:

http://hg.mozilla.org/users/mstange_themasta.com/tinderboxpushlog/rev/e7f5e23a428a

I think this can now be deployed.

Comment 51

[Markus Stange [:mstange]]

Done. Thanks!

Comment 52

[Gervase Markham [:gerv]]

/me arrives.

So, not my problem, then? :-)

Gerv

Comment 53

[(no longer active)]

Actually, people are still complaining that tbpl sometimes doesn't comment inside bugs.  Are we sure that there are no problems at Bugzilla API side?

The client code is pretty simple, it just constructs a URL and passes it the required parameters.  The code looks like this:

<http://hg.mozilla.org/users/mstange_themasta.com/tinderboxpushlog/file/tip/submitBugzillaComment.php>

Comment 54

[Gervase Markham [:gerv]]

Ehsan: a quick code review of that code leads me to make the following comments:

- Please use a proper JSON escaping function. If your comment has a backslash in, for example, the JSON will not be well-formed and BzAPI will probably fail.

- If you just want a list of comments, don't get the whole bug with comments, just get the comments:
https://wiki.mozilla.org/Bugzilla:REST_API:Methods#List_comments_for_bug_.28.2Fbug.2F.3Cid.3E.2Fcomment_GET.29

- Please check the return codes from the functions you call! This may well tell you if things are failing or not!

Looking at the logs on api-dev, the only errors in the last few days for /latest are people doing big searches which time out.

Gerv

Comment 55

[(no longer active)]

I filed comment 54 as bug 559452.