Reset settings and fix for AppConfig.ready #128
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #128 +/- ##
==========================================
- Coverage 85.51% 85.45% -0.07%
==========================================
Files 21 21
Lines 435 433 -2
==========================================
- Hits 372 370 -2
Misses 63 63
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
@ar0ne thank you for this PR, could you please split it into two smaller PRs, one for the fix and the other one for the admin integration (this one with a separate test module)? |
|
@ar0ne if you're still interested in working on this PR, please rebase it. |
|
@fabiocaccamo hi, sure i hope to find some time on the weekend to address you comments |
ar0ne
force-pushed
the
reset_settings
branch
3 times, most recently
from
f37c426 to
80206a7
Compare
renamed method, added unit tests
| @@ -22,6 +22,7 @@ config and manage typed extra settings using just the django admin. | |||
| ## Installation | |||
| - Run `pip install django-extra-settings` | |||
| - Add `extra_settings` to `settings.INSTALLED_APPS` | |||
| - Run `python manage.py reset_extra_settings` | |||
There was a problem hiding this comment.
I'm pretty sure that running this command before migrate on first install will raise an Exception.
| except (OperationalError, ProgrammingError): | ||
| pass | ||
|
|
||
| post_migrate.connect(Setting.set_defaults_from_settings, sender=self) |
There was a problem hiding this comment.
Removing this line current tests fail.
There was a problem hiding this comment.
I think that these tests are not enough and they don't cover well real usage, I try to explain better what I mean:
- Tests pass because
Setting.set_defaults_from_settings()is invoked manually (it should not) Setting.set_defaults_from_settings()should be called automatically at some point at the begininng, but now calling it on app ready is discouraged.- Should improve the tests using more
EXTRA_SETTINGS_DEFAULTS
name: Remove db calls from app ready hook
about: Submit a pull request for this project
assignees: fabiocaccamo
Describe your changes
First of all, Django "doesn't recommend" to run db calls in app ready hook (https://django.readthedocs.io/en/latest/ref/applications.html#django.apps.AppConfig.ready)
Instead, I'd like to explicitly ask users to set default values. For that they could use new dj-command:
Additionally, I added a button to "changelist" admin page.
Related issue
110
121
Checklist before requesting a review