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] Set EU consent cookie #600

Merged
merged 9 commits into from
Apr 9, 2021
Merged

[YouTube] Set EU consent cookie #600

merged 9 commits into from
Apr 9, 2021

Conversation

TobiGr
Copy link
Contributor

@TobiGr TobiGr commented Apr 3, 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.

Addresses #592

@TobiGr TobiGr added the youtube service, https://www.youtube.com/ label Apr 3, 2021
@opusforlife2 opusforlife2 changed the title [Youtube] Set EU cosent cookie [Youtube] Set EU consent cookie Apr 3, 2021
@AudricV AudricV added ASAP Issue needs to be fixed as soon as possible bug Issue is related to a bug labels Apr 3, 2021
@AudricV AudricV changed the title [Youtube] Set EU consent cookie [YouTube] Set EU consent cookie Apr 3, 2021
@TobiGr TobiGr force-pushed the youtube-eu-cosent branch 3 times, most recently from 9f48a7b to 84c6e7b Compare April 4, 2021 10:30
@TobiGr TobiGr force-pushed the youtube-eu-cosent branch 2 times, most recently from 8a6d015 to 7c3c3b9 Compare April 7, 2021 12:10
@TobiGr TobiGr force-pushed the youtube-eu-cosent branch 2 times, most recently from 27e460c to 2a046c0 Compare April 7, 2021 19:55
@TobiGr TobiGr changed the base branch from dev to master April 7, 2021 20:13
Redirion
Redirion previously approved these changes Apr 8, 2021
Copy link
Member

@Redirion Redirion left a comment

Choose a reason for hiding this comment

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

lgtm

@TobiGr
Copy link
Contributor Author

TobiGr commented Apr 8, 2021

I just pushed the same commit again to trigger the CI. GitHub Actions did not run all tests again after changing the target branch to master

@TobiGr TobiGr force-pushed the youtube-eu-cosent branch from 7355c48 to 682ec27 Compare April 8, 2021 12:22
@TobiGr
Copy link
Contributor Author

TobiGr commented Apr 8, 2021

Ah. what a bummer. The randomized consent cookie makes the mocks useless, because the recorded requests are not equal to the current anymore.
@XiangRongLin Is there a way to change this? I guess, we need to change the equals and hashCode method of Request. I do not have time for this anymore today.

@XiangRongLin
Copy link
Collaborator

@TobiGr Instead of directly declaring a random number generator, you can inject one from outside. This way you inject a deterministic one in tests. I'll take a look at it

This is something that can generally be done when trying to avoid randomness in tests. Thats also why Dependency Injection (DI) is so important

@XiangRongLin
Copy link
Collaborator

@TobiGr done

org.schabi.newpipe.extractor.services.youtube.stream.YoutubeStreamExtractorDefaultTest.StreamSegmentsTestOstCollection
I can't generate mocks, because the video is blocked in my country

org.schabi.newpipe.extractor.services.youtube.search.YoutubeSearchExtractorTest.Videos
Has test failures in the continuation tests, no idea why and didn't look into it.

@TobiGr
Copy link
Contributor Author

TobiGr commented Apr 9, 2021

regarding the two tests: that's why I wanted to set up a weekly mock update. I cannot make the tests complete successfully either. I'll merge this and release 0.21.1 now. We cannot wait any longer

Thank you for fixing the mock tests! This solution looks good to me.

Copy link
Member

@Redirion Redirion left a comment

Choose a reason for hiding this comment

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

CONENT_COOKIE constants should be private as only the method generateConsentCookie should be used

@TobiGr TobiGr force-pushed the youtube-eu-cosent branch from 3469b80 to e437d50 Compare April 9, 2021 09:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ASAP Issue needs to be fixed as soon as possible bug Issue is related to a bug youtube service, https://www.youtube.com/
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants