PreviewableMixin compatibility for settings #13050
Conversation
SebCorbin
added
status:Needs Review
component:Preview
There was a problem hiding this comment.
Hey @SebCorbin, thanks for picking this up (with tests)! I think this'd be a great feature to have for 7.0, but given the feature freeze is today I'm having doubts, especially around whether we should automatically handle the context processor or not for the initial rollout of the feature.
I think we can also enable the accessibility checks side panel here, which should be fairly simple to add (I'll add a commit and push).
Although, I do wonder how useful this is if we don't also somehow make the context processor automatically resolve to the setting that's currently being edited, so that users can reuse their existing template and have things automatically be reflected in the live preview?
Have you tried this with your use case, and if so, how do you find the DX of having to specify different templates (or make your templates somehow handle whether to use the live setting vs the one being previewed)?
FWIW I was able to hack something together for the context processor, which seems to work nicely:
diff --git a/wagtail/contrib/settings/context_processors.py b/wagtail/contrib/settings/context_processors.py
index 83722d8a03..9b35009343 100644
--- a/wagtail/contrib/settings/context_processors.py
+++ b/wagtail/contrib/settings/context_processors.py
@@ -43,6 +43,16 @@ class SettingModuleProxy(dict):
"""
Get a setting instance
"""
+ if (
+ (previewed := getattr(self.request_or_site, "_previewed_setting", None))
+ and isinstance(previewed, (BaseGenericSetting, BaseSiteSetting))
+ and previewed._meta.app_label == self.app_label
+ and previewed._meta.model_name == model_name
+ ):
+ # If the specified setting is being previewed,
+ # return the previewed setting
+ return previewed
+
Model = registry.get_by_natural_key(self.app_label, model_name)
if Model is None:
raise RuntimeError(
diff --git a/wagtail/contrib/settings/models.py b/wagtail/contrib/settings/models.py
index 290425ca84..f5b06e0141 100644
--- a/wagtail/contrib/settings/models.py
+++ b/wagtail/contrib/settings/models.py
@@ -3,7 +3,7 @@ from django.utils.functional import cached_property
from django.utils.translation import gettext as _
from wagtail.coreutils import InvokeViaAttributeShortcut
-from wagtail.models import Site
+from wagtail.models import PreviewableMixin, Site
from .registry import register_setting
@@ -208,3 +208,11 @@ class BaseGenericSetting(AbstractSetting):
def __str__(self):
return str(self._meta.verbose_name)
+
+
+class PreviewableSettingMixin(PreviewableMixin):
+ def serve_preview(self, request, mode_name):
+ # Attach the setting to the request so that it can be accessed by the
+ # context processor to swap out the setting in the template.
+ request._previewed_setting = self
+ return super().serve_preview(request, mode_name)Then you'd use PreviewableSettingMixin instead of PreviewableMixin, and then add something like the following to the setting model:
def get_preview_template(self, request, mode_name):
return HomePage.template
def get_preview_context(self, request, mode_name):
return {
**super().get_preview_context(request, mode_name),
"page": HomePage.objects.first(),
}However, given the feature freeze is today, I'm not sure if it'd be wise to add it so last-minute.
wagtail/test/testapp/models.py
Outdated
There was a problem hiding this comment.
While it's certainly nice to have tests, I don't think we necessarily need to have all the combinations here... the snippet models should already handle them and since we don't do any changes in that area I don't see much value compared to having one previewable setting model.
There was a problem hiding this comment.
Done, I only kept one model for testing purposes.
laymonage
force-pushed
the
11347-previewablemixin-compatibility-for-settings
branch
from
572c73a to
d5c3ed4
Compare
|
@SebCorbin @laymonage just wanted to check ahead of time how we feel about shipping this in time for the 7.1 release in August? Current feature freeze target is mid-July |
@thibaudcolas I'd be happy to get this in with some help. I think it currently needs a design decision on whether the "hack" I described above for the context processor is acceptable or if there's an alternative approach we'd like to do. Or we can also drop the idea entirely and let developers handle how the previewed setting is accessed. |
|
I'll try to work a bit on this, I don't remember adding some specific behavior so that the new values are taken in the template, maybe the cache around |
|
So indeed we worked around the context processor by injecting the edited object directly into the settings cache, meaning it would work either in the context processor or any subsequent class PreviewableSettingMixin(PreviewableMixin):
def get_preview_context(self, request, mode_name):
# Inject current instance into request
setattr(request, self.get_cache_attr_name(), self)
return super().get_preview_context(request, mode_name) |
|
Ah, thanks @SebCorbin! I didn't realise the setting model already has a mechanism to cache its instance to the request. That makes sense, I think we can proceed with your approach and include We'll need tests to ensure the previewed instance is indeed the one being loaded in the preview view, though. |
SebCorbin
force-pushed
the
11347-previewablemixin-compatibility-for-settings
branch
from
645d0b6 to
7629dc8
Compare
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
Fixes #11347
The tests are highly taken from model's tests