Hi,
I continued to hack on this:
I put the functions from errorlib into Debbugs::Errorlib, which then
exports them again. Seems to work fine. Had to change the "require
$configfile" to "do $configfile", since Debbugs::Errorlib needs to do
that, too, and it seems as if the same file can only require'ed once. I
chanced process and service to "use Debbugs::Errorlib", but have not
touched the other scripts yet.
A few "hacks" involving $main:: are still in there.
The checking whether the submitter is subscibed (with regart to
Reply-To) works fine.
The subscriptions are saved in the .summary file, but currently this
file gets read more than once, I think. I'm thinking about some magic,
that you can either pass the bugnumber ($ref) or the hash
(readbug($ref)) to the functions in Debbugs::Subscription.
Serializaion is simple: <$hash|'confirmed'><space><email-address><tab>.
All on one line - might look ugly, but technically no problem (i think).
No modification to the errorlib-code needed for this.
You can see my changes so far on: (will update that every now and then)
http://home.joachim-breitner.de/public/debbugs-subscription.diff
This is of course not finished. Especially someone should tell me where
I need to lock and where not.
nomeata
Am Mi, den 04.02.2004 schrieb Colin Watson um 00:17:
> 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 {
> > + ¬foundbug;
> > + }
> > + } 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 {
> > + ¬foundbug;
> > + }
>
> 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]
--
Joachim "nomeata" Breitner
nomeata@debian.org | ICQ# 74513189 | GPG-Keyid: 4743206C
JID: joachimbreitner@amessage.de | http://people.debian.org/~nomeata
Attachment:
signature.asc
Description: Dies ist ein digital signierter Nachrichtenteil