296860 - support mailaliases

Status: RESOLVED WONTFIX

(Bugzilla :: User Accounts, enhancement)

Description

[Albert Ting]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.5) Gecko/20041223 Firefox/1.0
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.5) Gecko/20041223 Firefox/1.0

At my company, each employee has up to three different email accounts. 
Moreover, I'm using the bugzilla email interface.  In this situation, it gets
confusing which email address should be used for the main bugzilla account (I'm
not able to standardize on a single format).

Ideally, bugzilla ought to support N number of secondary email addresses.  The
various email fields (cc/owner/login/etc.) should also support these secondary
email addresses. 

Reproducible: Always

Comment 1

[Albert Ting]

I have an initial cut working.  You can use any of your email addresses for login.  
CCing/reassigning will work with any email alias, the same goes for the bugzilla
email interface.  

This feature is optional via a userprefs.  Plus email aliases have their own set
of emailregexp settings.  

Only drawback is that the saved queries only works with the primary email address.  

Will wait until 2.20 is ready before deploying this at my company and will
submit a patch at that time. 

Comment 2

[timeless]

patch please?

Comment 3

[Albert Ting]

I'll have this patch posted within a week (working on upgrading my company's bugzilla to the tip)

Comment 4

[Albert Ting]

Attachment: v1

I've been using my original patch for the past 1.5 years and it's worked great.  Attached is the updated version based on the 12/26/06 cvs tip.  Done some limited testing and it works so far.  

You can supply an arbitrary number of email aliases (but you have to enter them one at a time).

Comment 5

[Max Kanat-Alexander]

Comment on attachment 250244 [details] [diff] [review]
v1

