code review initial comment/description should be editable

don't know where the attachment went, but i can post it if you need it.

I'm not sure what you mean by an updated diff email, so yes, seeing it would be helpful.

here is the mail.

Ah, this is actually a review request email, and as such there's possible overlap with bug #307461.

2009/12/8 Aaron Bentley <email address hidden>:
> Ah, this is actually a review request email, and as such there's
> possible overlap with bug #307461.

Maybe the "you have been subscribed" should say something like "...
which replaces the previous review xxxx" or something about whether
the mail is being sent actually because you've been subscribed, or
because this is a new review and you're subscribed to all of them?

--
Martin <http://launchpad.net/~mbp/>

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Martin Pool wrote:
> 2009/12/8 Aaron Bentley <email address hidden>:
>> Ah, this is actually a review request email, and as such there's
>> possible overlap with bug #307461.
>
> Maybe the "you have been subscribed"

It doesn't say "you have been subscribed", it says "You have been
requested to review..." There are no subscriptions on individual merge
proposals, which is a design decision.

> should say something like "...
> which replaces the previous review xxxx"

I suppose we could vary the message depending on whether you've
previously been asked to review, e.g. "You have been requested AGAIN to
review..."?

I don't see how that sort of thing would address your original issue
that the cover letter is stale.

> or something about whether
> the mail is being sent actually because you've been subscribed

Not possible

>, or
> because this is a new review and you're subscribed to all of them?

This mail would not be sent for that reason. It is only sent because
someone has requested you, in particular, to perform a review.

FWIW, there has been some discussion of making the cover letter an
editable field like a bug description, and that would have allowed vila
to update the cover letter as appropriate. Would that fix this bug?

Aaron
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAksdpikACgkQ0F+nu1YWqI2MzQCeJbYjZstddBZQJCDbVUoea2ep
SKwAnjXQWGR20DG/2dSKiM9BEC8T41s0
=uXmg
-----END PGP SIGNATURE-----

2009/12/8 Aaron Bentley <email address hidden>:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Martin Pool wrote:
>> 2009/12/8 Aaron Bentley <email address hidden>:
>>> Ah, this is actually a review request email, and as such there's
>>> possible overlap with bug #307461.
>>
>> Maybe the "you have been subscribed"
>
> It doesn't say "you have been subscribed", it says "You have been
> requested to review..."  There are no subscriptions on individual merge
> proposals, which is a design decision.
>
>> should say something like "...
>> which replaces the previous review xxxx"
>
> I suppose we could vary the message depending on whether you've
> previously been asked to review, e.g. "You have been requested AGAIN to
> review..."?

Sorry, "subscribe" was inaccurate. I meant distinguishing "you have
been requested to review" because ~bzr-core is asked to review all of
them vs "you personally have been requested to review", and also
distinguishing resubmitted from original mps.

> FWIW, there has been some discussion of making the cover letter an
> editable field like a bug description, and that would have allowed vila
> to update the cover letter as appropriate.  Would that fix this bug?

Something like that.

So I think this bug is: when you are requested to review a mp, you are
always sent an email with the contents of the first message, even if
there has been extensive discussion and updates to the branch such
that the first message is no longer a good representation of what's
going on.

Bugs largely avoid this by having an editable summary and description.

If there was a separate cover letter field, we would need to work out
how that meshes with the intended commit message. Perhaps it should
be cast as being supplementary to the commit message and also empty.

Also we would need some view on the ordering of events when somebody
pushes an update to the branch: if you are going to send mail about
the update, perhaps it would be best to send it when both the cover
letter and the diff have been updated, but they are done separately.
Perhaps it's enough to send them separately, but with a short delay
(as in bugs?) to give them a chance to get into the same email.

--
Martin <http://launchpad.net/~mbp/>

Martin Pool wrote:
> So I think this bug is: when you are requested to review a mp, you are
> always sent an email with the contents of the first message, even if
> there has been extensive discussion and updates to the branch such
> that the first message is no longer a good representation of what's
> going on.
>
> Bugs largely avoid this by having an editable summary and description.

That's true. Another option (which could complement editable cover
letters) might be to include an activity summary along the lines of “1
Approve, 2 Needs Fixing, and 6 Comments”. That would give you a pretty
good sense that you're diving into the middle of an active discussion,
rather than at the start. The +activereviews page has text like that
and it is a useful indication.

