Title bpo emails contain useless non-github pull_request number - users want a link to actual github PR
Priority bug Status chatting
Superseder Nosy List berker.peksag, ezio.melotti, gregory.p.smith, ncoghlan
Assigned To ezio.melotti Topics

Created on 2017-03-31.17:05:33 by gregory.p.smith, last changed 2018-06-12.15:04:21 by berker.peksag.

File name Uploaded Type Edit Remove
issue624.diff berker.peksag, 2018-06-09.20:09:26 text/plain
msg3327 (view) Author: gregory.p.smith Date: 2017-03-31.17:05:33 emails contain misleading text such as

Changes by so and so

pull_requests: +1081

When a Github Pull Request is associated with an issue.

Which is *entirely useless* as 1081 is NOT a github PR number.

It appears to be an internal bpo database key.  Please never expose that!

This makes an email based workflow impossible as I cannot go from the email to the Github PR in question.  What this needs to be is text containing the actual Github PR number hyperlinked directly to the Github PR itself.

In this example, the above was trigged by the "2017-03-31 09:36:35	dstufft	set	pull_requests: + pull_request1081" action on which should really link to if I understand properly.

Please consider email based workflows.

I do not want to open bpo to hunt through the UI for one of two places that may contain the link that eventually gets me to the right page.
msg3328 (view) Author: ezio.melotti Date: 2017-03-31.17:42:29
msg3329 (view) Author: ezio.melotti Date: 2017-04-01.18:47:08
I've been looking at the code and perhaps it can be done in detectors/  I'll try to put together a patch sometimes next week.
msg3330 (view) Author: ezio.melotti Date: 2017-04-01.19:00:21
(FWIW I've been also considering having a 1:1 match between PR numbers and internal roundup ids for pull_requests, however this requires two step:
  1) make sure that only one Roundup pull_request exists for each GitHub PR;
  2) make sure that the PR numbers and Roundup ids match;
I'd like to see point 1 implemented regardless.  I first suggested it during review but since it was not trivial to do we decided to postpone it as not to block the GitHub migration any further.  I'm not sure if point 2 can be implemented since the ids are auto-incremented and as far as I know they can't be changed.)
msg3474 (view) Author: berker.peksag Date: 2018-06-09.20:09:26
Here's patch to fix this issue.

Example output:

components: +Tkinter
versions: +Python 2.4

Old version:

components: +Tkinter
versions: +Python 2.4
pull_requests: +42
msg3476 (view) Author: ncoghlan Date: 2018-06-10.05:17:10
Berker's patch using a post-filtering step, where the mail sending code notices the internal DB reference, adds a link to the GitHub PR based on a DB lookup, and then filters out the DB reference.

That seems like a nice low impact way to resolve the UX problem to me, so +1 here.
msg3479 (view) Author: berker.peksag Date: 2018-06-10.15:34:11
Note that the regex code can be probably be simplified, I'm open to suggestions. I'm not sure whether re.S is still needed.

+        pr_number = db.pull_request.get(pr_number, 'number')

Also, I need to be more defensive here and don't do anything if the query returned an empty result.
msg3482 (view) Author: berker.peksag Date: 2018-06-12.15:04:21
I just noticed that I probably need to update the clean_ok_message() function in to remove pull_requests.
Date User Action Args
2018-06-12 15:04:21berker.peksagsetmessages: + msg3482
2018-06-10 15:34:11berker.peksagsetmessages: + msg3479
2018-06-10 05:17:10ncoghlansetnosy: + ncoghlan
messages: + msg3476
2018-06-09 20:09:26berker.peksagsetfiles: + issue624.diff
nosy: + berker.peksag
messages: + msg3474
2017-04-01 19:00:21ezio.melottisetmessages: + msg3330
2017-04-01 18:47:09ezio.melottisetassignedto: ezio.melotti
messages: + msg3329
2017-03-31 17:42:29ezio.melottisetstatus: unread -> chatting
nosy: + ezio.melotti
messages: + msg3328
2017-03-31 17:05:33gregory.p.smithcreate