coderedcms icon indicating copy to clipboard operation
coderedcms copied to clipboard

Make built-in models abstract

Open vsalvino opened this issue 6 years ago • 8 comments

If we want to continue using CODERED_FRONTEND_* settings (which I think we do, they will be the foundation of themes), we will have to make everything that uses our streamfield blocks abstract.

The issue is, many coderedcms models use streamfield blocks that pull values from CODERED_FRONTEND_* settings. Which means if you change one of these settings in your project, it triggers a migration in coderedcms. Which is a bad practice and leads to endless migrations.

This fits in with the paradigm we have been following with the page types. Provide beefy abstract models, and provide a very basic concrete model boilerplate in the project template.

One caveat to making the snippets abstract, is that we will no longer be able to provide a snippet chooser, or hard-code templates that pull in navbar/footer snippets. So this will have to be thought through to figure out how to gracefully handle all of this without re-defining everything in the project template (which would be too much boilerplate I think).

Anyhow, this pretty major regarding future stability. Implementing it will inevitably be a breaking change. But there is no other way to have dynamic blocks and themes. So I think it is necessary.

vsalvino avatar Dec 15 '18 00:12 vsalvino

I have to read into it more, but we can probably leverage GenericForeignKeys for the abstract snippets/choosers. https://docs.djangoproject.com/en/2.1/ref/contrib/contenttypes/#generic-relations

corysutyak avatar Jan 11 '19 05:01 corysutyak

Also see related issue: #8

vsalvino avatar Aug 07 '19 17:08 vsalvino

what about using django-polymorphic instead of generic relations?

auvipy avatar Dec 16 '19 08:12 auvipy

@vsalvino Following up on this old open issue. We're looking at CRX for use on a multi tenant type install and wanted to add some some fields as BasicSiteSettings. I realized that CRX provides a nice set of layout and analytics settings but I can't extend them due to their concrete nature. Although it appears as if the concerns referenced in the issue description may have been addressed in #496, I am curious if consideration was given to switching LayoutSettings and AnalyticsSettings to abstract models?

michaelsteigman avatar Jan 16 '24 18:01 michaelsteigman

@michaelsteigman LayoutSettings and AnalayticsSettings both support multi-site now. For example, you can use two different Google Analytics codes, two different logos, etc. on the multi-sites. All of our snippets, such as Navbar, Footer, etc. also support multi-site.

However, they are not abstract, so you cannot change or extend them. But the good news is these settings are conveniences, and are not "important" in the sense that you can ignore them and the site will still work perfectly fine.

For multi-site situation, I'd highly recommend implementing your own custom navbar and footer, and then implement your own custom settings model. This way, none of those concrete models should get in your way. And you'll probably need a lot of custom business logic anyhow. Then extend our base.html and web_page_notitle.html templates to add your custom tracking, navbar, etc.

vsalvino avatar Jan 16 '24 20:01 vsalvino

Thanks, @vsalvino. I'm aware that the settings are multi-site and aren't critical. The reason I am asking is because I wanted to try to utilize some of those settings. That said, I'm fine building out my own classes to manage settings and building upon CRX settings but what I don't want is multiple settings options in the admin interface. I don't see anything in Wagtail's API that will allow me to "unregister" settings. I bet I could monkey patch it but haven't figured out just how yet. Any clues?

Coming back to the issue at hand, though, these classes really ought to be abstract. Our needs in this area can't be unique. We use a different tracking system and need some other per-site settings. Is this something that was considered? (I forked the repo and was considering taking a crack at it...)

michaelsteigman avatar Jan 16 '24 23:01 michaelsteigman

Abstracting them in a way that is backwards compatible with the many thousands of existing sites is a huge challenge. So it has not yet been attempted as it is usually easier to ignore those settings. We would like to do it, but there are always higher priorities.

It might be possible to add a feature (Django setting) which unregisters them (or rather, never registers them) with the Wagtail admin though. If you want to take a crack at it, that would be useful and we could merge it in.

Specifically for the tracking settings, we did add generic "head" and "body" fields to allow putting anything in there. That has worked well for most of our clients. For anything else, we usually create a setting or snippet prefixed with "Custom" (or the client's name) to make it easy for the client to identify when editing.

If you find you're fighting the system too much, then I recommend just stealing what you want from the codebase and rolling your own wagtail installation. The SEO and Cache are already split out into separate packages.

vsalvino avatar Jan 17 '24 00:01 vsalvino

Abstracting them in a way that is backwards compatible with the many thousands of existing sites is a huge challenge. So it has not yet been attempted as it is usually easier to ignore those settings. We would like to do it, but there are always higher priorities.

Yup. Knew as I was hitting the "Comment" button that what I was suggesting was a heavy lift.

It might be possible to add a feature (Django setting) which unregisters them (or rather, never registers them) with the Wagtail admin though. If you want to take a crack at it, that would be useful and we could merge it in.

This idea was rolling around as I was responding but your comment helped crystalize things a bit. I went back to the settings models and immediately saw what looked like a feasible approach. I've created PR #623 with that implementation. Let me know what you think.

michaelsteigman avatar Feb 13 '24 22:02 michaelsteigman