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

Some redirects return 302 when they should probably return 301 #1110

Closed
dchiller opened this issue Oct 27, 2023 · 6 comments
Closed

Some redirects return 302 when they should probably return 301 #1110

dchiller opened this issue Oct 27, 2023 · 6 comments
Assignees
Labels
priority: medium simple fix Should be straightforward to implement

Comments

@dchiller
Copy link
Contributor

In my excavations of the CantusDB logs, I discovered that responses to some requests that I think are permanent redirect are actually returning 302 responses.

In particular, /node/<id> and /office return 302's rather than 301's. My understanding is that both of these redirects are permanent for the purpose of harmonizing Old and NewCantus URLs.

@jacobdgm
Copy link
Contributor

jacobdgm commented Oct 27, 2023

And 301 indicates a permanent redirect, correct?

We should be a bit careful - in something relating to DataLake earlier today, we were talking about renaming, say, /sources to /source. We've already switched /genre to redirect to /genres, so we should be careful not to swap things back and forth.

@jacobdgm
Copy link
Contributor

jacobdgm commented Oct 27, 2023

Redirects that should be permanent, I think:

  • sites/default/files/csv/<str:source_id>.csv -> source/<str:source_id>/csv/
  • node/<int:pk> -> source, chant, sequence, etc. detail page
  • indexer/<int:pk> -> the relevant user page
  • sites/default/files/documents/1. Quick Guide to Liturgy.pdf, sites/default/files/documents/2. Volpiano Protocols.pdf, ..., sites/default/files/HOW TO - manuscript descriptions-Nov6-20.pdf -> the relevant static files

some which we may not be certain about:

  • /genre -> /genres (but maybe this one is properly permanent?)
  • /office -> /offices (ditto)
  • (somehow, /feasts has always been /feasts...)

@dchiller
Copy link
Contributor Author

And 301 indicates a permanent redirect, correct?

Yes

@jacobdgm
Copy link
Contributor

Looks like this will be trivial to implement once we're very confident of our list of permanent redirects. return redirect(reverse("csv-export", args=[source_id])) becomes return redirect(reverse("csv-export", args=[source_id]), permanent=True)

@jacobdgm jacobdgm added priority: medium simple fix Should be straightforward to implement labels Oct 27, 2023
@jacobdgm
Copy link
Contributor

jacobdgm commented Mar 8, 2024

Going to make an executive decision and say we're currently happy with all of our redirects, and that we should communicate that they are permanent. This will just require someone going through all the redirects in our views and adding a permanent=True kwarg.

@dchiller
Copy link
Contributor Author

Looks like this was solved in #1582...closing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: medium simple fix Should be straightforward to implement
Projects
None yet
Development

No branches or pull requests

2 participants