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

[albrec...@googlemail.com]

Reviewers: techtonik, GvR, M-A,

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

Please review this at http://codereview.appspot.com/5687062/

Affected files:
M codereview/views.py
M upload.py

Index: codereview/views.py
===================================================================
--- a/codereview/views.py
+++ b/codereview/views.py
@@ -1494,9 +1494,10 @@
query = query.filter('status =', None) # all uploaded file have a
status
if query.count() > 0:
errors.append('Base files missing.')
- # Send notification mail.
- if request.POST.get('send_mail') == 'yes':
- msg = _make_message(request, request.issue, '', '', True)
+ # Create (and send) a message if needed.
+ if request.POST.get('send_mail') == 'yes' or request.POST.get('message'):
+ msg = _make_message(request, request.issue,
request.POST.get('message', ''),
+ send_mail=(request.POST.get('send_mail', '')
== 'yes'))
msg.put()
_notify_issue(request, request.issue, 'Mailed')
if errors:
Index: upload.py
===================================================================
--- a/upload.py
+++ b/upload.py
@@ -2261,12 +2261,10 @@
message = message or title

form_fields.append(("subject", title))
- if message:
- if not options.issue:
- form_fields.append(("description", message))
- else:
- # TODO: [ ] Use /<issue>/publish to add a comment.
- pass
+ # If it's a new issue send message as description. Otherwise a new
+ # message is created below on upload_complete.
+ if message and not options.issue:
+ form_fields.append(("description", message))

# Send a hash of all the base file so the server can determine if a copy
# already exists in an earlier patchset.
@@ -2325,6 +2323,8 @@
payload["send_mail"] = "yes"
if options.send_patch:
payload["attach_patch"] = "yes"
+ if options.issue and message:
+ payload["message"] = message
payload = urllib.urlencode(payload)
rpc_server.Send("/" + issue + "/upload_complete/" + (patchset or ""),
payload=payload)

[tech...@gmail.com]

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.

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

[Andi Albrecht]

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/

[albrec...@googlemail.com]

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?

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

[tech...@gmail.com]

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.

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