2009/12/8 Andrew Bennetts <email address hidden>:
> Martin Pool wrote:
>> So I think this bug is: when you are requested to review a mp, you are
>> always sent an email with the contents of the first message, even if
>> there has been extensive discussion and updates to the branch such
>> that the first message is no longer a good representation of what's
>> going on.
>>
>> Bugs largely avoid this by having an editable summary and description.
>
> That's true.  Another option (which could complement editable cover
> letters) might be to include an activity summary along the lines of “1
> Approve, 2 Needs Fixing, and 6 Comments”.  That would give you a pretty
> good sense that you're diving into the middle of an active discussion,
> rather than at the start.  The +activereviews page has text like that
> and it is a useful indication.

That would be very nice, or even more so if it said "1 Approve (spiv),
2 Needs fixing (ian-clatworthy, mbp)..."

--
Martin <http://launchpad.net/~mbp/>

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Martin Pool wrote:
> Sorry, "subscribe" was inaccurate. I meant distinguishing "you have
> been requested to review" because ~bzr-core is asked to review all of
> them

That isn't really what happens. ~bzr-core is the review team for
lp:bzr, which means that it is the *default* reviewer for all proposed
merges into lp:bzr, but it doesn't mean that it is asked to review all
branches. Even when it is, this doesn't cause its members to receive
any email.

Separately, ~bzr-core is subscribed to code review email on lp:bzr,
which means that its members receive email about code review involving
lp:bzr, whether or not they have been requested to review.

> vs "you personally have been requested to review"

This is the only case where this message is sent.

>, and also
> distinguishing resubmitted from original mps.

Oh, I thought you were talking about re-requesting a review.

> If there was a separate cover letter field, we would need to work out
> how that meshes with the intended commit message.

I don't think they're similar. They have different formats and target
audiences.

> Perhaps it should
> be cast as being supplementary to the commit message and also empty.

I think that the cover letter is essential, while the commit message is
not. Why do you think it should be supplementary?

> Also we would need some view on the ordering of events when somebody
> pushes an update to the branch: if you are going to send mail about
> the update, perhaps it would be best to send it when both the cover
> letter and the diff have been updated, but they are done separately.
> Perhaps it's enough to send them separately, but with a short delay
> (as in bugs?) to give them a chance to get into the same email.

At present, there's no message sent about updated diffs, so we can
design any such feature however we like and there's no reason to assume
that they would be done separately.

Aaron
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAksdtc0ACgkQ0F+nu1YWqI1nNQCggfoof9ZJJNTTNbbmAYzWnUGH
C8UAnjE1qvYhunC1mM+kcmWRRkPZXxzZ
=T7Ug
-----END PGP SIGNATURE-----

2009/12/8 Aaron Bentley <email address hidden>:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Martin Pool wrote:
>> Sorry, "subscribe" was inaccurate.  I meant distinguishing "you have
>> been requested to review" because ~bzr-core is asked to review all of
>> them
>
> That isn't really what happens.  ~bzr-core is the review team for
> lp:bzr, which means that it is the *default* reviewer for all proposed
> merges into lp:bzr, but it doesn't mean that it is asked to review all
> branches.  Even when it is, this doesn't cause its members to receive
> any email.
>
> Separately, ~bzr-core is subscribed to code review email on lp:bzr,
> which means that its members receive email about code review involving
> lp:bzr, whether or not they have been requested to review.

So this means they get mail starting "$person has proposed merging ..."?

>> vs "you personally have been requested to review"
>
> This is the only case where this message is sent.

>>, and also
>> distinguishing resubmitted from original mps.
>
> Oh, I thought you were talking about re-requesting a review.

I'm asking that the mail should make clear:

 * who has been asked to review it: you personally, a team to which
you belong, or something else
 * whether this is a totally new mp, or a request for a review on a
new mp, a request for additional review of something already reviewed
by other people, a request for a re-review of something I myself
looked at before, a new diff of an existing mp, or a resubmission
replacing an existing mp
 * any other variables relevant to understanding the context of the review

I am not asserting that all those cases are actually possible in
Launchpad today. However, they are all at least conceivable.

>> If there was a separate cover letter field, we would need to work out
>> how that meshes with the intended commit message.
>
> I don't think they're similar.  They have different formats and target
> audiences.

... but they're covering similar information?

>> Perhaps it should
>> be cast as being supplementary to the commit message and also empty.
>
> I think that the cover letter is essential, while the commit message is
> not.  Why do you think it should be supplementary?

