Closed Bug 99203 (bz-alias) Opened 23 years ago Closed 22 years ago

alias for a bug

Categories

(Bugzilla :: Bugzilla-General, enhancement)

2.10
x86
Windows 2000
enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
Bugzilla 2.18

People

(Reporter: gagan, Assigned: myk)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 11 obsolete files)

17.80 KB, patch
myk
: review+
myk
: review+
Details | Diff | Splinter Review
Need a way to address a bug using a string moniker. More specifically we'd like
to be able to add an alias to the |Depends on|, and the |Blocks| list.
Blocks: 99207
Some implementation and usability questions:

1. Can all bugs have an alias?
2 Is it first come, first serve on aliases? 
3 What does the UI look like for adding and removing an alias?
4 Do aliases get removed or retired when bugs are resolved or verified?
5 When (in buglist, show_bug, dependency_tree, etc.) does it display the number,
the alias and both.
6 Do we put a length limit on the alias?

We probably need to think about this some more.
1. Can all bugs have an alias?

Yes, why not?

2 Is it first come, first serve on aliases? 

Yes, I don't foresee great conflicts (this isn't domain name registration after
all), and the ones that do arise (who gets "xxx"?) can be resolved by discussion
in most cases, fiat in the rest.

3 What does the UI look like for adding and removing an alias?

Since the field values are arbitrary strings, I imagine the UI being a text
field on the show_bug form.

4 Do aliases get removed or retired when bugs are resolved or verified?

I don't think so.  People can retire them manually without too much trouble, and
they should stay around on some bugs.

5 When (in buglist, show_bug, dependency_tree, etc.) does it display the number,
the alias and both.

That's the 64,000$ question.

6 Do we put a length limit on the alias?

We have to put a limit on it, the question is how much.  I recommend something
small, like 20 characters.  Aliases are supposed to be easy to remember, so I
can't imagine the right limit being much more than that.

I'd like to propose that we keep the alias system simple. The association of an
alias with a bug should be totally independent from the rest of the bug system.
This way we start off with anyone being able to build an association of a bug
with an alias and also be able to retire that association. So that we can reuse
common aliases across time by applying them to different bugs-- so for example,
what is 'nsbeta' today can be another bug tomorrow (that way the history is all
preserved)   As Myk said there may or may not be a reason to keep the alias
around after the bug is fixed. 

Think of this alias as a Unix soft link :-)

>5 When (in buglist, show_bug, dependency_tree, etc.) does it display the
number, the alias and both.

Just the numbers! Keep It Simple! Additionally there should be a reverse mapping
page as well (which when fed a number would return an alias if one exists) More
importantly this functionality can be added later on if I am wrong about it
being needed right away...

IMHO the biggest reason to keep it simple is to get it rolled out faster and
have people start using such a system. 
Target Milestone: --- → Future
Re-targeting to Bugzilla 2.18 milestone, since I plan to work on these bugs
during that timeframe if not sooner.
Target Milestone: Future → Bugzilla 2.18
Attached patch patch v1: implements feature (obsolete) — Splinter Review
This patch implements this feature by modifying the ValidateBugID function
(through which all user-submitted IDs should pass before being used by Bugzilla
scripts) to check if the ID that was submitted is actually an alias.  If it is,
ValidateBugID modifies the *original* variable (not its local copy of it),
converting it from the alias to the corresponding ID.

This approach means we don't have to hack every script to recognize aliases
independently, because every script that uses ValidateBugID will automatically
get alias->id resolution.  Scripts that don't use ValidateBugID, of course,
will have to be updated, but this has to happen anyway and has already been
done for a number of scripts.

In addition to the changes to ValidateBugID, checksetup.pl has been modified to
add the "alias" column to the "bugs" table, UI has been added to bug_form.pl (I
put the field right under the bug ID row and moved the assignee field under the
reporter field to make space for it), and the corresponding code has been added
to process_bug.pl to update aliases in the database when users change them.
Comment on attachment 63807 [details] [diff] [review]
patch v1: implements feature

I haven't applied this yet, I'm just commenting on the code itsself.

