#4431 - Introduce sitescripts.reports.web.resolveReport handler

#4431 - Introduce sitescripts.reports.web.resolveReport handler

repo: https://hg.adblockplus.org/sitescripts
base rev: 862f92fafaa8

This will need to be attached to a trac ticket before it can land, but for now I'll use the hub ticket.

[Vasily Kuznetsov, 2017-10-20 14:21:49]

Hi Matze,

Here's the script for report redirection.

Not sure if we would need to document the dependency on cryptography library
more prominently, right now it's only mentioned in tox.ini (but so are all other
dependencies of sitescripts). Let me know if you think it would make sense to
add it to the README.

We also need a trac issue before this can land.

Cheers,
Vasily

[mathias, 2017-10-24 14:53:45]

Very promising!

https://codereview.adblockplus.org/29584613/diff/29584614/sitescripts/reports...
File sitescripts/reports/README.md (right):

https://codereview.adblockplus.org/29584613/diff/29584614/sitescripts/reports...
sitescripts/reports/README.md:29: decrypted id of the report.
There's a `the` missing between `by` and `decrypted`.

https://codereview.adblockplus.org/29584613/diff/29584614/sitescripts/reports...
File sitescripts/reports/tests/test_resolveReport.py (right):

https://codereview.adblockplus.org/29584613/diff/29584614/sitescripts/reports...
sitescripts/reports/tests/test_resolveReport.py:2: # Copyright (C) 2006-present
eyeo GmbH
Shouldn't this read 2017-present?

https://codereview.adblockplus.org/29584613/diff/29584614/sitescripts/reports...
sitescripts/reports/tests/test_resolveReport.py:29: URL_TEMPLATE =
'https://report.server.com/{report_id}'
I recommend always using example.com in defaults, examples, stubs and tests.

https://codereview.adblockplus.org/29584613/diff/29584614/sitescripts/reports...
File sitescripts/reports/web/resolveReport.py (right):

https://codereview.adblockplus.org/29584613/diff/29584614/sitescripts/reports...
sitescripts/reports/web/resolveReport.py:49: except (ValueError, TypeError,
InvalidTag):
Exceptions should get logged (they'll appear in our Nginx error log if written
to sys.stderr).

https://codereview.adblockplus.org/29584613/diff/29584614/sitescripts/reports...
sitescripts/reports/web/resolveReport.py:63: code, message, headers = 404, 'Not
Found', []
Please use httplib.NOT_FOUND. even if (or, for educational purpose, especially
because) every reader of the code should know that magic number.

https://codereview.adblockplus.org/29584613/diff/29584614/sitescripts/reports...
sitescripts/reports/web/resolveReport.py:66: code, message, headers = 302,
'Found', [('Location', location)]
Same here, httplib.FOUND should do the trick.

https://codereview.adblockplus.org/29584613/diff/29584614/sitescripts/reports...
sitescripts/reports/web/resolveReport.py:68: start_response('{} {}'.format(code,
message), headers)
Why don't you get the message via lookup based on the code? I.e. `message =
httplib.responses[code]` or something similar. Why the hazzle?

[Vasily Kuznetsov, 2017-10-24 15:36:58]

Thanks for the comments, all fixed!

https://codereview.adblockplus.org/29584613/diff/29584614/sitescripts/reports...
File sitescripts/reports/README.md (right):

https://codereview.adblockplus.org/29584613/diff/29584614/sitescripts/reports...
sitescripts/reports/README.md:29: decrypted id of the report.
On 2017/10/24 14:53:44, mathias wrote:
> There's a `the` missing between `by` and `decrypted`.

Why do you think so? I don't mean to disagree, just wondering.

Done.

https://codereview.adblockplus.org/29584613/diff/29584614/sitescripts/reports...
File sitescripts/reports/tests/test_resolveReport.py (right):

https://codereview.adblockplus.org/29584613/diff/29584614/sitescripts/reports...
sitescripts/reports/tests/test_resolveReport.py:2: # Copyright (C) 2006-present
eyeo GmbH
On 2017/10/24 14:53:44, mathias wrote:
> Shouldn't this read 2017-present?

Yeah, I guess, this would make more sense. Done

https://codereview.adblockplus.org/29584613/diff/29584614/sitescripts/reports...
sitescripts/reports/tests/test_resolveReport.py:29: URL_TEMPLATE =
'https://report.server.com/{report_id}'
On 2017/10/24 14:53:44, mathias wrote:
> I recommend always using http://example.com in defaults, examples, stubs and
tests.

Done.

https://codereview.adblockplus.org/29584613/diff/29584614/sitescripts/reports...
File sitescripts/reports/web/resolveReport.py (right):

https://codereview.adblockplus.org/29584613/diff/29584614/sitescripts/reports...
sitescripts/reports/web/resolveReport.py:49: except (ValueError, TypeError,
InvalidTag):
On 2017/10/24 14:53:44, mathias wrote:
> Exceptions should get logged (they'll appear in our Nginx error log if written
> to sys.stderr).

Done.

https://codereview.adblockplus.org/29584613/diff/29584614/sitescripts/reports...
sitescripts/reports/web/resolveReport.py:63: code, message, headers = 404, 'Not
Found', []
On 2017/10/24 14:53:45, mathias wrote:
> Please use httplib.NOT_FOUND. even if (or, for educational purpose, especially
> because) every reader of the code should know that magic number.

Done.

https://codereview.adblockplus.org/29584613/diff/29584614/sitescripts/reports...
sitescripts/reports/web/resolveReport.py:66: code, message, headers = 302,
'Found', [('Location', location)]
On 2017/10/24 14:53:44, mathias wrote:
> Same here, httplib.FOUND should do the trick.

Done.

https://codereview.adblockplus.org/29584613/diff/29584614/sitescripts/reports...
sitescripts/reports/web/resolveReport.py:68: start_response('{} {}'.format(code,
message), headers)
On 2017/10/24 14:53:44, mathias wrote:
> Why don't you get the message via lookup based on the code? I.e. `message =
> httplib.responses[code]` or something similar. Why the hazzle?

Done.

[mathias, 2017-10-24 16:08:19]

https://codereview.adblockplus.org/29584613/diff/29584614/sitescripts/reports...
File sitescripts/reports/README.md (right):

https://codereview.adblockplus.org/29584613/diff/29584614/sitescripts/reports...
sitescripts/reports/README.md:29: decrypted id of the report.
On 2017/10/24 15:36:57, Vasily Kuznetsov wrote:
> On 2017/10/24 14:53:44, mathias wrote:
> > There's a `the` missing between `by` and `decrypted`.
> Why do you think so? I don't mean to disagree, just wondering.

I cannot cite (read: am too lazy to dig up) some specific rule of grammar to
declare the former version wrong (yet I think it probably is), but only refer to
feel of both language and consistency as motivation.

[mathias, 2017-10-26 12:54:32]

LGTM.