Issue611

Title Update issue with a commit information
Priority critical Status testing
Superseder Nosy List berker.peksag, brett.cannon, ezio.melotti, maciej.szulik, r.david.murray, zach.ware
Assigned To maciej.szulik Topics

Created on 2017-01-23.22:28:08 by maciej.szulik, last changed 2017-02-02.21:30:47 by maciej.szulik.

Files
File name Uploaded Type Edit Remove
issue611.patch maciej.szulik, 2017-01-23.22:28:28 text/plain
issue611.patch maciej.szulik, 2017-01-28.11:04:02 text/plain
issue611.patch maciej.szulik, 2017-01-30.20:44:02 text/plain
issue611_empty_date.diff berker.peksag, 2017-02-02.20:03:23 text/plain
Messages
msg3228 (view) Author: ezio.melotti Date: 2017-01-24.02:21:53
I haven't tried the patch yet, but in

+        if len(user_id) > 1:
+            # pick first matching user, when multiple found
+            user_id = user_id[0]
+        elif len(user_id) < 1:
+            # set bpobot as userid when none is found
+            user_id = self.db.user.filter(None, {'username': 'python-dev'})
+            if len(user_id) != 1:
+                # python-dev does not exists, anonymous will be used instead
+                return
+        username = self.db.user.get(user_id[0], 'username')
+        self.db.setCurrentUser(username)

I think you'll end up with a user_id in the >1 case and a list (assuming filter returns a list) in the <1 case.  At the end you take the first element of the list, which is correct only for the <1 case (unless filters returns a single id, but in that case I'm not sure what the [0] at the end is for).  I would also suggest to user user_ids and user_id to distinguish the list from the single id.

I would rename set_current_user() to something more explicit, or, if the only purpose of this method is to affect the author=self.db.getuid(), perhaps remove the method and use a module/class-level constant.  The same constant could also be used in the snippet I pasted above to avoid duplication.

Does

+            newmsg = self.db.msg.create(content=msg, author=self.db.getuid())
+            issue_msgs.append(newmsg)
+            self.db.issue.set(issue_id, messages=issue_msgs)

produces the correct output in the history at the bottom of the issue? (i.e. does it show "set  messages: +msgXXX" with only the new message, rather than the whole list of messages?)

I would move the .encode('utf-8') from dispatch() to handle_action() (add it to the line "'branch': branch.encode('utf-8'),").

The commit_id variable in handle_action() is unused.

Any reason why you used Template() instead of a simply using format() (or %)?  You might also be able to get rid of all those encode if you use format().

github/roundup should be capitalized properly in the docstrings.  The docstring for handle_action() could be a bit more explicit :)
msg3230 (view) Author: brett.cannon Date: 2017-01-27.23:27:41
Ezio seems to be right about user_id and getting a list or a name depending on which branch is taken based on the line that sets user_id initially. It also might be clearer if you check for `not len(user_id)` or `len(user_id) == 0` instead of `len(user_id) < 1`.

And thanks for handling the case when there's no match. I wouldn't be surprised if people don't sign the CLA but still get a PR merged that e.g. fixes a single line of the documentation.
msg3231 (view) Author: maciej.szulik Date: 2017-01-28.11:04:02
Patch updated and most importantly tested with github. Comments inline below:

> I think you'll end up with a user_id in the >1 case and a list (assuming filter returns a list) in the <1 case.  At the end you take the first element of the list, which is 
> correct only for the <1 case (unless filters returns a single id, but in that case I'm not sure what the [0] at the end is for).  I would also suggest to user user_ids and 
> user_id to distinguish the list from the single id.

> Ezio seems to be right about user_id and getting a list or a name depending on which branch is taken based on the line that sets user_id initially. It also might be clearer if > you check for `not len(user_id)` or `len(user_id) == 0` instead of `len(user_id) < 1`.

Yeah, filter returns list always, I've updated the patch accordingly.


> I would rename set_current_user() to something more explicit, or, if the only purpose of this method is to affect the author=self.db.getuid(), perhaps remove the method and use 
> a module/class-level constant.  The same constant could also be used in the snippet I pasted above to avoid duplication.

I've changed it to set_roundup_user, not sure what kind of constant you were talking about.


> Does produces the correct output in the history at the bottom of the issue? (i.e. does it show "set  messages: +msgXXX" with only the new message, rather than the whole list 
> of messages?)

Yes, it's 'messages +msgX', as expected.


> I would move the .encode('utf-8') from dispatch() to handle_action() (add it to the line "'branch': branch.encode('utf-8'),").

Done


> The commit_id variable in handle_action() is unused.

Removed


> Any reason why you used Template() instead of a simply using format() (or %)?  You might also be able to get rid of all those encode if you use format().

Laziness, I just copy&pasted what I got from hglookup.py ;) Changed to format.


> github/roundup should be capitalized properly in the docstrings.  The docstring for handle_action() could be a bit more explicit :)

Done

> And thanks for handling the case when there's no match. I wouldn't be surprised if people don't sign the CLA but still get a PR merged that e.g. fixes a single line of the 
> documentation.

N/p :)
msg3232 (view) Author: ezio.melotti Date: 2017-01-30.02:43:21
> I've changed it to set_roundup_user, not sure what kind of constant you were talking about.

I meant a constant for the python-dev user you find with: user_ids = self.db.user.filter(None, {'username': 'python-dev'})
Since the python-dev user is not going to change at run-time, there should be no reason to look it up every time, however it might be difficult to get access to the db, so if it's too much trouble don't worry about it.


I think the COMMENT_TEMPLATE.format(...) in handle_action() can be further improved.  If you use unicode for the template string, and pass unicode to format, you should be able to just encode the result once at end (either after the format or in self.db.msg.create(), assuming create() doesn't accept unicode).

The rest of the patch LGTM.
msg3233 (view) Author: maciej.szulik Date: 2017-01-30.20:44:02
Patch with comments from Ezio addressed and additionally supporting closing issues.
msg3234 (view) Author: maciej.szulik Date: 2017-01-30.21:13:01
Committed in https://hg.python.org/tracker/roundup/rev/bcf18f92716d.
msg3235 (view) Author: zach.ware Date: 2017-02-02.05:10:04
This seems to have some odd behavior, see for example https://bugs.python.org/issue29381

Each action (add message, change status, change resolution, change stage) all happened separately, which means 4 separate emails sent to everyone on the nosy list.

Also, automatic closure isn't the correct behavior, IMO; it should match the existing hg integration where closure only occurs if the message contains "Closes #<num>" or "Fixes #<num>" rather than just "Issue #<num>".

Other than those two points, this seems to be working quite well; thank you Maciej!
msg3236 (view) Author: berker.peksag Date: 2017-02-02.05:17:35
> Also, automatic closure isn't the correct behavior, [...]

I agree with Zachary.

BTW, there is a separate issue to set appropriate fields when an issue is manually closed: http://psf.upfronthosting.co.za/roundup/meta/issue595 (I should probably apply it :))
msg3237 (view) Author: zach.ware Date: 2017-02-02.06:07:51
Note https://mail.python.org/pipermail/docs/2017-February/thread.html

The multiple email issue is definitely going to need to be fixed sooner rather than later :)

Also, I just noticed that the date is not set on the messages.
msg3238 (view) Author: maciej.szulik Date: 2017-02-02.12:43:36
> This seems to have some odd behavior, see for example https://bugs.python.org/issue29381

> Each action (add message, change status, change resolution, change stage) all happened separately, which means 4 separate emails sent to everyone on the nosy list.

Thanks for the pointer, I'll look into why there are multiple events. Yes, one should be done.


> Also, automatic closure isn't the correct behavior, IMO; it should match the existing hg integration where closure only occurs if the message contains "Closes #<num>" or "Fixes #<num>" rather than just "Issue #<num>".

Yes it should work like that, apparently something went wrong, will dig deeper.


> BTW, there is a separate issue to set appropriate fields when an issue is manually closed: http://psf.upfronthosting.co.za/roundup/meta/issue595 (I should probably apply it :))

Didn't know about that, I'll look and apply accordingly.


> Note https://mail.python.org/pipermail/docs/2017-February/thread.html
> The multiple email issue is definitely going to need to be fixed sooner rather than later :)

Multiple emails due to hg + gh will be fixed when we migrate to gh. 
Multiple emails due to above needs fixing - agreed.
Multiple emails due to multiple branches - unfortunately won't, b/c each push is against separate
branch and when processing that I don't know anything about related pushes.


> Also, I just noticed that the date is not set on the messages.

Good point, will fix.


Zach and Berker thanks for pointers, greatly appreciated!
msg3239 (view) Author: zach.ware Date: 2017-02-02.16:01:55
> Multiple emails due to multiple branches - unfortunately won't, b/c each push is against separate
> branch and when processing that I don't know anything about related pushes.

Fair enough; if we can't do it, we can't do it [1].  Fixing the automatic closing and separate actions will go a long way toward alleviating the email flood.  And of course the hg/git duplication is expected for now.

Thanks for working on this!

[1] Although, we could consider doing something heinous like checking if the latest message on the issue was a commit message and editing new commit information into it, but I did use the word "heinous" there for a reason :)
msg3240 (view) Author: berker.peksag Date: 2017-02-02.17:37:07
Another alternative is to only send notifications for 2.7 and master branches for now [1] For example, see http://bugs.python.org/issue29407 we would only see the latest notification with this approach: http://bugs.python.org/msg286687 Since commit hashes don't change, there shouldn't be much problem with this approach.

[1] Since we are going to use "commit to master; cherry-pick to bugfix branches" with git, perhaps we could store these events in a queue, then periodically group them together and send them to bugs.p.o?
msg3242 (view) Author: berker.peksag Date: 2017-02-02.20:03:23
The attached patch should fix the empty date issue.
msg3243 (view) Author: maciej.szulik Date: 2017-02-02.20:06:26
> Another alternative is to only send notifications for 2.7 and master branches for now 

That's reasonable approach. I think we've talked about it with Brett and Ezio some time ago.


> perhaps we could store these events in a queue, then periodically group them together and send them to bugs.p.o?

That would be nice improvement :)
msg3244 (view) Author: maciej.szulik Date: 2017-02-02.21:30:47
Berker's date patch applied here:

https://hg.python.org/tracker/roundup/rev/5d2e54d9b327

All the others were addressed here:

https://hg.python.org/tracker/roundup/rev/68b787ac295b
History
Date User Action Args
2017-02-02 21:30:47maciej.szuliksetmessages: + msg3244
2017-02-02 20:06:26maciej.szuliksetmessages: + msg3243
2017-02-02 20:03:24berker.peksagsetfiles: + issue611_empty_date.diff
messages: + msg3242
2017-02-02 17:37:07berker.peksagsetmessages: + msg3240
2017-02-02 16:01:55zach.waresetmessages: + msg3239
2017-02-02 12:43:36maciej.szuliksetmessages: + msg3238
2017-02-02 06:07:52zach.waresetmessages: + msg3237
2017-02-02 05:17:35berker.peksagsetnosy: + berker.peksag
messages: + msg3236
2017-02-02 05:10:04zach.waresetnosy: + zach.ware
messages: + msg3235
2017-01-30 21:13:01maciej.szuliksetstatus: in-progress -> testing
messages: + msg3234
2017-01-30 20:44:03maciej.szuliksetfiles: + issue611.patch
messages: + msg3233
2017-01-30 02:43:22ezio.melottisetmessages: + msg3232
2017-01-28 11:04:04maciej.szuliksetfiles: + issue611.patch
messages: + msg3231
2017-01-27 23:27:41brett.cannonsetmessages: + msg3230
2017-01-24 02:21:54ezio.melottisetmessages: + msg3228
2017-01-23 22:28:29maciej.szuliksetfiles: + issue611.patch
2017-01-23 22:28:08maciej.szulikcreate