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

fix links in prefixed sites built with the mass-build-sites pipeline #2121

Merged
merged 1 commit into from
Mar 5, 2024

Conversation

gumaerc
Copy link
Contributor

@gumaerc gumaerc commented Feb 29, 2024

What are the relevant tickets?

Closes #2113

Description (What does it do?)

This PR takes the prefix argument being sent to SitePipelineDefinition and in addition to prefixing the S3 path with it, the Hugo baseURL argument is now also prefixed. This means that links rendered inside of sites deployed at a prefix URL will correctly link you to the prefixed page, at least for content generated by Hugo.

How can this be tested?

  • Make sure you have run through the basic requirements for setting up ocw-studio and can publish sites locally
  • Run docker compose exec web ./manage.py upsert_mass_build_pipeline --prefix test-prefix
  • Browse to http://localhost:8080 and log into the Concourse web UI
  • Find the prefixed mass build pipeline, unpause it and trigger the first batch. You can pause it after, as you only need to run through one batch to test this.
  • In the batch's across step, find of of the sites it published and copy the url_path. Browse to the site by appending the url_path and the prefix to http://localhost:8045/. It should be something like http://localhost:8045/test-prefix/courses/site-name
  • Click the links in the sidebar to browse the different parts of the site and verify that you stay under the test-prefix

@gumaerc gumaerc marked this pull request as ready for review February 29, 2024 20:15
@ibrahimjaved12 ibrahimjaved12 self-requested a review March 1, 2024 13:37
Copy link
Contributor

@ibrahimjaved12 ibrahimjaved12 left a comment

Choose a reason for hiding this comment

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

Tested on both localhost:8044(draft) and localhost:8045(live). Tried multiple links there on few courses, works as expected.

However I noticed that it does not:

  • Work for ocw-www (http://localhost:8045/ or http://localhost:8044/)
    • Course links directing to ocw-www eg. When you're on a course's homepage, and you click to its relevant department. It redirects to the search page with department as query, but the URL is without the given prefix.

I think prefixing URLs on ocw-www would also be good to have?

@gumaerc gumaerc force-pushed the cg/fix-mass-build-prefix-links branch from 70bf4df to 8ea35bb Compare March 4, 2024 22:23
@gumaerc
Copy link
Contributor Author

gumaerc commented Mar 4, 2024

@ibrahimjaved12 Thanks for your review. I looked into home page links, and I believe it's a bit beyond the scope of what this PR is doing. This PR simply sets the Hugo --baseUrl parameter to include the prefix. This means that links generated by Hugo (.Permalink, .RelPermalink, pageRef) will contain the prefix and work as expected. However, The links to the search page are mostly hard coded in ocw-hugo-themes to be relative to the root of the site. I think that in order to support this, we would need a PR in ocw-hugo-themes to be able to pull the prefix from an env variable and set it where necessary. This especially applies to the links within the search page itself, as the search page is a separate React app not generated by Hugo.

Copy link
Contributor

@ibrahimjaved12 ibrahimjaved12 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 to me then 👍

@gumaerc
Copy link
Contributor Author

gumaerc commented Mar 5, 2024

I created an issue here for adding support to the www theme for prefixed deployments: mitodl/ocw-hugo-themes#1365

@gumaerc gumaerc merged commit 32c7ab8 into master Mar 5, 2024
4 checks passed
@gumaerc gumaerc deleted the cg/fix-mass-build-prefix-links branch March 5, 2024 20:12
@odlbot odlbot mentioned this pull request Mar 5, 2024
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The mass build sites pipeline's prefix feature doesn't properly set Hugo baseURL
2 participants