Merge pull request #298 from makinacorpus/improve_test_and_storage

Refactor media storage usage

.github/workflows/python-ci.yml

@@ -37,6 +37,10 @@ jobs:
         python-version: [ '3.8', '3.11' ]
         django-version: [ '3.2.*', '4.2.*' ]
 
+    env:
+      PYTHON: ${{ matrix.python-version }}
+      DJANGO: ${{ matrix.django-version }}
+
     steps:
     - uses: actions/checkout@v4
 
@@ -62,13 +66,16 @@ jobs:
 
     - name: Test with coverage
       run: |
-        coverage run ./manage.py test -v3
-        coverage report -m
+        coverage run --parallel-mode --concurrency=multiprocessing ./manage.py test --parallel -v 3
+        coverage combine
+        coverage xml -o coverage.xml
 
-    - name: Coverage upload
-      run: |
-        pip install codecov
-        codecov
+    - uses: codecov/codecov-action@v4
+      with:
+        files: ./coverage.xml
+        env_vars: PYTHON,DJANGO
+        token: ${{ secrets.CODECOV_TOKEN }} # not usually required for public repos
+        fail_ci_if_error: true # optional (default = false)
 
   publish:
     needs: [lint, test]

mapentity/helpers.py

@@ -1,26 +1,25 @@
 import json
 import logging
 import math
-import os
 import string
 import time
-from datetime import datetime
 from mimetypes import types_map
-from urllib.parse import urljoin, quote
+from urllib.parse import quote, urljoin
 
 import bs4
 import requests
 from django.conf import settings
 from django.contrib.gis.gdal.error import GDALException
 from django.contrib.gis.geos import GEOSException, fromstr
+from django.core.files.base import ContentFile
+from django.core.files.storage import default_storage
 from django.core.files.uploadedfile import SimpleUploadedFile
-from django.http import HttpResponse
 from django.template.exceptions import TemplateDoesNotExist
 from django.template.loader import get_template
 from django.urls import resolve
-from django.utils import timezone
 from django.utils.translation import get_language
-from .settings import app_settings, API_SRID
+
+from .settings import API_SRID, app_settings
 from .tokens import TokenManager
 
 logger = logging.getLogger(__name__)
@@ -66,19 +65,18 @@ def smart_urljoin(base, path):
 
 
 def is_file_uptodate(path, date_update, delete_empty=True):
-    if not os.path.exists(path):
+    if not default_storage.exists(path):
         return False
 
     if date_update is None:
         return False
 
-    if os.path.getsize(path) == 0:
+    if default_storage.size(path) == 0:
         if delete_empty:
-            os.remove(path)
+            default_storage.delete(path)
         return False
 
-    modified = datetime.utcfromtimestamp(os.path.getmtime(path))
-    modified = modified.replace(tzinfo=timezone.utc)
+    modified = default_storage.get_modified_time(path)
     return modified > date_update
 
 
@@ -91,12 +89,11 @@ def get_source(url, headers):
     content_error = 'Request on %s returned empty content' % url
     assert len(source.content) > 0, content_error
 
-    return source
+    return source.content
 
 
-def download_to_stream(url, stream, silent=False, headers=None):
-    """ Download url and writes response to stream.
-    """
+def download_content(url, silent=False, headers=None):
+    """ Download URL and return content."""
     source = None
     try:
         try:
@@ -111,24 +108,7 @@ def download_to_stream(url, stream, silent=False, headers=None):
             logger.info('Response: %s' % source.text[:150])
 
         if not silent:
-            raise
-
-    if source is None:
-        return source
-
-    try:
-        stream.write(source.content)
-        stream.flush()
-    except IOError as e:
-        logger.exception(e)
-        if not silent:
-            raise
-
-    if isinstance(stream, HttpResponse):
-        stream.status_code = source.status_code
-        # Copy headers
-        for header, value in source.headers.items():
-            stream[header] = value
+            raise e
 
     return source
 
@@ -150,16 +130,9 @@ def convertit_url(url, from_type=None, to_type=None, proxy=False):
     return url
 
 
-def convertit_download(url, destination, from_type=None, to_type='application/pdf', headers=None):
-    # Mock for tests
-    if getattr(settings, 'TEST', False):
-        with open(destination, 'w') as out_file:
-            out_file.write("Mock\n")
-            return
-
+def convertit_download(url, from_type=None, to_type='application/pdf', headers=None):
     url = convertit_url(url, from_type, to_type)
