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 user earns while native Windows notifications are suppressed #3651

Closed
jonathansampson opened this issue Mar 11, 2019 · 11 comments · Fixed by brave/brave-core#3084
Closed
Assignees

Comments

@jonathansampson
Copy link
Contributor

jonathansampson commented Mar 11, 2019

Description

When notifications are disabled for the browser, ad notifications still register as having been displayed and the user is able to earn BAT in spite of never having seen a notification or ad.

Steps to Reproduce

  1. Enable Brave Rewards and Ads
  2. Visit Notifications & Settings in Windows Settings
  3. Disable notifications for Brave

image

Actual result:

The browser believes it is showing ad notifications from time to time.

Expected result:

The browser ceases to show notifications, or is aware that they aren't visible.

Reproduces how often:

Easily.

Brave version (brave://version info)

0.63.4

Reproducible on current release:

Ads only enabled in Developer build at the moment.

@jonathansampson
Copy link
Contributor Author

Originally tracked in #2548.

@jsecretan jsecretan added the priority/P2 A bad problem. We might uplift this to the next planned release. label Mar 12, 2019
@tmancey tmancey self-assigned this Mar 19, 2019
@tmancey
Copy link
Contributor

tmancey commented Mar 20, 2019

This is possible on:

iOS/macOS using getNotificationSettingsWithCompletionHandler:, for further information see https://developer.apple.com/documentation/usernotifications/unusernotificationcenter/1649524-getnotificationsettingswithcompl?language=objc

Windows 10 using 'get_Setting:', for further information see https://docs.microsoft.com/en-us/windows/uwp/design/shell/tiles-and-notifications/send-local-toast-desktop-cpp-wrl and https://docs.microsoft.com/en-us/uwp/api/windows.ui.notifications.notificationsetting. Where winui below is equivalent to ABI::Windows::UI

winui::Notifications::IToastNotifier* notifier = notifier_.Get();
winui::Notifications::NotificationSetting setting;
HRESULT hr = notifier->get_Setting(&setting);
if (SUCCEEDED(hr)) {
      switch (setting) {
        case winui::Notifications::NotificationSetting_Enabled:
          break;
        case winui::Notifications::NotificationSetting_DisabledForApplication:
          // Notifications disabled for application
          break;
        case winui::Notifications::NotificationSetting_DisabledForUser:
          // Notifications disabled for user
          break;
        case winui::Notifications::NotificationSetting_DisabledByGroupPolicy:
          // Notifications disabled by group policy
          break;
        case winui::Notifications::NotificationSetting_DisabledByManifest:
          // Notifications disabled by manifest
          break;
      }
}

@tmancey
Copy link
Contributor

tmancey commented Apr 4, 2019

@tmancey tmancey changed the title User Earns While Notifications are Supressed User Earns While Notifications are Suppressed Apr 4, 2019
@anthonypkeane
Copy link

I ran into this issue on Mac too
I saw 2 notifications, put on DND and when I turned off DND, the Ads panel said I had received 8 ads

BTW, on Mac, like Slack do, it might be an idea to keep the Ad notification persistent in the notification area so users can see them again.

@tmancey tmancey changed the title User Earns While Notifications are Suppressed Fix User Earns While Notifications are Suppressed May 8, 2019
@tmancey tmancey changed the title Fix User Earns While Notifications are Suppressed Fix user earns while native Windows notifications are suppressed Aug 5, 2019
@tmancey tmancey added the QA/Yes label Aug 23, 2019
@tmancey tmancey added CI/skip and removed CI/skip labels Nov 4, 2019
@kjozwiak
Copy link
Member

kjozwiak commented Nov 4, 2019

Quick QA note: Please make sure that Catalina is checked as well as per #5541 once this lands. When disabling notifications via Notifications in System Preferences, ads will still be counted as a view even though alerts have been blocked.

@tmancey tmancey changed the title Fix user earns while native Windows notifications are suppressed Fix user earns while native Windows and macOS notifications are suppressed Nov 5, 2019
@tmancey tmancey added this to the 0.74.x - Nightly milestone Nov 6, 2019
@tmancey
Copy link
Contributor

tmancey commented Nov 6, 2019

#6786

@tmancey
Copy link
Contributor

tmancey commented Nov 8, 2019

We have removed the change for macOS which will be fixed as part of another issue

@LaurenWags
Copy link
Member

removed OS/macOS label per discussion with @kjozwiak and #3651 (comment)

@kjozwiak kjozwiak changed the title Fix user earns while native Windows and macOS notifications are suppressed Fix user earns while native Windows notifications are suppressed Jan 16, 2020
@GeetaSarvadnya
Copy link

GeetaSarvadnya commented Jan 24, 2020

Verification passed on

Brave 1.3.100 Chromium: 79.0.3945.130 (Official Build) beta (64-bit)
Revision e22de67c28798d98833a7137c0e22876237fc40a-refs/branch-heads/3945@{#1047}
OS Windows 10 OS Version 1803 (Build 17134.1006)

Clean profile:

  • Ensured ad notifications are NOT shown on windows when Focus Assist is enabled
    [5016:6316:0124/124645.543:INFO:ads_impl.cc(1148)] Notification not made: Notifications not allowed
  • Ensured ad notification is shown when Focus Assist is disabled
    [4428:16072:0124/124905.285:INFO:ads_impl.cc(1305)] Ad notification shown:
  • Ensured ad notifications are NOT shown on Windows when notifications are disabled for the user
    [3532:15496:0124/125224.584:INFO:ads_impl.cc(1148)] Notification not made: Notifications not allowed
  • Ensured ad notification is shown when notifications are enabled for the user
    [7976:11160:0124/125731.063:INFO:ads_impl.cc(1305)] Ad notification shown:
  • Ensured ad notifications are NOT shown on Windows when notifications are disabled for application
    [12112:15084:0124/130451.009:INFO:ads_impl.cc(1148)] Notification not made: Notifications not allowed
  • Ensured ad notifications are shown on Windows when notifications are enabled for application
    [13704:12196:0124/130847.269:INFO:ads_impl.cc(1305)] Ad notification shown:

Upgrade profile:
(Upgrade from 1.2.43 to 1.3.100)

  • Viewed ads in 1.2.43 ads collected in the notification center when focus assist is enabled --> upgrade to 1.3.100 ads are not shown in 1.3.100, Verified notifications not allowed text message in logs
    [5652:4820:0124/145717.418:INFO:ads_impl.cc(1148)] Notification not made: Notifications not allowed
  • Verified ads are shown when focus assist is disabled in an upgraded profile.
    [9440:11284:0124/150629.389:INFO:ads_impl.cc(1305)] Ad notification shown:
  • Verified ads Notification shown logs in the 1.2.43 even though there are no ads notification in UI when notifications are disabled for the user --->upgrade to 1.3.100 ad notifications are not shown in UI also there are no ads shown logs in 1.3.100, Verified notifications not allowed text message in logs
    [14188:10056:0124/152922.093:INFO:ads_impl.cc(1148)] Notification not made: Notifications not allowed
  • Verified ads are shown when notifications are enabled for the user in an upgraded profile.
    [3220:3312:0124/153823.950:INFO:ads_impl.cc(1305)] Ad notification shown:
  • Verified ads are not shown on 1.2.43 when notifications are disabled for the application -->upgrade to 1.3.100 ad notifications are not shown in 1.3.100, Verified notifications not allowed text message in logs
    [3420:4860:0124/155930.214:INFO:ads_impl.cc(1148)] Notification not made: Notifications not allowed
  • Verified ads are shown when notifications are enabled for the application in an upgraded profile.
    [14904:2724:0124/161248.057:INFO:ads_impl.cc(1305)] Ad notification shown:

Verification passed on

Brave 1.3.100 Chromium: 79.0.3945.130 (Official Build) beta (64-bit)
Revision e22de67c28798d98833a7137c0e22876237fc40a-refs/branch-heads/3945@{#1047}
OS Windows 10 OS Version 1909

@GeetaSarvadnya
Copy link

GeetaSarvadnya commented Feb 4, 2020

@tmancey Apart from this check rest other checks are validated from brave/brave-core#3084. Can you explain what is manifest? Confirm native Ad notifications are not shown on Windows if notifications are disabled by manifest.

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

Successfully merging a pull request may close this issue.

9 participants