248386 - Add support for Alias to post_bug.cgi

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, defect)

Description

[Dave Miller [:justdave]]

Whether this gets added to the new bug form is irrelevant, but adding support
for this to the form processing, so that if someone modified their form to use
it, or was using something along the lines of bugzilla-submit to post bugs, it
could be included.

This comes in handy for systems using bugzilla-submit to import bug data from
another system into Bugzilla where they can use the alias to hold the bug number
from the previous system.

The change to make this work turns out to be pretty easy.

Comment 1

[Dave Miller [:justdave]]

Attachment: Patch v1

This is almost too easy...  but it works.

Is there somewhere we should validate that it starts with a letter or
something? That's all I can think of that this might be missing.

Comment 2

[Albert Ting]

Actually this doesn't work.  When a form submits post_bug.cgi, need to ignore the
"alias" form value if it's set to blank.  Otherwise it will always complain about 
duplicate alias name values.

So I believe two things needs to be done.

   1. before processing the bug fields in post_bug.cgi, undef $::FORM{'alias'} 
      if the form value is set to blank.
   2. convert the process_bug.cgi alias checks into a ValidateAlias function, then
      call that function from both process_bug.cgi and post_bug.cgi

Comment 3

[Dave Miller [:justdave]]

Actually, in the place this was being used, the script wasn't even submitting
the alias field unless it was being used.  But yes, that would kind of make
putting the field on the new bug form kind of hard...

What you suggest with the alias validation is probably the way to go.

Comment 4

[Albert Ting]

Attachment: 2.17.7 patch

Based on the previous comments, I've moved the alias checking code from
process_bug.cgi to globals.pl, then updated post_bug.cgi accordingly. 
Combined with bug #165022, this works as expected.

Comment 5

[Dave Miller [:justdave]]

Comment on attachment 151962 [details] [diff] [review]
2.17.7 patch

I'll veto anything that adds functions to globals.pl.  We're trying to
eliminate that file.  Anything that's shared needs to be in one of the .pm
files.	Bugzilla/Bug.pm seems like the closest match, but I'm not sure if
that's a good place for it either (since we're not operating on a Bug object
here)

Comment 6

[Albert Ting]

