From 692ae8c31caa5a404142c9f46e03fdc8dc5b233f Mon Sep 17 00:00:00 2001 From: Wicklow <123956049+wickloww@users.noreply.github.com> Date: Thu, 2 Mar 2023 13:50:55 +0000 Subject: [PATCH] Hotfix/filter subscription videos (#5665) * Fix filters on subscription videos * Add tests to common video filters * Improve reliability when skipping subscrition path * Better parameters for skipping subscription videos --- .../api/check-params/videos-common-filters.ts | 3 +- server/tests/api/users/user-subscriptions.ts | 37 +++++++---- .../tests/api/videos/videos-common-filters.ts | 63 +++++++++++++------ server/tests/feeds/feeds.ts | 8 +-- .../fixtures/peertube-plugin-test/main.js | 2 +- server/tests/plugins/filter-hooks.ts | 8 +-- .../users/subscriptions-command.ts | 18 +----- .../server-commands/videos/videos-command.ts | 14 +++++ 8 files changed, 94 insertions(+), 59 deletions(-) diff --git a/server/tests/api/check-params/videos-common-filters.ts b/server/tests/api/check-params/videos-common-filters.ts index 95523ce3d..11d9fd95b 100644 --- a/server/tests/api/check-params/videos-common-filters.ts +++ b/server/tests/api/check-params/videos-common-filters.ts @@ -42,7 +42,8 @@ describe('Test video filters validators', function () { '/api/v1/video-channels/root_channel/videos', '/api/v1/accounts/root/videos', '/api/v1/videos', - '/api/v1/search/videos' + '/api/v1/search/videos', + '/api/v1/users/me/subscriptions/videos' ] for (const path of paths) { diff --git a/server/tests/api/users/user-subscriptions.ts b/server/tests/api/users/user-subscriptions.ts index b45cfe67e..ad2b82a4a 100644 --- a/server/tests/api/users/user-subscriptions.ts +++ b/server/tests/api/users/user-subscriptions.ts @@ -167,14 +167,14 @@ describe('Test users subscriptions', function () { it('Should list subscription videos', async function () { { - const body = await command.listVideos() + const body = await servers[0].videos.listMySubscriptionVideos() expect(body.total).to.equal(0) expect(body.data).to.be.an('array') expect(body.data).to.have.lengthOf(0) } { - const body = await command.listVideos({ token: users[0].accessToken, sort: 'createdAt' }) + const body = await servers[0].videos.listMySubscriptionVideos({ token: users[0].accessToken, sort: 'createdAt' }) expect(body.total).to.equal(3) const videos = body.data @@ -185,6 +185,17 @@ describe('Test users subscriptions', function () { expect(videos[1].name).to.equal('video 2-3') expect(videos[2].name).to.equal('video server 3 added after follow') } + + { + const body = await servers[0].videos.listMySubscriptionVideos({ token: users[0].accessToken, count: 1, start: 1 }) + expect(body.total).to.equal(3) + + const videos = body.data + expect(videos).to.be.an('array') + expect(videos).to.have.lengthOf(1) + + expect(videos[0].name).to.equal('video 2-3') + } }) it('Should upload a video by root on server 1 and see it in the subscription videos', async function () { @@ -196,14 +207,14 @@ describe('Test users subscriptions', function () { await waitJobs(servers) { - const body = await command.listVideos() + const body = await servers[0].videos.listMySubscriptionVideos() expect(body.total).to.equal(0) expect(body.data).to.be.an('array') expect(body.data).to.have.lengthOf(0) } { - const body = await command.listVideos({ token: users[0].accessToken, sort: 'createdAt' }) + const body = await servers[0].videos.listMySubscriptionVideos({ token: users[0].accessToken, sort: 'createdAt' }) expect(body.total).to.equal(4) const videos = body.data @@ -264,14 +275,14 @@ describe('Test users subscriptions', function () { it('Should still list subscription videos', async function () { { - const body = await command.listVideos() + const body = await servers[0].videos.listMySubscriptionVideos() expect(body.total).to.equal(0) expect(body.data).to.be.an('array') expect(body.data).to.have.lengthOf(0) } { - const body = await command.listVideos({ token: users[0].accessToken, sort: 'createdAt' }) + const body = await servers[0].videos.listMySubscriptionVideos({ token: users[0].accessToken, sort: 'createdAt' }) expect(body.total).to.equal(4) const videos = body.data @@ -295,7 +306,7 @@ describe('Test users subscriptions', function () { await waitJobs(servers) - const body = await command.listVideos({ token: users[0].accessToken, sort: 'createdAt' }) + const body = await servers[0].videos.listMySubscriptionVideos({ token: users[0].accessToken, sort: 'createdAt' }) expect(body.data[2].name).to.equal('video server 3 added after follow updated') }) }) @@ -311,7 +322,7 @@ describe('Test users subscriptions', function () { }) it('Should not display its videos anymore', async function () { - const body = await command.listVideos({ token: users[0].accessToken, sort: 'createdAt' }) + const body = await servers[0].videos.listMySubscriptionVideos({ token: users[0].accessToken, sort: 'createdAt' }) expect(body.total).to.equal(1) const videos = body.data @@ -360,7 +371,7 @@ describe('Test users subscriptions', function () { await waitJobs(servers) { - const body = await command.listVideos({ token: users[0].accessToken, sort: 'createdAt' }) + const body = await servers[0].videos.listMySubscriptionVideos({ token: users[0].accessToken, sort: 'createdAt' }) expect(body.total).to.equal(3) const videos = body.data @@ -569,13 +580,13 @@ describe('Test users subscriptions', function () { await waitJobs(servers) { - const { data } = await command.listVideos({ token: users[0].accessToken }) + const { data } = await servers[0].videos.listMySubscriptionVideos({ token: users[0].accessToken }) expect(data.find(v => v.name === 'internal')).to.not.exist } }) it('Should see internal from local user', async function () { - const { data } = await servers[2].subscriptions.listVideos({ token: servers[2].accessToken }) + const { data } = await servers[2].videos.listMySubscriptionVideos({ token: servers[2].accessToken }) expect(data.find(v => v.name === 'internal')).to.exist }) @@ -586,12 +597,12 @@ describe('Test users subscriptions', function () { await waitJobs(servers) { - const { data } = await command.listVideos({ token: users[0].accessToken }) + const { data } = await servers[0].videos.listMySubscriptionVideos({ token: users[0].accessToken }) expect(data.find(v => v.name === 'private')).to.not.exist } { - const { data } = await servers[2].subscriptions.listVideos({ token: servers[2].accessToken }) + const { data } = await servers[2].videos.listMySubscriptionVideos({ token: servers[2].accessToken }) expect(data.find(v => v.name === 'private')).to.not.exist } }) diff --git a/server/tests/api/videos/videos-common-filters.ts b/server/tests/api/videos/videos-common-filters.ts index 160091861..1ab78ac49 100644 --- a/server/tests/api/videos/videos-common-filters.ts +++ b/server/tests/api/videos/videos-common-filters.ts @@ -20,6 +20,8 @@ describe('Test videos filter', function () { let paths: string[] let remotePaths: string[] + const subscriptionVideosPath = '/api/v1/users/me/subscriptions/videos' + // --------------------------------------------------------------- before(async function () { @@ -49,6 +51,9 @@ describe('Test videos filter', function () { const attributes = { name: 'private ' + server.serverNumber, privacy: VideoPrivacy.PRIVATE } await server.videos.upload({ attributes }) } + + // Subscribing to itself + await server.subscriptions.add({ targetUri: 'root_channel@' + server.host }) } await doubleFollow(servers[0], servers[1]) @@ -57,7 +62,8 @@ describe('Test videos filter', function () { `/api/v1/video-channels/root_channel/videos`, `/api/v1/accounts/root/videos`, '/api/v1/videos', - '/api/v1/search/videos' + '/api/v1/search/videos', + subscriptionVideosPath ] remotePaths = [ @@ -70,10 +76,20 @@ describe('Test videos filter', function () { describe('Check deprecated videos filter', function () { - async function getVideosNames (server: PeerTubeServer, token: string, filter: string, expectedStatus = HttpStatusCode.OK_200) { + async function getVideosNames (options: { + server: PeerTubeServer + token: string + filter: string + skipSubscription?: boolean + expectedStatus?: HttpStatusCode + }) { + const { server, token, filter, skipSubscription = false, expectedStatus = HttpStatusCode.OK_200 } = options + const videosResults: Video[][] = [] for (const path of paths) { + if (skipSubscription && path === subscriptionVideosPath) continue + const res = await makeGetRequest({ url: server.url, path, @@ -93,7 +109,7 @@ describe('Test videos filter', function () { it('Should display local videos', async function () { for (const server of servers) { - const namesResults = await getVideosNames(server, server.accessToken, 'local') + const namesResults = await getVideosNames({ server, token: server.accessToken, filter: 'local' }) for (const names of namesResults) { expect(names).to.have.lengthOf(1) expect(names[0]).to.equal('public ' + server.serverNumber) @@ -105,7 +121,7 @@ describe('Test videos filter', function () { for (const server of servers) { for (const token of [ server.accessToken, server['moderatorAccessToken'] ]) { - const namesResults = await getVideosNames(server, token, 'all-local') + const namesResults = await getVideosNames({ server, token, filter: 'all-local', skipSubscription: true }) for (const names of namesResults) { expect(names).to.have.lengthOf(3) @@ -121,7 +137,7 @@ describe('Test videos filter', function () { for (const server of servers) { for (const token of [ server.accessToken, server['moderatorAccessToken'] ]) { - const [ channelVideos, accountVideos, videos, searchVideos ] = await getVideosNames(server, token, 'all') + const [ channelVideos, accountVideos, videos, searchVideos ] = await getVideosNames({ server, token, filter: 'all' }) expect(channelVideos).to.have.lengthOf(3) expect(accountVideos).to.have.lengthOf(3) @@ -162,17 +178,23 @@ describe('Test videos filter', function () { return res.body.data as Video[] } - async function getVideosNames (options: { - server: PeerTubeServer - isLocal?: boolean - include?: VideoInclude - privacyOneOf?: VideoPrivacy[] - token?: string - expectedStatus?: HttpStatusCode - }) { + async function getVideosNames ( + options: { + server: PeerTubeServer + isLocal?: boolean + include?: VideoInclude + privacyOneOf?: VideoPrivacy[] + token?: string + expectedStatus?: HttpStatusCode + skipSubscription?: boolean + } + ) { + const { skipSubscription = false } = options const videosResults: string[][] = [] for (const path of paths) { + if (skipSubscription && path === subscriptionVideosPath) continue + const videos = await listVideos({ ...options, path }) videosResults.push(videos.map(v => v.name)) @@ -196,12 +218,15 @@ describe('Test videos filter', function () { for (const server of servers) { for (const token of [ server.accessToken, server['moderatorAccessToken'] ]) { - const namesResults = await getVideosNames({ - server, - token, - isLocal: true, - privacyOneOf: [ VideoPrivacy.UNLISTED, VideoPrivacy.PUBLIC, VideoPrivacy.PRIVATE ] - }) + const namesResults = await getVideosNames( + { + server, + token, + isLocal: true, + privacyOneOf: [ VideoPrivacy.UNLISTED, VideoPrivacy.PUBLIC, VideoPrivacy.PRIVATE ], + skipSubscription: true + } + ) for (const names of namesResults) { expect(names).to.have.lengthOf(3) diff --git a/server/tests/feeds/feeds.ts b/server/tests/feeds/feeds.ts index 7345f728a..ecd1badc1 100644 --- a/server/tests/feeds/feeds.ts +++ b/server/tests/feeds/feeds.ts @@ -387,7 +387,7 @@ describe('Test syndication feeds', () => { } { - const body = await servers[0].subscriptions.listVideos({ token: feeduserAccessToken }) + const body = await servers[0].videos.listMySubscriptionVideos({ token: feeduserAccessToken }) expect(body.total).to.equal(0) const query = { accountId: feeduserAccountId, token: feeduserFeedToken } @@ -408,7 +408,7 @@ describe('Test syndication feeds', () => { }) it('Should list no videos for a user with videos but no subscriptions', async function () { - const body = await servers[0].subscriptions.listVideos({ token: userAccessToken }) + const body = await servers[0].videos.listMySubscriptionVideos({ token: userAccessToken }) expect(body.total).to.equal(0) const query = { accountId: userAccountId, token: userFeedToken } @@ -424,7 +424,7 @@ describe('Test syndication feeds', () => { await waitJobs(servers) { - const body = await servers[0].subscriptions.listVideos({ token: userAccessToken }) + const body = await servers[0].videos.listMySubscriptionVideos({ token: userAccessToken }) expect(body.total).to.equal(1) expect(body.data[0].name).to.equal('user video') @@ -442,7 +442,7 @@ describe('Test syndication feeds', () => { await waitJobs(servers) { - const body = await servers[0].subscriptions.listVideos({ token: userAccessToken }) + const body = await servers[0].videos.listMySubscriptionVideos({ token: userAccessToken }) expect(body.total).to.equal(2, 'there should be 2 videos part of the subscription') const query = { accountId: userAccountId, token: userFeedToken } diff --git a/server/tests/fixtures/peertube-plugin-test/main.js b/server/tests/fixtures/peertube-plugin-test/main.js index 8b918b634..f01da0226 100644 --- a/server/tests/fixtures/peertube-plugin-test/main.js +++ b/server/tests/fixtures/peertube-plugin-test/main.js @@ -91,7 +91,7 @@ async function register ({ registerHook, registerSetting, settingsManager, stora registerHook({ target: 'filter:api.user.me.subscription-videos.list.params', - handler: obj => Object.assign({}, obj, { count: 1 }) + handler: obj => addToCount(obj) }) registerHook({ diff --git a/server/tests/plugins/filter-hooks.ts b/server/tests/plugins/filter-hooks.ts index 6bd38cf65..a7e052d06 100644 --- a/server/tests/plugins/filter-hooks.ts +++ b/server/tests/plugins/filter-hooks.ts @@ -155,14 +155,14 @@ describe('Test plugin filter hooks', function () { }) it('Should run filter:api.user.me.subscription-videos.list.params', async function () { - const { data } = await servers[0].subscriptions.listVideos() + const { data } = await servers[0].videos.listMySubscriptionVideos({ start: 0, count: 2 }) - // 1 plugin set the count parameter to 1 - expect(data).to.have.lengthOf(1) + // 1 plugin do +1 to the count parameter + expect(data).to.have.lengthOf(3) }) it('Should run filter:api.user.me.subscription-videos.list.result', async function () { - const { total } = await servers[0].subscriptions.listVideos() + const { total } = await servers[0].videos.listMySubscriptionVideos({ start: 0, count: 2 }) // Plugin do +4 to the total result expect(total).to.equal(14) diff --git a/shared/server-commands/users/subscriptions-command.ts b/shared/server-commands/users/subscriptions-command.ts index edc60e612..b92f037f8 100644 --- a/shared/server-commands/users/subscriptions-command.ts +++ b/shared/server-commands/users/subscriptions-command.ts @@ -1,4 +1,4 @@ -import { HttpStatusCode, ResultList, Video, VideoChannel } from '@shared/models' +import { HttpStatusCode, ResultList, VideoChannel } from '@shared/models' import { AbstractCommand, OverrideCommandOptions } from '../shared' export class SubscriptionsCommand extends AbstractCommand { @@ -38,22 +38,6 @@ export class SubscriptionsCommand extends AbstractCommand { }) } - listVideos (options: OverrideCommandOptions & { - sort?: string // default -createdAt - } = {}) { - const { sort = '-createdAt' } = options - const path = '/api/v1/users/me/subscriptions/videos' - - return this.getRequestBody>({ - ...options, - - path, - query: { sort }, - implicitToken: true, - defaultExpectedStatus: HttpStatusCode.OK_200 - }) - } - get (options: OverrideCommandOptions & { uri: string }) { diff --git a/shared/server-commands/videos/videos-command.ts b/shared/server-commands/videos/videos-command.ts index 590244484..b5df9c325 100644 --- a/shared/server-commands/videos/videos-command.ts +++ b/shared/server-commands/videos/videos-command.ts @@ -210,6 +210,20 @@ export class VideosCommand extends AbstractCommand { }) } + listMySubscriptionVideos (options: OverrideCommandOptions & VideosCommonQuery = {}) { + const { sort = '-createdAt' } = options + const path = '/api/v1/users/me/subscriptions/videos' + + return this.getRequestBody>({ + ...options, + + path, + query: { sort, ...this.buildListQuery(options) }, + implicitToken: true, + defaultExpectedStatus: HttpStatusCode.OK_200 + }) + } + // --------------------------------------------------------------------------- list (options: OverrideCommandOptions & VideosCommonQuery = {}) {