Skip to content
This repository has been archived by the owner on Mar 9, 2022. It is now read-only.

Webhook endpoint #83

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from
Open

Webhook endpoint #83

wants to merge 7 commits into from

Conversation

nmigueles
Copy link
Contributor

@nmigueles nmigueles commented Sep 7, 2019

POST /admin/fetch-videos

Meant to be used as endpoint to fetch the videos from YouTube.

@ewenjo
Copy link
Contributor

ewenjo commented Sep 7, 2019

Since the endpoint returns "OK" no matter what, I tried refactoring it a little. This is what I came up with:
https://gist.github.com/ewenjo/5e78eef04edafaab84a0f5a92c5e80e1

NB: Only a suggestion, not finished and it's not fully tested.

@nmigueles
Copy link
Contributor Author

Since the endpoint returns "OK" no matter what, I tried refactoring it a little. This is what I came up with:
https://gist.github.com/ewenjo/5e78eef04edafaab84a0f5a92c5e80e1

NB: Only a suggestion, not finished and it's not fully tested.

Thats amazing! liked alot the custom responses. I gonna test and clean up here and there but great job. 🙌

@nmigueles
Copy link
Contributor Author

I forced the commit with --no-verify flag bc i don't know how to proper test this endpoint.

Jest: "global" coverage threshold for branches (80%) not met: 75.93%

Also I added the @ewenjo suggestions with some minor adjustments to the credit goes to him.

@nmigueles nmigueles changed the title Added endpoint to manually run the cron-job Webhook endpoint Sep 8, 2019
@spiray
Copy link
Contributor

spiray commented Sep 8, 2019

@nicomigueles I will do a full review tomorrow but one immediate issue that I see with using that fetchVideosJob function for the fetch-videos route and the cron job is the return structure vs log structure. If you return when the YouTube API responds with no videos wont that exit the job? Even if not we would still want logs...?

Regarding the testing, You can use the other admin test as an example and see if that gets your coverage up. Let me know if you need help.

@nmigueles
Copy link
Contributor Author

nmigueles commented Sep 8, 2019

@nicomigueles I will do a full review tomorrow but one immediate issue that I see with using that fetchVideosJob function for the fetch-videos route and the cron job is the return structure vs log structure. If you return when the YouTube API responds with no videos wont that exit the job? Even if not we would still want logs...?

It would log the first "error"/"message" that encounters.
You suggest pushing the errors and messages to error and message arrays respectively and then return those objects?

I don't think that the return will kill the cronjob but it wont matter becouse we don't gonna use cron-jobs anymore.

Regarding the testing, You can use the other admin test as an example and see if that gets your coverage up. Let me know if you need help.

The problem with the other admin test ( seed ) is that it would return 200, OK no matter what.
EDIT: Nevermind, It worked. Thanks!

@nmigueles nmigueles requested a review from w3cj September 10, 2019 16:39
@nmigueles
Copy link
Contributor Author

I assume that the CI is falling because of old or invalid YOUTUBE-API-KEY.
Please someone of DevOps check this out!

@nmigueles nmigueles added the help wanted Extra attention is needed label Feb 24, 2020
@snordmann
Copy link
Contributor

I don't think that I can fix that. I can't generate a new YouTube API key and put it into Travis.

If I am not mistaken only @w3cj can do that.

@nmigueles
Copy link
Contributor Author

nmigueles commented Feb 24, 2020

@sbibow yeah, if you put a new key into Travis it would work. We have to wait the @w3cj powers then.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants