[YouTube] Drop support for removed dislikes #7410
Conversation
pzygielo
commented
Dec 22, 2021
•
pzygielo
commented
- fixes YouTube video view count: "invalid response data" #7373
pzygielo
changed the title
services/youtube/youtube-base.js
Outdated
| viewCount: nonNegativeInteger, | ||
| likeCount: nonNegativeInteger, | ||
| commentCount: nonNegativeInteger, | ||
| favoriteCount: nonNegativeInteger, |
There was a problem hiding this comment.
As far as I know we don't actually need the favoriteCount value for any of our badges, so instead of adding what is essentially a duplicate schema for statistics, let's instead make the dislikeCount field optional by changing from nonNegativeInteger to optionalNonNegativeInteger (which can be imported from the same validators module)
| const likes = `${metric(statistics.likeCount)} 👍` | ||
| const dislikes = | ||
| statistics.dislikeCount !== undefined | ||
| ? `${metric(statistics.dislikeCount)} 👎` | ||
| : '' | ||
| renderedBadge = { | ||
| ...renderedBadge, | ||
| message: `${metric(statistics.likeCount)} 👍 ${metric( | ||
| statistics.dislikeCount | ||
| )} 👎`, | ||
| message: `${likes} ${dislikes}`, |
There was a problem hiding this comment.
We already have the withDislikes query parameter that allows the user to control whether or not the dislikes are included on their badge. If we hit this block then the user has explicitly asked for the dislike portion to be included as well, and as such we should instead display 0 👎.
Perhaps ${metric(statistics.dislikeCount || 0)} 👎 ?
There was a problem hiding this comment.
But 0 👎 looks like there is no 👎, while in fact this is unknown. I was choosing between not presenting this piece or using ? 👎. I'm against using 0 for the case.
There was a problem hiding this comment.
while in fact this is unknown.
What is your evidence for this? Do you have some documentation to the API or an example target that includes a "dislikeCount": 0 in the response?
There was a problem hiding this comment.
Youtube has dropped the dislike count from public view altogether, and that's not something we'll ever be able to see. I think we should actually remove dislikes across the board
There was a problem hiding this comment.
I'd rather like this PR to only restore YT stats.
I think total dislikes removal could be the subject for the next one.
There was a problem hiding this comment.
We have a feature that was based on an attribute in the API response that no longer exists, which in turn has broken badges. I don't understand what you're suggesting be split, could you elaborate?
We need a resolution to the actual issue, i.e. our badge needs to account for the upstream change. Splitting that into multiple PRs with ~5 line diffs is unnecessary overhead for us
There was a problem hiding this comment.
Splitting that into multiple PRs with ~5 line diffs is unnecessary overhead for us
Understood.
Removed code IMO related to the dislikes.
Locally tested that the old links, with (now unused) parameter withDislikes render the no-parameterized version fine.
There was a problem hiding this comment.
Awesome thanks so much! Will give this a final pass here shortly
| /^([1-9][0-9]*[kMGTPEZY]?|[1-9]\.[1-9][kMGTPEZY]) 👍 ([1-9][0-9]*[kMGTPEZY]?|[1-9]\.[1-9][kMGTPEZY]) 👎$/ | ||
| /^([1-9][0-9]*[kMGTPEZY]?|[1-9]\.[1-9][kMGTPEZY]) 👍$/ |
There was a problem hiding this comment.
Thanks for looking at the tests but as noted above we need to change the behavior a bit.
Additionally, we should instead add a new test for the scenario described in #7373. Given the specific use case of a missing dislikeCount field, it would probably be better for that to be a mocked test (https://github.com/badges/shields/blob/master/doc/service-tests.md#5-mocking-responses) given the likelihood for the upstream target to change/receive dislikes
pzygielo
changed the title
