New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Allow custom context when rendering a view #7847
base: develop-4
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello! Thank you for your contribution! 🎉
As it's your first contribution, be sure to check out the contribution docs.
If you're a Slack user and haven't joined us, please do here!
Welcome aboard ⛵️!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although it's unnecessary for my particular use case, we could add the custom context to other _handl_no_page()
and _render_welcome_page()
which also return a TemplateResponse
@micahcantor Thank you for this PR! I appreciate the work you've put into this and especially that you are sharing your ideas with the community!! I have looked at it and come to the conclusion that Your use case seems highly unusual in several senses:
Finally, let me point out that your implementation is prone to have side-effects since you use a single To implement A/B testing, I'd suggest taking one of the more traditional routes:
@marksweb What's your opinion? |
@fsbraun Thank you for the quick and detailed reply! I'm not surprised our use case seems unusual, as the codebase in question is ~10 years old and has gone through many iterations before I started 😄 . I also have little-to-no appreciation for how
That may be true, and I can't speak to why django CMS was chosen for the blog. Probably because we use the CMS on other pages and this reduced implementation complexity.
I'm not sure I understand why that would be. Our internal A/B system is also somewhat extensive, and may indeed cover for this.
That may be true, but even in this case the custom context wouldn't be able to be applied to
This is certainly an option. However, across the site, A/B test data is computed and added to the context in the appropriate view, not in a template tag, so this would add some complexity for other developers.
This would also be possible, but would also deviate from the usual process of having these variables available in the top-level context, which would add a bit of friction for other developers.
In our case I think this may require fairly significant refactoring of other views.
In this particular case, the A/B content doesn't need to be edit-able, and would add some more complexity to the implementation. In summary, this proposed change is essentially a nice-to-have feature for our particular use case of Django CMS. I couldn't find any other information on this online, and saw that the change required to |
Yeah this seems rather niche, but the changes aren't drastic. Furthermore, I used to work with a codebase that did something with cms which it really shouldn't - so I understand the situation. My first thought was that perhaps a context processor or middleware might be more suitable. Failing that you could monkeypatch You could probably even do it without monkey-patching. As Fabian said, you could implement the same logic in your application. So looking at |
Description
In the codebase I work on, we use
django-cms
to manage our blog. In particular we have aBlogView
class that calls thedetails()
function to render a blog article. Recently, we wanted to customize the template context of the rendered article (in our case, to add a variable with metadata about an A/B test).However, the
details()
function doesn't allow for a custom context to be specified. To allow this, we'd need to parameterizecontext
in therender_page()
function. Currently, that function always starts with an emptycontext = {}
. This change maintains that default, but allows the user to specify other variables in the template context.To work around this limitation, we found that we could intercept the response returned by
details()
and if it's aTemplateResponse
, update the context data. Here's the code:However, this relies on the particular implementation of the
details()
function, and the fact thatTemplateResponse.context_data
is a public, writable-field. It would be cleaner ifdetails()
itself accepted this context parameter.Checklist
develop-4