[ads] Do not fetch geo and catalog on iOS when Brave Rewards/News disabled #24570

aseren · 2024-07-09T19:38:37Z

Resolves brave/brave-browser#39621

Submitter Checklist:

I confirm that no security/privacy review is needed and no other type of reviews are needed, or that I have requested them
There is a ticket for my issue
Used Github auto-closing keywords in the PR description above
Wrote a good PR/commit description
Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
Checked the PR locally:
- npm run test -- brave_browser_tests, npm run test -- brave_unit_tests wiki
- npm run presubmit wiki, npm run gn_check, npm run tslint
Ran git rebase master (if needed)

Reviewer Checklist:

A security review is not needed, or a link to one is included in the PR description
New files have MPL-2.0 license header
Adequate test coverage exists to prevent regressions
Major classes, functions and non-trivial code blocks are well-commented
Changes in component dependencies are properly reflected in gn
Code follows the style guide
Test plan is specified in PR before merging

After-merge Checklist:

The associated issue milestone is set to the smallest version that the
changes has landed on
All relevant documentation has been updated, for instance:

Test Plan:

Test case 1

Fresh install
Restart the Brave Browser to make griffin seed applied
Open Settings -> New Tab Page. Make sure that Background Image Type is Sponsored.
Make sure that Brave Ads started and successfully initialized:
EXPECTATION: There should be a log line: [ads] Successfully initialized ads
Make sure that there is NO network request to https://geo.ads.bravesoftware.com/v1/getstate endpoint
Make sure that there is NO network request to https://static.ads.bravesoftware.com/v9/catalog endpoint
Enable Brave News
Make sure that there is network request to https://geo.ads.bravesoftware.com/v1/getstate endpoint
EXPECTATION: There should be a log lines:
```
[ads] URL Request:
  URL: https://geo.ads.bravesoftware.com/v1/getstate
  Method: kGet
```
Make sure that there is network request to https://static.ads.bravesoftware.com/v9/catalog endpoint
EXPECTATION: There should be a log lines:
```
[ads] URL Request:
  URL: https://static.ads.bravesoftware.com/v9/catalog
  Method: kGet
```

Test case 2

Fresh install
Open Settings -> New Tab Page. Change Background Image Type is Default Images.
Restart the Brave Browser to make griffin seed applied
Make sure that Brave Ads started and successfully initialized:
EXPECTATION: There should be a log line: [ads] Successfully initialized ads
Make sure that there is NO network request to https://geo.ads.bravesoftware.com/v1/getstate endpoint
Make sure that there is NO network request to https://static.ads.bravesoftware.com/v9/catalog endpoint
Enable Brave News
Make sure that there is network request to https://geo.ads.bravesoftware.com/v1/getstate endpoint
EXPECTATION: There should be a log lines:
```
[ads] URL Request:
  URL: https://geo.ads.bravesoftware.com/v1/getstate
  Method: kGet
```
Make sure that there is network request to https://static.ads.bravesoftware.com/v9/catalog endpoint
EXPECTATION: There should be a log lines:
```
[ads] URL Request:
  URL: https://static.ads.bravesoftware.com/v9/catalog
  Method: kGet
```

ios/browser/api/ads/brave_ads.mm

fallaciousreasoning

brave_news changes LGTM

components/brave_news/browser/locales_helper.h

github-actions · 2024-07-10T14:32:05Z

[puLL-Merge] - brave/brave-core@24570

Here's my review of the pull request:

Description

This PR refactors the Brave News preferences and P3A (Privacy Preserving Product Analytics) related code. It moves some functionality from the browser directory to the common directory, separates P3A-specific preferences, and updates various imports and function calls across the codebase to reflect these changes.

Changes

Changes

components/brave_news/common/:
- Moved locales_helper.cc and locales_helper.h from browser/ to common/
- Added new files p3a_pref_names.cc and p3a_pref_names.h
- Updated pref_names.cc and pref_names.h
components/brave_news/browser/:
- Removed locales_helper.cc and locales_helper.h
- Updated various files to use the new common headers
browser/brave_profile_prefs.cc:
- Updated function calls to use new namespaces and functions
browser/ui/day_zero_browser_ui_expt/:
- Updated imports to use new common headers
chromium_src/chrome/browser/prefs/browser_prefs.cc:
- Updated function calls to use new namespaces
chromium_src/ios/chrome/browser/shared/model/prefs/:
- Added dependency on brave_news/common
- Updated browser_prefs.mm to use new functions
ios/brave-ios/Sources/Brave/Frontend/Rewards/BraveRewards.swift:
- Updated to implement PreferencesObserver
- Added method to notify ads service about Brave News preference changes
ios/browser/api/ads/brave_ads.h and brave_ads.mm:
- Added new method notifyBraveNewsIsEnabledPreferenceDidChange:
- Updated preference handling

Possible Issues

The PR doesn't include any updates to tests for the refactored code, which might be necessary to ensure the changes don't introduce any regressions.

Security Hotspots

No significant security issues were identified in this change.

kylehickinson

ios++

brave-builds · 2024-07-10T15:29:00Z

A Storybook has been deployed to preview UI for the latest push

…abled

brave-builds · 2024-07-10T16:55:32Z

A Storybook has been deployed to preview UI for the latest push

mkarolin

chromium_src++

[ads] Do not fetch geo and catalog on iOS when Brave Rewards/News disabled

btlechowski · 2024-07-16T01:05:06Z

