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

serve react app on 40x errors #911

Merged
merged 1 commit into from
May 20, 2024
Merged

Conversation

ChristopherChudzicki
Copy link
Contributor

@ChristopherChudzicki ChristopherChudzicki commented May 15, 2024

What are the relevant tickets?

Description (What does it do?)

This PR fixes and issue with our 40x handling that was introduced as part of #678:

How can this be tested?

  1. In your env file, set DEBUG=False
  2. docker compose up
  3. Visit, for example, http://localhost:8063/api/cats-and-dogs. This triggers handle_404, which should send you to the frontend "Not Found" page.
  4. Visit the rest of the app. It should work.
  5. Try API pages and the admin panel, too.

main/views.py Outdated
while preserving the erroring URL in the browser.
"""
html_content = Path(f"{settings.STATIC_ROOT}/mit-open/index.html").read_text()
return HttpResponse(html_content)
Copy link
Contributor Author

@ChristopherChudzicki ChristopherChudzicki May 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a better way to serve a literally static HTML file? (I.e., not a template file). I could not find one.

Serving a purely static HTML file via django seems strange—we could just redirect—but I don't see any other way to preserve the URL (like http://localhost:8063/api/v1/cats-and-dogs).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any issue adding its path location to the Nginx config (as you suggested in the comments)? What's the content of the html file (to help me understand why we need to serve it at all)?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HTML files are valid django templates - they just don't have and django template directives in them.

Since the files is under the static root directory, you'll probably need to add that directory to the templates directory configuration: https://docs.djangoproject.com/en/3.2/ref/settings/#dirs

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given your other comment below, you probably want to put this directory first in the list and leave main/templates/index.html as the failover for tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ended up keeping the touch ./frontends/mit-open/build/index.html for tests. If the file is missing in prod, we probably want an error not a fallback.

The nginx error_page approach discussed in slack seems appealing, but I couldn't get it to work. It just always showed the django error_page.

@@ -71,6 +71,8 @@ jobs:
run: ./scripts/test/stub-data.sh
- name: Tests
run: |
touch frontends/mit-open/build/index.html
Copy link
Contributor Author

@ChristopherChudzicki ChristopherChudzicki May 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added this touch for discussion.

One issue I ran into when serving the webpack build in handle_40x handlers without using a Django template is that the file won't exist without running the webpack build. This causes one of our tests to fail:

  1. test_initials_avatar_fake_user calls name_initials_avatar_view ...
  2. which calls redirect("static/images/avatar_default.png")...
  3. which for some reason triggers handle_404 (I don't understand why redirect triggers that).
  4. handle_404 wants the webpack built html file to exist, which then errors with 500 because it does not exist.

If you want to see this locally:

docker compose exec web bash
rm -rf staticfiles
pytest profiles/views_test.py --no-cov -k test_initials_avatar_fake_user -s

I added this touch to illustrate the issue. Doesn't seem like a good solution.

Copy link
Contributor

@rhysyngsun rhysyngsun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything looks good except for this error I'm seeing on both the home and 404 pages:

Screenshot 2024-05-17 at 16-10-26 Not Found

@ChristopherChudzicki
Copy link
Contributor Author

@rhysyngsun I think you just need to rebuild the frontend. yarn install doesn't happen on up anymore, just on build. The CKFinderUploadAdapter export was renamed in a recent CKEditor update.

@rhysyngsun rhysyngsun added Waiting on author and removed Needs Review An open Pull Request that is ready for review labels May 20, 2024
Copy link
Contributor

@rhysyngsun rhysyngsun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM once tests pass

@ChristopherChudzicki ChristopherChudzicki merged commit 4e19572 into main May 20, 2024
12 checks passed
This was referenced May 21, 2024
mbertrand pushed a commit that referenced this pull request Jun 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants