Issue 5452 - Improve CMS API for local link generation

Issue 5452 - Improve CMS API for local link generation

[Vasily Kuznetsov, 2019-01-14 13:29:27]

Hi Rosie,

Looks good. I just have one suggestion on the code below.

Also, we probably should have a test for page_has_locale() or do you think it's
tested enough by being called by get_page_url()?

Cheers,
Vasily

https://codereview.adblockplus.org/29979564/diff/29979565/cms/converters.py
File cms/converters.py (right):

https://codereview.adblockplus.org/29979564/diff/29979565/cms/converters.py#n...
cms/converters.py:596: def get_page_url(self, page, locale=None,
redirect=False):
The redirect argument is great, but it's not described in the original ticket.
Could you add it there please so that everything is in sync.

https://codereview.adblockplus.org/29979564/diff/29979565/cms/converters.py#n...
cms/converters.py:599: if self.page_has_locale(page, locale):
Since the body of this if is the same as that of the next, and they are kind of
long, maybe it would be worth it to combine them:

    if self.page_has_locale(...) or redirect:
        ...

It makes the logic a bit less clear but the saved repetition seems to be worth
it. What do you think?

[rhowell, 2019-01-14 14:56:47]

Hi Vasily!

I added a test for page_has_locale() in the globals.tmpl file. It seemed to fit
better there, but let me know if you have another suggestion? I also updated the
ticket, let me know if the new description makes sense?  Thanks!  :)

https://codereview.adblockplus.org/29979564/diff/29979565/cms/converters.py
File cms/converters.py (right):

https://codereview.adblockplus.org/29979564/diff/29979565/cms/converters.py#n...
cms/converters.py:596: def get_page_url(self, page, locale=None,
redirect=False):
On 2019/01/14 13:29:27, Vasily Kuznetsov wrote:
> The redirect argument is great, but it's not described in the original ticket.
> Could you add it there please so that everything is in sync.

Done.

https://codereview.adblockplus.org/29979564/diff/29979565/cms/converters.py#n...
cms/converters.py:599: if self.page_has_locale(page, locale):
On 2019/01/14 13:29:27, Vasily Kuznetsov wrote:
> Since the body of this if is the same as that of the next, and they are kind
of
> long, maybe it would be worth it to combine them:
> 
>     if self.page_has_locale(...) or redirect:
>         ...
> 
> It makes the logic a bit less clear but the saved repetition seems to be worth
> it. What do you think?

Yeah, I agree. Done.

[Vasily Kuznetsov, 2019-01-14 16:14:06]

Hi Rosie,

A couple more suggestions:

https://codereview.adblockplus.org/29979564/diff/29981555/tests/expected_outp...
File tests/expected_output/common/en/global (right):

https://codereview.adblockplus.org/29979564/diff/29981555/tests/expected_outp...
tests/expected_output/common/en/global:1: METHOD TEST
PASSEDFalseTrueexistsTrueFalse
Let's add line breaks and maybe some descriptions to these values, otherwise
it's pretty hard to understand what's going on. Maybe like this:

    ...
    get_string('exists', 'global') = {{- get_string('exists', 'global') }}
    page_has_locale('global', 'en') = {{- page_has_locale('global', 'en') }}
    page_has_locale('global', 'de') = {{- page_has_locale('global', 'de') }}

A bit repetitive, but the output would be a lot easier to examine. What do you
think?

https://codereview.adblockplus.org/29979564/diff/29981555/tests/expected_outp...
File tests/expected_output/static/en/get_page_url (right):

https://codereview.adblockplus.org/29979564/diff/29981555/tests/expected_outp...
tests/expected_output/static/en/get_page_url:9: page_url_redirect = /en/global
It would be useful to also make an expected output for this in German, to really
see the difference here.

[rhowell, 2019-01-14 16:59:33]

Hi Vasily,

Thanks for the good suggestions! Let me know if you see anything else?

https://codereview.adblockplus.org/29979564/diff/29981555/tests/expected_outp...
File tests/expected_output/common/en/global (right):

https://codereview.adblockplus.org/29979564/diff/29981555/tests/expected_outp...
tests/expected_output/common/en/global:1: METHOD TEST
PASSEDFalseTrueexistsTrueFalse
On 2019/01/14 16:14:06, Vasily Kuznetsov wrote:
> Let's add line breaks and maybe some descriptions to these values, otherwise
> it's pretty hard to understand what's going on. Maybe like this:
> 
>     ...
>     get_string('exists', 'global') = {{- get_string('exists', 'global') }}
>     page_has_locale('global', 'en') = {{- page_has_locale('global', 'en') }}
>     page_has_locale('global', 'de') = {{- page_has_locale('global', 'de') }}
> 
> A bit repetitive, but the output would be a lot easier to examine. What do you
> think?

Done.

https://codereview.adblockplus.org/29979564/diff/29981555/tests/expected_outp...
File tests/expected_output/static/en/get_page_url (right):

https://codereview.adblockplus.org/29979564/diff/29981555/tests/expected_outp...
tests/expected_output/static/en/get_page_url:9: page_url_redirect = /en/global
On 2019/01/14 16:14:06, Vasily Kuznetsov wrote:
> It would be useful to also make an expected output for this in German, to
really
> see the difference here.

Good idea. Since global was only in English, I switched to using the translate
page instead for one of the examples.

[Vasily Kuznetsov, 2019-01-16 10:32:14]

LGTM

I have one small suggestion, but it's just pure cosmetics.

https://codereview.adblockplus.org/29979564/diff/29981585/tests/expected_outp...
File tests/expected_output/common/en/global (right):

https://codereview.adblockplus.org/29979564/diff/29981585/tests/expected_outp...
tests/expected_output/common/en/global:1: site_globals('test') =METHOD TEST
PASSED
Perhaps we can now remove the "-" from "{{- ... }}" in the template so that we
have more balanced whitespace here.

[rhowell, 2019-01-17 03:15:22]

Yeah, definitely looks better this way. Thanks, Vasily!  :)

https://codereview.adblockplus.org/29979564/diff/29981585/tests/expected_outp...
File tests/expected_output/common/en/global (right):

https://codereview.adblockplus.org/29979564/diff/29981585/tests/expected_outp...
tests/expected_output/common/en/global:1: site_globals('test') =METHOD TEST
PASSED
On 2019/01/16 10:32:13, Vasily Kuznetsov wrote:
> Perhaps we can now remove the "-" from "{{- ... }}" in the template so that we
> have more balanced whitespace here.

Done.

[Vasily Kuznetsov, 2019-01-17 10:06:03]

On 2019/01/17 03:15:22, rhowell wrote:
> Yeah, definitely looks better this way. Thanks, Vasily!  :)
> 
>
https://codereview.adblockplus.org/29979564/diff/29981585/tests/expected_outp...
> File tests/expected_output/common/en/global (right):
> 
>
https://codereview.adblockplus.org/29979564/diff/29981585/tests/expected_outp...
> tests/expected_output/common/en/global:1: site_globals('test') =METHOD TEST
> PASSED
> On 2019/01/16 10:32:13, Vasily Kuznetsov wrote:
> > Perhaps we can now remove the "-" from "{{- ... }}" in the template so that
we
> > have more balanced whitespace here.
> 
> Done.

Even more LGTM!