From 90de9c603a5f52a0329df1534fdf1ba033831c44 Mon Sep 17 00:00:00 2001 From: Jon Dufresne Date: Wed, 3 Jan 2018 04:38:20 -0800 Subject: [PATCH] Remove the SCHEDULER_BASE_CLASSES feature The feature is brittle with buggy edge cases. Use cases can be better solved through other methods such as the EventRelation model. Reduce the large maintenance burden by removing the feature. Results in cleaner code that follows Django reusable app conventions. Closes #357 Closes #310 Closes #306 --- README.md | 38 ------------------ docs/settings.txt | 43 --------------------- requirements.txt | 1 - schedule/admin.py | 5 +-- schedule/models/calendars.py | 8 ++-- schedule/models/events.py | 10 ++--- schedule/models/rules.py | 6 +-- schedule/utils.py | 28 -------------- tests/test_utils.py | 75 +----------------------------------- 9 files changed, 12 insertions(+), 202 deletions(-) diff --git a/README.md b/README.md index 2b745537..b68adc6b 100644 --- a/README.md +++ b/README.md @@ -176,44 +176,6 @@ Default: return calendar.event_set.all() ``` -### SCHEDULER_BASE_CLASSES - -This setting allows for custom base classes to be used on specific models. Useful for adding fields, managers, or other elements. - -Defaults to django.db.models.Model - -Extend all the models using a list: - -``` -SCHEDULER_BASE_CLASSES = ['my_app.models.BaseAbstract1', 'my_app.models.BaseAbstract1'] -``` - -Extend specific models using a dict, where the key is the model class name: - -``` -SCHEDULER_BASE_CLASSES = { - 'Event': ['my_app.models.EventAbstract1', 'my_app..models.EventAbstract2'] - 'Calendar': [my_app.models.CalendarAbstract'] -} -``` - -### SCHEDULER_ADMIN_FIELDS -This complements the SCHEDULER_BASE_CLASSES feature, by allowing fields -added via a base class to be shown in the admin form for that model. - -Example - EventBase adds a 'cost' field, and now the field can be shown -in the admin form too. - -``` -SCHEDULER_BASE_CLASSES = { - 'Event': ['main.models.EventBase'] -} - -SCHEDULER_ADMIN_FIELDS = { - 'Event': [('cost',)] -} - - ### SCHEDULER_PREVNEXT_LIMIT_SECONDS This settings allows to set the upper and lower limit in calendars navigation. diff --git a/docs/settings.txt b/docs/settings.txt index 7f986a9b..7e80bc11 100644 --- a/docs/settings.txt +++ b/docs/settings.txt @@ -57,46 +57,3 @@ example:: get_events(request, calendar): return calendar.event_set.all() - - -SCHEDULER_BASE_CLASSES ---------------- - -This setting allows for custom base classes to be used on specific models. Useful for adding fields, managers, or other elements. - -Defaults to django.db.models.Model - -Extend all the models using a list:: - -SCHEDULER_BASE_CLASSES = ['my_app.models.BaseAbstract1', 'my_app.models.BaseAbstract1'] - - -Extend specific models using a dict, where the key is the model class name:: - -SCHEDULER_BASE_CLASSES = { - 'Event': ['my_app.models.EventAbstract1', 'my_app..models.EventAbstract2'] - 'Calendar': [my_app.models.CalendarAbstract'] -} - -.. _ref-settings-scheduler-base-classes: - - -SCHEDULER_ADMIN_FIELDS ---------------- - -This complements the SCHEDULER_BASE_CLASSES feature, by allowing fields -added via a base class to be shown in the admin form for that model. - -Example - EventBase adds a 'cost' field, and now the field can be shown -in the admin form too. - -``` -SCHEDULER_BASE_CLASSES = { - 'Event': ['main.models.EventBase'] -} - -SCHEDULER_ADMIN_FIELDS = { - 'Event': [('cost',)] -} - -.. _ref-settings-scheduler-admin-fields diff --git a/requirements.txt b/requirements.txt index 4acb8bd6..ecb34f80 100644 --- a/requirements.txt +++ b/requirements.txt @@ -4,7 +4,6 @@ Django>=1.11 flake8 icalendar>=3.8.4 isort -mock python-dateutil>=2.1 pytz>=2013.9 wheel diff --git a/schedule/admin.py b/schedule/admin.py index 8180feae..95eaba37 100644 --- a/schedule/admin.py +++ b/schedule/admin.py @@ -2,7 +2,6 @@ from schedule.forms import EventAdminForm from schedule.models import Calendar, CalendarRelation, Event, Occurrence, Rule -from schedule.utils import get_admin_model_fields class CalendarAdminOptions(admin.ModelAdmin): @@ -13,7 +12,7 @@ class CalendarAdminOptions(admin.ModelAdmin): (None, { 'fields': [ ('name', 'slug'), - ] + get_admin_model_fields('Calendar') + ] }), ) @@ -46,7 +45,7 @@ class EventAdmin(admin.ModelAdmin): ('start', 'end'), ('creator', 'calendar'), ('rule', 'end_recurring_period'), - ] + get_admin_model_fields('Event') + ] }), ) form = EventAdminForm diff --git a/schedule/models/calendars.py b/schedule/models/calendars.py index a59e424c..8c59da01 100644 --- a/schedule/models/calendars.py +++ b/schedule/models/calendars.py @@ -5,17 +5,15 @@ from django.contrib.contenttypes.models import ContentType from django.db import models from django.db.models import Q -from django.db.models.base import ModelBase from django.template.defaultfilters import slugify from django.urls import reverse from django.utils import timezone from django.utils.encoding import python_2_unicode_compatible -from django.utils.six import with_metaclass from django.utils.six.moves.builtins import str from django.utils.translation import ugettext_lazy as _ from schedule.settings import USE_FULLCALENDAR -from schedule.utils import EventListManager, get_model_bases +from schedule.utils import EventListManager class CalendarManager(models.Manager): @@ -106,7 +104,7 @@ def get_calendars_for_object(self, obj, distinction=''): @python_2_unicode_compatible -class Calendar(with_metaclass(ModelBase, *get_model_bases('Calendar'))): +class Calendar(models.Model): ''' This is for grouping events so that batch relations can be made to all events. An example would be a project calendar. @@ -201,7 +199,7 @@ def create_relation(self, calendar, content_object, distinction='', inheritable= @python_2_unicode_compatible -class CalendarRelation(with_metaclass(ModelBase, *get_model_bases('CalendarRelation'))): +class CalendarRelation(models.Model): ''' This is for relating data to a Calendar, and possible all of the events for that calendar, there is also a distinction, so that the same type or kind of diff --git a/schedule/models/events.py b/schedule/models/events.py index 857731e5..30173bf8 100644 --- a/schedule/models/events.py +++ b/schedule/models/events.py @@ -9,18 +9,16 @@ from django.contrib.contenttypes.models import ContentType from django.db import models from django.db.models import Q -from django.db.models.base import ModelBase from django.template.defaultfilters import date from django.urls import reverse from django.utils import timezone from django.utils.encoding import python_2_unicode_compatible -from django.utils.six import with_metaclass from django.utils.translation import ugettext_lazy as _ from django.utils.translation import ugettext from schedule.models.calendars import Calendar from schedule.models.rules import Rule -from schedule.utils import OccurrenceReplacer, get_model_bases +from schedule.utils import OccurrenceReplacer freq_dict_order = { 'YEARLY': 0, @@ -49,7 +47,7 @@ def get_for_object(self, content_object, distinction='', inherit=True): @python_2_unicode_compatible -class Event(with_metaclass(ModelBase, *get_model_bases('Event'))): +class Event(models.Model): ''' This model stores meta data for a date. You can relate this data to many other models. @@ -527,7 +525,7 @@ def create_relation(self, event, content_object, distinction=''): @python_2_unicode_compatible -class EventRelation(with_metaclass(ModelBase, *get_model_bases('EventRelation'))): +class EventRelation(models.Model): ''' This is for relating data to an Event, there is also a distinction, so that data can be related in different ways. A good example would be, if you have @@ -563,7 +561,7 @@ def __str__(self): @python_2_unicode_compatible -class Occurrence(with_metaclass(ModelBase, *get_model_bases('Occurrence'))): +class Occurrence(models.Model): event = models.ForeignKey(Event, on_delete=models.CASCADE, verbose_name=_("event")) title = models.CharField(_("title"), max_length=255, blank=True) description = models.TextField(_("description"), blank=True) diff --git a/schedule/models/rules.py b/schedule/models/rules.py index 61ad9093..e3ddce27 100644 --- a/schedule/models/rules.py +++ b/schedule/models/rules.py @@ -5,14 +5,10 @@ WEEKLY, YEARLY, ) from django.db import models -from django.db.models.base import ModelBase from django.utils.encoding import python_2_unicode_compatible -from django.utils.six import with_metaclass from django.utils.six.moves.builtins import str from django.utils.translation import ugettext_lazy as _ -from schedule.utils import get_model_bases - freqs = (("YEARLY", _("Yearly")), ("MONTHLY", _("Monthly")), ("WEEKLY", _("Weekly")), @@ -23,7 +19,7 @@ @python_2_unicode_compatible -class Rule(with_metaclass(ModelBase, *get_model_bases('Rule'))): +class Rule(models.Model): """ This defines a rule by which an event will recur. This is defined by the rrule in the dateutil documentation. diff --git a/schedule/utils.py b/schedule/utils.py index dc8edaf4..564d3791 100644 --- a/schedule/utils.py +++ b/schedule/utils.py @@ -4,7 +4,6 @@ from django.conf import settings from django.http import HttpResponseNotFound, HttpResponseRedirect from django.utils import timezone -from django.utils.module_loading import import_string from schedule.settings import ( CALENDAR_VIEW_PERM, CHECK_CALENDAR_PERM_FUNC, CHECK_EVENT_PERM_FUNC, @@ -218,30 +217,3 @@ def coerce_date_dict(date_dict): except KeyError: break return modified and ret_val or {} - - -def get_model_bases(model_class_name): - from django.db.models import Model - base_classes = getattr(settings, 'SCHEDULER_BASE_CLASSES', {}) - - if isinstance(base_classes, dict): - base_class_names = base_classes.get(model_class_name, []) - else: - base_class_names = base_classes - - if base_class_names: - return [import_string(x) for x in base_class_names] - else: - return [Model] - - -def get_admin_model_fields(model_class_name): - admin_fields = getattr(settings, 'SCHEDULER_ADMIN_FIELDS', {}) - if isinstance(admin_fields, dict): - model_fields = admin_fields.get(model_class_name, []) - else: - model_fields = admin_fields - if model_fields: - return model_fields - else: - return [] diff --git a/tests/test_utils.py b/tests/test_utils.py index d90e5e31..d0b7da9c 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -1,14 +1,10 @@ import datetime -import mock -from django.test import TestCase, override_settings +from django.test import TestCase from django.utils import timezone from schedule.models import Calendar, Event, Occurrence, Rule -from schedule.utils import ( - EventListManager, OccurrenceReplacer, get_admin_model_fields, - get_model_bases, -) +from schedule.utils import EventListManager, OccurrenceReplacer class TestEventListManager(TestCase): @@ -140,70 +136,3 @@ def test_get_occurrence_works_for_event_like_object(self): occ_replacer = OccurrenceReplacer([self.occ]) with self.assertRaises(AttributeError): occ_replacer.get_occurrence(int) - - -class TestCommonUtils(TestCase): - def setUp(self): - patcher = mock.patch('schedule.utils.import_string') - self.import_string_mock = patcher.start() - self.addCleanup(patcher.stop) - - @override_settings(SCHEDULER_BASE_CLASSES={'ClassName': ['path.to.module.AbstractClass']}) - def test_get_model_bases_with_custom_dict_default(self): - from django.db.models import Model - expected_result = [Model] - actual_result = get_model_bases('Event') - - self.assertListEqual(actual_result, expected_result) - - @override_settings(SCHEDULER_BASE_CLASSES={'ClassName': ['path.to.module.AbstractClass']}) - def test_get_model_bases_with_custom_dict_specific(self): - model_mock = mock.Mock() - expected_result = [model_mock] - - self.import_string_mock.return_value = model_mock - actual_result = get_model_bases('ClassName') - - self.assertListEqual(actual_result, expected_result) - - self.import_string_mock.assert_called_once_with('path.to.module.AbstractClass') - - @override_settings(SCHEDULER_BASE_CLASSES=['path.to.module.AbstractClass1', 'path.to.module.AbstractClass2']) - def test_get_model_bases_with_custom_list(self): - model_mock1 = mock.Mock() - model_mock2 = mock.Mock() - - expected_result = [model_mock1, model_mock2] - - self.import_string_mock.side_effect = [model_mock1, model_mock2] - actual_result = get_model_bases('ClassName') - - self.assertListEqual(actual_result, expected_result) - - self.import_string_mock.assert_any_call('path.to.module.AbstractClass1') - self.import_string_mock.assert_any_call('path.to.module.AbstractClass2') - self.assertEqual(self.import_string_mock.call_count, 2) - - @override_settings(SCHEDULER_BASE_CLASSES=None) - def test_get_model_bases_with_no_setting(self): - from django.db.models import Model - expected_result = [Model] - actual_result = get_model_bases('Event') - - self.assertListEqual(actual_result, expected_result) - - @override_settings(SCHEDULER_ADMIN_FIELDS=[('cost',)]) - def test_get_admin_fields_with_custom_list(self): - self.assertListEqual(get_admin_model_fields('Event'), [('cost',)]) - - @override_settings(SCHEDULER_ADMIN_FIELDS={'ClassName': [('cost',)]}) - def test_get_admin_fields_with_custom_dict_specific(self): - self.assertListEqual(get_admin_model_fields('ClassName'), [('cost',)]) - - @override_settings(SCHEDULER_ADMIN_FIELDS={'ClassName': [('cost',)]}) - def test_get_admin_fields_with_custom_dict_default(self): - self.assertListEqual(get_admin_model_fields('Event'), []) - - @override_settings(SCHEDULER_ADMIN_FIELDS=None) - def test_get_admin_fields_with_no_setting(self): - self.assertListEqual(get_admin_model_fields('Event'), [])