(In reply to comment #5)

Since this deals with bugs, Bug.pm seems like a good place.  Or perhaps
Bugzilla/Util.pm?  In any caes, what's the difference between *.pm and 
Bugzilla/*.pm?  I'm assuming we're trying to migrate to Bugzilla/*.pm?

Comment 7

[Dave Miller [:justdave]]

Anyone have opionions where this function should go? (see comment 5)

Comment 8

[Dave Miller [:justdave]]

(In reply to comment #6)
> In any caes, what's the difference between *.pm and Bugzilla/*.pm?  I'm
> assuming we're trying to migrate to Bugzilla/*.pm?

Assuming you mean what's the difference between *.pl and *.pm, a *.pm file has a
distinct namespace, and doesn't pollute the global environment, which is
important for mod_perl.

Comment 9

[Christian Reis]

Today, bugs are created (INSERT INTO bugs ...) in post_bug.cgi. This is not the
place to do it, and I believe now that the only reasonable place would be in
Bugzilla/Bug.pm. 

Note that validation needs to be done to other fields (description_required,
invalid_product_name, entry_access_denied, require_*, unknown_keyword, ...).
Given this, and given the fact that the main consumer (once the moving around
has been done) is going to be Bug.pm's bug-creation code, I think having
ValidateAlias is fine in the same file. I'd suggest having it just before the
AUTOLOAD function, guarded off by a comment section that indicated

#
# Field Validation
# 

But that's really just icing.

Comment 10

[Albert Ting]

Attachment: updated 2.18rc1 patch

Enclosed is an updated 2.18RC1 patch.  Per your suggestions, I moved
the ValidateAlias routine into Bugzilla/Bug.pm.  I also changed the
approach so that the modifications to post_bug.cgi were kept to a
minimum.

Comment 11

[GavinS]

Not sure if these would be justifications for a r- (as the patch works fine),
but these would be my issues:

1) In the new routine, should you check for definedness before doing the trim()?

2) Can we be sure that now, and in the future, we will never have a bug id of zero?

3) The string in the GetBugLink() call is not localisable, and does not use
terms.Bug. Can that call be moved to the template? (This was still the case in
the old code, but it would be good to fix if it can be done easily.)

4) Though not a 100% consistent rule in Bugzilla, the majority of calls have
just @_, and not (@_) when reading parameters

5) Some sort of interface comment would be nice, in lieu of proper Perl
documentation, which we have in some of the Perl .pms

Comment 12

[Frédéric Buclin]

Comment on attachment 153823 [details] [diff] [review]
updated 2.18rc1 patch

>-my @bug_fields = ("version", "rep_platform",
>+my @bug_fields = ("version", "rep_platform", "alias",
>                   "bug_severity", "priority", "op_sys", "assigned_to",
>                   "bug_status", "bug_file_loc", "short_desc",
>                   "target_milestone", "status_whiteboard");
>+delete($::FORM{'alias'}) if (!Bugzilla::Bug::ValidateAlias($::FORM{'alias'},0));
>+
> my @used_fields;
> foreach my $field (@bug_fields) {
>     if (exists $::FORM{$field}) {

Instead of deleting an existing field, I would prefer just to ignore it if the
alias name is invalid:
  push (@bug_fields, "alias") if B::B::ValidateAlias($cgi->param('alias'), 0);



>+sub ValidateAlias {
>+    my ($alias,$curr_id) = (@_);

Missing space and useless ():
my ($alias, $curr_id) = @_;


>+    if (! defined($alias)) {
>+      return 0;
>+    }
>+
>+    if ($alias eq "") {
>+      return 0;
>+    }

I would merge them in a single test condition (and use the correct indentation
of 4 spaces):
if (!defined($alias) || $alias eq "") {
    return 0;
}


+    my $vars = { alias => $alias };

Please write $vars->{'alias'} = $alias; instead.


>+    &::SendSQL("SELECT bug_id FROM bugs WHERE alias = $escaped_alias " . 
>+            "AND bug_id != $curr_id");

What happens if $curr_id does not exist? You use 0 in post_bug.cgi. I think
this is a nit as bug 0 should not exist. But maybe should you use something
like:

my $query = "SELECT ....";
if (defined $curr_id) {
    $query .= "AND bug_id != $curr_id";
}

This way, you don't need to give a second (and arbitrary) parameter in
post_bug.cgi.


>+        $vars->{'bug_link'} = &::GetBugLink($id, "Bug $id");

"Bug" should be in templates using $terms.Bug.


>+        &::ThrowUserError("alias_in_use", $vars);

ThrowUserError does not require &::

Comment 13

[Albert Ting]

Attachment: update 2.19.2 patch

Update patch to address comment #11 and comment #12.  Note that I cheated and
did not create a template file for $terms.Bug.	Instead I updated
user-error.html.tmpl as I didn't feel it was worth creating another template
file.  There may be a better way to embedd a space in the .tmpl file, but I
didn't know how offhand.

Comment 14

[Frédéric Buclin]

Comment on attachment 176062 [details] [diff] [review]
update 2.19.2 patch

If you want your patch to be reviewed, you have to ask. ;)

Comment 15

[Albert Ting]

Thanks Frédéric.  I believe this should be set to the 2.20 Target milestone, as
the blocked bug was set to 2.20 by Dave.

Comment 16

[Frédéric Buclin]

Comment on attachment 176062 [details] [diff] [review]
update 2.19.2 patch

>--- post_bug.cgi	2005-02-01 22:44:51.000000000 -0800
>+++ post_bug.cgi	2005-03-02 11:32:25.000000000 -0800
>@@ -143,6 +143,10 @@
>                   "bug_status", "bug_file_loc", "short_desc",
>                   "target_milestone", "status_whiteboard");
> 
>+if (Bugzilla::Bug::ValidateAlias($::FORM{'alias'})) {
>+    push (@bug_fields,"alias");
>+}

I think this function should be called ValidateBugAlias for consistency with
ValidateBugID.
Maybe should we export ValidateBugAlias in Bug.pm, see the
@Bugzilla::Bug::EXPORT at the beginning of this file. This way, you don't need
to write Bugzilla::Bug::.



>+            Bugzilla::Bug::ValidateAlias($alias,$idlist[0]);

I think this part in process_bug.cgi should be cleaned up a bit. One example
could be:

if (Param("usebugaliases") && defined($::FORM{'alias'})) {
    if (scalar(@idlist) == 1) {
	my $alias = trim($::FORM{'alias'});

	# Add the alias change to the query. If the field contains the blank 
	# value, make the field be NULL to indicate that the bug has no alias.
	# Otherwise, if the field contains a value, update the record
	# with that value.
	DoComma();
	$::query .= "alias = ";

	if ($alias ne "") {
	    ValidateBugAlias($alias, $idlist[0]);
	    $::query .= SqlQuote($alias); # <- replace it with the new code
($dbh->quote, ...)
	} else {
	    $::query .= "NULL";
	}
    }
}


>+# ValidateAlias: If the alias string is empty or not defined, return
>+# 0.  Otherwise, verify it's a valid alias string and not used

I disagree. If no string is given or is empty, an error should be returned, in
the same way ValidateBugID works. An empty string is *not* a valid alias!!
Moreover, if the alias is undefined, the code should not call ValidateBugAlias.


>+sub ValidateAlias {
>+    my ($alias, $curr_id) = @_;
>+
>+    $alias = trim($alias) if (defined($alias));

$alias = trim($alias || ""); looks better.

>+
>+    if (! defined($alias) || $alias eq "") {
>+        return 0;
>+    }

With my previous comment and the notation used above, you should write:
if ($alias eq "") {
    ThrowUserError("alias_...");
}

>+    # Make sure the alias is unique.
>+    my $escaped_alias = &::SqlQuote($alias);
>+    my $query = "SELECT bug_id FROM bugs WHERE alias = $escaped_alias";
>+    if (defined $curr_id) {
>+        $query .= " AND bug_id != $curr_id";
>+    }
>+    &::SendSQL($query);
>+    my $id = &::FetchOneColumn();

Nowhere in Bug.pm is the old notation used. Please update this part using DBI
notation as well.


>+
>+    my $vars = {};
>+    $vars->{'alias'} = $alias;
>+    if ($id) {
>+        $vars->{'bug_link'} = &::GetBugLink($id, "$id");
>+        ThrowUserError("alias_in_use", $vars);
>+    }

Useless &::


>+    return 1;
>+}

ValidateBugAlias should not returned any value, but should update the alias
name to convert it to its trimmed value:

    # Modify the calling code's original variable to contain the trimmed alias.
    $_[0] = $id;


This was a quick review, so I may have missed some other problems or nits.

Comment 17

[Albert Ting]

Attachment: update patch, v2

Addressed all the comments by Frédéric.  However, I wasn't able to remove "&::"
in the &::GetBugLink() call as the function is in globals.pl.  It's also
similar to Template.pm

Comment 18

[Dave Miller [:justdave]]

Reassigning bugs that I'm not actively working on to the default component owner
in order to try to make some sanity out of my personal buglist.  This doesn't
mean the bug isn't being dealt with, just that I'm not the one doing it.  If you
are dealing with this bug, please assign it to yourself.

Comment 19

[Frédéric Buclin]

Comment on attachment 177570 [details] [diff] [review]
update patch, v2

>--- post_bug.cgi.orig	Tue Mar 15 16:03:05 2005
>+++ post_bug.cgi	Tue Mar 15 17:08:18 2005
>+if (Param("usebugaliases")) {
>+   my $alias = trim($::FORM{'alias'} || "");
>+   if ($alias ne "") {
>+       ValidateBugAlias($alias);
>+       push (@bug_fields,"alias");
>+   }
>+}

$::FORM{'alias'} needs to be updated using its trimmed value. Add
$::FORM{'alias'} = $alias; after the validation.
Note that process_bug.cgi is not affected by this problem as $alias is saved in
the DB instead of $::FORM{'alias'} (unlike post_bug.cgi above).


>--- Bugzilla/Bug.pm.orig	Tue Mar 15 16:03:05 2005
>+++ Bugzilla/Bug.pm	Tue Mar 15 16:54:28 2005
>+# ValidateBugAlias:
>+#   Verify it's a valid bug alias and not used elsewhere.  If
>+#   curr_id is specified, verify the alias is not used for any other
>+#   bug id.  Throw an error if the alias is blank or not specified

Nit: english is not my mother language, but I would reword it a bit: "Check the
bug alias is valid and not used by another bug...". Moreover, remove the last
sentence ("Throw an error...") as this function does more than that.


>+    # Make sure the alias is unique.
>+    my $query = "SELECT bug_id FROM bugs WHERE alias = ?";
>+    if (defined $curr_id) {
>+        $query .= " AND bug_id != $curr_id";
>+    }
>+    my $sth = $dbh->prepare($query);
>+    $sth->execute($alias);
>+    my $id = $sth->fetchrow_array();

Even better: my $id = $dbh->selectrow_array($query, undef, $alias); instead of
these last 3 lines.


>+    $vars->{'alias'} = $alias;
>+    if ($id) {
>+        $vars->{'bug_link'} = &::GetBugLink($id, "$id");
>+        ThrowUserError("alias_in_use", $vars);
>+    }

Nit: useless quotes around $id in GetBugLink.


>--- template/en/default/global/user-error.html.tmpl.orig	Tue Mar 15 16:18:17 2005
>+++ template/en/default/global/user-error.html.tmpl	Tue Mar 15 16:19:57 2005
>-    [% bug_link FILTER none %] has already taken the alias 
>+    [% terms.Bug %] [% " " %] [% bug_link FILTER none %] has already taken the alias 

Write [% terms.Bug %] [%+ bug_link FILTER none %] instead.

Comment 20

[Albert Ting]

Attachment: updated patch, v3

updated patch based on the review in the previous comments.  Thanks also for
the [%+ ... %] tip.  I didn't know how to insert a blank space in the .tmpl
file.

Comment 21

[Frédéric Buclin]

Comment on attachment 179079 [details] [diff] [review]
updated patch, v3

bitrotten. %::FORM no longer exists.

Comment 22

[Albert Ting]

Attachment: v4

FORM usage changed to $cgi->param

Comment 23

[Frédéric Buclin]

Comment on attachment 181473 [details] [diff] [review]
v4

>--- post_bug.cgi.orig	Tue Mar 15 16:03:05 2005
>+++ post_bug.cgi	Tue Mar 15 17:08:18 2005
>+   if ($alias ne "") {
>+       ValidateBugAlias($alias);
>+       $cgi->param(alias => $alias);
>+       push (@bug_fields,"alias");
>+   }

We usually use $cgi->param('alias', $alias) or $cgi->param(-name => 'alias',
-value => $alias) instead. For consistency, please choose one of these two
notations.


>--- process_bug.cgi.orig	Tue Mar 15 16:03:06 2005
>+++ process_bug.cgi	Tue Mar 15 16:08:26 2005
>@@ -737,54 +737,24 @@
> 
> # If this installation uses bug aliases, and the user is changing the alias,
> # add this change to the query.
>-if (Param("usebugaliases") && defined($::FORM{'alias'})) {
>-    my $alias = trim($::FORM{'alias'});
>-    

$::FORM{} no longer exists. How can you still have it in your patch??? Please
upgrade your local installation or do a cvs diff, please.


>+        if ($alias ne "") {
>+            ValidateBugAlias($alias,$idlist[0]);
>+            $::query .= $dbh->quote($alias);

Nit: Here and there, there are missing whitespaces... ($alias, $idlist[0]).


>--- Bugzilla/Bug.pm.orig	Tue Mar 15 16:03:05 2005
>+++ Bugzilla/Bug.pm	Tue Mar 15 16:54:28 2005
>@@ -50,6 +50,7 @@
>     AppendComment
>     bug_alias_to_id
>     ValidateComment
>+    ValidateBugAlias
> );

This part has changed. Be sure to use the latest CVS version of this file.


>+sub ValidateBugAlias {
>+    my ($alias, $curr_id) = @_;
>+    my $dbh = Bugzilla->dbh;
>+
>+    trick_taint($alias);
>+    $alias = trim($alias || "");

Put trick_taint() after trim(). This way, we are sure we can't have
trick_taint(undef).


>+    # Make sure the alias is unique.
>+    my $query = "SELECT bug_id FROM bugs WHERE alias = ?";
>+    if (defined $curr_id) {
>+        $query .= " AND bug_id != $curr_id";
>+    }

Nit: Shouldn't we check for $curr_id using a detaint_natural($curr_id)? In
principle, this is safe, but we don't know how a developer could use it in the
future. So I would add this check in the IF block.


These are minor points. So the next patch should be the last one. :)

Comment 24

[Albert Ting]

Attachment: v5

Had originally tested the v4 patch on a 2 week old tip.  Surprised how quickly
Bugzilla got outdated!	But I suppose that's a good thing, lots of
enhancements. 

In any case, updated to yesterday's tip and addressed all the issues mentioned.

Comment 25

[Frédéric Buclin]

Comment on attachment 182000 [details] [diff] [review]
v5

r=LpSolit

Comment 26

[Frédéric Buclin]

Checking in post_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/post_bug.cgi,v  <--  post_bug.cgi
new revision: 1.115; previous revision: 1.114
done
Checking in process_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi
new revision: 1.252; previous revision: 1.251
done
Checking in Bugzilla/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Bug.pm,v  <--  Bug.pm
new revision: 1.76; previous revision: 1.75
done
Checking in template/en/default/global/user-error.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/user-error.html.tmpl,v
 <--  user-error.html.tmpl
new revision: 1.106; previous revision: 1.105
done