Skip to content

Conversation

@DHardy-WMF
Copy link
Contributor

@DHardy-WMF DHardy-WMF commented Aug 28, 2025

Description

Added new templates for custom error pages in Django, along with 500.html in static for the NGINX config.

Rationale

Currently, Wikilink only has generic 500 error pages. These can be confusing for end users. We should create more informative error pages.

Phabricator Ticket

https://phabricator.wikimedia.org/T350820

How Has This Been Tested?

Setting DEBUG = False in extlinks/settings/local.py.

Screenshots of your changes (if appropriate):

Screenshot 2025-09-03 at 3 32 05 PM

Types of changes

What types of changes does your code introduce? Add an x in all the boxes that apply:

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

katydidnot

This comment was marked as resolved.

katydidnot

This comment was marked as resolved.

@DHardy-WMF
Copy link
Contributor Author

Also, not sure if you saw my comment in the other review, but we should try to keep the PR template for documentation purposes.

Yes I saw it, but I can't find the PR template markdown.

@katydidnot

This comment was marked as resolved.

@DHardy-WMF
Copy link
Contributor Author

Also, not sure if you saw my comment in the other review, but we should try to keep the PR template for documentation purposes.

Yes I saw it, but I can't find the PR template markdown.

You should be able to go to old/closed pull requests and copy the template from there. Though, this isn't necessarily a blocker to merging it in.

Done!

Copy link
Contributor

@suecarmol suecarmol left a comment

Choose a reason for hiding this comment

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

Thanks for creating the templates. Some changes need to be made before we can merge this.

Copy link
Member

@jsnshrmn jsnshrmn left a comment

Choose a reason for hiding this comment

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

Noting that I'm not able to get the custom 500 page when I raise arbitrary exceptions within django; I've spent some time debugging, but have to call it a day.

@jsnshrmn
Copy link
Member

jsnshrmn commented Sep 4, 2025

I'd expect to see a custom 500 handler in there somewhere?

@DHardy-WMF
Copy link
Contributor Author

DHardy-WMF commented Sep 5, 2025

Noting that I'm not able to get the custom 500 page when I raise arbitrary exceptions within django; I've spent some time debugging, but have to call it a day.

That's strange... I can't seem to reproduce the issue. Did you explicitly run docker compose up -d and docker compose restart nginx to ensure the configs have updated properly?

@jsnshrmn
Copy link
Member

jsnshrmn commented Sep 5, 2025

yep, the 500 page is working fine from nginx if I take down the django service, but if I try to get it served from django I get the generic page. Verified by connecting directly to the django http server
image

@suecarmol
Copy link
Contributor

yep, the 500 page is working fine from nginx if I take down the django service, but if I try to get it served from django I get the generic page. Verified by connecting directly to the django http server image

Did you build your local environment with the Django Debug Toolbar? Right now, the 500 pages are only added to Django when DDT is on.

@DHardy-WMF
Copy link
Contributor Author

yep, the 500 page is working fine from nginx if I take down the django service, but if I try to get it served from django I get the generic page. Verified by connecting directly to the django http server image

Okay I was able to reproduce by connecting directly to Django. This should be fixed now!

Copy link
Member

@jsnshrmn jsnshrmn left a comment

Choose a reason for hiding this comment

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

👍

@jsnshrmn jsnshrmn merged commit 4f3f54a into master Sep 8, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants