Add message from upload.py when updating an issue (fixes issue351).

Add message from upload.py when updating an issue (fixes issue351).

[techtonik, 2012-02-21 08:15:05]

The code seems to do what it is aimed for, but I'd like to throw in a proposal
to make it more explicit.

http://codereview.appspot.com/5687062/diff/1/codereview/views.py
File codereview/views.py (right):

http://codereview.appspot.com/5687062/diff/1/codereview/views.py#newcode1497
codereview/views.py:1497: # Create (and send) a message if needed.
It is not clear what is the point in creating message, but not sending it? An
ideal pseudocode is something like:
# Create message
# Post message if there is a message
# Email message if 'send_mail' is set

The people who are reading this code are asking:
- What is sent if there is no message?
- What does _notify_issue() does?

I remember that empty replies are inserted on issue update. It doesn't help
much. It would be nice to insert the phrase "XXX added Patch Set NNN" if we add
these messages anyway.

[Andi Albrecht, 2012-02-21 12:05:49]

On Tue, Feb 21, 2012 at 9:15 AM,  <techtonik@gmail.com> wrote:
> The code seems to do what it is aimed for, but I'd like to throw in a
> proposal to make it more explicit.
>
>
> http://codereview.appspot.com/5687062/diff/1/codereview/views.py
> File codereview/views.py (right):
>
> http://codereview.appspot.com/5687062/diff/1/codereview/views.py#newcode1497
> codereview/views.py:1497: # Create (and send) a message if needed.
> It is not clear what is the point in creating message, but not sending
> it? An ideal pseudocode is something like:
> # Create message
> # Post message if there is a message
> # Email message if 'send_mail' is set
>
> The people who are reading this code are asking:
> - What is sent if there is no message?
> - What does _notify_issue() does?
>
> I remember that empty replies are inserted on issue update. It doesn't
> help much. It would be nice to insert the phrase "XXX added Patch Set
> NNN" if we add these messages anyway.

I'd prefer just to make the "-m" switch work as expected. We can do
the server side refactorings later.

--
Andi

>
> http://codereview.appspot.com/5687062/

[M-A, 2012-02-21 14:41:51]

On 2012/02/21 12:05:49, Andi Albrecht wrote:
> I'd prefer just to make the "-m" switch work as expected. We can do
> the server side refactorings later.

Sadly our code misinterprets -m for -t on patchset uploads. I could fix that.

M-A

[Andi Albrecht, 2012-02-21 14:50:33]

http://codereview.appspot.com/5687062/diff/1/upload.py
File upload.py (right):

http://codereview.appspot.com/5687062/diff/1/upload.py#newcode2261
upload.py:2261: message = message or title
@M-A: Do we mix up message and title here?

[M-A, 2012-02-21 20:52:37]

On 2012/02/21 14:50:33, Andi Albrecht wrote:
> http://codereview.appspot.com/5687062/diff/1/upload.py
> File upload.py (right):
> 
> http://codereview.appspot.com/5687062/diff/1/upload.py#newcode2261
> upload.py:2261: message = message or title
> @M-A: Do we mix up message and title here?

Yes. Let me fix our tools first.

[techtonik, 2012-02-21 23:07:43]

On 2012/02/21 14:41:51, M-A wrote:
> On 2012/02/21 12:05:49, Andi Albrecht wrote:
> > I'd prefer just to make the "-m" switch work as expected. We can do
> > the server side refactorings later.
> 
> Sadly our code misinterprets -m for -t on patchset uploads. I could fix that.
> 
> M-A

I thought it was a feature. Supply one message in external file and the first
line will also be used as a title.

[Andi Albrecht, 2012-02-22 18:42:18]

On Tue, Feb 21, 2012 at 9:52 PM,  <maruel@chromium.org> wrote:
> On 2012/02/21 14:50:33, Andi Albrecht wrote:
>>
>> http://codereview.appspot.com/5687062/diff/1/upload.py
>> File upload.py (right):
>
>
>> http://codereview.appspot.com/5687062/diff/1/upload.py#newcode2261
>> upload.py:2261: message = message or title
>> @M-A: Do we mix up message and title here?
>
>
> Yes. Let me fix our tools first.

sure, let me know when you're ready - But I don't really understand
how this mix up blocks the change in this changeset :)

>
> http://codereview.appspot.com/5687062/

[M-A, 2012-03-02 18:28:02]

http://codereview.appspot.com/5687062/diff/1/upload.py
File upload.py (right):

http://codereview.appspot.com/5687062/diff/1/upload.py#newcode2261
upload.py:2261: message = message or title
On 2012/02/21 14:50:34, Andi Albrecht wrote:
> @M-A: Do we mix up message and title here?

Yes, actually, feel free to fix it. We shouldn't mix them up. I'll just fix our
scripts accordingly whenever I update our copy of upload.py.

[Andi Albrecht, 2012-03-03 21:00:56]

hm, I'm not sure what to do now with the proposed change. Is it good to go in or
is there something that has to be changed?

[Andi Albrecht, 2012-04-04 12:54:32]

Looks like this issue got stock. M-A, is there anything you need to change
before this can go in?

[techtonik, 2012-04-09 10:15:14]

Multi step issue submission process became hard to review.