Issue 2823 - Display new subscription types on subscriptions page

See Sitescripts changes: https://codereview.adblockplus.org/29326068/

[kzar, 2015-09-09 12:23:36]

https://codereview.adblockplus.org/29326085/diff/29326086/includes/subscripti...
File includes/subscriptionList.tmpl (right):

https://codereview.adblockplus.org/29326085/diff/29326086/includes/subscripti...
includes/subscriptionList.tmpl:70: {% macro display_subscriptions(subscriptions)
%}
The logic in this macro is getting increasingly hard to grok. The duplication is
getting worse as well. "type_" + current_type keeps showing up, the <h2> tags
are duplicated which sucks more the more complicated they get and the the
conditional logic is getting very confusing.

I don't have any concrete suggestions but can you see a way to tidy this all up?

[Thomas Greiner, 2015-09-15 13:21:05]

https://codereview.adblockplus.org/29326085/diff/29326086/includes/subscripti...
File includes/subscriptionList.tmpl (right):

https://codereview.adblockplus.org/29326085/diff/29326086/includes/subscripti...
includes/subscriptionList.tmpl:70: {% macro display_subscriptions(subscriptions)
%}
On 2015/09/09 12:23:35, kzar wrote:
> The logic in this macro is getting increasingly hard to grok. The duplication
is
> getting worse as well. "type_" + current_type keeps showing up, the <h2> tags
> are duplicated which sucks more the more complicated they get and the the
> conditional logic is getting very confusing.
> 
> I don't have any concrete suggestions but can you see a way to tidy this all
up?

I agree, that this entire template is quite convoluted and confusing. A page
redesign in the future might help. However, for now I tried to clean it up a bit
to at least avoid duplication.

[kzar, 2015-09-15 15:54:55]

Certainly looking better than before.

https://codereview.adblockplus.org/29326085/diff/29327700/includes/subscripti...
File includes/subscriptionList.tmpl (right):

https://codereview.adblockplus.org/29326085/diff/29327700/includes/subscripti...
includes/subscriptionList.tmpl:71: {%- set current_type = None -%}
Subscriptions always have a type right? If so we can remove this set statement
and check that current_type is truthy instead of that it isn't None.

https://codereview.adblockplus.org/29326085/diff/29327700/includes/subscripti...
includes/subscriptionList.tmpl:73: {%- if not subscription["supplements"] or
current_type != subscription["type"] -%}
How about an application of De Morgan's law?

{% if not (subscription["supplements"] and current_type == subscription["type"])
%}

https://codereview.adblockplus.org/29326085/diff/29327700/includes/subscripti...
includes/subscriptionList.tmpl:75: {%- if current_type != None %}
Nit: In Python I think you should use `is` for checking if a variable is / isn't
None. If we don't just check for `if current_type` as mentioned above, then I
think we should be doing `if current_type is not None`.

[Thomas Greiner, 2015-09-16 12:06:28]

https://codereview.adblockplus.org/29326085/diff/29327700/includes/subscripti...
File includes/subscriptionList.tmpl (right):

https://codereview.adblockplus.org/29326085/diff/29327700/includes/subscripti...
includes/subscriptionList.tmpl:71: {%- set current_type = None -%}
On 2015/09/15 15:54:55, kzar wrote:
> Subscriptions always have a type right? If so we can remove this set statement
> and check that current_type is truthy instead of that it isn't None.

Done. It was just a personal preference to make the code more explicit.

https://codereview.adblockplus.org/29326085/diff/29327700/includes/subscripti...
includes/subscriptionList.tmpl:73: {%- if not subscription["supplements"] or
current_type != subscription["type"] -%}
On 2015/09/15 15:54:55, kzar wrote:
> How about an application of De Morgan's law?
> 
> {% if not (subscription["supplements"] and current_type ==
subscription["type"])
> %}

Done. Haven't heard about that theorem yet so thanks for sharing.

https://codereview.adblockplus.org/29326085/diff/29327700/includes/subscripti...
includes/subscriptionList.tmpl:75: {%- if current_type != None %}
On 2015/09/15 15:54:55, kzar wrote:
> Nit: In Python I think you should use `is` for checking if a variable is /
isn't
> None. If we don't just check for `if current_type` as mentioned above, then I
> think we should be doing `if current_type is not None`.

Done. `is not` was throwing a Jinja2 exception when I tried it out earlier but
checking it like that should be sufficient since I'm no longer explicitly
setting it to `None`.

[kzar, 2015-09-16 16:40:26]

Nice, looks way clearer now IMO. LGTM

[Felix Dahlke, 2015-09-17 15:28:11]

LGTM