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 support for privacy config v4 #3664

Merged
merged 1 commit into from
Oct 16, 2023

Conversation

marcosholgado
Copy link
Contributor

@marcosholgado marcosholgado commented Oct 13, 2023

Task/Issue URL: https://app.asana.com/0/1202552961248957/1205690187399816/f

Description

Add support for remote config v4

Steps to test this PR

Update

  • Install from develop
  • Check the DB for the different features (privacy_config.db, autoconsent, autofill, cookies, etc). Go to the different exceptions tables and seen that the column for reasons has content
  • Update from this branch
  • The app should not crash
  • Check the DBs and the reason column should now be null but domains should still be there.

New install

  • Install from this branch
  • The app should not crash
  • Check the DBs and the reason column should now be null but domains should still be there.

Tests

@marcosholgado
Copy link
Contributor Author

Current dependencies on/for this PR:

This comment was auto-generated by Graphite.

@marcosholgado marcosholgado force-pushed the feature/marcos/privacy_config_v4 branch 6 times, most recently from eb1d5af to 2c91eee Compare October 13, 2023 12:18
Copy link
Collaborator

@aitorvs aitorvs left a comment

Choose a reason for hiding this comment

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

lgtm. Tested and also worked as expected.

data class AdClickAttributionFeature(
val state: String,
val minSupportedVersion: Int?,
val settings: AdClickAttributionSettings,
val exceptions: List<AdClickAttributionException>,
val exceptions: List<FeatureException>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: to be clear, this is fine.
It feels odd for now because all these features (ad-click) etc related to the privacy-config module but take the model from the feature-toggles module.
No need to change, just a comment

@@ -31,6 +31,7 @@ kotlin {
}

dependencies {
implementation project(path: ':feature-toggles-api')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, that's what I was saying before. Maybe we want a PrivacyConfigException instead of using the FeatureException.
No strong opinions tbh. I think long term we want to migrate everything to use the feature-toggles module

@marcosholgado marcosholgado force-pushed the feature/marcos/privacy_config_v4 branch from 2c91eee to 626ced6 Compare October 13, 2023 15:21
@marcosholgado marcosholgado merged commit a93ae3a into develop Oct 16, 2023
@marcosholgado marcosholgado deleted the feature/marcos/privacy_config_v4 branch October 16, 2023 08:47
joshliebe pushed a commit that referenced this pull request Nov 7, 2023
Task/Issue URL:
https://app.asana.com/0/1202552961248957/1205690187399816/f

### Description
Add support for remote config v4

### Steps to test this PR

_Update_
- [ ] Install from develop
- [ ] Check the DB for the different features (privacy_config.db,
autoconsent, autofill, cookies, etc). Go to the different exceptions
tables and seen that the column for reasons has content
- [ ] Update from this branch
- [ ] The app should not crash
- [ ] Check the DBs and the reason column should now be null but domains
should still be there.

_New install_
- [ ] Install from this branch
- [ ] The app should not crash
- [ ] Check the DBs and the reason column should now be null but domains
should still be there.

_Tests_
- [ ] Privacy Tests should pass, see
https://github.com/duckduckgo/Android/actions/runs/6510081560
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.

2 participants