Issue 29424810: Issue 5191 - Add Terms of Use to adblockplus.org homepage (Closed)
|
Created:
April 28, 2017, 6:57 p.m. by juliandoucette Modified:
May 4, 2017, 3:14 p.m. CC:
Felix Dahlke, tamara, Lisa, j.nink, Vasily Kuznetsov Base URL:
https://hg.adblockplus.org/web.adblockplus.org Visibility:
Public. |
Patch Set 1 : Minimal / Incomplete #
Total comments: 2
Patch Set 2 : Took a stab at it #
Total comments: 7
Patch Set 3 : Fixed has_string #
Total comments: 6
Patch Set 4 : Removed fr/index.json #Patch Set 5 : Changed 'en' to defaultlocale #Patch Set 6 : Added check for locale page #
Total comments: 1
Patch Set 7 : Removed locale logic #
Total comments: 7
Patch Set 8 : Changed class to id #Patch Set 9 : Removed '>' from selector #MessagesTotal messages: 19
Note: I'm publishing comments on two changesets at once here. There are questions for Jon and Saroyanm below. https://codereview.adblockplus.org/29424810/diff/29424814/includes/index.tmpl File includes/index.tmpl (right): https://codereview.adblockplus.org/29424810/diff/29424814/includes/index.tmpl... includes/index.tmpl:70: {% if locale in ("en", "de", "fr") %} Note: I'd rather not hardcode this list. The first solution that comes to mind is to use a global function that checks if a string exists in the current locale by id. What do you guys think? https://codereview.adblockplus.org/29424810/diff/29424814/includes/index.tmpl... includes/index.tmpl:81: <a class="install-button" href="https://update.adblockplus.org/latest/adblockplusfirefox.xpi" id="install-firefox"> Note: This will work if we just don't push updated translations for these strings in other languages. I'd rather not duplicate all of these [once for install, again for agree and install]. https://codereview.adblockplus.org/29424810/diff/29424817/globals/has_string.py File globals/has_string.py (right): https://codereview.adblockplus.org/29424810/diff/29424817/globals/has_string.... globals/has_string.py:4: def has_string(context, name, page=None): Too bad get_string throws an exception :/ If we choose this solution, do you want to fix that in cms before we push this change Jon? https://codereview.adblockplus.org/29424810/diff/29424817/globals/platforms.py File globals/platforms.py (right): https://codereview.adblockplus.org/29424810/diff/29424817/globals/platforms.p... globals/platforms.py:1: platforms = { Note: I don't have a strong opinion about this being a global. I think it makes sense. But it *could* just as easily be set inside includes/index.tmpl (since that is the only place that we are currently using it). https://codereview.adblockplus.org/29424810/diff/29424817/includes/index.tmpl File includes/index.tmpl (right): https://codereview.adblockplus.org/29424810/diff/29424817/includes/index.tmpl... includes/index.tmpl:70: {% if locale == "en" or has_string("terms-message", "index") %} Can you confirm that "index" is the correct locale page saroyanm? It looks like we are not translating "firefox", "chrome", etc individually (nor should we, but there seems to be some magic going on here...). https://codereview.adblockplus.org/29424810/diff/29424817/includes/index.tmpl... includes/index.tmpl:86: {% if platform_id in ('chrome', 'yandex-browser') %} Note: I don't have a strong opinion about whether or not onclick should be part of platforms (e.g. if platform.onclick -> onclick="{{ platform.onclick }}"). I decided not to include it because it *only* applies to chrome and yandex. But I acknowledge that that logic may be inconsistent with making platforms global in the first place.
https://codereview.adblockplus.org/29424810/diff/29424817/globals/has_string.py File globals/has_string.py (right): https://codereview.adblockplus.org/29424810/diff/29424817/globals/has_string.... globals/has_string.py:4: def has_string(context, name, page=None): On 2017/04/28 21:56:01, juliandoucette wrote: > Too bad get_string throws an exception :/ > > If we choose this solution, do you want to fix that in cms before we push this > change Jon? The thing is that we have it raise the exception so that if expected strings are missing the website will generate errors letting us know so that we can see there are problems. Additionally, it seems that you are not actually calling the get_string global in this code. In order to access this global it would be as follows: context['get_string'](name, page) This is because the globals exist outside of the pages but only under the context object, this is due to jinja's handling of the namespaces across template, locale, and python code contexts. If we were to change the functionality it would be exactly the same as what you have here with a try/except catching the specific exception rather than all possible exceptions(as you do now, and which lead to the confusion as to whether or not you are even calling the global you want to). My suggestion here would be to change the code to something like this: try: context['get_string'](name, page) return True except KeyError, RuntimeError: return False However, I am open to discussion as to why it would be better to change this in the CMS itself. For example, a has_string global would actually be pretty useful in the CMS core.
Thanks Jon! See comment below. https://codereview.adblockplus.org/29424810/diff/29424817/globals/has_string.py File globals/has_string.py (right): https://codereview.adblockplus.org/29424810/diff/29424817/globals/has_string.... globals/has_string.py:4: def has_string(context, name, page=None): > The thing is that we have it raise the exception so that if expected strings are > missing the website will generate errors letting us know so that we can see > there are problems. Ack. > Additionally, it seems that you are not actually calling the > get_string global in this code. In order to access this global it would be as > follows: > > context['get_string'](name, page) > > This is because the globals exist outside of the pages but only under the > context object, this is due to jinja's handling of the namespaces across > template, locale, and python code contexts. Good catch! I thought that I had tested this properly... I was mistaken. > I am open to discussion as to why it would be better to change this in > the CMS itself. For example, a has_string global would actually be pretty useful > in the CMS core. We have at least one use case: 1. When a string should not appear if it has not been translated and approved by a manager in crowdin for legal reasons I think this applies to all legal text. (By default, a non-translated + non-approved string will appear in the website's default language.)
https://codereview.adblockplus.org/29424810/diff/29424817/globals/has_string.py File globals/has_string.py (right): https://codereview.adblockplus.org/29424810/diff/29424817/globals/has_string.... globals/has_string.py:4: def has_string(context, name, page=None): On 2017/04/29 08:30:55, Jon Sonesen wrote: > On 2017/04/28 21:56:01, juliandoucette wrote: > > Too bad get_string throws an exception :/ > > > > If we choose this solution, do you want to fix that in cms before we push this > > change Jon? > > The thing is that we have it raise the exception so that if expected strings are > missing the website will generate errors letting us know so that we can see > there are problems. Additionally, it seems that you are not actually calling the > get_string global in this code. In order to access this global it would be as > follows: > > context['get_string'](name, page) > > This is because the globals exist outside of the pages but only under the > context object, this is due to jinja's handling of the namespaces across > template, locale, and python code contexts. > > If we were to change the functionality it would be exactly the same as what you > have here with a try/except catching the specific exception rather than all > possible exceptions(as you do now, and which lead to the confusion as to whether > or not you are even calling the global you want to). > > My suggestion here would be to change the code to something like this: > > try: > context['get_string'](name, page) > return True > except KeyError, RuntimeError: > return False > > However, I am open to discussion as to why it would be better to change this in > the CMS itself. For example, a has_string global would actually be pretty useful > in the CMS core. Done.
https://codereview.adblockplus.org/29424810/diff/29427558/includes/index.tmpl File includes/index.tmpl (right): https://codereview.adblockplus.org/29424810/diff/29427558/includes/index.tmpl... includes/index.tmpl:70: {% if locale == "en" or has_string("terms-message", "index") %} We should check against "defaultlocale" I think instead of the value of "defaultlocale". Imagine one day the locale might change into en-US ? https://codereview.adblockplus.org/29424810/diff/29427558/includes/index.tmpl... includes/index.tmpl:70: {% if locale == "en" or has_string("terms-message", "index") %} This is wrong, the string can be translated while the page is not. We should check if the page is translated to current language or not. Probably similar to what does linkify.
@Jen & @Vasily please check question below. https://codereview.adblockplus.org/29424810/diff/29427558/includes/index.tmpl File includes/index.tmpl (right): https://codereview.adblockplus.org/29424810/diff/29427558/includes/index.tmpl... includes/index.tmpl:70: {% if locale == "en" or has_string("terms-message", "index") %} On 2017/05/02 09:26:37, saroyanm wrote: > We should check against "defaultlocale" I think instead of the value of > "defaultlocale". > Imagine one day the locale might change into en-US ? Acknowledged. https://codereview.adblockplus.org/29424810/diff/29427558/includes/index.tmpl... includes/index.tmpl:70: {% if locale == "en" or has_string("terms-message", "index") %} On 2017/05/02 09:26:37, saroyanm wrote: > This is wrong, the string can be translated while the page is not. > We should check if the page is translated to current language or not. > > Probably similar to what does linkify. I agree. But I don't know how to do this without changing CMS core. Do you have any ideas Jon?
Added a detail for Jon and Vasily below. (Sorry about the spelling mistake in the last message Jon) https://codereview.adblockplus.org/29424810/diff/29427558/includes/index.tmpl File includes/index.tmpl (right): https://codereview.adblockplus.org/29424810/diff/29427558/includes/index.tmpl... includes/index.tmpl:70: {% if locale == "en" or has_string("terms-message", "index") %} On 2017/05/02 09:26:37, saroyanm wrote: > We should check against "defaultlocale" I think instead of the value of > "defaultlocale". > Imagine one day the locale might change into en-US ? Done. https://codereview.adblockplus.org/29424810/diff/29427558/includes/index.tmpl... includes/index.tmpl:70: {% if locale == "en" or has_string("terms-message", "index") %} On 2017/05/02 09:26:37, saroyanm wrote: > This is wrong, the string can be translated while the page is not. > We should check if the page is translated to current language or not. > > Probably similar to what does linkify. Detail: I think that we have to check if the message AND the page is translated
@Jon & @Vasily I may have found a solution. See notes below. https://codereview.adblockplus.org/29424810/diff/29427574/includes/index.tmpl File includes/index.tmpl (right): https://codereview.adblockplus.org/29424810/diff/29427574/includes/index.tmpl... includes/index.tmpl:70: {% if locale == defaultlocale or (has_string("term-message", "index") and source.has_localizable_file(locale, "terms")) %} Warnings: 1. The terms page is not included in this patchset 2. source.has_localizable_file seems to work differently on the testing server vs production Can you look into this Jon & Vasily?
Removed locale conditions based on a discussion with Judith at lunch today.
Just a small suggestion. https://codereview.adblockplus.org/29424810/diff/29427582/includes/index.tmpl File includes/index.tmpl (right): https://codereview.adblockplus.org/29424810/diff/29427582/includes/index.tmpl... includes/index.tmpl:79: <a class="install-button" href="https://update.adblockplus.org/latest/adblockplusfirefox.xpi" id="install-firefox">{{"Agree and Install for Firefox"|translate("s13")}}</a> what about ? {{"Agree and Install for <fix>Firefox</fix>"}}? I think we can reuse the string, otherwise we will need to translate for each Browser separately. While we already updating the text, maybe we can make this bit more optimized already.
LGTM https://codereview.adblockplus.org/29424810/diff/29427582/includes/index.tmpl File includes/index.tmpl (right): https://codereview.adblockplus.org/29424810/diff/29427582/includes/index.tmpl... includes/index.tmpl:79: <a class="install-button" href="https://update.adblockplus.org/latest/adblockplusfirefox.xpi" id="install-firefox">{{"Agree and Install for Firefox"|translate("s13")}}</a> On 2017/05/02 12:48:30, saroyanm wrote: > what about ? > {{"Agree and Install for <fix>Firefox</fix>"}}? This doesn't make sense. We can not modify value of fix for each separate string.
https://codereview.adblockplus.org/29424810/diff/29427582/static/css/index.css File static/css/index.css (right): https://codereview.adblockplus.org/29424810/diff/29427582/static/css/index.cs... static/css/index.css:178: .terms-message > a A detail: We can use IDs instead, while it's suppose to be a unique item and #terms-message a to be consistent with other styles as well.
Ready for translation. https://codereview.adblockplus.org/29424810/diff/29427582/includes/index.tmpl File includes/index.tmpl (right): https://codereview.adblockplus.org/29424810/diff/29427582/includes/index.tmpl... includes/index.tmpl:79: <a class="install-button" href="https://update.adblockplus.org/latest/adblockplusfirefox.xpi" id="install-firefox">{{"Agree and Install for Firefox"|translate("s13")}}</a> On 2017/05/02 14:16:40, saroyanm wrote: > On 2017/05/02 12:48:30, saroyanm wrote: > > what about ? > > {{"Agree and Install for <fix>Firefox</fix>"}}? > This doesn't make sense. > We can not modify value of fix for each separate string. Acknowledged. https://codereview.adblockplus.org/29424810/diff/29427582/static/css/index.css File static/css/index.css (right): https://codereview.adblockplus.org/29424810/diff/29427582/static/css/index.cs... static/css/index.css:178: .terms-message > a On 2017/05/02 14:22:32, saroyanm wrote: > A detail: We can use IDs instead, while it's suppose to be a unique item and > #terms-message a to be consistent with other styles as well. Done.
LGTM with nit https://codereview.adblockplus.org/29424810/diff/29427582/static/css/index.css File static/css/index.css (right): https://codereview.adblockplus.org/29424810/diff/29427582/static/css/index.cs... static/css/index.css:178: .terms-message > a On 2017/05/02 14:42:35, juliandoucette wrote: > On 2017/05/02 14:22:32, saroyanm wrote: > > A detail: We can use IDs instead, while it's suppose to be a unique item and > > #terms-message a to be consistent with other styles as well. > > Done. I meant "#terms-message a" instead of "#terms-message > a"
https://codereview.adblockplus.org/29424810/diff/29427582/static/css/index.css File static/css/index.css (right): https://codereview.adblockplus.org/29424810/diff/29427582/static/css/index.cs... static/css/index.css:178: .terms-message > a On 2017/05/02 14:50:09, saroyanm wrote: > On 2017/05/02 14:42:35, juliandoucette wrote: > > On 2017/05/02 14:22:32, saroyanm wrote: > > > A detail: We can use IDs instead, while it's suppose to be a unique item and > > > #terms-message a to be consistent with other styles as well. > > > > Done. > > I meant "#terms-message a" instead of "#terms-message > a" Done.
LGTM again :)
On 2017/05/02 14:58:54, saroyanm wrote: > LGTM again :)
After some quick testing I realized we can drop "RuntimeError" from the except catch :) other than that LGTM 
|
