-
Notifications
You must be signed in to change notification settings - Fork 9
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
Feat: agency logos configurable via Admin #2514
Conversation
Coverage reportClick to see where and how coverage changed
This report was generated by python-coverage-comment-action |
6f4337f
to
e255665
Compare
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 is a good start, and I realize you're focused on getting this field into the model so it is configurable via the admin. This does that, but it also feels incomplete.
Based on my reading of https://docs.djangoproject.com/en/5.1/ref/models/fields/#django.db.models.FileField.storage it sounds like there are just a few more steps:
- Define
MEDIA_URL
as the base public URL of theMEDIA_ROOT
directory - Map this URL and directory in nginx, similar to
/static
- Use the convenience
url
attribute provided by Django onImageField
, to get the absolute path to the image in a template e.g.{{ object.logo.url }}
Are you thinking one, two, three... more PRs to implement this? I'd be in favor of just going all the way in this PR, making it a complete feature. We can't even view the image that we upload right now, kind of hard to validate that it is working correctly.
e255665
to
6f3ff9e
Compare
Thanks for the comments @thekaveman and for walking through this earlier today, I think this is ready for a re-review, just 2 things to note: We thought that adding
to Also, L43 in urls.py is not covered, but it seems ok since we would only be testing |
I think we can say this will close #2489 at this point 😁 |
148a0a6
to
1a303c0
Compare
Preview url: https://benefits-2514--cal-itp-previews.netlify.app |
Thanks for looking at this this morning @thekaveman, I think it's ready for a review. Also, once this PR is merged and I've updated the logos in |
1a303c0
to
4f73f70
Compare
we keep the cst logo in the repo for conveninence for the dev environment
4f73f70
to
6bea2ea
Compare
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.
Great work on this @lalver1!
Closes #2489
This PR implements storing the agency logos as a property on the
TransiAgency
model. The logos are saved to the application's local filesystem.Notes
The logo's file format
We expect the logo to be uploaded to be a
.png
file and smaller than 1MB (the default value of theclient_max_body_size size
nginx directive) due to the current nginx configuration. Otherwise, a413 request entity too large
error will be raised. If we expect larger files, we should explicitly setclient_max_body_size size
to a larger size innginx.conf
.Updating a logo
Currently, an existing logo is not overwritten if a new logo is uploaded. Instead, the new logo has the same name as the old logo but with a random string appended to the end of it.
Do we want this behavior or do we prefer to overwrite the files? If we prefer an overwrite, we may need to overrideFor the current iteration we will use this default behavior but in the future we will implement file management improvements.save
inFileSystemStorage
.Reviewing
Assuming we start from the
main
branch, withDJANGO_STORAGE_DIR=.
(which used to beDJANGO_DB_DIR
before this PR), and from an exited dev containergit status
)docker compose build --no-cache client
to rebuild the app imagepng
file) and you can also upload logos for the other agencies for testing.env
setDJANGO_STORAGE_DIR=/home/calitp/app/data
compose.yml
, set thevolumes
of theclient
service to- ./:/home/calitp/app/data
(discard this change after this review)docker compose up -d client
to start a new app container mimicking the location of the persistent storage in the deploymentspng
for a transit agency (you can use the same file for both the small and the large logo). Test the site to see the newpng
logos being displayed.Exec
window (click on the app container and navigate to theExec
tab) ordocker compose exec -ti client /bin/bash
to connect to running container, confirm that the files (cst-sm.png
andcst-lg.png
, for example if the selected transit agency was CST) were created in/home/calitp/app/data/uploads/agencies
.