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

Feat: agency logos configurable via Admin #2514

Merged
merged 11 commits into from
Nov 18, 2024
Merged

Conversation

lalver1
Copy link
Member

@lalver1 lalver1 commented Nov 7, 2024

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 the client_max_body_size size nginx directive) due to the current nginx configuration. Otherwise, a 413 request entity too large error will be raised. If we expect larger files, we should explicitly set client_max_body_size size to a larger size in nginx.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 override save in FileSystemStorage. For the current iteration we will use this default behavior but in the future we will implement file management improvements.

Reviewing

Assuming we start from the main branch, with DJANGO_STORAGE_DIR=. (which used to be DJANGO_DB_DIR before this PR), and from an exited dev container

  1. Check out this branch locally, ensure you have no pending/unstaged changes (git status)
  2. (outside of the dev container) docker compose build --no-cache client to rebuild the app image
  3. Rebuild and reopen the dev container to confirm that the migrations ran ok and that the database has the new fields
  4. Test the site locally using the dev container, CST should display its logo (which is the new png file) and you can also upload logos for the other agencies for testing
  5. Reopen folder locally in VSCode (stopping the dev container)
  6. In .env set DJANGO_STORAGE_DIR=/home/calitp/app/data
  7. In compose.yml, set the volumes of the client service to - ./:/home/calitp/app/data (discard this change after this review)
  8. (outside of the dev container) docker compose up -d client to start a new app container mimicking the location of the persistent storage in the deployments
  9. Navigate to the app's admin panel using your browser and upload a png for a transit agency (you can use the same file for both the small and the large logo). Test the site to see the new png logos being displayed.
  10. Using either Docker Desktop's Exec window (click on the app container and navigate to the Exec tab) or docker compose exec -ti client /bin/bash to connect to running container, confirm that the files (cst-sm.png and cst-lg.png, for example if the selected transit agency was CST) were created in /home/calitp/app/data/uploads/agencies.

@lalver1 lalver1 self-assigned this Nov 7, 2024
@github-actions github-actions bot added back-end Django views, sessions, middleware, models, migrations etc. deployment-dev [auto] Changes that will trigger a deploy if merged to dev migrations [auto] Review for potential model changes/needed data migrations updates and removed deployment-dev [auto] Changes that will trigger a deploy if merged to dev labels Nov 7, 2024
Copy link

github-actions bot commented Nov 7, 2024

Coverage report

Click to see where and how coverage changed

FileStatementsMissingCoverageCoverage
(new stmts)
Lines missing
  benefits
  settings.py
  urls.py 43
  benefits/core
  context_processors.py
  models.py
Project Total  

This report was generated by python-coverage-comment-action

@lalver1 lalver1 force-pushed the feat/admin-agency-logos branch 5 times, most recently from 6f4337f to e255665 Compare November 14, 2024 21:52
@lalver1 lalver1 marked this pull request as ready for review November 14, 2024 22:07
@lalver1 lalver1 requested a review from a team as a code owner November 14, 2024 22:07
Copy link
Member

@thekaveman thekaveman left a 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:

  1. Define MEDIA_URL as the base public URL of the MEDIA_ROOT directory
  2. Map this URL and directory in nginx, similar to /static
  3. Use the convenience url attribute provided by Django on ImageField, 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.

benefits/settings.py Outdated Show resolved Hide resolved
@lalver1
Copy link
Member Author

lalver1 commented Nov 15, 2024

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

uploads/
!uploads/agencies/cst-*

to .gitignore in the root of the repo would make git ignore all files inside uploads except the 2 CST logos, but it didn't seem to be working, everything inside uploads was being ignored. From what I read, since uploads/ is ignored, the agencies folder is not visible to git so it won't be able to re-include the 2 CST logos. The changes in .gitignore in this PR work but I feel like they can be simplified in case you know of a more straightforward way to do this.

Also, L43 in urls.py is not covered, but it seems ok since we would only be testing .extend and static which I guess are already tested.

benefits/settings.py Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
benefits/settings.py Show resolved Hide resolved
@thekaveman
Copy link
Member

I think we can say this will close #2489 at this point 😁

Copy link

@lalver1
Copy link
Member Author

lalver1 commented Nov 18, 2024

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 dev, I'll probably ask Andy in Slack to confirm that the new logos look ok. I've tested on my screen but it'll be good to get confirmation from someone else too.

appcontainer/nginx.conf Outdated Show resolved Hide resolved
uploads/.gitignore Outdated Show resolved Hide resolved
Copy link
Member

@thekaveman thekaveman left a 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!

@lalver1 lalver1 merged commit f8f6fe5 into main Nov 18, 2024
15 checks passed
@lalver1 lalver1 deleted the feat/admin-agency-logos branch November 18, 2024 23:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
back-end Django views, sessions, middleware, models, migrations etc. migrations [auto] Review for potential model changes/needed data migrations updates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make agency logos configurable via Admin
2 participants