From f92d99bd9a56409b3d2a49cfd5e2e8763cc3e1a4 Mon Sep 17 00:00:00 2001 From: Roberto Rosario Date: Fri, 1 Feb 2019 03:55:43 -0400 Subject: [PATCH] Refactor the converter app Don't cache the entire converter class to lower memory usage. Instead a get_converter_class() function is now provided to load the converter backend class. Add model permission inheritance to transformations to removel custom permission checking code in the views. User keyword arguments. Update URL parameters to the '_id' form. Add missing edit and delete icons. Improve the create icon using composition. Update add to comply with MERCs 5 and 6. Signed-off-by: Roberto Rosario --- HISTORY.rst | 4 + mayan/apps/converter/__init__.py | 1 - mayan/apps/converter/apps.py | 20 +- mayan/apps/converter/icons.py | 7 +- mayan/apps/converter/links.py | 17 +- mayan/apps/converter/models.py | 4 + mayan/apps/converter/tests/test_views.py | 56 ++--- mayan/apps/converter/transformations.py | 1 - mayan/apps/converter/urls.py | 14 +- mayan/apps/converter/{runtime.py => utils.py} | 5 +- mayan/apps/converter/views.py | 226 ++++++------------ 11 files changed, 152 insertions(+), 203 deletions(-) rename mayan/apps/converter/{runtime.py => utils.py} (66%) diff --git a/HISTORY.rst b/HISTORY.rst index 4106ec19a0..22eb460148 100644 --- a/HISTORY.rst +++ b/HISTORY.rst @@ -229,6 +229,10 @@ the same time. - Move file and storage code to the storage app. The setting COMMON_TEMPORARY_DIRECTORY is now STORAGE_TEMPORARY_DIRECTORY. +- To lower memory usage and reduce memory leaks, the entire + entire converter class is no longer cached and instead loaded + on demand. This allows the garbage collector to clear the memory + used. 3.1.9 (2018-11-01) ================== diff --git a/mayan/apps/converter/__init__.py b/mayan/apps/converter/__init__.py index a643e11bef..d5750d6e40 100644 --- a/mayan/apps/converter/__init__.py +++ b/mayan/apps/converter/__init__.py @@ -1,6 +1,5 @@ from __future__ import unicode_literals -from .runtime import converter_class # NOQA from .transformations import ( # NOQA BaseTransformation, TransformationResize, TransformationRotate, TransformationZoom diff --git a/mayan/apps/converter/apps.py b/mayan/apps/converter/apps.py index 0d545877a8..920a73249a 100644 --- a/mayan/apps/converter/apps.py +++ b/mayan/apps/converter/apps.py @@ -1,8 +1,8 @@ from __future__ import unicode_literals -from django.utils.encoding import force_text from django.utils.translation import ugettext_lazy as _ +from mayan.apps.acls.classes import ModelPermission from mayan.apps.common import MayanAppConfig, menu_object, menu_sidebar from mayan.apps.navigation import SourceColumn @@ -25,12 +25,20 @@ class ConverterApp(MayanAppConfig): Transformation = self.get_model(model_name='Transformation') - SourceColumn(attribute='order', source=Transformation) - SourceColumn( - source=Transformation, label=_('Transformation'), - func=lambda context: force_text(context['object']) + ModelPermission.register_inheritance( + model=Transformation, related='content_object' + ) + + SourceColumn( + attribute='order', include_label=True, source=Transformation + ) + SourceColumn( + attribute='get_transformation_label', is_identifier=True, + source=Transformation + ) + SourceColumn( + attribute='arguments', include_label=True, source=Transformation ) - SourceColumn(attribute='arguments', source=Transformation) menu_object.bind_links( links=(link_transformation_edit, link_transformation_delete), diff --git a/mayan/apps/converter/icons.py b/mayan/apps/converter/icons.py index 38285447b4..61803fddc0 100644 --- a/mayan/apps/converter/icons.py +++ b/mayan/apps/converter/icons.py @@ -3,4 +3,9 @@ from __future__ import absolute_import, unicode_literals from mayan.apps.appearance.classes import Icon icon_transformation = Icon(driver_name='fontawesome', symbol='crop') -icon_transformation_create = Icon(driver_name='fontawesome', symbol='plus') +icon_transformation_create = Icon( + driver_name='fontawesome-dual', primary_symbol='crop', + secondary_symbol='plus' +) +icon_transformation_delete = Icon(driver_name='fontawesome', symbol='times') +icon_transformation_edit = Icon(driver_name='fontawesome', symbol='pencil-alt') diff --git a/mayan/apps/converter/links.py b/mayan/apps/converter/links.py index 99ab3cef71..2ff7805914 100644 --- a/mayan/apps/converter/links.py +++ b/mayan/apps/converter/links.py @@ -5,7 +5,10 @@ from django.utils.translation import ugettext_lazy as _ from mayan.apps.navigation import Link -from .icons import icon_transformation, icon_transformation_create +from .icons import ( + icon_transformation, icon_transformation_create, icon_transformation_delete, + icon_transformation_edit +) from .permissions import ( permission_transformation_create, permission_transformation_delete, permission_transformation_edit, permission_transformation_view @@ -37,12 +40,16 @@ link_transformation_create = Link( text=_('Create new transformation'), view='converter:transformation_create' ) link_transformation_delete = Link( - args='resolved_object.pk', permission=permission_transformation_delete, - tags='dangerous', text=_('Delete'), view='converter:transformation_delete' + icon_class=icon_transformation_delete, + kwargs={'transformation_id': 'resolved_object.pk'}, + permission=permission_transformation_delete, tags='dangerous', + text=_('Delete'), view='converter:transformation_delete' ) link_transformation_edit = Link( - args='resolved_object.pk', permission=permission_transformation_edit, - text=_('Edit'), view='converter:transformation_edit' + icon_class=icon_transformation_edit, + kwargs={'transformation_id': 'resolved_object.pk'}, + permission=permission_transformation_edit, text=_('Edit'), + view='converter:transformation_edit' ) link_transformation_list = Link( icon_class=icon_transformation, diff --git a/mayan/apps/converter/models.py b/mayan/apps/converter/models.py index e241049bcd..8b98df45ee 100644 --- a/mayan/apps/converter/models.py +++ b/mayan/apps/converter/models.py @@ -60,7 +60,11 @@ class Transformation(models.Model): verbose_name_plural = _('Transformations') def __str__(self): + return self.get_transformation_label() + + def get_transformation_label(self): return self.get_name_display() + get_transformation_label.short_description = _('Name') def save(self, *args, **kwargs): if not self.order: diff --git a/mayan/apps/converter/tests/test_views.py b/mayan/apps/converter/tests/test_views.py index 06de5fc9da..c298acd068 100644 --- a/mayan/apps/converter/tests/test_views.py +++ b/mayan/apps/converter/tests/test_views.py @@ -17,31 +17,6 @@ from .literals import ( class TransformationViewsTestCase(GenericDocumentViewTestCase): - def setUp(self): - super(TransformationViewsTestCase, self).setUp() - self.login_user() - - def _transformation_list_view(self): - return self.get( - viewname='converter:transformation_list', kwargs={ - 'app_label': 'documents', 'model': 'document', - 'object_id': self.document.pk - } - ) - - def test_transformation_list_view_no_permissions(self): - response = self._transformation_list_view() - - self.assertEqual(response.status_code, 403) - - def test_transformation_list_view_with_permissions(self): - self.grant_permission(permission=permission_transformation_view) - response = self._transformation_list_view() - - self.assertContains( - response, text=self.document.label, status_code=200 - ) - def _transformation_create_view(self): return self.post( viewname='converter:transformation_create', kwargs={ @@ -56,7 +31,7 @@ class TransformationViewsTestCase(GenericDocumentViewTestCase): def test_transformation_create_view_no_permissions(self): response = self._transformation_create_view() - self.assertEqual(response.status_code, 403) + self.assertEqual(response.status_code, 404) self.assertEqual(Transformation.objects.count(), 0) def test_transformation_create_view_with_access(self): @@ -80,7 +55,7 @@ class TransformationViewsTestCase(GenericDocumentViewTestCase): def _transformation_delete_view(self): return self.post( viewname='converter:transformation_delete', kwargs={ - 'transformation_pk': self.transformation.pk + 'transformation_id': self.transformation.pk } ) @@ -88,7 +63,7 @@ class TransformationViewsTestCase(GenericDocumentViewTestCase): self._create_transformation() response = self._transformation_delete_view() - self.assertEqual(response.status_code, 403) + self.assertEqual(response.status_code, 404) self.assertEqual(Transformation.objects.count(), 1) def test_transformation_delete_view_with_access(self): @@ -104,7 +79,7 @@ class TransformationViewsTestCase(GenericDocumentViewTestCase): def _transformation_edit_view(self): return self.post( viewname='converter:transformation_edit', kwargs={ - 'transformation_pk': self.transformation.pk + 'transformation_id': self.transformation.pk }, data={ 'arguments': TEST_TRANSFORMATION_ARGUMENT_EDITED, 'name': self.transformation.name @@ -114,7 +89,7 @@ class TransformationViewsTestCase(GenericDocumentViewTestCase): def test_transformation_edit_view_no_permission(self): self._create_transformation() response = self._transformation_edit_view() - self.assertEqual(response.status_code, 403) + self.assertEqual(response.status_code, 404) self.transformation.refresh_from_db() self.assertNotEqual( @@ -133,3 +108,24 @@ class TransformationViewsTestCase(GenericDocumentViewTestCase): self.assertEqual( self.transformation.arguments, TEST_TRANSFORMATION_ARGUMENT_EDITED ) + + def _transformation_list_view(self): + return self.get( + viewname='converter:transformation_list', kwargs={ + 'app_label': 'documents', 'model': 'document', + 'object_id': self.document.pk + } + ) + + def test_transformation_list_view_no_permissions(self): + response = self._transformation_list_view() + + self.assertEqual(response.status_code, 404) + + def test_transformation_list_view_with_permissions(self): + self.grant_permission(permission=permission_transformation_view) + response = self._transformation_list_view() + + self.assertContains( + response, text=self.document.label, status_code=200 + ) diff --git a/mayan/apps/converter/transformations.py b/mayan/apps/converter/transformations.py index 1caecc9117..f6038a864c 100644 --- a/mayan/apps/converter/transformations.py +++ b/mayan/apps/converter/transformations.py @@ -58,7 +58,6 @@ class BaseTransformation(object): def register(cls, transformation): cls._registry[transformation.name] = transformation - def __init__(self, **kwargs): self.kwargs = {} for argument_name in self.arguments: diff --git a/mayan/apps/converter/urls.py b/mayan/apps/converter/urls.py index 5334d62b33..d744c20c36 100644 --- a/mayan/apps/converter/urls.py +++ b/mayan/apps/converter/urls.py @@ -9,19 +9,19 @@ from .views import ( urlpatterns = [ url( - regex=r'^create_for/(?P[-\w]+)/(?P[-\w]+)/(?P\d+)/$', - name='transformation_create', view=TransformationCreateView.as_view() - ), - url( - regex=r'^list_for/(?P[-\w]+)/(?P[-\w]+)/(?P\d+)/$', + regex=r'^objects/(?P[-\w]+)/(?P[-\w]+)/(?P\d+)/transformations/$', name='transformation_list', view=TransformationListView.as_view() ), url( - regex=r'^delete/(?P\d+)/$', + regex=r'^objects/(?P[-\w]+)/(?P[-\w]+)/(?P\d+)/transformations/create/$', + name='transformation_create', view=TransformationCreateView.as_view() + ), + url( + regex=r'^transformations/delete/(?P\d+)/$', name='transformation_delete', view=TransformationDeleteView.as_view() ), url( - regex=r'^edit/(?P\d+)/$', + regex=r'^transformations/edit/(?P\d+)/$', name='transformation_edit', view=TransformationEditView.as_view() ), ] diff --git a/mayan/apps/converter/runtime.py b/mayan/apps/converter/utils.py similarity index 66% rename from mayan/apps/converter/runtime.py rename to mayan/apps/converter/utils.py index acfc7d1e7f..c82f1a42a2 100644 --- a/mayan/apps/converter/runtime.py +++ b/mayan/apps/converter/utils.py @@ -7,4 +7,7 @@ from django.utils.module_loading import import_string from .settings import setting_graphics_backend logger = logging.getLogger(__name__) -backend = converter_class = import_string(setting_graphics_backend.value) + + +def get_converter_class(): + return import_string(dotted_path=setting_graphics_backend.value) diff --git a/mayan/apps/converter/views.py b/mayan/apps/converter/views.py index 3a728db467..ca8d519d29 100644 --- a/mayan/apps/converter/views.py +++ b/mayan/apps/converter/views.py @@ -2,18 +2,15 @@ from __future__ import absolute_import, unicode_literals import logging -from django.contrib.contenttypes.models import ContentType -from django.http import Http404 -from django.shortcuts import get_object_or_404 from django.template import RequestContext from django.urls import reverse from django.utils.translation import ugettext_lazy as _ -from mayan.apps.acls.models import AccessControlList from mayan.apps.common.generics import ( SingleObjectCreateView, SingleObjectDeleteView, SingleObjectEditView, SingleObjectListView ) +from mayan.apps.common.mixins import ContentTypeViewMixin, ExternalObjectMixin from .forms import TransformationForm from .icons import icon_transformation @@ -27,103 +24,26 @@ from .permissions import ( logger = logging.getLogger(__name__) -class TransformationDeleteView(SingleObjectDeleteView): - model = Transformation - pk_url_kwarg = 'transformation_pk' - - def dispatch(self, request, *args, **kwargs): - self.transformation = get_object_or_404( - klass=Transformation, pk=self.kwargs['transformation_pk'] - ) - - AccessControlList.objects.check_access( - obj=self.transformation.content_object, - permission=permission_transformation_delete, user=request.user - ) - - return super(TransformationDeleteView, self).dispatch( - request, *args, **kwargs - ) - - def get_post_action_redirect(self): - return reverse( - viewname='converter:transformation_list', kwargs={ - 'app_label': self.transformation.content_type.app_label, - 'model': self.transformation.content_type.model, - 'object_id': self.transformation.object_id - } - ) - - def get_extra_context(self): - return { - 'content_object': self.transformation.content_object, - 'navigation_object_list': ('content_object', 'transformation'), - 'previous': reverse( - viewname='converter:transformation_list', kwargs={ - 'app_label': self.transformation.content_type.app_label, - 'model': self.transformation.content_type.model, - 'object_id': self.transformation.object_id - } - ), - 'title': _( - 'Delete transformation "%(transformation)s" for: ' - '%(content_object)s?' - ) % { - 'transformation': self.transformation, - 'content_object': self.transformation.content_object - }, - 'transformation': self.transformation, - } - - -class TransformationCreateView(SingleObjectCreateView): +class TransformationCreateView(ContentTypeViewMixin, ExternalObjectMixin, SingleObjectCreateView): + external_object_permission = permission_transformation_create + external_object_pk_url_kwarg = 'object_id' form_class = TransformationForm - def dispatch(self, request, *args, **kwargs): - content_type = get_object_or_404( - klass=ContentType, app_label=self.kwargs['app_label'], - model=self.kwargs['model'] - ) - - try: - self.content_object = content_type.get_object_for_this_type( - pk=self.kwargs['object_id'] - ) - except content_type.model_class().DoesNotExist: - raise Http404 - - AccessControlList.objects.check_access( - obj=self.content_object, permission=permission_transformation_create, - user=request.user - ) - - return super(TransformationCreateView, self).dispatch( - request, *args, **kwargs - ) - - def form_valid(self, form): - instance = form.save(commit=False) - instance.content_object = self.content_object - try: - instance.full_clean() - instance.save() - except Exception as exception: - logger.debug('Invalid form, exception: %s', exception) - return super(TransformationCreateView, self).form_invalid( - form=form - ) - else: - return super(TransformationCreateView, self).form_valid(form=form) + def get_external_object_queryset(self): + return self.get_content_type().model_class().objects.all() def get_extra_context(self): return { - 'content_object': self.content_object, + 'content_object': self.external_object, 'navigation_object_list': ('content_object',), 'title': _( 'Create new transformation for: %s' - ) % self.content_object, + ) % self.external_object, } + def get_instance_extra_data(self): + return {'content_object': self.external_object} + def get_post_action_redirect(self): return reverse( viewname='converter:transformation_list', kwargs={ @@ -134,96 +54,100 @@ class TransformationCreateView(SingleObjectCreateView): ) def get_queryset(self): - return Transformation.objects.get_for_model(obj=self.content_object) + return Transformation.objects.get_for_model(obj=self.external_object) + + +class TransformationDeleteView(SingleObjectDeleteView): + model = Transformation + object_permission = permission_transformation_delete + pk_url_kwarg = 'transformation_id' + + def get_extra_context(self): + transformation = self.get_object() + + return { + 'content_object': transformation.content_object, + 'navigation_object_list': ('content_object', 'transformation'), + 'previous': reverse( + viewname='converter:transformation_list', kwargs={ + 'app_label': transformation.content_type.app_label, + 'model': transformation.content_type.model, + 'object_id': transformation.object_id + } + ), + 'title': _( + 'Delete transformation "%(transformation)s" for: ' + '%(content_object)s?' + ) % { + 'transformation': transformation, + 'content_object': transformation.content_object + }, + 'transformation': transformation, + } + + def get_post_action_redirect(self): + transformation = self.get_object() + + return reverse( + viewname='converter:transformation_list', kwargs={ + 'app_label': transformation.content_type.app_label, + 'model': transformation.content_type.model, + 'object_id': transformation.object_id + } + ) class TransformationEditView(SingleObjectEditView): form_class = TransformationForm model = Transformation - pk_url_kwarg = 'transformation_pk' - - def dispatch(self, request, *args, **kwargs): - self.transformation = get_object_or_404( - klass=Transformation, pk=self.kwargs['transformation_pk'] - ) - - AccessControlList.objects.check_access( - obj=self.transformation.content_object, - permission=permission_transformation_edit, user=request.user - ) - - return super(TransformationEditView, self).dispatch( - request=request, *args, **kwargs - ) - - def form_valid(self, form): - instance = form.save(commit=False) - try: - instance.full_clean() - instance.save() - except Exception as exception: - logger.debug('Invalid form, exception: %s', exception) - return super(TransformationEditView, self).form_invalid(form=form) - else: - return super(TransformationEditView, self).form_valid(form=form) + object_permission = permission_transformation_edit + pk_url_kwarg = 'transformation_id' def get_extra_context(self): + transformation = self.get_object() + return { - 'content_object': self.transformation.content_object, + 'content_object': transformation.content_object, 'navigation_object_list': ('content_object', 'transformation'), 'title': _( 'Edit transformation "%(transformation)s" for: %(content_object)s' ) % { - 'transformation': self.transformation, - 'content_object': self.transformation.content_object + 'transformation': transformation, + 'content_object': transformation.content_object }, - 'transformation': self.transformation, + 'transformation': transformation, } def get_post_action_redirect(self): + transformation = self.get_object() + return reverse( viewname='converter:transformation_list', kwargs={ - 'app_label': self.transformation.content_type.app_label, - 'model': self.transformation.content_type.model, - 'object_id': self.transformation.object_id + 'app_label': transformation.content_type.app_label, + 'model': transformation.content_type.model, + 'object_id': transformation.object_id } ) -class TransformationListView(SingleObjectListView): - def dispatch(self, request, *args, **kwargs): - content_type = get_object_or_404( - klass=ContentType, app_label=self.kwargs['app_label'], - model=self.kwargs['model'] - ) +class TransformationListView(ContentTypeViewMixin, ExternalObjectMixin, SingleObjectListView): + external_object_permission = permission_transformation_view + external_object_pk_url_kwarg = 'object_id' - try: - self.content_object = content_type.get_object_for_this_type( - pk=self.kwargs['object_id'] - ) - except content_type.model_class().DoesNotExist: - raise Http404 - - AccessControlList.objects.check_access( - obj=self.content_object, - permission=permission_transformation_view, - user=request.user - ) - - return super(TransformationListView, self).dispatch( - request=request, *args, **kwargs - ) + def get_external_object_queryset(self): + return self.get_content_type().model_class().objects.all() def get_extra_context(self): return { - 'content_object': self.content_object, + 'content_object': self.external_object, 'hide_link': True, 'hide_object': True, 'navigation_object_list': ('content_object',), 'no_results_icon': icon_transformation, 'no_results_main_link': link_transformation_create.resolve( context=RequestContext( - self.request, {'content_object': self.content_object} + dict_={'content_object': self.external_object}, + request=self.request ) ), 'no_results_text': _( @@ -232,8 +156,8 @@ class TransformationListView(SingleObjectListView): 'document file themselves.' ), 'no_results_title': _('No transformations'), - 'title': _('Transformations for: %s') % self.content_object, + 'title': _('Transformations for: %s') % self.external_object } def get_source_queryset(self): - return Transformation.objects.get_for_model(obj=self.content_object) + return Transformation.objects.get_for_model(obj=self.external_object)