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

Add ability to disable registration of CRX layout and analytics settings #623

Merged

Conversation

michaelsteigman
Copy link
Contributor

Allow developers who need to store additional site-specific settings and do not wish to have multiple "Settings" menus to disable the default models and create custom settings models.

@vsalvino
Copy link
Contributor

vsalvino commented Feb 14, 2024

Thanks for doing this work. I think this implementation may be a bit over-engineered though. I don't plan on supporting having users create drop-in replacement settings models, as that could also get quite messy.

My though would be a Django setting which simply does not register our settings models in the Wagtail Admin (UI hides them in the UI). However the models would still be intact as-is. As you've probably learned, too much stuff touches those models that the change of breakage becomes extremely high if someone customizes them.

The assumption would be if you are disabling them in the Wagtail Admin, you'll need to customize the templates in which those settings are used, otherwise you'll end up getting the default values.

@michaelsteigman
Copy link
Contributor Author

Thanks for doing this work. I think this implementation may be a bit over-engineered though. I don't plan on supporting having users create drop-in replacement settings models, as that could also get quite messy.

My though would be a Django setting which simply does not register our settings models in the Wagtail Admin (UI hides them in the UI). However the models would still be intact as-is. As you've probably learned, too much stuff touches those models that the change of breakage becomes extremely high if someone customizes them.

I admit I was having fun and got carried away! It did exactly what I was hoping (and would not have affected existing installs) but I realize it was more than what you were expecting. A simple mechanism to disable the registration of the CRX models can also work for us. Just means a little more duplication on our side.

The assumption would be if you are disabling them in the Wagtail Admin, you'll need to customize the templates in which those settings are used, otherwise you'll end up getting the default values.

I started working on a template tag to replace the settings references in the templates but stopped short of going there!

I've backed out everything except the decorator that conditionally will register the layout and analytics models. Let me know if this will work for you.

@michaelsteigman
Copy link
Contributor Author

@vsalvino - back to bump this. Is current PR acceptable or did you have another approach in mind?

@vsalvino
Copy link
Contributor

Thanks, this looks great! Appreciate the tests as well! I am going to do a bit of modification and split this setting into a couple individual settings.

@vsalvino vsalvino changed the base branch from main to disable-settings May 18, 2024 16:19
@vsalvino vsalvino merged commit 836ed97 into coderedcorp:disable-settings May 18, 2024
9 of 16 checks passed
@vsalvino vsalvino added this to the 4.0 milestone May 18, 2024
vsalvino added a commit that referenced this pull request May 21, 2024
Building on the work from #623 

See settings:
* CRX_DISABLE_LAYOUT (not recommended)
* CRX_DISABLE_ANALYTICS
* CRX_DISABLE_NAVBAR
* CRX_DISABLE_FOOTER

The plan, based on what @jlchilders11 and I discussed, is to disable
navbar and footer in the `pro` template, and provide a simple
boilerplate navbar and footer for customization.

Note: template filters `crx_settings` and `django_settings` have been
removed, and replaced with tag ` {% django_setting "MY_SETTING" %}`.

---------

Co-authored-by: Michael Steigman <msteigman@mgh.harvard.edu>
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

2 participants