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

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) {

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
       }
     })

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)

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 }

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({

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)

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<ResultList<Video>>({
-      ...options,
-
-      path,
-      query: { sort },
-      implicitToken: true,
-      defaultExpectedStatus: HttpStatusCode.OK_200
-    })
-  }
-
   get (options: OverrideCommandOptions & {
     uri: string
   }) {

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<ResultList<Video>>({
+      ...options,
+
+      path,
+      query: { sort, ...this.buildListQuery(options) },
+      implicitToken: true,
+      defaultExpectedStatus: HttpStatusCode.OK_200
+    })
+  }
+
   // ---------------------------------------------------------------------------
 
   list (options: OverrideCommandOptions & VideosCommonQuery = {}) {