Skip to content
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

Closed
wants to merge 8 commits into from

Conversation

FraCata00
Copy link
Contributor

@FraCata00 FraCata00 commented Mar 19, 2024


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.

@FraCata00 FraCata00 changed the title Feat/logo static file Use static file as logo Mar 19, 2024
Copy link

codecov bot commented Mar 19, 2024

Codecov Report

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

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

Files with missing lines 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.

@FraCata00 FraCata00 force-pushed the feat/logo-static-file branch 2 times, most recently from d59656f to 2341253 Compare March 19, 2024 23:31
@FraCata00 FraCata00 marked this pull request as draft March 19, 2024 23:36
@FraCata00 FraCata00 marked this pull request as ready for review March 19, 2024 23:40
@FraCata00 FraCata00 changed the title Use static file as logo Use static file as logo (if sets) Mar 19, 2024
@@ -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/")
Copy link
Contributor

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.

Copy link
Contributor Author

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

Copy link
Owner

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.

@FraCata00 FraCata00 marked this pull request as draft March 20, 2024 09:10
@@ -62,6 +68,14 @@ class Theme(models.Model):
verbose_name=_("visible"),
)

static_logo_path = models.FilePathField(
Copy link
Owner

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():
Copy link
Owner

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)$",
Copy link
Owner

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)
Copy link
Owner

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",
Copy link
Owner

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.

Copy link
Owner

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/")
Copy link
Owner

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")
Copy link
Owner

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.

Copy link
Owner

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(
Copy link
Owner

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 fabiocaccamo added the enhancement New feature or request label Mar 20, 2024
@FraCata00
Copy link
Contributor Author

FraCata00 commented Mar 20, 2024

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

@fabiocaccamo
Copy link
Owner

fabiocaccamo commented Mar 20, 2024

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

@FraCata00
Copy link
Contributor Author

@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
Copy link
Owner

@FraCata00 any update?

@FraCata00
Copy link
Contributor Author

@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
Copy link
Owner

@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
Copy link
Contributor Author

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...

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

Hey just wanted to know if this PR is ready or not, I'd be interested in this feature

@fabiocaccamo
Copy link
Owner

@FraCata00 any update?

@FraCata00
Copy link
Contributor Author

Sorry guys, I didn't have much time to continue the development, (due to work projects in my company)

@FraCata00
Copy link
Contributor Author

I'll do my best to resume development as soon as possible... sorry

@fabiocaccamo
Copy link
Owner

@FraCata00 no prob, thank you for the update!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Enable possibility to use a static file as logo
4 participants