-
-
Notifications
You must be signed in to change notification settings - Fork 181
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use static file as logo (if sets) #378
Conversation
Codecov ReportAttention: Patch coverage is
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
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
d59656f
to
2341253
Compare
@@ -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/") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe change the settings name with ADMIN_INTERFACE_STATIC_LOGO_PATH
is better
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Considering that this is a setting for specifying the static logo directory, I would name it ADMIN_INTERFACE_STATIC_LOGO_DIR
.
@@ -62,6 +68,14 @@ class Theme(models.Model): | |||
verbose_name=_("visible"), | |||
) | |||
|
|||
static_logo_path = models.FilePathField( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please name this field logo_static
?
|
||
from .cache import del_cached_active_theme | ||
|
||
|
||
def static_logo_directory_path(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please name this method get_logo_static_path
?
path=static_logo_directory_path, | ||
blank=True, | ||
verbose_name=_("static logo"), | ||
match=r"^.*\.(jpg|jpeg|png|svg)$", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also gif
and webp
formats should be accepted.
if self.static_logo_path: | ||
# clear cache if static logo path has changed | ||
try: | ||
obj = Theme.objects.get(pk=self.pk) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@@ -58,6 +58,7 @@ class ThemeAdmin(admin.ModelAdmin): | |||
"logo_max_height", | |||
"logo_color", | |||
"logo_visible", | |||
"static_logo_path", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would show this after the logo
field.
tests/test_models.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please move these tests to a new specific test module test_logo_static
?
@@ -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/") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Considering that this is a setting for specifying the static logo directory, I would name it ADMIN_INTERFACE_STATIC_LOGO_DIR
.
@@ -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") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This setting should be optional, it makes no sense to ask users to set a setting that they probably don't need.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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"
|
||
if not hasattr(settings, setting_attribute): | ||
raise ImproperlyConfigured( | ||
"You must set the {} setting in your settings module.".format( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests coverage should be improved.
@fabiocaccamo okay man, I'll provide the request changes soon 👍🏻 (I'm at work now) |
@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 |
@FraCata00 any update? |
I'm sorry, I've been busy for my office..., I'll continue this week to PR, is this a problem? |
@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). |
Oh yeah yeah, I'm interested in continuing the feature. Sorry for the late... |
rel to #728d847d86ddfbdcf75f4c32856153bd7839ba8d
…o render first the static logo (server-uploaded), after the other logo (client-uploaded) ref to fabiocaccamo#355
…bc3d285e6d3f208cf
…3dab8bc3d285e6d3f208cf
1562983
to
ee8c179
Compare
Hey just wanted to know if this PR is ready or not, I'd be interested in this feature |
@FraCata00 any update? |
Sorry guys, I didn't have much time to continue the development, (due to work projects in my company) |
I'll do my best to resume development as soon as possible... sorry |
@FraCata00 no prob, thank you for the update! |
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
FilePathField
for absolute path and make it a relative pathRelated issue
closes #355
Checklist before requesting a review