>--- Bugzilla/DB/Schema.pm.alias	2005-06-21 17:11:51.000000000 -0700
>+++ Bugzilla/DB/Schema.pm	2005-06-21 19:44:39.000000000 -0700
>@@ -603,6 +603,17 @@
>         ],
>     },
> 
>+    profiles_aliases => {

  All new tables should be singular. Thus, this one should be profile_alias

>+   name => 'allowemailaliases',
>+   type => 'b',
>+   default => '0'

  Let's default this to on. Also, let's call it "allow username aliases," since these aliases should work just fine for any form of login, including LDAP.

>@@ -118,6 +124,20 @@
>   },
> 
>   {
>+   name => 'emailaliasregexp',

  I don't really want a separate regexp than the standard username regexp.

>--- userprefs.cgi.orig	2006-12-29 10:53:40.000000000 -0800
>+++ userprefs.cgi	2006-12-29 14:47:12.000000000 -0800
>@@ -138,6 +138,27 @@
>         }
>     }
> 
>+    if (Bugzila->params->{'allowemailaliases'} &&
>+        Bugzilla->params->{'allowemailchange'}) {
>+        $dbh->do("DELETE FROM profiles_aliases
>+                  WHERE  userid = ?", undef, $user->id);

  None of this code should be manually in this CGI. You should be using User->update.

  Also, if these are actually email addresses, they should be validated with the token system.

>--- editusers.cgi	2006-11-10 08:51:27.000000000 -0800
>+++ editusers.cgi	2006-12-29 15:37:27.000000000 -0800
>+    my $query = 'SELECT DISTINCT profiles.userid, login_name, realname, disabledtext ';
>+    if (Bugzilla->params->{'allowemailaliases'}) {
>+        $query .= ', group_concat(DISTINCT alias ORDER BY alias) AS alias ';

  GROUP_CONCAT is a function and thus should be capitalized. Does it exist on PostgreSQL (is it ANSI-standard)?

  Also, why not just call User->new_from_list and then you can grab the aliases there for each user in the list? I know it'd be slower, but the architecture might make more sense.

>@@ -133,6 +141,8 @@
>                 $expr = "profiles.userid";
>             } elsif ($matchvalue eq 'realname') {
>                 $expr = "profiles.realname";
>+            } elsif ($matchvalue eq 'alias_name') {
>+                $expr = "profiles_aliases.alias";

  Why even have a separate search for alias_name? Why not just include it in the normal name search?

>@@ -205,6 +218,13 @@
>         disabledtext  => scalar $cgi->param('disabledtext'),
>         disable_mail  => scalar $cgi->param('disable_mail')});
> 
>+    if (Bugzilla->params->{'allowemailaliases'}) {
>+        my @aliases = ($cgi->param('email_alias1'),);

  You don't need the comma--the parens already make it list context. Actually, just having @aliases on the left makes it list context--you don't need the parens.

>+        # force an update for the aliases
>+        $new_user->update();

  Is this just because the whole page isn't using update() yet? If it is, there should *always* only be one call to update()--did you know that it returns a hashref that's used in the results page? Or at least, that can be used in the results page.

>+        if (Bugzilla->params->{'allowemailaliases'}) {
>+            my @aliases;
>+            my $total = $cgi->param('alias_total');
>+            for (my $i=1; $i <= $total; $i++) {
>+                my $aliasid = 'email_alias' . $i;

  This looks odd to me. Shouldn't the UI for the email aliases just be a comma-separated list in a single textbox?

>@@ -151,6 +152,24 @@
>     # XXX Can update profiles_activity here as soon as it understands
>     #     field names like login_name.
> 
>+    if (Bugzilla->params->{'allowemailaliases'}) {

  Either check the parameter in the library or in the CGIs, but not in both. Having too many parameter checks makes Bugzilla code too complicated.

>+            $dbh->do('DELETE FROM profiles_aliases WHERE userid = ?',
>+                     undef, $self->id);
>+            foreach (@{$self->email_aliases}) {
>+                my $name = $_;

  Just use a loop variable instead. That's silly. :-)

>@@ -179,6 +198,33 @@
>+sub check_email_aliases_for_creation {
>+    my ($invocant, $aliases) = @_;
>+
>+    my @valid_aliases;
>+
>+    foreach (@{$aliases}) {

  What if it's undefined?

>+        my $name = $_;

  Once again, use a loop variable. (foreach my $name)

>+        $name = trim($name);
>+        next unless $name;

  Maybe somebody wants to be called "0". Who knows...

  You also need to modify check_login_name_for_creation to check the aliases table. Perhaps the code for both of these can be fused somehow, since they're so closely related?

>+    @valid_aliases = sort {$a cmp $b} (@valid_aliases);

  Nit: You don't need the parens.

>+sub email_aliases {
>+    if (!defined $_[0]->{email_aliases}) {

  Instead of using $_[0] so much, actually make an $self variable like the other complex accessors in Bugzilla do.

>+        my $dbh = Bugzilla->dbh;
>+        my $aliases = $dbh->selectcol_arrayref(
>+                                               "SELECT alias FROM profiles 
>+                                                LEFT JOIN profiles_aliases 
>+                                                ON profiles.userid = profiles_aliases.userid
>+                                                WHERE profiles.userid = ?
>+                                                ORDER BY alias",

  You don't need that join at all. Just select from the alias table using your id.

>+                                               undef, $_[0]->id);
>+        @{$_[0]->{email_aliases}} = @$aliases;
>+    }
>+    return \@{$_[0]->{email_aliases}};

  Why make that into an array just to make it back into an arrayref?

>@@ -1001,9 +1063,17 @@
>             $query .= "INNER JOIN user_group_map
>                                ON user_group_map.user_id = profiles.userid ";
>         }
>+        if (Bugzilla->params->{'allowemailaliases'}) {
>+            $query .= "LEFT JOIN profiles_aliases ON profiles.userid = profiles_aliases.userid ";
>+        }

  I think this is one of the most valid places to check the parameter, definitely.

  Do we really need a parameter, though? If people validate that an email address belongs to them, then it's theirs. Why not just always enable the feature?


  That's all the reviewing that I have time for tonight, but I bet it's enough to make a new patch with. :-)

Comment 6

[Max Kanat-Alexander]

Bugzilla 3.2 is now frozen. Only enhancements blocking 3.2 or specifically approved for 3.2 may be checked in to the 3.2 branch. If you would like to nominate your enhancement for Bugzilla 3.2, set "blocking3.2" tp "?", and either the target milestone will be changed back, or the blocking3.2 flag will be granted, if we will accept this enhancement for Bugzilla 3.2.

Comment 7

[Frédéric Buclin]

We are going to branch for Bugzilla 4.4 next week and this bug is either too invasive to be accepted for 4.4 at this point or shows no recent activity. The target milestone is reset and will be set again *only* when a patch is attached and approved.

I ask the assignee to reassign the bug to the default assignee if you don't plan to work on this bug in the near future, to make it clearer which bugs should be fixed by someone else.

Comment 8

[:glob ✱]

this is an edge-case, and this feature adds a lot of complexity for little benefit.