dominno / django-moderation

django-moderation is reusable application for Django framework, that allows to moderate any model objects.
BSD 3-Clause "New" or "Revised" License
269 stars 90 forks source link

Sites framework is required for using this package #213

Closed MaybeThisIsRu closed 2 years ago

MaybeThisIsRu commented 2 years ago

Sites framework offered by Django is optional but it seems this package requires that it be installed and configured:

Django comes with an optional “sites” framework. It’s a hook for associating objects and functionality to particular websites, and it’s a holding place for the domain names and “verbose” names of your Django-powered sites.

Using django-moderation without sites results in:

Exception in thread django-main-thread:
Traceback (most recent call last):
  File "/Users/djangouser/.asdf/installs/python/3.9.6/lib/python3.9/threading.py", line 973, in _bootstrap_inner
    self.run()
  File "/Users/djangouser/.asdf/installs/python/3.9.6/lib/python3.9/threading.py", line 910, in run
    self._target(*self._args, **self._kwargs)
  File "/Users/djangouser/.local/share/virtualenvs/project-3SAjfhh8/lib/python3.9/site-packages/django/utils/autoreload.py", line 64, in wrapper
    fn(*args, **kwargs)
  File "/Users/djangouser/.local/share/virtualenvs/project-3SAjfhh8/lib/python3.9/site-packages/django/core/management/commands/runserver.py", line 110, in inner_run
    autoreload.raise_last_exception()
  File "/Users/djangouser/.local/share/virtualenvs/project-3SAjfhh8/lib/python3.9/site-packages/django/utils/autoreload.py", line 87, in raise_last_exception
    raise _exception[1]
  File "/Users/djangouser/.local/share/virtualenvs/project-3SAjfhh8/lib/python3.9/site-packages/django/core/management/__init__.py", line 375, in execute
    autoreload.check_errors(django.setup)()
  File "/Users/djangouser/.local/share/virtualenvs/project-3SAjfhh8/lib/python3.9/site-packages/django/utils/autoreload.py", line 64, in wrapper
    fn(*args, **kwargs)
  File "/Users/djangouser/.local/share/virtualenvs/project-3SAjfhh8/lib/python3.9/site-packages/django/__init__.py", line 24, in setup
    apps.populate(settings.INSTALLED_APPS)
  File "/Users/djangouser/.local/share/virtualenvs/project-3SAjfhh8/lib/python3.9/site-packages/django/apps/registry.py", line 122, in populate
    app_config.ready()
  File "/Users/djangouser/.local/share/virtualenvs/project-3SAjfhh8/lib/python3.9/site-packages/django/contrib/admin/apps.py", line 27, in ready
    self.module.autodiscover()
  File "/Users/djangouser/.local/share/virtualenvs/project-3SAjfhh8/lib/python3.9/site-packages/django/contrib/admin/__init__.py", line 24, in autodiscover
    autodiscover_modules('admin', register_to=site)
  File "/Users/djangouser/.local/share/virtualenvs/project-3SAjfhh8/lib/python3.9/site-packages/django/utils/module_loading.py", line 47, in autodiscover_modules
    import_module('%s.%s' % (app_config.name, module_to_search))
  File "/Users/djangouser/.asdf/installs/python/3.9.6/lib/python3.9/importlib/__init__.py", line 127, in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
  File "<frozen importlib._bootstrap>", line 1030, in _gcd_import
  File "<frozen importlib._bootstrap>", line 1007, in _find_and_load
  File "<frozen importlib._bootstrap>", line 986, in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line 680, in _load_unlocked
  File "<frozen importlib._bootstrap_external>", line 850, in exec_module
  File "<frozen importlib._bootstrap>", line 228, in _call_with_frames_removed
  File "/Users/djangouser/.local/share/virtualenvs/project-3SAjfhh8/lib/python3.9/site-packages/moderation/admin.py", line 17, in <module>
    from .helpers import automoderate
  File "/Users/djangouser/.local/share/virtualenvs/project-3SAjfhh8/lib/python3.9/site-packages/moderation/helpers.py", line 1, in <module>
    from .register import RegistrationError
  File "/Users/djangouser/.local/share/virtualenvs/project-3SAjfhh8/lib/python3.9/site-packages/moderation/register.py", line 9, in <module>
    from .moderator import GenericModerator
  File "/Users/djangouser/.local/share/virtualenvs/project-3SAjfhh8/lib/python3.9/site-packages/moderation/moderator.py", line 2, in <module>
    from django.contrib.sites.models import Site
  File "/Users/djangouser/.local/share/virtualenvs/project-3SAjfhh8/lib/python3.9/site-packages/django/contrib/sites/models.py", line 78, in <module>
    class Site(models.Model):
  File "/Users/djangouser/.local/share/virtualenvs/project-3SAjfhh8/lib/python3.9/site-packages/django/db/models/base.py", line 113, in __new__
    raise RuntimeError(
RuntimeError: Model class django.contrib.sites.models.Site doesn't declare an explicit app_label and isn't in an application in INSTALLED_APPS.

Is there any particular reason we assume that sites is mandatory and this package cannot be used without it?

Python 3.9, Django 3.2.x

MaybeThisIsRu commented 2 years ago

It appears to me that sites is only used on the send and send_many methods which sends emails to informs users of new moderation items OR accept/reject status for a moderation item. They populate context which is then used to render a template — none of the templates however use the site context variable.

Where the Site package is imported: https://github.com/dominno/django-moderation/blob/master/moderation/moderator.py#L2

Where it is used: https://github.com/dominno/django-moderation/blob/master/moderation/moderator.py#L138-L202

Templates: notification_subject_user.txt, notification_subject_moderator.txt, notification_message_user.txt, notification_message_moderator.txt

Could probably write a helper/util method to check INSTALLED_APPS in django.conf.settings and act accordingly. Then we make it so that it is only included in the context by default if sites app is installed by the user.

With this approach, anyone who relies on the site context variable for their templates (in case they have over-ridden it) will not be left hanging.

If this sounds about right, let me know and I'll try to PR.

DmytroLitvinov commented 2 years ago

Good question. Thank you for providing informative text 👍

For me, it sounds reasonable to make it optionally as you proposed. For that change, I would like also to hear from @dominno. Do you think we need to do it and release in new version?