-    fd = open(destination, 'wb') if isinstance(destination, str) else destination
-    download_to_stream(url, fd, headers=headers)
+    return download_content(url, headers=headers)
 
 
 def capture_url(url, width=None, height=None, selector=None, waitfor=None):
@@ -180,10 +153,10 @@ def capture_url(url, width=None, height=None, selector=None, waitfor=None):
     return final_url
 
 
-def capture_image(url, stream, **kwargs):
+def capture_image(url, **kwargs):
     """ Capture url to stream. """
     url = capture_url(url, **kwargs)
-    download_to_stream(url, stream)
+    return download_content(url)
 
 
 def capture_map_image(url, destination, size=None, aspect=1.0, waitfor='.leaflet-tile-loaded', printcontext=None):
@@ -206,10 +179,10 @@ def capture_map_image(url, destination, size=None, aspect=1.0, waitfor='.leaflet
     # Run head-less capture (takes time)
     auth_token = TokenManager.generate_token()
     url += f"?lang={get_language()}&auth_token={auth_token}&context={quote(serialized)}"
-    with open(destination, 'wb') as fd:
-        capture_image(url, fd,
-                      selector='.map-panel',
-                      waitfor=waitfor)
+    map_image = capture_image(url, selector='.map-panel', waitfor=waitfor)
+    if default_storage.exists(destination):
+        default_storage.delete(destination)
+    default_storage.save(destination, ContentFile(map_image))
 
 
 def extract_attributes_html(url, request):

mapentity/management/commands/update_permissions_mapentity.py

@@ -8,7 +8,6 @@
 from django.contrib.contenttypes.models import ContentType
 from django.core.management.base import BaseCommand
 
-from mapentity.middleware import clear_internal_user_cache
 from mapentity.registry import create_mapentity_model_permissions, registry
 
 logger = logging.getLogger(__name__)
@@ -30,9 +29,6 @@ def execute(self, *args, **options):
         # Make sure apps are registered at this point
         import_module(settings.ROOT_URLCONF)
 
-        # Tests reset DB so we have to reset this cache too
-        clear_internal_user_cache()
-
         # For all models registered, add missing bits
         for model in registry.registry.keys():
             if not model._meta.abstract:

mapentity/middleware.py

@@ -1,7 +1,8 @@
 import logging
+from hashlib import sha256
 
-from django.conf import settings
 from django.contrib.auth import get_user_model, login
+from django.core.cache import cache
 
 from .settings import app_settings
 from .tokens import TokenManager
@@ -10,24 +11,25 @@
 
 
 def get_internal_user():
-    if not hasattr(get_internal_user, 'instance'):
-        username = app_settings['INTERNAL_USER']
-        User = get_user_model()
-
-        internal_user, created = User.objects.get_or_create(
-            username=username,
-            defaults={'password': settings.SECRET_KEY,
-                      'is_active': True,
-                      'is_staff': False}
-        )
-
-        get_internal_user.instance = internal_user
-    return get_internal_user.instance
-
-
-def clear_internal_user_cache():
-    if hasattr(get_internal_user, 'instance'):
-        del get_internal_user.instance
+    User = get_user_model()
+    cache_key = sha256(app_settings['INTERNAL_USER'].encode()).hexdigest()
+
+    id = 0
+
+    if cache_key in cache:
+        id = int(cache.get(cache_key))
+
+    internal_user, created = User.objects.get_or_create(
+        id=int(id),
+        defaults={
+            'username': app_settings['INTERNAL_USER'],
+            'password': '',
+            'is_active': True
+        }
+    )
+    if created:
+        cache.set(cache_key, internal_user.pk)
+    return internal_user
 
 
 class AutoLoginMiddleware:

mapentity/models.py

@@ -9,6 +9,7 @@
 from django.contrib.contenttypes.fields import GenericRelation
 from django.contrib.contenttypes.models import ContentType
 from django.core.exceptions import FieldError, ObjectDoesNotExist
+from django.core.files.storage import default_storage
 from django.db import models, transaction
 from django.db.utils import OperationalError
 from django.urls import reverse, NoReverseMatch
@@ -204,8 +205,8 @@ def get_geom(self):
     def delete(self, *args, **kwargs):
         # Delete map image capture when delete object
         image_path = self.get_map_image_path()
-        if os.path.exists(image_path):
-            os.unlink(image_path)
+        if default_storage.exists(image_path):
+            default_storage.delete(image_path)
         super().delete(*args, **kwargs)
 
     @classmethod
@@ -299,14 +300,15 @@ def prepare_map_image(self, rooturl):
         else:
             size = app_settings['MAP_CAPTURE_SIZE']
         printcontext = self.get_printcontext() if hasattr(self, 'get_printcontext') else None
-        capture_map_image(url, path, size=size, waitfor=self.capture_map_image_waitfor, printcontext=printcontext)
+        capture_map_image(url,
+                          default_storage.path(path),
+                          size=size,
+                          waitfor=self.capture_map_image_waitfor,
+                          printcontext=printcontext)
         return True
 
     def get_map_image_path(self):
-        basefolder = os.path.join(settings.MEDIA_ROOT, 'maps')
-        if not os.path.exists(basefolder):
-            os.makedirs(basefolder)
-        return os.path.join(basefolder, '%s-%s.png' % (self._meta.model_name, self.pk))
+        return os.path.join('maps', '%s-%s.png' % (self._meta.model_name, self.pk))
 
     def get_attributes_html(self, request):
         return extract_attributes_html(self.get_detail_url(), request)

mapentity/registry.py

@@ -253,7 +253,6 @@ def create_mapentity_model_permissions(model):
         return
 
     db = DEFAULT_DB_ALIAS
-
     internal_user = get_internal_user()
     perms_manager = Permission.objects.using(db)
 
@@ -282,15 +281,22 @@ def create_mapentity_model_permissions(model):
                                                                          content_type=ctype)
 
         if not internal_user_permission.exists():
