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

fix: delete boilerplate favicon on login page #6146

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

willmurphyscode
Copy link

@willmurphyscode willmurphyscode commented Dec 23, 2024

Previously, network tools show a 404 getting /users/assets/images/favicon.svg on /users/sign_in, and some tests intermittently failed with this 404. Delete the extraneous favicon link from devise boilerplate to let the normal favicon work on the login screen.

Fixes test flake at
https://github.com/rubyforgood/casa/wiki/Flaky-tests#suspected-testing-tool-crash

What github issue is this PR for, if any?

Resolves flaky test at . The test reports sometimes failing with No route matches [GET] "/users/assets/images/favicon.svg".

What changed, and why?

The devise-generated view at app/views/layouts/devise.html.erb had its own favicon, which would make 404s on the page, and these 404s would fail some tests. Deleting this link tag lets the normal favicon present on other pages work.

Note: I initially tried using href="<%= asset_path("favicon.svg")%." instead, but favicon.svg is committed directly at public/assets/images/favicon.svg, so Rails doesn't seem to actually have an asset path for it. I don't know why this was done, so I don't want to undo it. The file at public/assets/images/favicon.svg does not seem to be the right favicon for the project.

How is this tested? (please write tests!) 💖💪

Note: if you see a flake in your test build in github actions, please post in slack #casa "Flaky test: " :) 💪
Note: We love capybara tests! If you are writing both haml/js and ruby, please try to test your work with tests at every level including system tests like https://github.com/rubyforgood/casa/tree/main/spec/system

On main, in dev tools, you can see a 404 on GET http://0.0.0.0:3000/users/assets/images/favicon.svg. On this branch, the favicon loads and is the expected CASA logo.

I'm happy to add an automated test for this, but I wasn't sure where one belonged.

One important note is that during testing, I was not able to make the failure described at https://github.com/rubyforgood/casa/wiki/Flaky-tests#suspected-testing-tool-crash happen on purpose, so this is a bit of a speculative fix. But it does fix a 404 that was happening during local development, and that currently happens in prod on https://casavolunteertracking.org/users/sign_in

Screenshots please :)

Run your local server and take a screenshot of your work! Try to include the URL of the page as well as the contents of the page.

Before:

  • browser tab: image
  • network tools: image

After:

  • browser tab: image

  • network tools: image

@willmurphyscode
Copy link
Author

Maybe we should change favicon to https://casavolunteertracking.org/favicon.ico instead? That seems to be what's actually used in prod, and seems to be an actual logo of the org. Will update the PR.

Previously, network tools show a 404 getting
/users/assets/images/favicon.svg on /users/sign_in, and some tests
intermittently failed with this 404. Delete the extraneous favicon link
from devise boilerplate to let the normal favicon work on the login
screen.

Fixes test flake at
https://github.com/rubyforgood/casa/wiki/Flaky-tests#suspected-testing-tool-crash
@willmurphyscode willmurphyscode marked this pull request as draft December 23, 2024 19:13
@willmurphyscode willmurphyscode changed the title fix: look for favicon at absolute path fix: delete boilerplate favicon on login page Dec 23, 2024
@willmurphyscode willmurphyscode marked this pull request as ready for review December 23, 2024 19:18
@willmurphyscode
Copy link
Author

Hi friends! I think this is ready for review.

  1. I can't reproduce the docker test failure. I think it is probably a flake. (To attempt: I got the docker tests running locally and re-ran with the same seed number that failed in CI, and it didn't fail.)
  2. I don't think adding a system test that says something like, "the login in page doesn't add a random extra favicon" is a super useful test. The test will be slow and fiddly (because a JS library actually chooses the icon), and both the impact of a regression (e.g. login page has no favicon) and the likelihood of a regression (someone re-adds copy/paste boilerplate to the devise view) are super low.

Let me know if there's anything else I should do to get this in.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant