django-siteprefs icon indicating copy to clipboard operation
django-siteprefs copied to clipboard

More monkey magic

Open jayvdb opened this issue 4 years ago • 10 comments

This app is one of the best 'app config' solutions that I have found.

It would be great, and I expect not to difficult, for this app to add the same functionality as https://github.com/baranbartu/onthefly. Note my PRs on https://github.com/baranbartu/onthefly/pulls to make it semi-usable.

The existing monkey patching being done here isnt too different from what they are doing, and can be used to 'grandfather' in other apps which dont yet support siteprefs, and even a subset of Django settings where it is verified that the monkey patching is effective. I think being able to use siteprefs to configure most apps would make this app ubiquitous, which would then cause more apps to explicitly use siteprefs in order to work better ;-)

jayvdb avatar May 26 '20 13:05 jayvdb

Hi. Thank you. And what exact features do you want to see?

idlesign avatar May 27 '20 10:05 idlesign

Prefs of all install apps automatically added to Django admin; is that asking too much ? ;-)

jayvdb avatar May 27 '20 14:05 jayvdb

Hm. But autodiscovery is already handled out of the box. SITEPREFS_MODULE_NAME setting is taken from project's settings.py and all apps are seatched for settings module (defaults to settings.py).

On may be I misunderstood your needs?

idlesign avatar May 28 '20 05:05 idlesign

I read the code, got a bit excited as I could see it was intended to be doing this, but getting it working is a bit challenging.

I expected that "most" of the settings in my global <project>.settings file would automatically be visible in the admin interface, somewhere, and I could change "most" of them and see the effect. I did expect some apps will have a cached copy, and changing those in siteprefs wouldnt have effect.

I tried with Django 2.2 and Django 3.0, and on Python 3.8. Maybe the stack frames are different on these environments, but the tox.ini suggests this is at least being tested. I havent checked how extensive the test suite is.

I've raised some issues around possible reasons I have struggled. And I am keen to help fix those with patches if you dont have time/motivation, as onthefly is not a great solution, but it works for me, which is a significant factor in its favour at the moment.

If I explicitly add autodiscover_siteprefs(admin.site) to urls.py, I get 'Preferences' in Django Admin appearing twice for app which are using siteprefs.

If I do not add it to urls.py, the startup never goes through if 'self' not in settings_locals: in utils.py

So I currently have set SITEPREFS_DISABLE_AUTODISCOVER = False and explicit invocation in urls.py.

However I dont see any magic occurring, except for two local apps where I have set up siteprefs.

More about my use-case.

As an example, django-oscar does not have a settings module. It only uses django.conf.settings. It has settings which are to be imported into the global conf using from oscar.defaults import * . I have some overrides as well in my global settings.py . Then I try using siteprefs admin interface 'Add preference', and the UI form leaves me wondering what I do here. There is no oscar.foo to be used for "Application". I try "project_name" which I hope will find the global "project_name.settings". For "name", I try "OSCAR_SHOP_NAME" and value is anything. No effect. Restart the app server; still no effect.

Other apps like oscar which use django.conf.settings directly without their own settings module, include django-health-check, django-qsessions, django-anonymoususer-permissions.

Many packages use django-appconf, but that doesnt provide an admin UI. It has enough metadata that it should be possible to make its settings dynamic, but there are so many helpers like this - explicitly supporting each one is a game of whack-a-mole.

Even leaving auto-discovery aside, I would like to build a PY file which uses prefs.group/one/etc to explicitly provide the UI & names of only the prefs of installable apps which I want to have in the admin UI for managers to use, after I have tested that the voodoo with siteprefs does work as expected with that installable app.

jayvdb avatar May 28 '20 09:05 jayvdb

autodicover is triggered by default in https://github.com/idlesign/django-siteprefs/blob/master/siteprefs/config.py#L19 There should be no need for explicit discovery in recent Django versions.

if 'self' not in settings_locals is meant for project wide settings, as opposed to app settings.

If I understood correctly, you want to be able to make dynamic and override project wide settings provided by thirdparty apps which are out ot your control. So basically you want the same outcome as if thirdparty apps support siteprefs out-of-box.

This indeed will require some thoughts before implementation. Patching third parties can be error prone, but if you're willing to provide a prototype that we could discuss, it'll be nice.

idlesign avatar May 28 '20 10:05 idlesign

So basically you want the same outcome as if thirdparty apps support siteprefs out-of-box.

I expect it wont be as good, and may not work at all, but rudimentary support would be awesome.

Patching third parties can be error prone, but if you're willing to provide a prototype that we could discuss, it'll be nice.

Already done ;) https://github.com/baranbartu/onthefly/pulls/jayvdb

Their monkey patching is a bit reckless, and I dont want to sink lots of my time into it, esp as there has been no feedback on my issues or patches there. I looked through your monkey patching and saw it has lots of similarities, but you started with a much better design.

But I am happy to try get a POC for transplanting that functionality.

jayvdb avatar May 28 '20 12:05 jayvdb

Already done ;) https://github.com/baranbartu/onthefly/pulls/jayvdb

Hm. I can there only two PR's: 5 and 6. One about ordering and the other is on serialization. And that obviously is not what we're discussing here -- not about patching thirdparties for sure %)

idlesign avatar May 28 '20 12:05 idlesign

Correct. It already has the basic functionality, working with redis, but a bit buggy so hard to demo. https://github.com/baranbartu/onthefly#screenshot . I havent tried the file backend available in the pulls.

I also have another patch to not depend on adminplus https://github.com/jayvdb/onthefly/commit/a38d3edf23f03224a44025a3e3af7ddcc030ff8c ; after that you need to add your own url route for its view.

To get them all, install git+https://github.com/jayvdb/onthefly/commits/all

jayvdb avatar May 28 '20 13:05 jayvdb

The 'proper' way to do project wide settings still mystify me a little

However https://github.com/idlesign/django-siteprefs/pull/24 means I can include the VAR=val and pref UI definitions in my <project>/settings.py .

I am still using the explicit SITEPREFS_DISABLE_AUTODISCOVER = False and explicit invocation of autodiscover_siteprefs(admin.site) in urls.py.

I now have OSCAR_SHOP_NAME = 'invalid' in my project wide ``settings.py, and a siteprefs UI definition in the same settings.py to allow it to be set in the DB as project.oscar_shop_name. With https://github.com/idlesign/django-siteprefs/pull/25 , that DB sitepref is then injected back into django.conf.settings.OSCAR_SHOP_NAME, and Oscar shows the DB value. Hooray!

The structure I would like is

  • app1:
    • settings.py - defaults and UI defs
  • <project>:
    • settings.py - Django and third-party settings where necessary and not suitable for live modification.
    • project_preferences.py - UI definitions for project wide settings

There are all sorts of config items from third party apps which it would be nice to expose for frontend devs & operational managers to fiddle with (e.g. colors), but I dont want them cluttering up an already large <project>/settings.py.

For that structure, I think I need https://github.com/idlesign/django-siteprefs/issues/22

I can emulate it a bit, by having <project>/settings.py import <project>/project_preferences.py

jayvdb avatar May 29 '20 06:05 jayvdb

Hooray!

Congratulations!

The 'proper' way to do project wide settings still mystify me a little

Yeah, it might be. The thing is there are at least two project layouts widely used: apps on project level, and apps inside project. It seems to me that one of this cases is not currently covered by autodiscovery. We should investigate it first.

If indeed so, we'd better try to support the layout, rather than moving imports as in #24.

*Next two-three days I'll be out of reach, nevertheless feel free to add information here.

idlesign avatar May 29 '20 14:05 idlesign