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

Re: Subscriptions feature patch

Hi Colin,

I must ashamedly admit that I haven't kept up with debbugs development
since DebConf 3. I guess I have to catch up now. I will do my best...

Am Mi, den 04.02.2004 schrieb Colin Watson um 00:17:
> 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.

> 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.
Sounds good. Who else should not be put there? The package maintainer?
We would have to compare e-mail-adresses. Should we use a module for
mormalistion or comparisation, or just use some simple regex hacks? What
modules may we use, BTW - only from stable, or also ones that are only
in unstable?

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

> 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.
My original motivation was that I might come across a case where
sendmessage is called with nothing (so that "-t" kicks in), but where I
would have to add subscribers as recipients. I am not sure if that was
the case. Maybe the auto-"-t"-feature should be dropped and the
recipient should be specified explicitly, or we fill in all addresses in
"To:" and "Bcc:" headers, but not rely on this mixture.

> We ought to think about some way to make sure that people don't get too
> many duplicate messages.
Reasonable to just compare each with every recipient, dropping all
duplications, keeping the one in "To:", if the other one is in "Bcc:".
Could reuse the code that checks if the Reply-To: is subscribed.

> > +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.
Good Point. I will check all my code for places where more
error-checking is due.

>  I think I'd prefer the e-mail address first, too.
I put it this way, because the hash or "confirmed" is sure and easy to
match, and I can use (.*) for the e-mail address (and the match would be
faster *g*). Why would you prefer it the other way?

> > +sub bugfile {

> That's getbugcomponent().
Oh, I should have seen that...

> 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.
> > -END
> > -    } elsif (m/^unsubscribe/i) {
> > -        &transcript(<<END);
> > -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.
True. Same question as above, concerning modules.

> You've duplicated confirm here.
Will fix.

> 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().

> 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.
Would you want the module to export these functions and only separate
the source, or also do namespace sparation?

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

> What if somebody tries to subscribe to the same bug twice?
(assuming that $email is the same) he will have to re-confim it, but
since we read and write the whole thread, he won't be subscribed twice.
I will see if I can find a perl module that normalises the address well.

> 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).
Will do

> > +sub sendconfirmationmail {
> > +    # Function to create a better mail asking for subscription
> Better than what? :)
I just wanted to put the text of the mail a bit separate. Currently, it
is a bit short...

Which brings my mind to: Was it ever considered to move all
e-mail-content away out of the source, e.g. in a module or using
something like TT?

> 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.
I will look into that.

> 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.
I did not yet want to change any behaviour as it is now, but then - why
not. I will look into that, too.

> 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.
Yes, that makes sense.

> 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.
What about using some serialisation module for this? I guess there are
quite a few around...

> 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.
Absolutely. I'll start right away getting used to the debbugs code base


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

Reply to: