This branch looks good. I just have a few small comments.
I like the substitution of a function for the mix-in.
> ./lib/lp/bugs/templates/bugtarget-bugs.pt
> 169: not well-formed (invalid token)
The above can be fixed by replacing the ampersands with "&".
> ./lib/lp/registry/browser/tests/test_subscription_links.py
> 295: E301 expected 1 blank line, found 2
> 387: E301 expected 1 blank line, found 2
> 481: E301 expected 1 blank line, found 2
> 567: E301 expected 1 blank line, found 2
These are indeed spurious, but are caused by the commented-out
functions. Looking at the functions it seems that they should be
removed or made to work. I tried a little to get them to work without
luck.
The DistributionSourcePackageActionMenu class now has a mutable class
attribute ("links"), which of course is normally a bad thing but it
doesn't appear that it is ever accessed because there is a property of
the same name. I suggest just removing the class attribute altogether.
The snippet
use_advanced_features = getFeatureFlag( 'malone.advanced-structural-subscriptions.enabled')
if use_advanced_features: links.append('subscribe_to_bug_mail')
else: links.append('subscribe')
is repeated so often I wonder if a helper function named something like
"add_subscribe_link" might not be DRYer.
This branch looks good. I just have a few small comments.
I like the substitution of a function for the mix-in.
> ./lib/lp/ bugs/templates/ bugtarget- bugs.pt
> 169: not well-formed (invalid token)
The above can be fixed by replacing the ampersands with "&".
> ./lib/lp/ registry/ browser/ tests/test_ subscription_ links.py
> 295: E301 expected 1 blank line, found 2
> 387: E301 expected 1 blank line, found 2
> 481: E301 expected 1 blank line, found 2
> 567: E301 expected 1 blank line, found 2
These are indeed spurious, but are caused by the commented-out
functions. Looking at the functions it seems that they should be
removed or made to work. I tried a little to get them to work without
luck.
The DistributionSou rcePackageActio nMenu class now has a mutable class
attribute ("links"), which of course is normally a bad thing but it
doesn't appear that it is ever accessed because there is a property of
the same name. I suggest just removing the class attribute altogether.
The snippet
use_ advanced_ features = getFeatureFlag(
'malone. advanced- structural- subscriptions. enabled' ) features:
links. append( 'subscribe_ to_bug_ mail')
links. append( 'subscribe' )
if use_advanced_
else:
is repeated so often I wonder if a helper function named something like link" might not be DRYer.
"add_subscribe_
In these files:
lib/ lp/bugs/ templates/ bug-subscriptio n-list. pt lp/bugs/ templates/ bugtarget- bugs.pt lp/bugs/ templates/ bugtarget- subscription- list.pt lp/registry/ templates/ distribution- index.pt lp/registry/ templates/ distributionsou rcepackage- index.pt lp/registry/ templates/ distroseries- index.pt lp/registry/ templates/ milestone- index.pt lp/registry/ templates/ product- index.pt lp/registry/ templates/ productseries- index.pt
lib/
lib/
lib/
lib/
lib/
lib/
lib/
lib/
I think you want a "var" on the line:
module = Y.lp.registry. structural_ subscription;