657690 - Autostar comments not getting added to comment box

Status: RESOLVED FIXED

(Tree Management Graveyard :: TBPL, defect)

Description

[Steve Fink [:sfink] [:s:]]

Currently not yet live on the site, but this was broken by my change 42964668d16b for bug 655887. (Thanks philor for bisecting the regression and helping me see the bug.)

The specific failure is that if you click on a star next to a bug in the suggestion box, then bring up the comment box, the *text box* will not have "Bug nnnn" in it, nor will the bug number be highlighted.

What is happening is that my change caused AddCommentUI.updateUI() to be called twice, which ends up calling _toggleSuggestion. As its name implies, this results in the bug being added to the comment, then removed.

I need to figure out how to make updateUI() idempotent, but it's a little tangled because it seems like state is being kept in multiple places (the comment box text itself and _autoStarBugs), and at least after my change, it's inconsistent whether the comment box text is considered valid when the comment box is not open.

Comment 1

[Steve Fink [:sfink] [:s:]]

Attachment: Dynamically update "autostar" bugs

This is not exactly a minimal fix. It makes the suggested bug stars dynamic -- as in, if you have the comment box open, you can toggle a bug either via the bug buttons in the "Suggestions" area of the comment box (previous functionality) or via the star icon in the light blue suggestions area. It made sense with the current behavior of other links outside of the comment box being active, though it was mostly a side effect of the bug fix.

Note that I don't fully understand the _autoStarBugs mechanism, so I may have broken something there. As it is, I don't actually see a reason for it to exist. What's the difference between _autoStarBugs and addToBugs? It seems to implement a reference count per bug, but I wonder if that's just because it was using the toggling behavior before so it needed to suppress redundant updates?

In case it makes sense to remove entirely, I'll tack on another patch that does just that.

Comment 2

[Steve Fink [:sfink] [:s:]]

Attachment: Remove _autoStarBugs

Remove _autoStarBugs. There are some functions that could stand to be renamed, but to be sure I'd want to understand what the "auto star" stuff actually refers to (that is different from addToBugs).

Comment 3

[Phil Ringnalda (:philor)]

I *think* _autoStarBugs is the footgun we were recently talking about in another bug, where if you pick a suggestion from the summary, and then click the star to the left of the "add a comment" link, it will automatically star the build and comment on the bug without opening the comment form, which is cool, but it will also remember that one, and the next time you use it it will comment on both the new bug and the old bug from the last time you used it. Since we haven't made any progress on fixing that, if that's what _autoStarBugs really is, we should remove it.

Comment 4

[Steve Fink [:sfink] [:s:]]

It looks like _autoStarBugs is for that (AddCommentUI.commentWithoutUI()), but it seems to be more broadly used within the code.

These patches don't remove that behavior. They might even fix it. I'd just need to figure out how to test it without spamming the tbpl comment DB... (bugzilla is safe from me since I don't have the password.)

Comment 5

[Arpad Borsos [:Swatinem]]

Comment on attachment 533061 [details] [diff] [review]
Dynamically update "autostar" bugs

Review of attachment 533061 [details] [diff] [review]:
-----------------------------------------------------------------

Comment 6

[Arpad Borsos [:Swatinem]]

Comment on attachment 533064 [details] [diff] [review]
Remove _autoStarBugs

Review of attachment 533064 [details] [diff] [review]:
-----------------------------------------------------------------

Both patches look good codewise, but I’m not too familiar with the semantics of autostarring either.

Comment 7

[Arpad Borsos [:Swatinem]]

(In reply to comment #4)
> These patches don't remove that behavior. They might even fix it. I'd just
> need to figure out how to test it without spamming the tbpl comment DB...
> (bugzilla is safe from me since I don't have the password.)

Yes, please do make sure to test this thoroughly.

Comment 8

[Steve Fink [:sfink] [:s:]]

(In reply to comment #7)
> (In reply to comment #4)
> > These patches don't remove that behavior. They might even fix it. I'd just
> > need to figure out how to test it without spamming the tbpl comment DB...
> > (bugzilla is safe from me since I don't have the password.)
> 
> Yes, please do make sure to test this thoroughly.

Ok, I added a _testing flag that changes all of the POSTs done via NetUtils.js into log messages, and can easily recreate the problem mentioned.

This patch does not fix the bug. It preserves the existing (buggy) behavior. I have a trivial fix for the footgun multi-bug issue, but the fix doesn't really belong in this bug.

Comment 9

[Steve Fink [:sfink] [:s:]]

http://hg.mozilla.org/users/mstange_themasta.com/tinderboxpushlog/rev/31e2a4ffe4ed
http://hg.mozilla.org/users/mstange_themasta.com/tinderboxpushlog/rev/e2f6a2855ab2