Use static file as logo (if sets) #378

FraCata00 · 2024-03-19T23:07:21Z

name: Pull request
about: Submit a pull request for this project
assignees: fabiocaccamo

Describe your changes
Add possibility to use a server (maybe a default software house file) static logo as a logo

Use FilePathField for absolute path and make it a relative path

Related issue
closes #355

Checklist before requesting a review

I have performed a self-review of my code.
I have added tests for the proposed changes.
I have run the tests and there are not errors.

codecov · 2024-03-19T23:09:36Z

Codecov Report

Attention: Patch coverage is 96.15385% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 97.30%. Comparing base (3293bea) to head (ee8c179).
Report is 18 commits behind head on main.

Files	Patch %	Lines
admin_interface/settings.py	66.66%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #378      +/-   ##
==========================================
- Coverage   97.35%   97.30%   -0.05%     
==========================================
  Files          38       39       +1     
  Lines         416      446      +30     
==========================================
+ Hits          405      434      +29     
- Misses         11       12       +1

Flag	Coverage Δ
unittests	`97.30% <96.15%> (-0.05%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

merwok · 2024-03-20T02:49:24Z

tests/settings.py

@@ -120,3 +120,4 @@

 STATIC_ROOT = os.path.join(BASE_DIR, "admin_interface/public/static/")
 STATIC_URL = "/static/"
+LOCAL_FILE_DIR = os.path.join(BASE_DIR, "admin_interface/public/")


I think this name is too generic (doesn’t indicate it’s for django-admin-interface) and at the same time too vague! (local file dir? what is a file dir, what is a non-local file dir)

So: I think this could be named something like ADMIN_INTERFACE_STATIC_LOGO_PATH, with an example value of theme-logos/logo.png. Then let django use the normal static file system to resolve the actual full path to project-dir/some-app/theme-logos/logo.png.

Maybe change the settings name with ADMIN_INTERFACE_STATIC_LOGO_PATH is better

Considering that this is a setting for specifying the static logo directory, I would name it ADMIN_INTERFACE_STATIC_LOGO_DIR.

fabiocaccamo · 2024-03-20T09:22:34Z

admin_interface/models.py

@@ -62,6 +68,14 @@ class Theme(models.Model):
        verbose_name=_("visible"),
    )

+    static_logo_path = models.FilePathField(


Could you please name this field logo_static?

fabiocaccamo · 2024-03-20T09:23:10Z

admin_interface/models.py


 from .cache import del_cached_active_theme


+def static_logo_directory_path():


Could you please name this method get_logo_static_path?

fabiocaccamo · 2024-03-20T09:24:07Z

admin_interface/models.py

+        path=static_logo_directory_path,
+        blank=True,
+        verbose_name=_("static logo"),
+        match=r"^.*\.(jpg|jpeg|png|svg)$",


Also gif and webp formats should be accepted.

fabiocaccamo · 2024-03-20T09:30:25Z

admin_interface/models.py

+        if self.static_logo_path:
+            # clear cache if static logo path has changed
+            try:
+                obj = Theme.objects.get(pk=self.pk)


This query (and the related logic) can be avoided by checking if the logo_static path value is different from it's initial value: https://stackoverflow.com/a/1793323/2096218

fabiocaccamo · 2024-03-20T09:34:32Z

admin_interface/admin.py

@@ -58,6 +58,7 @@ class ThemeAdmin(admin.ModelAdmin):
                    "logo_max_height",
                    "logo_color",
                    "logo_visible",
+                    "static_logo_path",


I would show this after the logo field.

fabiocaccamo · 2024-03-20T09:36:35Z

tests/test_models.py

Could you please move these tests to a new specific test module test_logo_static?

fabiocaccamo · 2024-03-20T09:40:31Z

tests/settings.py

@@ -120,3 +120,4 @@

 STATIC_ROOT = os.path.join(BASE_DIR, "admin_interface/public/static/")
 STATIC_URL = "/static/"
+LOCAL_FILE_DIR = os.path.join(BASE_DIR, "admin_interface/public/")


Considering that this is a setting for specifying the static logo directory, I would name it ADMIN_INTERFACE_STATIC_LOGO_DIR.

fabiocaccamo · 2024-03-20T09:43:26Z

admin_interface/apps.py

@@ -11,3 +11,6 @@ def ready(self):
        from admin_interface import settings

        settings.check_installed_apps()
+
+        # must check if LOCAL_FILE_DIR is set in settings
+        settings.check_settings("LOCAL_FILE_DIR")


This setting should be optional, it makes no sense to ask users to set a setting that they probably don't need.

fabiocaccamo · 2024-03-20T09:50:43Z

admin_interface/settings.py

The best way to define settings in third-party packages that allow the best flexibility when testing them (overridding them with different values) is this:

In package __init__.py:

from django.core.exceptions import ImproperlyConfigured try: from admin_interface import settings # noqa: F401 except ImproperlyConfigured: pass

In package settings.py:

from django.conf import settings if not hasattr(settings, "ADMIN_INTERFACE_MY_SETTING"): settings.ADMIN_INTERFACE_MY_SETTING = "default value"

fabiocaccamo · 2024-03-20T09:51:32Z

admin_interface/settings.py

+
+    if not hasattr(settings, setting_attribute):
+        raise ImproperlyConfigured(
+            "You must set the {} setting in your settings module.".format(


Tests coverage should be improved.

FraCata00 · 2024-03-20T09:56:01Z

@fabiocaccamo okay man, I'll provide the request changes soon 👍🏻 (I'm at work now)

fabiocaccamo · 2024-03-20T09:57:25Z

@FraCata00 thanks for the PR, sorry for premature review, I was convinced it was ready for review, no rush ;)

FraCata00 · 2024-03-20T10:01:18Z

@FraCata00 thanks for the PR, sorry for premature review, I was convinced it was ready for review, no rush ;)

It's okay 👌🏼, I'll mark it when it's ready for review
Thanks for the review

fabiocaccamo · 2024-04-06T16:29:18Z

@FraCata00 any update?

FraCata00 · 2024-04-06T16:31:29Z

@FraCata00 any update?

I'm sorry, I've been busy for my office..., I'll continue this week to PR, is this a problem?

fabiocaccamo · 2024-04-06T16:40:39Z

@FraCata00 no problem, it was just to know if you were willing to continue working on this PR or not (in that case I would have closed the PR).

FraCata00 · 2024-04-06T16:45:41Z

no problem, it was just to know if you were willing to continue working on this PR or not (in that case I would have closed the PR).

Oh yeah yeah, I'm interested in continuing the feature. Sorry for the late...

…ribute are sets

rel to #728d847d86ddfbdcf75f4c32856153bd7839ba8d

ref to fabiocaccamo#355

…o render first the static logo (server-uploaded), after the other logo (client-uploaded) ref to fabiocaccamo#355

…bc3d285e6d3f208cf

…3dab8bc3d285e6d3f208cf

…tatic logo field

FraCata00 changed the title ~~Feat/logo static file~~ Use static file as logo Mar 19, 2024

FraCata00 mentioned this pull request Mar 19, 2024

Enable possibility to use a static file as logo #355

Open

FraCata00 force-pushed the feat/logo-static-file branch 2 times, most recently from d59656f to 2341253 Compare March 19, 2024 23:31

FraCata00 marked this pull request as draft March 19, 2024 23:36

FraCata00 marked this pull request as ready for review March 19, 2024 23:40

FraCata00 changed the title ~~Use static file as logo~~ Use static file as logo (if sets) Mar 19, 2024

merwok reviewed Mar 20, 2024

View reviewed changes

FraCata00 marked this pull request as draft March 20, 2024 09:10

fabiocaccamo requested changes Mar 20, 2024

View reviewed changes

fabiocaccamo assigned FraCata00 Mar 20, 2024

fabiocaccamo added the enhancement New feature or request label Mar 20, 2024

FraCata00 added 8 commits April 15, 2024 20:37

feat(settings): add on ready application config check if settings att…

ddb480b

…ribute are sets

test: add missing tests check settings

ae6b6b5

rel to #728d847d86ddfbdcf75f4c32856153bd7839ba8d

feat(model): add FilePathField for static file (logo) path

80a3bf7

ref to fabiocaccamo#355

feat(template): integrate #8f0abe843dd3b839403dab8bc3d285e6d3f208cf t…

89fe3e5

…o render first the static logo (server-uploaded), after the other logo (client-uploaded) ref to fabiocaccamo#355

test: add missing tests model new fields for #8f0abe843dd3b839403dab8…

892b7ac

…bc3d285e6d3f208cf

feat(locale): add missing traduction field rel to #8f0abe843dd3b83940…

5e78796

…3dab8bc3d285e6d3f208cf

fix(template): use static template django keys, instead of {{}} new s…

b4d8d0e

…tatic logo field

add missing test for static_logo_relative_url property

ee8c179

FraCata00 force-pushed the feat/logo-static-file branch from 1562983 to ee8c179 Compare April 15, 2024 18:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use static file as logo (if sets) #378

Use static file as logo (if sets) #378

FraCata00 commented Mar 19, 2024 •

edited

codecov bot commented Mar 19, 2024 •

edited

merwok Mar 20, 2024

FraCata00 Mar 20, 2024

fabiocaccamo Mar 20, 2024

fabiocaccamo Mar 20, 2024

fabiocaccamo Mar 20, 2024

fabiocaccamo Mar 20, 2024

fabiocaccamo Mar 20, 2024

fabiocaccamo Mar 20, 2024

fabiocaccamo Mar 20, 2024

fabiocaccamo Mar 20, 2024

fabiocaccamo Mar 20, 2024

fabiocaccamo Mar 20, 2024

fabiocaccamo Mar 20, 2024

FraCata00 commented Mar 20, 2024 •

edited

fabiocaccamo commented Mar 20, 2024 •

edited

FraCata00 commented Mar 20, 2024

fabiocaccamo commented Apr 6, 2024

FraCata00 commented Apr 6, 2024

fabiocaccamo commented Apr 6, 2024

FraCata00 commented Apr 6, 2024


		from .cache import del_cached_active_theme


		def static_logo_directory_path():

Use static file as logo (if sets) #378

Are you sure you want to change the base?

Use static file as logo (if sets) #378

Conversation

FraCata00 commented Mar 19, 2024 • edited

codecov bot commented Mar 19, 2024 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FraCata00 commented Mar 20, 2024 • edited

fabiocaccamo commented Mar 20, 2024 • edited

FraCata00 commented Mar 20, 2024

fabiocaccamo commented Apr 6, 2024

FraCata00 commented Apr 6, 2024

fabiocaccamo commented Apr 6, 2024

FraCata00 commented Apr 6, 2024

FraCata00 commented Mar 19, 2024 •

edited

codecov bot commented Mar 19, 2024 •

edited

FraCata00 commented Mar 20, 2024 •

edited

fabiocaccamo commented Mar 20, 2024 •

edited