Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Youtube] Added channel view count and subscriber count badges #6333

Merged
merged 15 commits into from
Apr 5, 2021

Conversation

tarun7singh
Copy link
Contributor

Added Channel view count and subscriber count badge.

  • Moved Youtube base to youtube video base.
  • Added Youtube channel subscriber badge
  • Added Youtube channel total view count badge
  • Added tests for both badges

Fixes #6111

@shields-ci
Copy link

shields-ci commented Mar 29, 2021

Warnings
⚠️

📚 Remember to ensure any changes to config.private in services/youtube/youtube-base.js are reflected in the server secrets documentation

Messages
📖 ✨ Thanks for your contribution to Shields, @tarun7singh!

Generated by 🚫 dangerJS against b05a458

@tarun7singh tarun7singh mentioned this pull request Mar 31, 2021
3 tasks
@calebcartwright calebcartwright added the service-badge Accepted and actionable changes, features, and bugs label Apr 1, 2021
@shields-cd shields-cd temporarily deployed to shields-staging-pr-6333 April 1, 2021 23:52 Inactive
@shields-cd shields-cd temporarily deployed to shields-staging-pr-6333 April 2, 2021 04:56 Inactive
@tarun7singh
Copy link
Contributor Author

I had added another issue's code in this PR by mistake. Reverted it with a revert commit.

@PyvesB PyvesB temporarily deployed to shields-staging-pr-6333 April 2, 2021 09:47 Inactive
Copy link
Member

@PyvesB PyvesB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution @tarun7singh ! 👍🏻

The timing of this pull request is a bit unfortunate. I've just completed quite a tedious compliance process with the YouTube team (see #6276 for a bit more details). One of their conclusive notes is the following:

Please note that the quota extension has been granted ONLY for the use-case(s) mentioned in your quota extension request application and we may change your quota based on your actual quota usage as well as on your continued compliance with the YouTube API Services Terms of Service at any time.

Obviously I hadn't mentioned the new channel badges as they didn't yet exist at the time, but equally one could argue that it's an extension of the same use case. I suggest we move ahead with this, but one my review comments will be particularly important to address to make sure we don't break compliance.

services/youtube/youtube-channel-base.js Outdated Show resolved Hide resolved
services/youtube/youtube-channel-base.js Outdated Show resolved Hide resolved
services/youtube/youtube-subscribers.service.js Outdated Show resolved Hide resolved
services/youtube/youtube-subscribers.service.js Outdated Show resolved Hide resolved
@shields-cd shields-cd temporarily deployed to shields-staging-pr-6333 April 2, 2021 18:28 Inactive
@shields-cd shields-cd temporarily deployed to shields-staging-pr-6333 April 2, 2021 20:59 Inactive
@shields-cd shields-cd temporarily deployed to shields-staging-pr-6333 April 2, 2021 21:05 Inactive
@tarun7singh
Copy link
Contributor Author

@PyvesB Completed all the requested changes.

@shields-cd shields-cd temporarily deployed to shields-staging-pr-6333 April 3, 2021 04:46 Inactive
@PyvesB
Copy link
Member

PyvesB commented Apr 3, 2021

@tarun7singh thanks for promptly addressing my review comments. Things mostly look good, though I think YouTubeBase can further be improved. Here's what I came up with after a bit of experimentation: 55ecd94

Note that https://www.youtube.com/video/SOME-ID redirects to https://www.youtube.com/watch?v=SOME-ID, which I've leveraged for consistency with https://www.youtube.com/channel/SOME-ID.

Let me know what you think and feel free to cherry-pick that commit back onto your PR branch. Otherwise, happy to approve as it is, I don't want to be too picky, so it's your call. 😉

@tarun7singh
Copy link
Contributor Author

@PyvesB Yes your code looks much mor econcise but I was not able to successfully cherry pick this commit. can you please do it or tell me how to do it?

@PyvesB
Copy link
Member

PyvesB commented Apr 5, 2021

Try the following:

git remote add fork-pyvesb https://github.com/PyvesB/shields
git fetch fork-pyvesb
git cherry-pick 55ecd9416dce02da4b5f83fe2824c48c124d65bc

Alternatively, give me write permissions to your fork so that I can push the commit.

@shields-cd shields-cd temporarily deployed to shields-staging-pr-6333 April 5, 2021 08:04 Inactive
@tarun7singh
Copy link
Contributor Author

tarun7singh commented Apr 5, 2021

@PyvesB Added your commit to the PR.

Copy link
Member

@PyvesB PyvesB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Looks all good to me, let's get this merged! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
service-badge Accepted and actionable changes, features, and bugs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Youtube] subscriber counter/ total view time
5 participants