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

Add AccountTerminatedException for terminated channels #580

Merged
merged 5 commits into from
Jun 8, 2021

Conversation

TobiGr
Copy link
Contributor

@TobiGr TobiGr commented Mar 22, 2021

  • I carefully read the contribution guidelines and agree to them.
  • I have tested the API against NewPipe.
  • I agree to create a pull request for NewPipe as soon as possible to make it compatible with the changed API.

Adds a new ContentNotAvailableException: AccountTerminatedException to distinguish between terminated channels. The field reason provides more info on why the account has been terminated. This fixes a NPE when getting the feed for a terminated channel in YouTubeFeedExtractor.

  • add CLOSED_BY_OWNER reason (I need a dedicated channel for this
  • implement in other services
  • add mocks for tests

@TobiGr TobiGr added bug Issue is related to a bug enhancement New feature or request youtube service, https://www.youtube.com/ labels Mar 22, 2021
@TobiGr TobiGr force-pushed the accountTerminated branch 3 times, most recently from 3abbdea to f1a1591 Compare April 2, 2021 19:36
@TobiGr TobiGr marked this pull request as ready for review April 2, 2021 19:48
@TobiGr
Copy link
Contributor Author

TobiGr commented Apr 2, 2021

I could not find more info on deleted and terminated channels in the docs of the other services.

Comment on lines 72 to 73
assertEquals(e.getMessage(),
"This account has been terminated for a violation of YouTube's Terms of Service.");
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would remove the assertions for the error message. They are bound to fail very soon, if they change the wording. Only usecase i can think of for them, is directly displaying them to the end-user in which case, the exact wording does not change the correctness of the implementation.

Comment on lines +692 to +764
if (alertText != null && alertText.contains("This account has been terminated")) {
if (alertText.contains("violation") || alertText.contains("violating")
|| alertText.contains("infringement")) {
Copy link
Collaborator

@XiangRongLin XiangRongLin Apr 3, 2021

Choose a reason for hiding this comment

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

what happens if the localization is changed to non-english. I would assume the error messages are localized

Copy link
Member

Choose a reason for hiding this comment

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

@XiangRongLin Yes, they are, it's the same thing for the error messages from YouTube videos.
I think in the future we need to implement something which stores strings of the error messages for each localization and compare with the value provided by YouTube in the language used for the extraction of YouTube datas.

@TobiGr TobiGr force-pushed the accountTerminated branch from f1a1591 to 91297ca Compare May 1, 2021 15:50
Copy link
Member

@Stypox Stypox left a comment

Choose a reason for hiding this comment

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

LGTM

@Stypox
Copy link
Member

Stypox commented Jun 6, 2021

Oh wait, this has to be addressed: I would remove the assertions for the error message. They are bound to fail very soon, if they change the wording. Only usecase i can think of for them, is directly displaying them to the end-user in which case, the exact wording does not change the correctness of the implementation.

@TobiGr TobiGr force-pushed the accountTerminated branch from 91297ca to fa444c8 Compare June 6, 2021 09:39
@TobiGr TobiGr merged commit d4186d1 into dev Jun 8, 2021
@TobiGr TobiGr deleted the accountTerminated branch June 8, 2021 07:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issue is related to a bug enhancement New feature or request youtube service, https://www.youtube.com/
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants