[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

Re: Subscriptions feature patch

On Thu, Jul 17, 2003 at 12:47:57PM +0200, Joachim Breitner wrote:
> Attached is a patch that adds a subscription feature to the bts that
> allows you to subscribe to single bugs, and then you will recieve all
> with that bug related mails. (as agreed with aj and colin) It required
> confirmation. The interface is only in request@.

OK, let's (finally!) have a look through this:

> diff -ru source-alt/debian/changelog source/debian/changelog
> --- source-alt/debian/changelog	2003-06-25 19:51:51.000000000 +0200
> +++ source/debian/changelog	2003-07-15 11:14:12.000000000 +0200
> @@ -1,3 +1,9 @@
> +debbugs (2.4.3) UNRELEASED; urgency=low
> +
> +  * Added bug subscription (nomeata)
> +
> + -- root <mail@joachim-breitner.de>  Tue, 15 Jul 2003 11:14:12 +0200
> +
>  debbugs (2.4.2) UNRELEASED; urgency=low
>  
>    * Colin Watson:

Heh, don't put extra UNRELEASED entries on top of existing ones.

> diff -ru source-alt/scripts/process.in source/scripts/process.in
> --- source-alt/scripts/process.in	2003-06-23 13:23:35.000000000 +0200
> +++ source/scripts/process.in	2003-07-16 09:20:08.000000000 +0200
[...]
> @@ -398,7 +398,7 @@
>  END
>              &htmllog("Notification","sent",$data->{originator}, 
>  		"$gBug acknowledged by developer.");
> -            &sendmessage(<<END.join("\n",@msg),'');
> +            &sendmessage(<<END.join("\n",@msg),undef,undef,&subscribers);
>  From: $gMaintainerEmail ($gProject $gBug Tracking System)
>  To: $data->{originator}
>  Subject: $gBug#$ref acknowledged by developer

Subscribers should only get one of "acknowledged by developer" or
"marked as done", not both. The PTS gets "marked as done", but actually
I think in this case it might be better to send subscribers
"acknowledged by developer", since they tend to have a more user-like
role than people who subscribe to a whole package.

> @@ -426,7 +426,7 @@
>  (administrator, $gProject $gBugs database)
>  
>  END
> -        }
> +        };
>  	&appendlog;
>      }
>      &finish;

Huh?

> @@ -633,7 +633,7 @@
>  
>  if ($codeletter eq 'U') {
>      &htmllog("Message", "sent on", $data->{originator}, "$gBug#$ref.");
> -    &sendmessage(<<END,[$data->{originator},@resentccs],[@bccs]);
> +    &sendmessage(<<END,[$data->{originator},@resentccs],[@bccs],&subscribers);
>  Subject: $gBug#$ref: $newsubject
>  Reply-To: $replyto, $ref-quiet\@$gEmailDomain
>  ${orgsender}Resent-To: $data->{originator}
> @@ -651,7 +651,7 @@
>               "<code>$gBug#$ref</code>".
>               (length($data->{package})? "; Package <code>".&sani($data->{package})."</code>" : '').
>               ".");
> -    &sendmessage(<<END,["$gSubmitList\@$gListDomain",@resentccs],[@bccs]);
> +    &sendmessage(<<END,["$gSubmitList\@$gListDomain",@resentccs],[@bccs],&subscribers);
>  Subject: $gBug#$ref: $newsubject
>  Reply-To: $replyto, $ref\@$gEmailDomain
>  Resent-From: $header{'from'}

It seems that it would sometimes make sense for the Reply-To: header to
be changed. For instance, if $replyto is already subscribed to the bug,
or is the submitter, then you don't want to direct people to cc that
person again.

> @@ -681,7 +681,7 @@
>                   (length($data->{package}) ? "; Package <code>".&sani($data->{package})."</code>" : '').
>                   ".");
>      }
> -    &sendmessage(<<END,[@resentccs],[@bccs]);
> +    &sendmessage(<<END,[@resentccs],[@bccs],&subscribers);
>  Subject: $gBug#$ref: $newsubject
>  Reply-To: $replyto, $ref-$baddressroot\@$gEmailDomain
>  Resent-From: $header{'from'}

This is the -maintonly/-quiet case, and subscribers shouldn't receive
messages from here.

> @@ -953,8 +953,8 @@
>  }
>  
>  sub sendmessage {
> -    local ($msg,$recips,$bcc) = @_;
> -    if ((!ref($recips) && $recips eq '') || @$recips == 0) {
> +    local ($msg,$recips,$bcc,$subscr) = @_;
> +    if ((!ref($recips) && $recips eq '') || @$recips == 0 || !defined $recips) {
>          $recips = ['-t'];
>      }
>      $msg = "X-Loop: $gMaintainerEmail\n" . $msg;
> @@ -972,6 +972,11 @@
>          push @$recips, @$bcc;
>      }
>  
> +    if (ref($subscr)) {	# separate from bcc in case this need special handling
> +        shift @$recips if $recips->[0] eq '-t'; 
> +        push @$recips, @$subscr;
> +    }
> +
>  #if debugging.. save email to a log
>  #    open AP, ">>debug";
>  #    print AP join( '|', @$recips )."\n>>";

I think we can just put the subscribers into @$bcc, really. If they turn
out to need special handling later than we can separate them out then,
but for now I'd just keep the interface simple.

We ought to think about some way to make sure that people don't get too
many duplicate messages.

> @@ -1088,3 +1093,31 @@
>          }
>      }
>  }
> +
> +
> +sub subscribers {
> +    # Returns a list of sucessfully subscribed email addresses
> +    return () unless -e bugfile("subs");
> +    my $data = subfile_load(bugfile("subs"));
> +    [ grep {$data->{$_} eq "confirmed"} keys %$data ];
> +}
> +
> +sub subfile_load {
> +    # Loads all subscriptions from the file into a hashref
> +    my $file = shift;
> +    my $data = {};
> +    open SUBFILE,"<$file" || &quit("open $file: $!");
> +    while (<SUBFILE>) {
> +      m/^([a-z0-9]+)\s+(.*)$/;

Should check whether this match succeeded. I think I'd prefer the e-mail
address first, too.

> +      $data->{$2} = $1;
> +    }
> +    close SUBFILE;
> +    return $data;
> +}
> +
> +sub bugfile {
> +    # Return the path from the spool dir to the bug file with the given extention
> +    my $ext = shift;
> +    my $hash = get_hashname($ref);
> +    return "db-h/$hash/$ref.$ext";
> +}

That's getbugcomponent().

subfile_load is common with service.in, so instead of duplicating code
this should live in errorlib.in.

> diff -ru source-alt/scripts/service.in source/scripts/service.in
> --- source-alt/scripts/service.in	2003-06-23 13:23:35.000000000 +0200
> +++ source/scripts/service.in	2003-07-16 09:23:32.000000000 +0200
[...]
> @@ -214,18 +215,58 @@
>          $ok++;
>      } elsif (m/^refcard/i) {
>          &sendtxthelp("bug-mailserver-refcard.txt","mail servers' reference card");
> -    } elsif (m/^subscribe/i) {
> -        &transcript(<<END);
> -There is no $gProject $gBug mailing list.  If you wish to review bug reports
> -please do so via http://$gWebDomain/ or ask this mail server
> -to send them to you.
> -soon: MAILINGLISTS_TEXT
> -END
> -    } elsif (m/^unsubscribe/i) {
> -        &transcript(<<END);
> -soon: UNSUBSCRIBE_TEXT
> -soon: MAILINGLISTS_TEXT
> -END
> +    } elsif (m/^subscribe\s+\#?(-?\d+)(?:\s+(\w*))?/i) {
> +        $ok++;
> +        $ref = $1;
> +        $email = $2 || ($replyto =~ /<(.*@.*)>/)[0];

The e-mail address might not be "foo <bar@baz>"; it could be "bar@baz"
or "bar@baz (foo)". I think there are Perl modules to handle details
like this for you.

> +    } elsif (m/^confirm\s+\#?(-?\d+)\s+([a-z0-9]*)/i) {
> +        $ok++;
> +        $ref = $1;
> +        $hash = $2;
> +        if ($ref =~ m/^-\d+$/ && defined $clonebugs{$ref}) {
> +            $ref = $clonebugs{$ref};
> +        }
> +        if (-e bugfile("report")) {
> +	    &foundbug;
> +            &confirm;
> +        } else {
> +            &notfoundbug;
> +	}
> +    } elsif (m/^unsubscribe\s+\#?(-?\d+)(?:\s+(\w*))?/i) {
> +        $ok++;
> +        $ref = $1;
> +        $email = $2 || ($replyto =~ /<(.*@.*)>/)[0];

See above.

> +    } elsif (m/^confirm\s+\#?(-?\d+)\s+([a-z0-9]*)/i) {
> +        $ok++;
> +        $ref = $1;
> +        $hash = $2;
> +        if ($ref =~ m/^-\d+$/ && defined $clonebugs{$ref}) {
> +            $ref = $clonebugs{$ref};
> +        }
> +        if (-e bugfile("report")) {
> +	    &foundbug;
> +            &confirm;
> +        } else {
> +            &notfoundbug;
> +	}

You've duplicated confirm here.

> @@ -699,6 +742,13 @@
>      $midix++;
>  }
>  
> +sub sendsubscriptions {
> +    # Extra funcion in case we want to change the text.
> +    local ($ref, $text, $title) = @_;
> +    my @subscribers = subscribers($ref);
> +    sendmailmessage($text,@subscribers) if @subscribers;
> +}
> +

As with my sendmessage() comment above, I think this is overengineered.
It's fine (and, in fact, desirable) for subscribers to receive what the
package maintainer sees, so it can just go in sendmailmessage().

> @@ -1077,3 +1127,101 @@
>      $ok++;
>      &transcript("\n");
>  }
> +
> +sub subscribe {
> +    # Enters  a new address to the subs file and creates the confirmation hash
> +    my $bhash = get_hashname($ref);
> +    my $subfile = bugfile("subs");
> +    unless (-e $subfile) {
> +	open TMP, ">$subfile" || &quit("create $subfile: $!"); close TMP
> +    }
> +    &filelock($subfile."lock") || &quit("lock $subfile: $!");
> +    my $subs = subfile_load($subfile);
> +    my $chash = md5_hex($email.$$);
> +    $subs->{$email} = $chash;
> +    subfile_write($subfile,$subs);
> +    &unfilelock;
> +    &sendconfirmationmail($chash);
> +}
> +

I sort of feel the need for a Debbugs::Subscriptions package here. I
know modularization isn't exactly traditional in debbugs, but I wouldn't
mind starting here ... However, I found Debbugs::Status a bit difficult
to write because I needed to push all the file-finding functions out to
Debbugs::Files or something, so I suppose we can do that later.

I would prefer this function, and others that write files, to write to a
temporary file and then rename it into place.

What if somebody tries to subscribe to the same bug twice?

It's probably better for locks to live in the 'lock' directory like all
the others.

> +sub subfile_write {
> +    my $file = shift;
> +    my $data = shift;
> +    open SUBFILE,">$file" || &quit("write $file: $!");
> +    while (my ($email,$hash) = each %$data) {
> +      print SUBFILE "$hash $email\n";
> +    }
> +    close SUBFILE;
> +}

You don't check for errors on print or close here (so you'll miss ENOSPC
conditions).

> +sub sendconfirmationmail {
> +    # Function to create a better mail asking for subscription

Better than what? :)


You don't seem to have arranged for subscribers to receive copies of
control@ acknowledgements. I think this is a mistake: one good thing
about bug subscriptions (although some might disagree when in a sneaky
mood :)) are that we can arrange for submitters (by default) to be told
about changes to things like severity and tags.

One thing you haven't addressed at all (perhaps deliberately) is the
question of default subscriptions. It's not at all unreasonable to
suggest that the submitter of a bug ought to be subscribed to it by
default, and can either unsubscribe from it later or set some
X-Debbugs-* header in the initial mail if this isn't desired. This way,
we can start assuming that the submitter generally gets told about state
changes to a bug, which I think would be an improvement.

Since you wrote your original patch, we have extensible metadata in the
form of .summary files. I think it would be reasonable to use those
rather than having a separate file, which will also give you the benefit
of the I/O functions already existing. However, somebody will have to
work out the field format: hashes as field values aren't currently
supported, although there's no reason why they couldn't be made to work.
Perhaps a one-space indent followed by
escaped-key+whitespace+escaped-value or something like that would be OK.


Overall, this wasn't a bad effort, and I'm sorry it took us so long to
get round to reviewing it. I think it needs a couple more iterations and
a bit of discussion, but it should provide an excellent starting point.

Thanks,

-- 
Colin Watson                                  [cjwatson@flatline.org.uk]
Reply to: