Move wagtailsettings into contrib module #1754
Conversation
|
|
||
|
|
||
| @register.simple_tag(takes_context=True) | ||
| def get_settings(context, use_default_site=False): |
There was a problem hiding this comment.
Could this be turned into an assignment_tag? allowing the following syntax {% get_settings as wagtail_settings %}
There was a problem hiding this comment.
NB: assignment_tag is deprecated in Django 1.9, where simple_tag will gain support for the 'as' syntax.
There was a problem hiding this comment.
This could be implemented for get_settings, but I don't see the template tag being used much. It is only as a fallback in case you are in a strange situation. With the pending deprecation that @nealtodd pointed out, supporting assignment tags across multiple versions of Django will become quite annoying. I do not think it is worth the bother.
There was a problem hiding this comment.
ok, that's fine by me.
Just note that when the "as" syntax arrives, this tag will fill the specified variable with a blank string which may not be what the developer expects.
mx-moth
force-pushed
the
merge-settings
branch
from
1ab5439
to
2047f45
Compare
mx-moth
force-pushed
the
merge-settings
branch
from
2047f45
to
c07c980
Compare
|
|
||
| {{ settings.app_label.SocialMediaSettings.instagram }} | ||
|
|
||
| If you are not in a ``RequestContext``, then context processors will not have run, and the ``settings`` variable will not be availble. To get the ``settings``, use the provided ``{% get_settings %}`` template tag. If a ``request`` is in the template context, but for some reason it is not a ``RequestContext``, just use ``{{ get_settings }}``: |
There was a problem hiding this comment.
Presumably this last {{ get_settings }} should be {% get_settings %}?
|
The menu items registered by https://github.com/torchbox/wagtail/pull/1754/files#diff-45699f1eed7357d14bd23f5b9b73a817R30 need an It would be nice (but not a blocker) to register these models to appear within the groups permission interface too, via the |
|
|
||
|
|
||
| def user_can_edit_setting_type(user, model): | ||
| """ Check if a user has any permission related to a content type """ |
There was a problem hiding this comment.
The practice of saying "allow users to access this area if they have any permission over model X" is something that we've tightened up a lot in Wagtail 1.1 - we now tend to list the specific permissions, e.g. https://github.com/torchbox/wagtail/blob/master/wagtail/wagtailredirects/views.py#L17. (Amongst other things, this opens up the possibility of using permissions for 'lesser' tasks that don't require access to the model's admin area - "can download document", "can link to image" and so on.)
In the case of settings, it probably makes sense for change_[foo] to be the only permission type in use - so that would be the one that gets checked in the view and MenuItem.is_shown, and the one that gets passed to register_permissions to appear in the groups admin (if we go ahead with that).
|
One thing that's quite troublesome here is how, when you edit settings, the site is picked up from the hostname you're accessing the admin from (https://github.com/torchbox/wagtail/pull/1754/files#diff-ffdb5fc31a093a7b481589b85744df46R42). To me, that goes against the design of Wagtail admin as a single unified interface for managing all your sites (such as what you see when you go to the top level of the page tree). The redirects module previously did something like this, which was backed out in 1.1 as it was causing confusion (#1461) - and there's also some demand for being able to host wagtailadmin on a completely separate hostname to your frontend sites (#1807, as well as the whole pattern of static site generation a la wagtailmedusa / wagtailbakery). Not entirely sure what the neatest way of addressing this is. One possibility is for the menu item to take you straight to the edit form if only one site exists, and to an intermediate "select a site" listing otherwise - but that's a bit clunky. Maybe a dropdown in the header bar of the edit form, where you can select the appropriate site? |
|
I'm in the middle of exams right now, so I will not be able to get to these changes in the next few days. Hopefully I can find some time on Wednesday after all my exams. |
|
@timheap No worries - thanks for your work on this. Good luck with the exams! |
|
Alright, a bunch of fixes pushed: docs typo, better permissions intgration, and the ability to switch sites using a form in the header. I've pushed these as separate commits for ease of reviewing, but they can be squashed by someone (myself or you) before being merged. |
The `wagtailsettings` module is useful enough that it should be included in the Wagtail contrib section, to make it available to all Wagtail developers. All the code has been given a once-over to make sure it is nice and polished before being copied in. As such, this is not a direct copy of the `wagtailsettings` module. It should be backwards compatible though, excepting the new location. It has been moved to `wagtail.contrib.settings`, following the naming scheme set out in wagtail#1504. Documentation has been concatenated in to a single page, and added to the contrib reference section.
Instead of checking for any kind of permission, the change permission is used to allow/disallow peoples access to change a setting. The change permission for each setting has added to the Group editing view with `register_permission`.
mx-moth
force-pushed
the
merge-settings
branch
from
6a95e61
to
5d81269
Compare
|
Also rebased on master. Does Travis run tests on the resulting merge with master? Travis failed with a 'conflicting migrations' error from Django, which could only have happened through a merge/rebase with master. |
|
Thanks! |
|
👍 |
The
wagtailsettingsmodule is useful enough that it should be included in the Wagtail contrib section, to make it available to all Wagtail developers.All the code has been given a once-over to make sure it is nice and polished before being copied in. As such, this is not a direct copy of the
wagtailsettingsmodule. It should be backwards compatible though, excepting the new location.It has been moved to
wagtail.contrib.settings, following the naming scheme set out in #1504.Documentation has been concatenated in to a single page, and added to the contrib reference section.