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

Server-side redirects for plone.app.redirector aliases #4854

Merged
merged 15 commits into from
May 20, 2024

Conversation

JeffersonBledsoe
Copy link
Member

@JeffersonBledsoe JeffersonBledsoe commented Jun 7, 2023

Solves the Volto part of #4800. We're stripping the ++api++ if it is in a location change as we probably don't want to ever navigate directly to a page with ++api++ in it in the browser. Change the action causes the redux store to be correct, but the URL updating occurs earlier in the react-router stack, and so we also manually update the URL to not have ++api++ in it. History is still preserved correctly (++api++ pages aren't in the history stack but we can still go back and forwards normally).

The logic is currently mixed in with the blacklistRoutes middleware while I get some feedback, but this logic should probably be its own middleware for readability.

Note that the <Redirect> at

return <Redirect to={`${redirect}${this.props.location.search}`} />;
is causing SSR'd pages to send a 302 rather than forwarding on the status code sent by the REST API. Applying the changes mentioned in the react-router-dom docs for StaticRouting to pass the status along to the <StaticRouter> should resolve this and can be done in a follow-on PR.

@netlify
Copy link

netlify bot commented Jun 7, 2023

Deploy Preview for volto canceled.

Name Link
🔨 Latest commit 5b17e9c
🔍 Latest deploy log https://app.netlify.com/sites/volto/deploys/64e4cab01b989a000994dcab

@JeffersonBledsoe
Copy link
Member Author

Hi @mamico I've incorporated your changes from #4821 into this PR and fixed some of the issues mentioned by @davisagli as well as resolved some other issues I had with client-side and SSR'd content while trialing your PR. I don't think either PR makes sense without the other really which is why I've merged them into 1.

@JeffersonBledsoe JeffersonBledsoe deleted the handle-api-redirect branch June 7, 2023 16:57
@JeffersonBledsoe JeffersonBledsoe restored the handle-api-redirect branch June 7, 2023 16:59
@davisagli
Copy link
Member

@JeffersonBledsoe This is the new one you opened today. Did you mean to close it?

@JeffersonBledsoe
Copy link
Member Author

@davisagli Not sure what happened there, I didn't close it. I had a GitHub bug where i ended up with two branches of the same name which is why i created a second copy of this PR. Mind reviewing this one?

djay
djay previously requested changes Jun 8, 2023
src/components/theme/View/View.jsx Outdated Show resolved Hide resolved
@mamico

This comment was marked as outdated.

src/components/theme/View/View.jsx Outdated Show resolved Hide resolved
src/helpers/Api/Api.js Outdated Show resolved Hide resolved
@cypress
Copy link

cypress bot commented Jul 3, 2023

Passing run #6945 ↗︎

0 553 20 0 Flakiness 0

Details:

Merge branch 'master' into handle-api-redirect
Project: Volto Commit: 5b17e9c3e4
Status: Passed Duration: 14:30 💡
Started: Aug 22, 2023 2:51 PM Ended: Aug 22, 2023 3:05 PM

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

@mamico
Copy link
Member

mamico commented Jul 6, 2023

@JeffersonBledsoe @djay @davisagli there are any news on this? thx.

@JeffersonBledsoe
Copy link
Member Author

@mamico We’ve been running this in production for a while now, and while it’s mostly been okay, we’ve noticed some weird URLs appearing. I need to verify that this PR isn’t the cause before I’d feel comfortable merging it

@mamico
Copy link
Member

mamico commented Aug 22, 2023

@JeffersonBledsoe @djay @davisagli @sneridagh are there any updates on this? thx

@djay
Copy link
Member

djay commented Aug 23, 2023

@mamico what we are seeing is that google search console is ending up indexing urls like

https://digitalnsw.pretagov.com.au/components/cards?expand=actions,breadcrumbs,navigation&expand.navigation.depth=2

(where there was an old url of https://digitalnsw.pretagov.com.au/demo-pages/cards)

it might not be change to blame though. We just haven't had the time yet to investigate where these are coming from.

pnicolli pushed a commit to RedTurtle/design-comuni-plone-theme that referenced this pull request Aug 23, 2023
giuliaghisini pushed a commit to RedTurtle/design-comuni-plone-theme that referenced this pull request Aug 24, 2023
* fix: backport plone/volto#5069

* fix: backport plone/volto#4854

* strip query string
mamico added a commit to RedTurtle/design-comuni-plone-theme that referenced this pull request Aug 28, 2023
pnicolli pushed a commit to RedTurtle/design-comuni-plone-theme that referenced this pull request Sep 4, 2023
Copy link

netlify bot commented Mar 22, 2024

Deploy Preview for volto failed. Why did it fail? →

Name Link
🔨 Latest commit e623ad6
🔍 Latest deploy log https://app.netlify.com/sites/volto/deploys/66467c351f6a9000081ba387

Copy link

netlify bot commented Mar 22, 2024

Deploy Preview for plone-components canceled.

Name Link
🔨 Latest commit e623ad6
🔍 Latest deploy log https://app.netlify.com/sites/plone-components/deploys/66467c356337f200089f947e

@davisagli davisagli force-pushed the handle-api-redirect branch from 7322e19 to 8fdab9f Compare May 14, 2024 01:05
@davisagli davisagli mentioned this pull request May 14, 2024
@davisagli davisagli dismissed djay’s stale review May 14, 2024 03:33

lots of changes since this review

Copy link
Member

@davisagli davisagli left a comment

Choose a reason for hiding this comment

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

@sneridagh I think this is ready now. The fix is basically the same as Rob's from #5999 but it strips /++api++ from the redirect location if it is there.

@JeffersonBledsoe
Copy link
Member Author

Thanks for finishing this!

@davisagli davisagli changed the title Strip ++api++ from in-browser location changes Server-side redirects for plone.app.redirector aliases May 15, 2024
@sneridagh sneridagh merged commit 96cc4b1 into main May 20, 2024
69 of 73 checks passed
@sneridagh sneridagh deleted the handle-api-redirect branch May 20, 2024 08:18
davisagli added a commit that referenced this pull request May 20, 2024
sneridagh pushed a commit that referenced this pull request May 21, 2024
Co-authored-by: Jefferson Bledsoe <[email protected]>
Co-authored-by: Mauro Amico <[email protected]>
Co-authored-by: Steve Piercy <[email protected]>
pnicolli added a commit that referenced this pull request Jul 4, 2024
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