Allow passing kwargs to flag_enabled and flag_disabled template tags

[Scotchester]

This will be used in a forthcoming cfgov-refresh PR to pass the Wagtail page object to a condition, so that it can test whether or not a page property matches something else.

It also adds an extension for Jinja2 tags, so that apps don't have to develop their own.

Might need some more tests?

[coveralls]

Pull Request Test Coverage Report for Build 144

• 0 of 0 changed or added relevant lines in 0 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage decreased (-1.5%) to 98.521%

---

Totals
Change from base Build 128:	-1.5%
Covered Lines:	533
Relevant Lines:	541

---

💛 - Coveralls

[willbarton]

Because this will be a breaking change to the Jinja2 API, I'd prefer to do a major release of this along with #11 and #12.

[chosak]

a breaking change to the Jinja2 API

@willbarton part of the reason for leaving the request=None parameter here (and not simplifying to (context, flag_name, **kwargs) was to maintain backwards compatibility with the existing API, specifically with the numerous uses of this tag in cfgov-refresh Jinja2 templates like flag_enabled('SOMETHING', request).

I agree that it'd be better to deprecate template_functions.py as you suggest, but the approach here would let us add kwargs support without breaking anything, and push off the breaking changes to the future major release.

[willbarton]

@chosak I don't have a problem retaining request=None as a kwarg, but I don't understand why it needs a function wrapper, except for DRY that's a bit too extreme for my tastes here.

def  flag_enabled(context, flag_name, request=None, **kwargs):
    request = request or context.get('request')
    return base_flag_enabled(flag_name, request=request, **kwargs)

Would work just as well.

future major release.

The future can be now! 😀

I do think if we're going to introduce a Jinja2 extension, it'd be better to leave the template functions in template_functions.py behind in 3.0 and make the extension a 4.0 thing. That feels like a big enough change to justify breaking how the Jinja2 environment is created.

[chosak]

request = request or context.get('request')

When @Scotchester and I were pairing on this we had this initially, but then I was wondering if there might be any cases where a "falsey" request might be a valid input to this method that we wouldn't want to confuse with None. For example flag_enabled('SOMETHING', request={}). Perhaps this isn't realistic or needed; in which case this simplification is nice.

[willbarton]

@chosak so, an explicit if request is None still works, no? I don't have a problem with either approach, I just think the wrapper function is unnecessary.

[Scotchester]

I was initially in favor of doing it without the function wrapper for ease of understanding for people like me who aren't as in tune with Python, but Andy convinced me to submit it with the wrapper and see what others think. I agree with you, Will, and will revert it back to two simpler-but-slightly-repetitious functions.

[Scotchester]

Simplification update pushed. I'm not sure who wins on the major or minor discussion. I abstain from that one :)

[Scotchester]

Additionally, the jinja2tags.py file and its FlagsExtension should replace the template_functions.py file (template_functions.py should be deleted), and the Jinja2 API documentation needs to be rewritten to show how to use the extension, rather than the independent functions. The FlagsExtension will also need unit tests.

I'll work on these items, as well.

[Scotchester]

I have updated the docs and added tests for FlagsExtension. Ready for another review!

[Scotchester]

Hmm... the Python 3 tests are failing with "ModuleNotFoundError: No module named 'templatetags'". Is there some kind of change in how local modules are imported in Python 3?

[Scotchester]

Oh, sorry, looks like Andy addressed that question while I wasn't looking :)

[Scotchester]

Docs and importing updated.

[Scotchester]

OK, again, isort befuddles me. Because flags is in front now, it wants it to go above the jinja2 external library imports.