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

Set canonical link to original video/playlist url #3406

Conversation

tcitworld
Copy link
Collaborator

The local URL was used instead of the original url

Description

The local URL was used instead of the original url

Related issues

Previously fixed in #1539

Added a test to check but didn't manage to run them, so they're all yours.

The local URL was used instead of the original url

Signed-off-by: Thomas Citharel <[email protected]>
Signed-off-by: Thomas Citharel <[email protected]>
@tcitworld tcitworld added the Type: Bug 🐛 Confirmed bug, at least replicated once by another contributor label Dec 6, 2020
@@ -47,8 +51,19 @@ describe('Test a client controllers', function () {
before(async function () {
this.timeout(120000)

server = await flushAndRunServer(1)
server.accessToken = await serverLogin(server)
servers = await flushAndRunMultipleServers(2)
Copy link
Contributor

Choose a reason for hiding this comment

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

A fancy way would be [server, secondServer] = await flushAndRunMultipleServers(2)

await setAccessTokensToServers(servers)

{
const res = await uploadVideo(servers[0].url, servers[0].accessToken, { name: 'video' })
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really need to upload a video here? According to line 78 there's already a video on the server.

If we need this line, then line 78 has to be updated (that's why tests are failing).


it('Should use the original video URL for the canonical tag', async function () {
const res = await makeHTMLRequest(servers[1].url, '/videos/watch/' + videoUUID)
expect(res.text).to.contain(`<link rel="canonical" href="${videoOriginalUrl}" />`)
Copy link
Contributor

Choose a reason for hiding this comment

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

By using server.video we could change the test to:

      const res = await makeHTMLRequest(servers[1].url, '/videos/watch/' + server.video.uuid)
      expect(res.text).to.contain(`<link rel="canonical" href="${server.video.url}" />`)

@Chocobozzz
Copy link
Owner

Thanks @tcitworld @kontrollanten

@Chocobozzz Chocobozzz merged commit a59f210 into Chocobozzz:develop Dec 8, 2020
@creasysee creasysee mentioned this pull request Aug 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug 🐛 Confirmed bug, at least replicated once by another contributor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants