Add Video Indexer v2 - #66
Conversation
CommonService route params
noce2
changed the title
|
Thanks @noce2! The 4 tests pass. Only nitpick is the missing |
|
@miparnisari @noce2 I was going to open a PR for this myself but this looks done. If I were to add the |
|
Hi @miparnisari, can you close this open request? It would be great for me to continue. Moreover, I tried to look around any file/folder for indexer under vision folder. But I didn't find any. |
|
@noce2 can you review my comment and fix the merge conflict? |
|
sure :) |
| * @param {boolean} obj.allowEdit | ||
| * @returns {Promise<[Object]>} A promise containing an array of objects | ||
| */ | ||
| getAccounts({ |
There was a problem hiding this comment.
Is this specific to the video indexer api? If not, can we pull this out into a separate file?
| throw new Error('no video to upload. A video must be specified, either via videoUrl or a path'); | ||
| } | ||
|
|
||
| const pathToVideo = (params.path) ? params.path : params.videoUrl; |
There was a problem hiding this comment.
Can this be simplified via params.path || params.videoUrl?
|
|
||
| const pathToVideo = (params.path) ? params.path : params.videoUrl; | ||
|
|
||
| const isFormatSupported = this.supportedFormats.filter(sf => { |
There was a problem hiding this comment.
Is this covered by tests?
| */ | ||
| uploadVideo(params){ | ||
| if(!params.path && !params.videoUrl) { | ||
| throw new Error('no video to upload. A video must be specified, either via videoUrl or a path'); |
There was a problem hiding this comment.
Is this covered by tests?
|
|
||
| if(params.videoUrl && !((/(http)s?(:\/\/)/).test(params.videoUrl))){ | ||
| // video should point to public url | ||
| throw new Error('videoUrl is not an http/https link. A videoUrl must contain a public path'); |
There was a problem hiding this comment.
Is this covered by tests?
| apiKey: config.videoIndexerV2.apiKey | ||
| }); | ||
|
|
||
| const videoUrl = 'https://mparnisari.blob.core.windows.net/storage/video_girl_laughing.mp4'; |
| client.getUserAccessToken(requestParams) | ||
| .then(response => { | ||
| should(response).not.be.undefined(); | ||
| should(response).startWith("ey", "the encoding of brackets alwys generates this at the start"); |
| client.getAccountAccessToken(requestParams) | ||
| .then(response => { | ||
| should(response).not.be.undefined(); | ||
| should(response).startWith("ey", "the encoding of brackets alwys generates this at the start"); |
|
|
||
| const requestHeaders = {}; | ||
|
|
||
| if(params.videoUrl && !((/(http)s?(:\/\/)/).test(params.videoUrl))){ |
There was a problem hiding this comment.
This line and line 190 can be simplified: if(<regex>.test(pathToVideo)). No need to do this check twice
| * Uploads a video to the Video Indexer | ||
| */ | ||
| uploadVideo(params){ | ||
| if(!params.path && !params.videoUrl) { |
There was a problem hiding this comment.
What if I specify both? Which will be used?
| const videoUrl = 'https://mparnisari.blob.core.windows.net/storage/video_girl_laughing.mp4'; | ||
| const videoPath = './test/assets/video_girl_laughing.mp4'; | ||
|
|
||
| describe('Get Accounts', () => { |
There was a problem hiding this comment.
I don't like having several describes in one file. It makes it harder to run just once suite of tests. If the Auth API is specific to the Video Indexer API, please only have one describe. Else, move them to a separate test file.
| }) | ||
| }) | ||
|
|
||
| describe.only('Upload Video', () => { |
miparnisari
changed the title
4 participants
To ensure the
getAccountsmethod of v2 of the video indexer api is working properly, I have written a unit test. As the video indexer has a large API, I wanted to do a small PR to check that my approach is correct before I plough ahead with the rest of the work.