Skip to content
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

Open
wants to merge 1 commit into
base: develop-4
Choose a base branch
from

Conversation

micahcantor
Copy link

@micahcantor micahcantor commented Mar 19, 2024

Description

In the codebase I work on, we use django-cms to manage our blog. In particular we have a BlogView class that calls the details() 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 parameterize context in the render_page() function. Currently, that function always starts with an empty context = {}. 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 a TemplateResponse, update the context data. Here's the code:

class BlogView(H2PushView, TemplateView):
    entry_points = ["blog"]

    def get_context_data(self, *args, **kwargs):
        ...

    def get(self, request: HttpRequest, *args, **kwargs):
        slug = request.path.strip("/")
        response = details(request, slug=slug)
        if isinstance(response, TemplateResponse):
            response.context_data.update(self.get_context_data())
        return response

However, this relies on the particular implementation of the details() function, and the fact that TemplateResponse.context_data is a public, writable-field. It would be cleaner if details() itself accepted this context parameter.

Checklist

  • I have opened this pull request against develop-4
  • I have added or modified the tests when changing logic
  • I have followed the conventional commits guidelines to add meaningful information into the changelog
  • I have read the contribution guidelines and I have joined #workgroup-pr-review on Slack to find a “pr review buddy” who is going to review my pull request.

@micahcantor micahcantor changed the title Feat: Allow custom context when rendering a view feat: Allow custom context when rendering a view Mar 19, 2024
Copy link

@github-actions github-actions bot left a 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 ⛵️!

Copy link
Author

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

@fsbraun
Copy link
Sponsor Member

fsbraun commented Mar 19, 2024

@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 views.details - django CMS central view function - is not for reusing within other views. Indeed, it may change in structure.

Your use case seems highly unusual in several senses:

  • django CMS' page tree seems not optimal for blog posts. Many people use djangocms-blog.
  • Your way of a/b testing will hamper with caching
  • Your view probably needs only some bits of the details logic. Why not copy those bits from the detail view? It's essentially get_page_from_request and render_page.

Finally, let me point out that your implementation is prone to have side-effects since you use a single {} object to initialize all context objects. This will leave context entries from previous pages displayed as a remainder for other pages.

To implement A/B testing, I'd suggest taking one of the more traditional routes:

  • Create a custom template tag that can add elements to the context or directly change what is rendered
  • Add a property to the request object to make a/b selections (available as request in the context)
  • Add a context processor for A/B selection
  • Create custom plugins that allow displaying different content (which would also allow editors to change both a and b content!)

@marksweb What's your opinion?

@micahcantor
Copy link
Author

micahcantor commented Mar 19, 2024

@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 django-cms is used outside of our codebase, this is just something I ran into. Given that, here's some additional context that may help:

django CMS' page tree seems not optimal for blog posts. Many people use djangocms-blog.

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.

Your way of a/b testing will hamper with caching

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.

Your view probably needs only some bits of the details logic. Why not copy those bits from the detail view? It's essentially get_page_from_request and render_page.

That may be true, but even in this case the custom context wouldn't be able to be applied to render_page.

Create a custom template tag that can add elements to the context or directly change what is rendered

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.

Add a property to the request object to make a/b selections (available as request in the context)

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.

Add a context processor for A/B selection

In our case I think this may require fairly significant refactoring of other views.

Create custom plugins that allow displaying different content (which would also allow editors to change both a and b content!)

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 django-cms is relatively minimal. However, if maintainers believe this added flexibility wouldn't be particularly useful for other developers, then feel free to close this PR -- no hard feelings!

@marksweb
Copy link
Member

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 details, render_pagecontent and render_page to accept your context.

You could probably even do it without monkey-patching. As Fabian said, you could implement the same logic in your application. So looking at BlogView, I might be tempted to setup a class which had it's own version of details and render_page so that they work how you want them to, even if we end up making changes to them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants