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

Redirect to ManuscriptListView when manuscript is not found #803

Merged
merged 6 commits into from
Nov 21, 2023

Conversation

dchiller
Copy link
Collaborator

@dchiller dchiller commented Nov 20, 2023

Overrides the default retrieve method of the ManuscriptDetailView class so that when a manuscript with the requested ID is not found, a custom 404 page is returned that directs users to the ManuscriptListView (see below).

As mentioned in the function's comment, this is meant to mitigate cases where a link to a specific manuscript was published before manuscripts in Cantus Ultimus had stable ID's.

Closes #802

Custom 404 Page
https://user-images.githubusercontent.com/11023634/284639619-89ed0747-a41a-46ae-82f8-a69cdda4b741.png

@ahankinson
Copy link
Member

Why not do both? Return a 404 (signalling to browsers, caches, search engines, etc. that it's no longer there) but with a custom 404 page pointing to the MSS list, and letting the users know that they should update their bookmarks.

@ahankinson
Copy link
Member

@dchiller
Copy link
Collaborator Author

Ok, this makes sense! thank you

@dchiller dchiller marked this pull request as draft November 21, 2023 13:17
@dchiller
Copy link
Collaborator Author

image

@dchiller dchiller marked this pull request as ready for review November 21, 2023 15:58
Copy link

@jacobdgm jacobdgm left a comment

Choose a reason for hiding this comment

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

Looks good! I suggested a couple of small tweaks which you can implement if you'd like. The one in the docstring is extremely optional.

to return a custom 404 page if the manuscript
is not found. Implemented to prevent generic
404 errors from links to specific manuscripts
made before manuscript ids were stable.

Choose a reason for hiding this comment

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

would suggest IDs rather than ids.

<div class="row" style="padding-top:20vh;padding-bottom:20vh">
<div class="col-lg-12">
<p class="text-center">This manuscript has been moved.</p>
<p class="text-center">You can find it on <a href="{% url 'manuscript-list' %}">the Manuscripts page</a>.</p>
Copy link

@jacobdgm jacobdgm Nov 21, 2023

Choose a reason for hiding this comment

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

should the link be applied to "Manuscripts page", or even just "Manuscripts"? Including the article feels weird to me.

Both the title and the path of the page in question use just "manuscripts". Suggesting you move the "the" and the "page" out of the link.

@dchiller dchiller merged commit 0cf1533 into DDMAL:main Nov 21, 2023
2 checks passed
@dchiller dchiller deleted the i-802-redirect-old-manuscript-ids branch November 21, 2023 20:47
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.

Redirect requests to old manuscript id's to /manuscripts
3 participants