>Index: bug_form.pl
>===================================================================
>RCS file: /cvsroot/mozilla/webtools/bugzilla/bug_form.pl,v
>retrieving revision 1.85
>diff -u -r1.85 bug_form.pl

You get to redo this after templatisation ;)

>Index: process_bug.cgi

>+foreach my $field ("dependson", "blocked") {
>+    if (defined($::FORM{$field}) && $::FORM{$field} ne "") {
>+        my @validvalues;
>+        foreach my $id (split(/[\s,]+/, $::FORM{$field})) {
>+            next unless $id;
>+            ValidateBugID($id);

I'm worried that this could be slow. Look at how dkl rewrote CanSeeBug on the
groups branch. You could
SELECT bug_id, alias from bugs where bug_id in (...), and nthen just remap
those bugs which return a value.

>+# If the bug has an alias, validate it for uniqueness.
>+if (defined($::FORM{'alias'})) {
>+    my $alias = trim($::FORM{'alias'});
>+    if ($alias ne "") {
>+        my $sqlalias = SqlQuote($alias);
>+        SendSQL("SELECT bug_id FROM bugs where alias = $sqlalias");

Why not use the helper func?

Also, do we want to restrict creating aliases to people with editbugs? Those
without can see the name,
but can't create a new alias.
Blocks: 132181
Comment on attachment 63807 [details] [diff] [review]
patch v1: implements feature

This is interesting. I'd be glad to test this, but not before the patch gets
pumped up to support the modern world (templates in this case).
Attachment #63807 - Flags: review-
Myk: do you still want this for the b.m.o upgrade? If so, could you please
refresh and update the patch. If not, could you remove it from the dep list of
bug 132181? Thanks :-)

Gerv
Attached patch patch v2: updated to tip (obsolete) — Splinter Review
Attachment #63807 - Attachment is obsolete: true
Keywords: patch, review
Comment on attachment 87141 [details] [diff] [review]
patch v3: here's a version with minimal impact on the "show bug" page

>+    if ($id !~ /^\s*([1-9][0-9]*)\s*$/) {
>+        $id = BugAliasToID($_[0]);
>+        if ($id) { $_[0] = $id }

Nit: can we line this up properly, please? :-)

>>-        <b>Bug#:</b>
>+        <b>Bug&nbsp;#/Alias:</b>
>       </td>
>       <td>
>         <a href="[% Param('urlbase') %]show_bug.cgi?id=[% bug.bug_id %]">
>           [% bug.bug_id %]</a>
>+          <input name="alias" value="[% bug.alias FILTER html %]" length="10" maxlength="20">
>       </td>

I suggest making it look like:

Bug#: 12345 (alias [<none>     ])

where <none> is checked for and eliminated, much like "http://" in the URL
field 
on bug entry. You could also say "a.k.a." instead of "alias". It depends which 
you think would be understood by more people.

Other than that, r=gerv.

Gerv
Attachment #87141 - Flags: review+
Attached patch patch v4: uses "a.k.a." (obsolete) — Splinter Review
This version uses "a.k.a." instead of alias, fixes a size problem (I was using
the "length" attribute instead of the "size" attribute), and adds a label and
tooltip for the form control.  I didn't do "<none>", since it seems
inconsistent with the rest of the "show bug" interface.
Attachment #87141 - Attachment is obsolete: true
testing
Alias: bugAlias
Does this allow people without editbugs to set aliases, if they're the
reporter/qa/assignee?

Should that be allowed? I thik we shouldn't, else we'll encourage lots of
'my-really-important-bug' aliases.
Alias: bugAlias → bz-alias
Comment on attachment 87164 [details] [diff] [review]
patch v4: uses "a.k.a."

The patch is also missing a call to AddFDef for the alias column - it should be
mapping "alias" -> "Alias", ie with the capital.

myk, you may have to update bmo manually, although the checksetup.pl code uses
REPLACE, so maybe not.

I don't know what the mailhead column in the fielddefs table is for, it seems
to be working on bmo now with the default of 0, though.
Attachment #87164 - Flags: review-
IMHO, this field needs to be explained (especially at first, when it's new to
people). Tooltip is good, but IMHO not good enough, mainly because tooltips do
not currently wrap in Mozilla.

What I would suggest is changin it back to "alias" and making the "alias" word a
link to bug_status.html (or similar).

P.S. The tooltip curently starts with "an name...", should probably be "A name..."
The text field should be wider, and I thunk 20 chars is not enough - it should
be 30, or so.
Attached patch patch v5: misc fixes (obsolete) — Splinter Review
This patch fixes the "an name" typo and shortens the tooltip, fixes the taint
error, and adds the FDef.

>Nit: can we line this up properly, please? :-)

The style is correct according to the Perl style guide upon which the Bugzilla
style guide is based.

>IMHO, this field needs to be explained (especially at first, when it's new to
>people).

I'll do that in my "state of b.m.o address" once the upgrade settles down.

>The text field should be wider, and I thunk 20 chars is not enough - it should

>be 30, or so.

At that point it starts to become a mini-summary and is redundant with the
summary field.	This field is designed to hold a short name that is easier to
remember and type than a number.  Twenty character names are most likely too
long already.  Users should try to keep names at ten characters or less (like
IRC nicks), hence the size of the form field.
Attachment #87164 - Attachment is obsolete: true
Hmm. I guess - we'll see how it goes.

You need to enforce that limit in process_bug, though, and error out if someon
tries an entry which is too big. I don't know if validatebugid needs the change
- can I query for a varchar(20 column with a 21 character string?
> At that point it starts to become a mini-summary and is redundant
> with the summary field.  This field is designed to hold a short name
> that is easier to remember and type than a number.

So?  Users are quite capable of doing this themselves without directives from up
high.

> Twenty character names are most likely too long already.  Users should
> try to keep names at ten characters or less (like IRC nicks), hence the
> size of the form field.

This is not true, I wanted to put bz-sanity-check-cleanup on a bug but I had to
reduce it to bz-sancheckclean.  It is smaller names that are harder to remember,
because you have to use abbreviations.  It shouldn't suprise anyone that bug
numbers are the hardest to remember and they are also the shortest.
Depends on: 150831
No longer depends on: 150831
Depends on: 151018
Attached patch patch v6: adds validation (obsolete) — Splinter Review
Aliases shouldn't be able to contain commas or spaces or be all numbers.
Attachment #87187 - Attachment is obsolete: true
Comment on attachment 87309 [details] [diff] [review]
patch v6: adds validation

process_bug still doesn't check that the alias is the right length.

You should also add teh alias to xml.cgi/Bug.pm and bugzilla.dtd
Attachment #87309 - Flags: review-
Blocks: 151413
Attached patch patch v7: review fixes (obsolete) — Splinter Review
>You need to enforce that limit in process_bug, though, and error out if someon

>tries an entry which is too big.

Done.


> I don't know if validatebugid needs the change
>- can I query for a varchar(20 column with a 21 character string?

Yup.


>So?  Users are quite capable of doing this themselves without directives from
up
>high.

By this logic we should have no restrictions on data entry in any field.
Restrictions are logical ways to structure reports in a useful way.


>It is smaller names that are harder to remember,
>because you have to use abbreviations.

Abbreviations, like names, are mnemonic, which is what's important 
about aliases, not that they are comprehensive.

>It shouldn't suprise anyone that bug
>numbers are the hardest to remember and they are also the shortest.

Bug numbers are hard to remember because they are arbitrary, not because
they are short.  f.e. I haven't forgotten "bz-perf" since I added it
to the Bugzilla performance meta-bug, even though it's much shorter
(and less explanatory) than "bz-performance".

>You should also add teh alias to xml.cgi/Bug.pm and bugzilla.dtd

Done.

I also increased the size of the field to 20 characters, since otherwise users
will see only a portion of long aliases and may think the alias is the
truncated version they see.
Attachment #87309 - Attachment is obsolete: true
Comment on attachment 87606 [details] [diff] [review]
patch v7: review fixes

>Index: Bug.pm
>===================================================================
>RCS file: /cvsroot/mozilla/webtools/bugzilla/Bug.pm,v
>retrieving revision 1.12
>diff -u -r1.12 Bug.pm
>--- Bug.pm	27 Apr 2002 06:25:41 -0000	1.12
>+++ Bug.pm	14 Jun 2002 00:57:50 -0000
>@@ -33,7 +33,7 @@
> use CGI::Carp qw(fatalsToBrowser);
> my %ok_field;
> 
>-for my $key (qw (bug_id product version rep_platform op_sys bug_status 
>+for my $key (qw (bug_id alias product version rep_platform op_sys bug_status 
>                 resolution priority bug_severity component assigned_to
>                 reporter bug_file_loc short_desc target_milestone 
>                 qa_contact status_whiteboard creation_ts groupset 
>@@ -108,7 +108,7 @@
> 

You added alias first here, but...

>-<!ELEMENT bug (bug_id, (bug_status, product, priority, version, rep_platform, assigned_to, delta_ts, component, reporter, target_milestone?, bug_severity, creation_ts, qa_contact?, op_sys, resolution?, bug_file_loc?, short_desc?, keywords*, status_whiteboard?, dependson*, blocks*, cc*, long_desc*, attachment*)?)>
>+<!ELEMENT bug (bug_id, (bug_status, product, priority, version, rep_platform, assigned_to, delta_ts, component, reporter, target_milestone?, bug_severity, creation_ts, qa_contact?, op_sys, resolution?, bug_file_loc?, short_desc?, keywords*, status_whiteboard?, dependson*, blocks*, cc*, long_desc*, attachment*, alias?)?)>

added it to the end here (bugzilla.dtd)

Thats invalid XML. The alias should probably be after the bug_id, for
consistency

Also, I'm beginning to wonder if the use of aliases shouldn't be controlled by
a param - some installations may not want this.
Attachment #87606 - Flags: review-
Attached patch patch v8: review fixes (obsolete) — Splinter Review
>Thats invalid XML. The alias should probably be after the bug_id, for
>consistency

Done.

>Also, I'm beginning to wonder if the use of aliases shouldn't be controlled by

>a param - some installations may not want this.

[echoes of mpt in my ears] Done.

This patch also fixes the adding/removing dependencies problem.
Attachment #87606 - Attachment is obsolete: true
The latest patch also changes "a.k.a." to alias.  The word "alias" is used in
error messages and other places, since it's much easier to work into sentences
than "a.k.a.", and it makes sense to be consistent even though the style of
"a.k.a." is very nice.
Comment on attachment 87610 [details] [diff] [review]
patch v8: review fixes

aka is still there - in fact, this looks identical to the previous patch (the
xml fu isn't fixed, there isn't a param, etc)

Also, the reporter can still change the alias. Do we want to let that go for
now, and deal with it in that other bug we're takling about this in?
Attachment #87610 - Flags: review-
Attached patch patch v9: review fixes (obsolete) — Splinter Review
Hmm, perhaps I submitted the wrong patch.  This should be the right one.
Attachment #87610 - Attachment is obsolete: true
Let's deal with the reporter problem in the other bug.
Comment on attachment 87614 [details] [diff] [review]
patch v9: review fixes

You're going to hate this, but:

>Index: checksetup.pl
>===================================================================
>RCS file: /cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v
>retrieving revision 1.155
>diff -u -r1.155 checksetup.pl
>--- checksetup.pl	4 Jun 2002 05:47:28 -0000	1.155
>+++ checksetup.pl	14 Jun 2002 02:08:32 -0000
>     index (delta_ts),
>@@ -1348,7 +1349,9 @@
>     index (resolution),
>     index (target_milestone),
>     index (qa_contact),
>-    index (votes)';
>+    index (votes),
>+    
>+    unique(alias)';
> 

You can't do this as-is. This creates a 17th index on the bugs table, and ISAM
tables only allow 16. Before doing this, we'll have to 'fix' the bug for
updating our mysql version to at least 3.23.6, and require myisam table
formats. The first is ok (We probably want to push it up higher, later) and
given that I don't see a problem with the second.

I can't find the postgres limit in a quick glance through its manual. We may
eventually remove the votes index (its only used for searching, and we have to
handle the mutli-grouping fu differnetly for attachment statuses anyway). We
should ask dkl what the postgres/oracle/etc limit is, though, first.

>Index: xml.cgi
>===================================================================
>RCS file: /cvsroot/mozilla/webtools/bugzilla/xml.cgi,v
>retrieving revision 1.6
>diff -u -r1.6 xml.cgi
>--- xml.cgi	27 Apr 2002 06:25:41 -0000	1.6
>+++ xml.cgi	14 Jun 2002 02:08:32 -0000
>@@ -49,7 +49,8 @@
> print Bug::XML_Header(Param("urlbase"), $::param{'version'}, 
>                       Param("maintainer"), $exporter);
> foreach my $id (@ids) {
>-  my $bug = new Bug(trim($id), $::userid);
>+  ValidateBugID($id);
>+  my $bug = new Bug($id, $::userid);
>   print $bug->emitXML;
> }

You can't do this; xml.cgi must let Bug.pm handle validation so that the error
messages are in xml.
Attachment #87614 - Flags: review-
dkl - please see the above, and comment on the max number of keys/table in
postgres/oracle/sybase/etc.

We also need to check the limits for the other mysql table types (innodb, in
particular)
http://www.au.postgresql.org/users-lounge/limitations.html:

"Maximum number of indexes on a table: unlimited"

That answers that question...
Oh, and you need Param checks in process_bug, and bugaliastoid should return
undef if not Param("..."); 
Attached patch patch v10: review fixes (obsolete) — Splinter Review
>Before doing this, we'll have to 'fix' the bug for
>updating our mysql version to at least 3.23.6, and require myisam table
>formats. 

Done and done.

>You can't do this; xml.cgi must let Bug.pm handle validation so that the error

>messages are in xml.

Done.

>Oh, and you need Param checks in process_bug, and bugaliastoid should return
>undef if not Param("..."); 

Done.
Attachment #87614 - Attachment is obsolete: true
Attached patch patch v11: fixes bug 155460 (obsolete) — Splinter Review
Attachment #88561 - Attachment is obsolete: true
Comment on attachment 90004 [details] [diff] [review]
patch v11: fixes bug 155460

This seems to have been working on bmo for a while, and the patch looks OK to
me now.

In checksetup.pl, move the 'alias' field addition to the bottom, and change the
date to whenever you check it in.

r=bbaetz
Attachment #90004 - Flags: review+
Comment on attachment 90004 [details] [diff] [review]
patch v11: fixes bug 155460

>       # no bug number given

Add "or the alias didn't match a bug" (or something similar).

>+sub BugAliasToID {
>+    # Queries the database for the bug with a given alias, and returns
>+    # the ID of the bug if it exists.

... "and undef if it doesn't."

>+              The bug ID <em>$htmlid</em> is invalid (i.e. it is neither
>+              a bug ID nor an alias to a bug ID). If you are trying to use 

Note that this part mentions bug aliases even if aliases are disabled. Consider
whether or not this should be fixed. 

>+DefParam("use_bug_aliases",

Params haven't traditionally had underscores in them. Unless there is a good
reason, remove them?

>+    # Since aliases are unique (like bug numbers), they can only be changed
>+    # for one bug at a time, so ignore the alias change unless only a single
>+    # bug is being changed.

Unless the value is being set to empty, but maybe we don't need to care about
this right now.


With those addressed, r=jouni.
Attachment #90004 - Flags: review+
bbaetz:

>In checksetup.pl, move the 'alias' field addition to the bottom, and change
the
>date to whenever you check it in.

Done.


jouni:

>Add "or the alias didn't match a bug" (or something similar).

Done.

>... "and undef if it doesn't."

Done.

>Note that this part mentions bug aliases even if aliases are disabled.
Consider
>whether or not this should be fixed. 

Fixed.

>Params haven't traditionally had underscores in them. Unless there is a good
>reason, remove them?

There are advantages to underscores, but we should probably switch en masse
rather than only for new parameters.  Removed underscores and created bug
155628
for this issue.

>>+    # Since aliases are unique (like bug numbers), they can only be changed
>>+    # for one bug at a time, so ignore the alias change unless only a single

>>+    # bug is being changed.
>
>Unless the value is being set to empty, but maybe we don't need to care about
>this right now.

Right.
Attachment #90004 - Attachment is obsolete: true
Comment on attachment 90089 [details] [diff] [review]
patch v12: review fixes

Comments indicate this inherits reviews.
Attachment #90089 - Flags: review+
Checking in Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bug.pm,v  <--  Bug.pm
new revision: 1.13; previous revision: 1.12
done
Checking in CGI.pl;
/cvsroot/mozilla/webtools/bugzilla/CGI.pl,v  <--  CGI.pl
new revision: 1.160; previous revision: 1.159
done
Checking in bug_form.pl;
/cvsroot/mozilla/webtools/bugzilla/bug_form.pl,v  <--  bug_form.pl
new revision: 1.95; previous revision: 1.94
done
Checking in bugzilla.dtd;
/cvsroot/mozilla/webtools/bugzilla/bugzilla.dtd,v  <--  bugzilla.dtd
new revision: 1.6; previous revision: 1.5
done
Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.158; previous revision: 1.157
done
Checking in defparams.pl;
/cvsroot/mozilla/webtools/bugzilla/defparams.pl,v  <--  defparams.pl
new revision: 1.76; previous revision: 1.75
done
Checking in process_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi
new revision: 1.130; previous revision: 1.129
done
Checking in template/en/default/bug/edit.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/edit.html.tmpl,v  <--
 edit.html.tmpl
new revision: 1.11; previous revision: 1.10
done
Resolving fixed.
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Has the textbox been made wider than the b.m.o. version? That's my current gripe
with the alias system :-) (Also the fact that it and QuickSearch don't yet play
nicely together.)

I still also think that "a.k.a." is the right styling for the show_bug page,
even though it's not the same as the error usage. "a.k.a." and "alias" are
interchangeable words, but a.k.a. has less other meanings.

Gerv
>I still also think that "a.k.a." is the right styling for the show_bug page,
>even though it's not the same as the error usage. "a.k.a." and "alias" are
>interchangeable words, but a.k.a. has less other meanings.

I'd go for alias nonetheless, since a.k.a. is pretty hard for non-native English
speakers. The word alias is used as such in many languages and even when not,
most people understand it because they've heard it before anyway.
this causes multiple taint errirs in show_bug.cgi and process_bug.cgi

perl is : This is perl, version 5.005_03 built for i386-freebsd  

fisrt error is in CGI.pl line 305 

SendSQL("SELECT bug_id FROM bugs WHERE bug_id = $id");   call to DB with tainted
data

$id used to be detaint_natural before the call

changing that to : SendSQL("SELECT bug_id FROM bugs WHERE bug_id = " .
SqlQuote($id)); 
 fixed that error but next fails in global.pl line 800

 Attempted to send tainted string 'SELECT bugs.bug_id FROM bugs LEFT JOIN cc
selectVisible_cc ON bugs.bug_id = selectVisible_cc.bug_id AND
selectVisible_cc.who = 1 WHERE ((bugs.groupset & 9223372036854775807) =
bugs.groupset OR (bugs.reporter_accessible = 1 AND bugs.reporter = 1) OR
(bugs.cclist_accessible = 1 AND selectVisible_cc.who = 1 AND not
isnull(selectVisible_cc.who)) OR (bugs.assigned_to = 1)) AND bugs.bug_id = 2597'
to the database at globals.pl line 260.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
daa@distributed.net:

Is the problem you reported covered by bug 155700 or is it a separate issue? It
would seem to me that these are the same issue.
I think it is...  I'd also rather have this dealt with on a separate bug... 
trying to keep track of what patches are actually ready to check in is difficult
when we have a reviewed and checked in patch on a reopened bug.  The aliases
feature is implimented.  That fixes this bug.  Other errors caused by it are new
bugs.
Status: REOPENED → RESOLVED
Closed: 22 years ago22 years ago
Resolution: --- → FIXED
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: