Closed Bug 248386 Opened 17 years ago Closed 16 years ago

Add support for Alias to post_bug.cgi


(Bugzilla :: Creating/Changing Bugs, defect)

Not set



Bugzilla 2.20


(Reporter: justdave, Assigned: altlist)




(2 files, 6 obsolete files)

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.
Attached patch Patch v1Splinter Review
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.
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
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.
Attached patch 2.17.7 patch (obsolete) — Splinter Review
Based on the previous comments, I've moved the alias checking code from
process_bug.cgi to, then updated post_bug.cgi accordingly. 
Combined with bug #165022, this works as expected.
Comment on attachment 151962 [details] [diff] [review]
2.17.7 patch

I'll veto anything that adds functions to  We're trying to
eliminate that file.  Anything that's shared needs to be in one of the .pm
files.	Bugzilla/ 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
(In reply to comment #5)

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

Anyone have opionions where this function should go? (see comment 5)
(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.
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

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'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.
Attached patch updated 2.18rc1 patch (obsolete) — Splinter Review
Enclosed is an updated 2.18RC1 patch.  Per your suggestions, I moved
the ValidateAlias routine into Bugzilla/  I also changed the
approach so that the modifications to post_bug.cgi were kept to a
Attachment #151962 - Attachment is obsolete: true
Attachment #153823 - Flags: review?
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 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

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

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

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

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

ThrowUserError does not require &::
Attachment #153823 - Flags: review? → review-
Attached patch update 2.19.2 patch (obsolete) — Splinter Review
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.
Attachment #153823 - Attachment is obsolete: true
Comment on attachment 176062 [details] [diff] [review]
update 2.19.2 patch

If you want your patch to be reviewed, you have to ask. ;)
Attachment #176062 - Flags: review?
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 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
Maybe should we export ValidateBugAlias in, 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.
	$::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 "") {

>+    # 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 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.
Attachment #176062 - Flags: review? → review-
Attached patch update patch, v2 (obsolete) — Splinter Review
Addressed all the comments by Frédéric.  However, I wasn't able to remove "&::"
in the &::GetBugLink() call as the function is in  It's also
similar to
Attachment #176062 - Attachment is obsolete: true
Attachment #177570 - Flags: review?(LpSolit)
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.
Assignee: justdave → create-and-change
QA Contact: mattyt-bugzilla → default-qa
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/	Tue Mar 15 16:03:05 2005
>+++ Bugzilla/	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.
Attachment #177570 - Flags: review?(LpSolit) → review-
Assignee: create-and-change → altlst
Target Milestone: --- → Bugzilla 2.20
Attached patch updated patch, v3 (obsolete) — Splinter Review
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
Attachment #177570 - Attachment is obsolete: true
Attachment #179079 - Flags: review?(LpSolit)
Comment on attachment 179079 [details] [diff] [review]
updated patch, v3

bitrotten. %::FORM no longer exists.
Attachment #179079 - Flags: review?(LpSolit) → review-
Attached patch v4 (obsolete) — Splinter Review
FORM usage changed to $cgi->param
Attachment #179079 - Attachment is obsolete: true
Attachment #181473 - Flags: review?(LpSolit)
Comment on attachment 181473 [details] [diff] [review]

>--- 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

>--- 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/	Tue Mar 15 16:03:05 2005
>+++ Bugzilla/	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

>+    # 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. :)
Attachment #181473 - Flags: review?(LpSolit) → review-
Attached patch v5Splinter Review
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

In any case, updated to yesterday's tip and addressed all the issues mentioned.
Attachment #181473 - Attachment is obsolete: true
Attachment #182000 - Flags: review?(LpSolit)
Comment on attachment 182000 [details] [diff] [review]

Attachment #182000 - Flags: review?(LpSolit) → review+
Flags: approval?
Flags: approval? → approval+
Checking in post_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/post_bug.cgi,v  <--  post_bug.cgi
new revision: 1.115; previous revision: 1.114
Checking in process_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi
new revision: 1.252; previous revision: 1.251
Checking in Bugzilla/;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/,v  <--
new revision: 1.76; previous revision: 1.75
Checking in template/en/default/global/user-error.html.tmpl;
 <--  user-error.html.tmpl
new revision: 1.106; previous revision: 1.105
Closed: 16 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.