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

fix advanced_settings_purposes_default with custom purposes (#241) #242

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

atoiv
Copy link
Contributor

@atoiv atoiv commented Mar 28, 2019

Hi,

This pr should fix #241. It makes config option advanced_settings_purposes_default=true to also work with custom purposes.

This appeared to be a rather straightforward fix, and as far as I looked, it didn't affect anything else than the exact functionality with the advanced_settings_purposes_default option, when the option is enabled.

Tests were green after the change.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 863

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 26.346%

Totals Coverage Status
Change from base Build 862: 0.0%
Covered Lines: 1101
Relevant Lines: 3386

💛 - Coveralls

@JeppeSigaard
Copy link

@atoiv This is great! can you solve the merge conflicts?
@Waschnick what would be required to get this into a release?

@atoiv
Copy link
Contributor Author

atoiv commented Aug 12, 2019

Sorry it took a while to check back on this. It seems I've missed all the notifications for the PR.

The merge conflicts are now solved.

@atoiv
Copy link
Contributor Author

atoiv commented Oct 22, 2019

It seems reading custom purposes from a cookie is also broken. I have a local fix for that, but I'll look into adding it here as well, after I get time to do it.

Edit: Yikes, pushed send too fast!

Meant to ask if I should add the code to that fix here as well, or create a new PR? The changes are close to the ones made for this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

advanced_settings_purposes_default doesn't work with custom purposes
3 participants