Eventually, if the mp is accepted, it will have a commit message - at
least it will be put into the commit of the merge, even if Launchpad
never knows about it. So at least at that conceptual level, the
commit message is the key thing.

Things appropriate for a cover letter that might not get into the commit message

 * who you already spoke to
 * how it relates to other attempts
 * why in detail it was done this way
 * how you tested it

--
Martin <http://launchpad.net/~mbp/>

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Martin Pool wrote:
> 2009/12/8 Aaron Bentley <email address hidden>:

>> Separately, ~bzr-core is subscribed to code review email on lp:bzr,
>> which means that its members receive email about code review involving
>> lp:bzr, whether or not they have been requested to review.
>
> So this means they get mail starting "$person has proposed merging ..."?

Yes.

> I'm asking that the mail should make clear:
>
> * who has been asked to review it: you personally, a team to which
> you belong, or something else

I believe this request is based on the misapprehension that it is
possible for this kind of mail to be sent when the receipient has not
personally been asked to do the review.

I think that clarification like "You, not a team you are in, have been
requested to review..." would actually encourage people to believe that
a similar message *could* be sent when a team that they were in was
requested to perform a review.

We could tweak the wording slightly, but if people accept the plain
meaning of the phrase "You have been requested...", they will correctly
understand the situation. I don't see a great need for clarification.

> * whether this is a totally new mp, or a request for a review on a
> new mp, a request for additional review of something already reviewed
> by other people, a request for a re-review of something I myself
> looked at before, a new diff of an existing mp, or a resubmission
> replacing an existing mp

We can certainly look at providing more context here.

>>> If there was a separate cover letter field, we would need to work out
>>> how that meshes with the intended commit message.
>> I don't think they're similar. They have different formats and target
>> audiences.
>
> ... but they're covering similar information?

There is a bit of overlap in content. The commit message usually covers
only what was changed. The cover lettermay cover "what" at greater
length, why the change was made, how it was made and where it was made.

>>> Perhaps it should
>>> be cast as being supplementary to the commit message and also empty.
>> I think that the cover letter is essential, while the commit message is
>> not. Why do you think it should be supplementary?
>
> Eventually, if the mp is accepted, it will have a commit message - at
> least it will be put into the commit of the merge, even if Launchpad
> never knows about it. So at least at that conceptual level, the
> commit message is the key thing.

I don't think Code Review should consider it key if Launchpad need never
know about it. It may make sense to review the commit message at the
same time as the code, to ensure that it, too is up to quality
standards, but it's not an essential ingredient.

For code reviews of any complexity, the cover letter is at least a
helpful guide, and often an absolute necessity. I think that making
them supplementary would make code review much harder to do.

Aaron
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkse0q4ACgkQ0F+nu1YWqI2TzwCdHP5xA4pbkzQWHRfFWugofFYj
NLQAn1f/aZnCcvb/CpJPVJO1z7fbm3zE
=QhNA
-...

2009/12/9 Aaron Bentley <email address hidden>:
>> I'm asking that the mail should make clear:
>>
>>  * who has been asked to review it: you personally, a team to which
>> you belong, or something else
>
> I believe this request is based on the misapprehension that it is
> possible for this kind of mail to be sent when the receipient has not
> personally been asked to do the review.

The web interface does have a action "request review from team" so I
think it's likely that many people will believe this. Also, I think
it's a bug (possibly a complicated bug) that it doesn't
<https://bugs.edge.launchpad.net/launchpad-code/+bug/281056>.

> I think that clarification like "You, not a team you are in, have been
> requested to review..." would actually encourage people to believe that
> a similar message *could* be sent when a team that they were in was
> requested to perform a review.
>
> We could tweak the wording slightly, but if people accept the plain
> meaning of the phrase "You have been requested...", they will correctly
> understand the situation.  I don't see a great need for clarification.

Perhaps the best way to clarify it is along the lines spiv suggests:
just list all the currently pending reviews? That's useful
information anyhow.

> I don't think Code Review should consider it key if Launchpad need never
> know about it.  It may make sense to review the commit message at the
> same time as the code, to ensure that it, too is up to quality
> standards, but it's not an essential ingredient.
>
> For code reviews of any complexity, the cover letter is at least a
> helpful guide, and often an absolute necessity.  I think that making
> them supplementary would make code review much harder to do.

Maybe this should be a separate bug?

--
Martin <http://launchpad.net/~mbp/>

I've edited the summary to reflect what seems to be the most sensible solution -- hope that's ok.