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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

lalver1
Copy link
Member

@lalver1 lalver1 commented Nov 7, 2024

Part of #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

Pending work (for a followup PR)

  • Change the modal--agency-selector template to use the new png logos and ensure that the new png logos display correctly on the browser (initial testing shows that the aspect ratio is off)

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_DB_DIR=., 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. Reopen folder locally in VSCode (stopping the dev container)
  5. In .env set DJANGO_DB_DIR=/home/calitp/app/data
  6. In compose.yml, set the volumes of the client service to - ./:/home/calitp/app/data (discard this change after this review)
  7. (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
  8. 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)
  9. 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
  benefits/core
  models.py
Project Total  

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

@lalver1 lalver1 force-pushed the feat/admin-agency-logos branch 3 times, most recently from 44d8492 to 6f4337f Compare November 14, 2024 20:38
@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
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.

1 participant