-            permission = perms_manager.get(codename=codename, content_type=ctype)
-            internal_user.user_permissions.add(permission)
-            logger.info("Added permission %s to internal user %s" % (codename,
-                                                                     internal_user))
+            try:
+                permission = perms_manager.get(codename=codename, content_type=ctype)
+                internal_user.user_permissions.add(permission)
+                logger.info("Added permission %s to internal user %s" % (codename,
+                                                                         internal_user))
+            except Exception:
+                pass
 
     attachmenttype = ContentType.objects.db_manager(db).get_for_model(get_attachment_model())
     read_perm = dict(codename='read_attachment', content_type=attachmenttype)
     if not internal_user.user_permissions.filter(**read_perm).exists():
         permission = perms_manager.get(**read_perm)
-        internal_user.user_permissions.add(permission)
-        logger.info("Added permission %s to internal user %s" % (permission.codename,
-                                                                 internal_user))
+        try:
+            internal_user.user_permissions.add(permission)
+            logger.info("Added permission %s to internal user %s" % (permission.codename,
+                                                                     internal_user))
+
+        except Exception:
+            pass

mapentity/templatetags/mapentity_tags.py

@@ -1,9 +1,9 @@
-import os
-
 from django import template
 from django.conf import settings
 from django.contrib.gis.geos import GEOSGeometry
+from django.contrib.staticfiles.storage import staticfiles_storage
 from django.core.exceptions import FieldDoesNotExist
+from django.core.files.storage import default_storage
 from django.template import Context
 from django.template.exceptions import TemplateDoesNotExist
 from django.utils.timezone import now
@@ -75,16 +75,16 @@ def field_verbose_name(obj, field):
 
 @register.simple_tag()
 def media_static_fallback(media_file, static_file, *args, **kwarg):
-    if os.path.exists(os.path.join(settings.MEDIA_ROOT, media_file)):
-        return os.path.join(settings.MEDIA_URL, media_file)
-    return os.path.join(settings.STATIC_URL, static_file)
+    if default_storage.exists(media_file):
+        return default_storage.url(media_file)
+    return staticfiles_storage.url(static_file)
 
 
 @register.simple_tag()
 def media_static_fallback_path(media_file, static_file, *args, **kwarg):
-    if os.path.exists(os.path.join(settings.MEDIA_ROOT, media_file)):
-        return os.path.join(settings.MEDIA_ROOT, media_file)
-    return os.path.join(settings.STATIC_ROOT, static_file)
+    if default_storage.exists(media_file):
+        return default_storage.path(media_file)
+    return staticfiles_storage.path(static_file)
 
 
 @register.filter(name='timesince')