search

cal-itp / benefits

Transit benefits enrollment, minus the paperwork.
https://docs.calitp.org/benefits
GNU Affero General Public License v3.0
28 stars 9 forks source link

Feat: agency logos configurable via Admin #2514

Closed lalver1 closed 1 week ago

lalver1 commented 2 weeks ago

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.
github-actions[bot] commented 2 weeks ago

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 commented 1 week ago

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.

thekaveman commented 1 week ago

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

github-actions[bot] commented 1 week ago

Preview url: https://benefits-2514--cal-itp-previews.netlify.app

lalver1 commented 1 week ago

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.