Issue 29323033: Issue 2846 - Service for processing Microsoft Edge announcement subscription (Closed)
|
Created:
July 30, 2015, 1:30 a.m. by Oleksandr Modified:
Aug. 20, 2015, 10:24 a.m. Visibility:
Public. |
DescriptionThis tries to reuse as much as possible the already implemented mechanism of email verification for Adblock Browser. The result is minimum changes, but probably not the prettiest code.
Patch Set 1 #
Total comments: 1
Patch Set 2 : Don't hardcode 'edge'. Change the verification success URL. #
Total comments: 14
Patch Set 3 : Comments addressed #
Total comments: 3
Patch Set 4 : Default to adblockbrowser #
Total comments: 2
Patch Set 5 : Move "adblockbrowser" to global variable #Patch Set 6 : Rename the ADBLOCKBROWSER_PRODUCT_NAME to DEFAULT_PRODUCT #
MessagesTotal messages: 14
https://codereview.adblockplus.org/29323033/diff/29323034/sitescripts/submit_... File sitescripts/submit_email/web/submit_email.py (right): https://codereview.adblockplus.org/29323033/diff/29323034/sitescripts/submit_... sitescripts/submit_email/web/submit_email.py:52: if data.get('edge', '') == '': I'd rather not hard-code "edge" here. How about calling the parameter "product", adding its value to the template name, and the filename below? That way the code here can be agnostic of the actual products, and adding new ones will be just a matter of adding a new template, and changing the config.
https://codereview.adblockplus.org/29323033/diff/29323536/sitescripts/submit_... File sitescripts/submit_email/web/submit_email.py (right): https://codereview.adblockplus.org/29323033/diff/29323536/sitescripts/submit_... sitescripts/submit_email/web/submit_email.py:52: if data.get('product', '') == '': This "if" and the one below are here so that we would produce a non-ugly file and template names. We might have stuff like edge_verification_template and ios_verification_template, but I wasn't sure if we should be touching/renaming the ios part.
https://codereview.adblockplus.org/29323033/diff/29323536/sitescripts/submit_... File sitescripts/submit_email/template/edge_verification.mail (right): https://codereview.adblockplus.org/29323033/diff/29323536/sitescripts/submit_... sitescripts/submit_email/template/edge_verification.mail:12: {{ verification_url }} Nit: We usually don't have the spaces inside the "{{" and "}}"s. I see that we have added them for the existing email template though, so up to you. https://codereview.adblockplus.org/29323033/diff/29323536/sitescripts/submit_... File sitescripts/submit_email/web/submit_email.py (right): https://codereview.adblockplus.org/29323033/diff/29323536/sitescripts/submit_... sitescripts/submit_email/web/submit_email.py:52: if data.get('product', '') == '': On 2015/08/13 04:12:07, Oleksandr wrote: > This "if" and the one below are here so that we would produce a non-ugly file > and template names. We might have stuff like edge_verification_template and > ios_verification_template, but I wasn't sure if we should be touching/renaming > the ios part. Nit: `if data.get('product') is None`? (Or `if not data.get('product')`?) https://codereview.adblockplus.org/29323033/diff/29323536/sitescripts/submit_... sitescripts/submit_email/web/submit_email.py:55: email_template = data.get('product', '') + '_verification_email_template' We should probably assign data.get('product') to a variable above, and then re-use it here. https://codereview.adblockplus.org/29323033/diff/29323536/sitescripts/submit_... sitescripts/submit_email/web/submit_email.py:55: email_template = data.get('product', '') + '_verification_email_template' Use string formatting like `"%s_verification_email_template" % product`? https://codereview.adblockplus.org/29323033/diff/29323536/sitescripts/submit_... sitescripts/submit_email/web/submit_email.py:56: params.append(('product', data.get('product', ''))) Again, we should re-use the variable instead of performing `data.get('product', '')`. https://codereview.adblockplus.org/29323033/diff/29323536/sitescripts/submit_... sitescripts/submit_email/web/submit_email.py:88: if params.get('product', '') == '': Same issues as above in this code.
https://codereview.adblockplus.org/29323033/diff/29323536/sitescripts/submit_... File sitescripts/submit_email/web/submit_email.py (right): https://codereview.adblockplus.org/29323033/diff/29323536/sitescripts/submit_... sitescripts/submit_email/web/submit_email.py:52: if data.get('product', '') == '': I think we should simply fallback to "adblockbrowser", adjusting the configuration on the server, rather than having conditional logic here. https://codereview.adblockplus.org/29323033/diff/29323536/sitescripts/submit_... sitescripts/submit_email/web/submit_email.py:55: email_template = data.get('product', '') + '_verification_email_template' On 2015/08/18 09:28:57, kzar wrote: > Use string formatting like `"%s_verification_email_template" % product`? I think using the + operator when concatenating only two strings is fine. Format strings neither make the code more readable nor faster in that case. Actually it does the opposite. While it is the other way around when you concatenate more strings.
https://codereview.adblockplus.org/29323033/diff/29323536/sitescripts/submit_... File sitescripts/submit_email/template/edge_verification.mail (right): https://codereview.adblockplus.org/29323033/diff/29323536/sitescripts/submit_... sitescripts/submit_email/template/edge_verification.mail:12: {{ verification_url }} On 2015/08/18 09:28:56, kzar wrote: > Nit: We usually don't have the spaces inside the "{{" and "}}"s. I see that we > have added them for the existing email template though, so up to you. Well, indeed, we add or omit the optimal spaces in the beginning/end of a template expression quite inconsistently, currently. But I usually advice to add them in new code. Reasons: * Even in files where we currently omit these spaces, we don't omit them for template tags. So this is an inconsistency. * Jinja2's documentation and examples use the extra space. So this can be considered the official convention. * If the expression is more complex than printing a single variable, it improves the readability, as humans are used to text that is separated by spaces.
https://codereview.adblockplus.org/29323033/diff/29323536/sitescripts/submit_... File sitescripts/submit_email/template/edge_verification.mail (right): https://codereview.adblockplus.org/29323033/diff/29323536/sitescripts/submit_... sitescripts/submit_email/template/edge_verification.mail:12: {{ verification_url }} On 2015/08/18 16:45:09, Sebastian Noack wrote: > On 2015/08/18 09:28:56, kzar wrote: > > Nit: We usually don't have the spaces inside the "{{" and "}}"s. I see that we > > have added them for the existing email template though, so up to you. > > Well, indeed, we add or omit the optimal spaces in the beginning/end of a > template expression quite inconsistently, currently. But I usually advice to add > them in new code. Reasons: > > * Even in files where we currently omit these spaces, we don't omit them for > template tags. So this is an inconsistency. > * Jinja2's documentation and examples use the extra space. So this can be > considered the official convention. > * If the expression is more complex than printing a single variable, it improves > the readability, as humans are used to text that is separated by spaces. Acknowledged. https://codereview.adblockplus.org/29323033/diff/29323536/sitescripts/submit_... File sitescripts/submit_email/web/submit_email.py (right): https://codereview.adblockplus.org/29323033/diff/29323536/sitescripts/submit_... sitescripts/submit_email/web/submit_email.py:52: if data.get('product', '') == '': On 2015/08/18 16:35:03, Sebastian Noack wrote: > I think we should simply fallback to "adblockbrowser", adjusting the > configuration on the server, rather than having conditional logic here. Done. https://codereview.adblockplus.org/29323033/diff/29323536/sitescripts/submit_... sitescripts/submit_email/web/submit_email.py:55: email_template = data.get('product', '') + '_verification_email_template' On 2015/08/18 09:28:56, kzar wrote: > We should probably assign data.get('product') to a variable above, and then > re-use it here. Done. https://codereview.adblockplus.org/29323033/diff/29323536/sitescripts/submit_... sitescripts/submit_email/web/submit_email.py:56: params.append(('product', data.get('product', ''))) On 2015/08/18 09:28:56, kzar wrote: > Again, we should re-use the variable instead of performing `data.get('product', > '')`. Done.
https://codereview.adblockplus.org/29323033/diff/29324271/sitescripts/submit_... File sitescripts/submit_email/web/submit_email.py (right): https://codereview.adblockplus.org/29323033/diff/29324271/sitescripts/submit_... sitescripts/submit_email/web/submit_email.py:53: product = data.get('product') What I meant is: product = data.get('product', 'adblockbrowser') That way we don't need to explicitly handle None. Yes, we need to adapt the server configuration then. But that is fine. https://codereview.adblockplus.org/29323033/diff/29324271/sitescripts/submit_... sitescripts/submit_email/web/submit_email.py:89: product = params.get('product') Same here.
Fingers crossed! https://codereview.adblockplus.org/29323033/diff/29324271/sitescripts/submit_... File sitescripts/submit_email/web/submit_email.py (right): https://codereview.adblockplus.org/29323033/diff/29324271/sitescripts/submit_... sitescripts/submit_email/web/submit_email.py:53: product = data.get('product') On 2015/08/19 10:10:08, Sebastian Noack wrote: > What I meant is: > > product = data.get('product', 'adblockbrowser') > > That way we don't need to explicitly handle None. Yes, we need to adapt the > server configuration then. But that is fine. Done.
https://codereview.adblockplus.org/29323033/diff/29324357/sitescripts/submit_... File sitescripts/submit_email/web/submit_email.py (right): https://codereview.adblockplus.org/29323033/diff/29324357/sitescripts/submit_... sitescripts/submit_email/web/submit_email.py:52: product = data.get('product', 'adblockbrowser') Nit: Since we use the same default below, mind storing it in a constant global variable, just like VERIFICATION_PATH?
https://codereview.adblockplus.org/29323033/diff/29324357/sitescripts/submit_... File sitescripts/submit_email/web/submit_email.py (right): https://codereview.adblockplus.org/29323033/diff/29324357/sitescripts/submit_... sitescripts/submit_email/web/submit_email.py:52: product = data.get('product', 'adblockbrowser') On 2015/08/19 15:02:47, Sebastian Noack wrote: > Nit: Since we use the same default below, mind storing it in a constant global > variable, just like VERIFICATION_PATH? Done.
LGTM
LGTM 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