Verified with 1.69.107 on iPhone 13 Pro Max

Testcase 1

Brave Ads started and successfully initialized

info	02:52:49.754250+0200	Client	[ads] Successfully initialized ads

there is NO network request to https://geo.ads.bravesoftware.com/v1/getstate endpoint
there is NO network request to https://static.ads.bravesoftware.com/v9/catalog endpoint

there is network request to https://geo.ads.bravesoftware.com/v1/getstate endpoint after enabling Brave News

info	02:48:44.719172+0200	Client	[ads] URL Response:
  URL: https://geo.ads.brave.com/v1/getstate
  Response Status Code: 200

there is network request to https://static.ads.bravesoftware.com/v9/catalog endpoint after enabling Brave News

info	02:48:44.596693+0200	Client	[ads] URL Request:
  URL: https://static.ads.brave.com/v9/catalog
  Method: kGet

Testcase 2

Brave Ads started and successfully initialized

info	02:52:49.754250+0200	Client	[ads] Successfully initialized ads

there is NO network request to https://geo.ads.bravesoftware.com/v1/getstate endpoint
there is NO network request to https://static.ads.bravesoftware.com/v9/catalog endpoint

there is network request to https://geo.ads.bravesoftware.com/v1/getstate endpoint after enabling Brave News

info	02:55:51.327893+0200	Client	[ads] URL Request:
  URL: https://geo.ads.brave.com/v1/getstate
  Method: kGet

there is network request to https://static.ads.bravesoftware.com/v9/catalog endpoint after enabling Brave News

info	02:55:51.327385+0200	Client	[ads] URL Request:
  URL: https://static.ads.brave.com/v9/catalog
  Method: kGet

…abled (uplift to 1.68.x) (#24616) Merge pull request #24570 from brave/issues/39621 [ads] Do not fetch geo and catalog on iOS when Brave Rewards/News disabled

aseren requested review from a team as code owners July 9, 2024 19:38

github-actions bot assigned aseren Jul 9, 2024

aseren requested review from DJAndries, fallaciousreasoning and tmancey July 9, 2024 19:38

github-actions bot added the puLL-Merge label Jul 9, 2024

aseren force-pushed the issues/39621 branch from 766e8e8 to 4d70414 Compare July 9, 2024 21:14

diracdeltas reviewed Jul 9, 2024

View reviewed changes

ios/browser/api/ads/brave_ads.mm Show resolved Hide resolved

fallaciousreasoning approved these changes Jul 9, 2024

View reviewed changes

components/brave_news/browser/locales_helper.h Outdated Show resolved Hide resolved

aseren force-pushed the issues/39621 branch from 4d70414 to ef79950 Compare July 10, 2024 14:30

aseren requested a review from diracdeltas July 10, 2024 14:30

kylehickinson approved these changes Jul 10, 2024

View reviewed changes

[ads] Do not fetch geo and catalog on iOS when Brave Rewards/News dis…

a6e7caa

…abled

aseren force-pushed the issues/39621 branch from ef79950 to a6e7caa Compare July 10, 2024 16:04

mkarolin approved these changes Jul 10, 2024

View reviewed changes

DJAndries approved these changes Jul 10, 2024

View reviewed changes

diracdeltas approved these changes Jul 10, 2024

View reviewed changes

aseren merged commit 6bb2cf7 into master Jul 10, 2024
17 checks passed

aseren deleted the issues/39621 branch July 10, 2024 22:32

github-actions bot added this to the 1.69.x - Nightly milestone Jul 10, 2024

aseren mentioned this pull request Jul 11, 2024

[ads] Do not fetch geo and catalog on iOS when Brave Rewards/News disabled (uplift to 1.68.x) #24616

Merged

7 tasks

aseren added a commit that referenced this pull request Jul 12, 2024

Merge pull request #24570 from brave/issues/39621

f39033a

[ads] Do not fetch geo and catalog on iOS when Brave Rewards/News disabled

btlechowski mentioned this pull request Jul 25, 2024

[ads] Should not fetch geo and catalog on iOS when Brave Rewards/News disabled brave/brave-browser#39621

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ads] Do not fetch geo and catalog on iOS when Brave Rewards/News disabled #24570

[ads] Do not fetch geo and catalog on iOS when Brave Rewards/News disabled #24570

aseren commented Jul 9, 2024 •

edited

Loading

fallaciousreasoning left a comment

github-actions bot commented Jul 10, 2024

Changes

kylehickinson left a comment

brave-builds commented Jul 10, 2024

brave-builds commented Jul 10, 2024

mkarolin left a comment

btlechowski commented Jul 16, 2024

[ads] Do not fetch geo and catalog on iOS when Brave Rewards/News disabled #24570

[ads] Do not fetch geo and catalog on iOS when Brave Rewards/News disabled #24570

Conversation

aseren commented Jul 9, 2024 • edited Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

Test case 1

Test case 2

fallaciousreasoning left a comment

Choose a reason for hiding this comment

github-actions bot commented Jul 10, 2024

Description

Changes

Possible Issues

Security Hotspots

kylehickinson left a comment

Choose a reason for hiding this comment

brave-builds commented Jul 10, 2024

brave-builds commented Jul 10, 2024

mkarolin left a comment

Choose a reason for hiding this comment

btlechowski commented Jul 16, 2024

Testcase 1

Testcase 2

aseren commented Jul 9, 2024 •

edited

Loading