Use a helper to build context for management views instead of a context processor#2
Open
rebeccacremona wants to merge 7 commits into
Open
Use a helper to build context for management views instead of a context processor#2rebeccacremona wants to merge 7 commits into
rebeccacremona wants to merge 7 commits into
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Hi @teovin! Thanks again for tackling the big mess of our user management templates in harvard-lil#3763.
As mentioned, I really liked the direction you took, of using named boolean flags in templates instead of intricate and repeated
request.userlogic. Big improvement!In this PR, I'm proposing a slightly different architecture:
I made views/user_management into a package, so that your UI logic could live right alongside the views that use it, instead of in independent top-level packages (or, for instance, moving it to the already-giant utils.py file)
I moved the UI-flag-building logic into a helper function inside that module, so that views that need it can invoke it directly, instead of calling that logic inside a context processor (which is automatically called on -every- request the Django application processes)... and then, I bundled that into an optional helper
render_user_management. I think this will be easier to reason about, and will save the overhead on the rest of the app's views.I shortened up some of the flag names, using the conventions:
show_*— visibility (layout, filters, stats)can_*— actions and linksExample: instead of
can_show_user_management_title→show_full_manage_titleI added a few more tests.
So, even though the diff is big... it's not as many changes as it looks like :-)
When you're back, let's chat about whether you like these ideas, or whether I messed up something about your original structure!
Cheers,