Skip to content

Conversation

GregKaleka
Copy link
Collaborator

@GregKaleka GregKaleka commented Sep 12, 2025

Fixes #1918

It turns out we were already doing the thing Sam suggested in the ticket (detecting meta redirects and redirecting server-side), but that code was not being triggered when fetching rendered content from the database - only from S3.

This turned out to be a simple fix of calling the detection code in the database fetching branch of code.

Note that to reproduce the bug locally, I needed to turn on the ENABLE_DB_CACHE setting, which is off by default. This was the main reason this bug was difficult to track down.


QA testing procedure

  1. Set the ENABLE_DB_CACHE environment variable to True (or as an easy hack, change the default in settings.py for testing)
  2. Manually navigate to /libs/url/
  3. Repeat step 2 now that the page has been cached in the db

On step 3, you should see the incorrect behavior (flicker) on the master branch, but the correct behavior (no flicker) on this branch.

Copy link
Collaborator

@daveoconnor daveoconnor left a comment

Choose a reason for hiding this comment

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

The general approach makes sense but I'd recommend putting the handling at a higher level, with the goal of a function doing only one thing with no side effects, for better readability.

Was going to approve this as is so you could get it out before you go, but looking at core/views.py around line 291 I suspect this might unintentionally short-circuit the database refresh logic that's there.

If this isn't urgent I can work this fix into the refactor I'm doing instead of changes to this PR. Might make more sense anyway. Let me know.

@GregKaleka
Copy link
Collaborator Author

@daveoconnor yep, that makes sense, and I did think about trying to do that. You're probably right it's a better approach.

Line 291 is in the parent class, and we override that method, so it doesn't come into play, but I do think it's still a better idea to move this up to keep it DRY.

@GregKaleka
Copy link
Collaborator Author

I don't see this as super urgent, given it's a bit of an edge case (you can't reproduce this bug without copy/pasting a url). If you think your work will land in the next week or so, that should be good enough.

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.

Improve the processing of client side automatic redirects
2 participants