Initial work for integration with YouTube API #94
Conversation
|
Apologies for not really having a clear definition of done for this feature. Need to start getting better at writing stories. Is this going to be like the Twitter feature where we post new videos in a Slack channel? |
|
Yeah, I have print statements where it'll go back to slack to say "new video posted" and tested that the logic up to there works correctly. I just don't know if you want all that logic in the "view" or separated out into a different file.
…On Sun, Mar 17, 2019, at 10:27 AM, Aly Sivji wrote:
Apologies for not really having a clear definition of done for this feature. Need to start getting better at writing stories.
Is this going to be like the Twitter feature where we post new videos in a Slack channel?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub <https://github.com/alysivji/busy-beaver/pull/94#issuecomment-473681622>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEHPsne7GBL0mCwqU0uI24Faa-o8t4Mqks5vXmz1gaJpZM4b4eTA>.
|
| db.session.add(youtube_video) | ||
| db.session.commit() | ||
| return 'post to slack' | ||
| return make_response(200, json={"run": "complete"}) |
There was a problem hiding this comment.
Would this only post the latest video?
Our video person posts in batches so we could have up to 10 videos at once.
There was a problem hiding this comment.
Currently yes, only the latest would be shared. I will update this so that it posts all new videos. If busy-beaver has an empty YoutubeVideo table do you think a good default would be just post the latest video?
There was a problem hiding this comment.
Yah. That definitely makes sense to me.
I don't like the idea of checking for an empty table every time but cannot offer any meaningful alternatives. Let's proceed this way for now, but start thinking about a better of dealing with this edge case.
We had the same issue for the Twitter key-value store. I manually did the work, which I didn't like, but it was a one-time thing. Now that we are thinking about installing this app in other workspaces, we should think about a better way forward.
Maybe we can populate the database once on application start? But that would require all contributors to have all keys to guarantee things work.
A separate script?
|
I think a separate script would be a good idea. We can have it check
which environment variables are set and then populate the appropriate
tables with data.
|
|
@alysivji I've update this pull request. I broke the logic up a little bit, added the posting of multiple videos as well as posting to slack. I was using httpie for testing and tested with the following command. I tested both with no videos in the database as well as with multiple new videos and they posted correctly to slack so I think that this is working as intended. I will look at adding some tests if you think the code looks aight. |
|
This is looking good! Thanks for getting it updated. Taking a look at the I'm currently adding a task queue to the project; right now I'm testing it in an example project. Should be completed in the next couple of days; might have you refactor to kick off a background job when making a request to the |
|
Also, loving the video selection! I totally forgot about Big Wreck. I have their first 2 CDs from back in the day. |
thornycrackers
force-pushed
the
youtube_api
branch
from
872aa70 to
dda9355
Compare
Codecov Report
@@ Coverage Diff @@
## master #94 +/- ##
=========================================
- Coverage 93.36% 90.57% -2.8%
=========================================
Files 41 43 +2
Lines 919 997 +78
=========================================
+ Hits 858 903 +45
- Misses 61 94 +33
Continue to review full report at Codecov.
|
|
@alysivji I added an environment variable to sleep in between posts! |
|
This is awesome! Thanks so much @thornycrackers! I'm gonna merge this in the next couple of days and work with our YouTube person to ensure we have proper video names so I can enable this feature for the ChiPy workspace. To enable, we'll need a shell script that hits the polling endpoint and then we'll have to set up a CRON job in Ansible. Created a new task, #146. |
alysivji
changed the title
This is not ready to be merged in yet, but I think we can start the review process now that there's enough work. I'm not sure where some of the logic is supposed to sit so I figured it would just be easier to get another set of eyes on it so as I finish this up I can move logic to the appropriate places