107085 - RFC: comments should be able to have User-Agent auto-appended

Status: NEW

(Bugzilla :: Creating/Changing Bugs, enhancement, P4)

Description

[Frank Tobin]

Just like Bugzilla does some magic to pre-fill some fields when submitting a new
bug, to help bug commenters remember to report their environment, there should
be an option to stuff a bug commenter's User-Agent into their comment.

I'm new to Bugzilla, and I've been in the habit habit of pasting my User-Agent
string to the end of each comment.  I think that this functionality could help
any commentators who are commenting things like "WorksForMe", or trying to
verify a bug works by ensuring that the proper agent information gets reported.

Comment 1

[Frank Tobin]

Attachment: proposed patch (entirely untested; I only know it compiles)

Comment 2

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 55365 [details] [diff] [review]
proposed patch (entirely untested; I only know it compiles)

There should probably be an extra new-line before "User-Agent" for formatting
reasons.

We also need to run the check for if $::FORM{'append-agent'} is set in either a 
separate control block or add parentheses around the two concatenated strings.  
I prefer the control block because then you avoid the .= if the checkbox was
unchecked.

And also, you had scoped a $comment variable, appended the $::FORM{'comment'}
content 
to it with the User Agent stuff, and then you ignored it and kept the existing
call
to AppendComment() with $::FORM{'comment'} instead of the new variable.

The way you had it, we would always appened "\nUser-Agent: " no matter if the 
form control was checked or not.  It might also be a wise idea to see if 
$ENV{'HTTP_USER_AGENT'} is defined or not; some browsers don't send one and if
we can
avoid having just plain old "User-Agent: " at the bottom of comments, we
should.

And <label> should be used on the checkbox text, for easier clickability.

Comment 3

[Frank Tobin]

Attachment: proposed patch, take #2

Again, this code is entirely untested; I only know it compiles.

I took Christopher Aillon's comments and have applied them.  They are good
suggestions (and a stupid bug on my part to not apply back my locally-scoped 
$comment).

WARNING: I don't know if at process_bug.cgi is supposed to be responsible for
doing things like sanitizing the user agent string.  If it is, my patch needs
to be modified to do this.

There are now two newlines before the User-Agent string would be appended, and
one after (one more before and after from my last submission).

Comment 4

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 57337 [details] [diff] [review]
proposed patch, take #2

>+print "<p><label><input name='append-agent' type='checkbox'>Append my User-Agent string to comment</label></p>";

I'm being picky here, but use <input id="foo"><label for="foo">Bar</label>. 
The other syntax you used is valid, but MSIE doesn't understand it. 

And even pickier:  use qq{<input name="append-agent" etc...} to quote the
values with double quotes for consistency with other stuff.  If we use quotes
in HTML they are double quotes.

>+        my $agent = $ENV{'HTTP_USER_AGENT'};
>+        if ($::FORM{'append-agent'} and defined $agent) {

The above is wrong:  $my agent = $foo // is defining $agent, so the latter part
of your check will always be true.

There is no need to assign $ENV{'HTTP_USER_AGENT'} to a variable.  Just check
it directly:
       if (exists $::FORM{'append-agent'} 
	   && defined $ENV{'HTTP_USER_AGENT'} 
	     && $ENV{'HTTP_USER_AGENT'})  // also checks for if it is defined
as 0 or '' 


>+            $::FORM{'comment'} .= "\n\nUser-Agent: $agent\n";

Again, being picky, but No need for the trailing newline.  That just adds more
vertical space to Bugzilla.  We already add some AFAIK.

Comment 5

[Frank Tobin]

Attachment: patch take #3

> I'm being picky here, but use <input id="foo"><label for="foo">Bar</label>. 
> The other syntax you used is valid, but MSIE doesn't understand it. 

Using this syntax is bad, because it requires the <input> element to have both
an id and name attribute (label <em>must refer to an id</em>, and form inputs
<em>must have a name</em>.  But I'll do it anyways.

>The above is wrong:  $my agent = $foo // is defining $agent, so the latter
part
> of your check will always be true.
You are incorrect.  If $foo is undef (or a non-existent hash entry), then
$agent is undef, and defined($agent) returns false.

I'm not sure you want to not display the User-Agent string if it is empty,
because the user <em>explicitly</em> selected that he wanted his agent string
attached, whatever it might be.  But I'll have it do it anyways.

> There is no need to assign $ENV{'HTTP_USER_AGENT'} to a variable.  Just check

> it directly:

I disagree.  The hash-key auto-vivifying of Perl is a great evil, a primary
source of many bugs due to typos in the key.  By minimizing direct hash lookups
(and assigning to scalars like $agent instead), we avoid bugs due to typos.

> And even pickier:  use qq{<input name="append-agent" etc...} to quote the
> values with double quotes for consistency with other stuff.  If we use quotes

> in HTML they are double quotes.

That's funny.  Most Bugzilla code doesn't use any quotes at all.

Comment 6

[Christopher Aillon (sabbatical, not receiving bugmail)]

> Using this syntax is bad, because it requires the <input> element to have both
> an id and name attribute (label <em>must refer to an id</em>, and form inputs
> <em>must have a name</em>.  But I'll do it anyways.

Why is it bad?  It's perfectly legal per the specs.  The name attribute will
always be there because it is used for naming the form control.  The id
attribute is there for identifiying the control within the DOM.  The two
attributes have nothing to do with each other and thus it is fine to use them
together.  If we were talking about <form name id> I would agree because they
both do the same thing by identifying a unique form within the document.  Again
though, with <input> it is fine.  Especially with checkboxes or radio buttons
which often have the same name, yet different IDs.

> That's funny.  Most Bugzilla code doesn't use any quotes at all.

Yeah, that is changing.

Comment 7

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 57383 [details] [diff] [review]
patch take #3

I still think we want to check for definedness of $::FORM{'append-agent'} for
consistency with the syntax used elsewhere, such as right above in the parent
control structure.

And I still would prefer to just reference $ENV{'HTTP_USER_AGENT'} directly
without the additional variable.  As far as typos, that is why we have our
review process.  Two reviews are needed except in rare cases before it makes it
into the tree.	These things get caught rather quickly.

>     if (defined $::FORM{'comment'}) {
>+        my $agent = $ENV{'HTTP_USER_AGENT'};
>+        if ($::FORM{'append-agent'} and $agent) {
>+            $::FORM{'comment'} .= "\n\nUser-Agent: $agent";
>+        }
>         AppendComment($id, $::COOKIE{'Bugzilla_login'}, $::FORM{'comment'});
>     }
>     


Apart from those two nits, the patch looks ok.	I'll get a second person to
look at the patch and offer comments as well.

Comment 8

[Gervase Markham [:gerv]]

We shouldn't do this. We certainly shouldn't do it unconditionally, because
Bugzilla is used to track bugs in all manner of things which aren't browsers,
for which the reporter's User Agent would be useless. 

And we shouldn't do it at all because people often report bugs with a different
browser, or a version of Mozilla other than the one that they couldn't get to
start. :-)

Gerv

Comment 9

[Frank Tobin]

[e-mail], I believe your arguments do not follow the current design of
Bugzilla's features.  As I stated in my original comment, Bugzilla
<em>already</em> fills in stuff based upon what browser/platform the user is
running on.  There are many instances where the reporter's platform is
pointless; why would we fill this information in automatically, then?  Whatever
reason you come up for that, simply apply the same reasoning for this change.

I don't disagree that the user agent string is useful for all installations of
Bugzilla.  That's why we have a <em>checkbox</em> for the user.

Comment 10

[Dave Miller [:justdave]]

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

Comment 11

[Dave Miller [:justdave]]

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

Comment 12

[Dave Miller [:justdave]]

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

Comment 13

[[e-mail]]

> defined $ENV{'HTTP_USER_AGENT'}

Don't you mean exists?

Comment 14

[Dave Miller [:justdave]]

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

Comment 15

[Dave Miller [:justdave]]

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

Comment 16

[Frank Tobin]

>> defined $ENV{'HTTP_USER_AGENT'}

>Don't you mean exists?

Actually, no.  It's quite useless if the key exists in the hash but the value is
undefined.  Silly Perl...

Comment 17

[Bradley Baetz (:bbaetz)]

Comment on attachment 57383 [details] [diff] [review]
patch take #3

This patch has bitrotted, since bug_form.pl is now templatised.

Gerv, you were wrokgin on comment templates, weren't you? Can this be done that
way, instead?

Comment 18

[Joel Peshkin]

Unloved bugs targetted for 2.18 but untouched since 9-15-2003 are being
retargeted to 2.20
If you plan to act on one immediately, go ahead and pull it back to 2.18.

Comment 19

[vassily "vr" ragosin]

We need to be able to file User-Agent string along with a bug, probably on
project by project basis., as it is already done with OS/Platform and as it is
being done on mozilla.org installation of bugzilla for browser projects. Make a
new entry in DB for that.

This way Bugzilla can be used to track problems with websites (as in "collection
of webpages managed by a webmaster"). It is not very hard to implement this, but
it will get buzilla more spread, basically completely new user base --
webmaster, web design companies etc. 

Comment 20

[Dave Miller [:justdave]]

Bugzilla 2.20 feature set is now frozen as of 15 Sept 2004.  Anything flagged
enhancement that hasn't already landed is being pushed out.  If this bug is
otherwise ready to land, we'll handle it on a case-by-case basis, please set the
blocking2.20 flag to '?' if you think it qualifies.