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

Android 13 show Warning modal condition update #16128

Merged
merged 4 commits into from
Dec 10, 2022

Conversation

sujitacharya2005
Copy link
Contributor

@sujitacharya2005 sujitacharya2005 commented Nov 29, 2022

Resolves brave/brave-browser#27032
Resolves brave/brave-browser#27004
Resolves brave/brave-browser#27108

Submitter Checklist:

  • I confirm that no security/privacy review is needed, or that I have requested one
  • 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, npm run lint, 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:

Test Plan:

1. Application re-launch 3rd time
2. From Brave setting -> notification click each time

  1. If main notification is off, Rewards feature is off, privacy feature is off -> Show Generic Dialog
  2. if main notification is off, Rewards feature is on, privacy feature is off -> Show rewards message dialog
  3. if main notification if off, Rewards feature is off, privacy feature is on -> show Privacy message dialog
  4. if main notification is off, Rewards feature is on, Privacy feature is on -> Show both rewards & privacy message dialog

image

OnBoarding Second Header
https://user-images.githubusercontent.com/32419898/206022774-cb715337-ab99-4809-a751-d63588666f8f.mp4 https://user-images.githubusercontent.com/32419898/206021292-7df5f682-798a-4108-8df1-6291a5b58b07.mp4
Light Mode Dark Mode
Screenshot_20221207_011150 Screenshot_20221207_011319

@sujitacharya2005 sujitacharya2005 added CI/skip-linux CI/skip-macos-x64 Do not run CI builds for macOS x64 CI/skip-ios Do not run CI builds for iOS labels Nov 29, 2022
@sujitacharya2005 sujitacharya2005 added this to the 1.46.x - Release milestone Nov 29, 2022
@sujitacharya2005 sujitacharya2005 force-pushed the 27032_android_13_showWarningDialog_condition branch 2 times, most recently from 93eeff4 to b5574ae Compare December 2, 2022 13:56
@sujitacharya2005 sujitacharya2005 force-pushed the 27032_android_13_showWarningDialog_condition branch 4 times, most recently from 214d312 to b6c5161 Compare December 6, 2022 20:54
@github-actions
Copy link
Contributor

github-actions bot commented Dec 6, 2022

⚠️ PR head is an unsigned commit
commit: b6c5161cdeb9610113e8c5c177cfa9b57ed51962
reason: unsigned
Please follow the handbook to configure commit signing
cc: @sujitacharya2005

@sujitacharya2005 sujitacharya2005 force-pushed the 27032_android_13_showWarningDialog_condition branch from b6c5161 to 1a27a1a Compare December 7, 2022 06:01
private void showNotificationWarningDialog() {
BraveNotificationWarningDialog notificationWarningDialog =
BraveNotificationWarningDialog.newInstance(
BraveNotificationWarningDialog.FROM_LAUNCHED_BRAVE_ACTIVITY);
notificationWarningDialog.setCancelable(false);
Copy link
Contributor

Choose a reason for hiding this comment

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

what was the reason to remove it ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed this line, because back key was not working.
If we want to force user to click on close( X mark) then we can have this line.
I feel user this dialog can close if click outside / press on back key.

Copy link
Contributor

Choose a reason for hiding this comment

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

my suggestion would be to disable dismiss on outside click. as we are showing close button on the UI

Copy link
Contributor Author

Choose a reason for hiding this comment

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

reverted this.
Added back that line
notificationWarningDialog.setCancelable(false);

@@ -117,11 +138,11 @@ private void init(View view) {
clickOnNotNow(view);
}

public boolean isBraveRewardsEnabled() {
return OnboardingPrefManager.getInstance().isBraveAdsEnabled();
public static boolean isBraveRewardsEnabled() {
Copy link
Contributor

Choose a reason for hiding this comment

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

are we sure to check this function after native initialization ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This flow actually starts from BraveActivity::finishNativeInitialization
It's sure that we are calling after native initialization.

@sujitacharya2005 sujitacharya2005 force-pushed the 27032_android_13_showWarningDialog_condition branch from 1a27a1a to 3174fa0 Compare December 9, 2022 06:52
Copy link
Contributor

@deeppandya deeppandya left a comment

Choose a reason for hiding this comment

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

LGTM

@sujitacharya2005 sujitacharya2005 force-pushed the 27032_android_13_showWarningDialog_condition branch 2 times, most recently from 79266a8 to b29c030 Compare December 9, 2022 18:12
@kjozwiak
Copy link
Member

kjozwiak commented Dec 9, 2022

Please only add the milestone once the issue has been resolved/merged into master 👍

@kjozwiak kjozwiak removed this from the 1.48.x - Nightly milestone Dec 9, 2022
@deeppandya deeppandya force-pushed the 27032_android_13_showWarningDialog_condition branch from b29c030 to c0681cd Compare December 9, 2022 20:45
@kjozwiak kjozwiak removed their request for review December 9, 2022 21:06
@kjozwiak
Copy link
Member

kjozwiak commented Jan 12, 2023

Verification PASSED on Pixel 6 running Android 13 using the following build(s):

Brave | 1.49.20 Chromium: 109.0.5414.87 (Official Build) canary (32-bit)
--- | ---
Revision | 2dc18eb511c56e012081b4abc9e38c81c885f7d4-refs/branch-heads/5414@{#1241}
OS | Android 13; Build/T2B1.221118.006

Test Case #1 (Generic Modal/Warning) - Both Rewards & Privacy Report disabled

  • launch 1.49.20 Chromium: 109.0.5414.87 and run through onboarding
  • tap on Continue via the Brave notifications make it easier modal
    • also ran through this case by tapping on Not now and ensuring notifications weren't being enabled with this method
  • tap on Dont Allow via the Allow Brave to send you notifications modal
  • go into Settings -> Notifications and notice that Notifications are disabled as expected
  • tap on <- (back) to go back into Settings and ensure that you receive a generic warning re: notifications being disabled
  • ensured that taping on Not now dismisses the modal without any issues
  • ensured that tapping on Continue takes you back into the Notifications screen without an issues
    • ensured that going back into Settings didn't display the generic warning modal re: notifications being disabled
  • ensured that once either Not now or Continue has been tapped, the generic warning modal doesn't appear again
    • also ensured that the generic modal doesn't re-appear if the browser was closed while the modal was still opened
Example Example Example Example
Screenshot_20230111-182106 Screenshot_20230111-182109 Screenshot_20230111-182859 Screenshot_20230111-182904

Test Case #2 (Generic Modal/Warning) - Rewards enabled & Privacy Report disabled

  • launch 1.49.20 Chromium: 109.0.5414.87 and run through onboarding
  • tap on Continue via the Brave notifications make it easier modal
  • tap on Allow via the Allow Brave to send you notifications modal to enable notifications
  • enable rewards via the rewards panel (run through country selection/onboarding)
  • once rewards has been enabled, go into Settings -> Notifications
  • toggle/disable All Brave - notifications from the Notifications page
  • tap on <- (back) to return into Settings and ensure that you don't get any warning modals
  • close the Settings page
  • go back into Settings -> Notifications
  • tap on <- (back) and ensure that you receive a generic warning re: notifications being disabled
  • ensured that taping on Not now dismisses the modal without any issues
  • ensured that tapping on Continue takes you back into the Notifications screen without an issues
    • ensured that going back into Settings didn't display the generic warning modal re: notifications being disabled
  • ensured that once either Not now or Continue has been tapped, the generic warning modal doesn't appear again
    • also ensured that the generic modal doesn't re-appear if the browser was closed while the modal was still opened
Example Example Example Example Example Example Example Example
Screenshot_20230111-184920 Screenshot_20230111-184930 Screenshot_20230111-185106 Screenshot_20230111-185538 Screenshot_20230111-190401 Screenshot_20230111-185132 Screenshot_20230111-185137 Screenshot_20230111-190432

Test Case #3 (Generic Modal/Warning) - Rewards disabled & Privacy Report enabled

  • launch 1.49.20 Chromium: 109.0.5414.87 and run through onboarding
  • tap on Continue via the Brave notifications make it easier modal
  • tap on Allow via the Allow Brave to send you notifications modal to enable notifications
  • tap on the Brave Stats via NTP and run through the Privacy Report onboarding (ensure you enable Privacy Report)
  • visit Settings -> Notifications and toggle/disable All Brave - notifications from the Notifications page
  • tap on <- (back) to return into Settings and ensure that you don't get any warning modals
  • close the Settings page
  • go back into Settings -> Notifications
  • tap on <- (back) and ensure that you receive a generic warning re: notifications being disabled
  • ensured that taping on Not now dismisses the modal without any issues
  • ensured that tapping on Continue takes you back into the Notifications screen without an issues
    • ensured that going back into Settings didn't display the generic warning modal re: notifications being disabled
  • ensured that once either Not now or Continue has been tapped, the generic warning modal doesn't appear again
    • also ensured that the generic modal doesn't re-appear if the browser was closed while the modal was still opened
  • ensured that there's a banner warning the user that Notifications are disabled within the Privacy Report
    • ensured that tapping on Turn on notifications redirects you to Settings -> Notifications
    • ensured that tapping on <- (back) within Notifications takes you back to the Privacy Report
    • ensured that warning re-appears if the user dismissed via X and then restarted the browser
Example Example Example Example Example Example Example Example
Screenshot_20230111-234756 Screenshot_20230111-234801 Screenshot_20230111-234817 Screenshot_20230111-234821 Screenshot_20230111-234840 Screenshot_20230111-234844 Screenshot_20230111-234920 Screenshot_20230111-234947

Test Case #4 (Generic Modal/Warning) - Rewards enabled & Privacy Report enabled

With the following case, basically follow Test Case #2 & Test Case #3 in terms of enabling both features and then disabling All Brave - notifications and ensuring you get a the generic warning modal as per the following:

Example Example Example Example Example Example Example
Screenshot_20230112-000955 Screenshot_20230112-000959 Screenshot_20230112-001020 Screenshot_20230112-001046 Screenshot_20230112-001119 Screenshot_20230112-001122 Screenshot_20230112-001139

Test Case #5 (Reward/Warning) - Rewards enabled & Privacy Report disabled

  • launch 1.49.20 Chromium: 109.0.5414.87 and run through onboarding
  • tap on Continue via the Brave notifications make it easier modal
  • tap on Allow via the Allow Brave to send you notifications modal to enable notifications
  • enable rewards via the rewards panel (run through country selection/onboarding)
  • once rewards has been enabled, go into Settings -> Notifications
  • toggle/disable All "Brave Ads" notifications from the Notifications page
  • tap on <- (back) to return into Settings and ensure that you get the You're not earning Brave Rewards warning modal
    • ensure that tapping on X or Got it dismiss the modal without any issues
    • ensured that the same modal never appears again when going back from Notifications -> Settings again
  • close the Settings page and restart the browser
  • ensure that you get the You're not earning Brave Rewards warning modal
  • ensured that taping on Not now dismisses the modal without any issues
  • ensured that tapping on Turn on Brave notifications takes you to the Settings -> Notifications page within Brave
  • ensured that once either Not now or Turn on Brave notifications has been tapped, the warning modal never appears again
    • also ensured that the rewards warning modal doesn't re-appear if the browser was closed while the modal was still opened
Example Example Example Example Example Example Example
Screenshot_20230112-032207 Screenshot_20230112-032213 Screenshot_20230112-032241 Screenshot_20230112-032251 Screenshot_20230112-032302 Screenshot_20230112-032311 Screenshot_20230112-032335

Test Case #6 (Privacy Report/Warning) - Rewards disabled & Privacy Report enabled

  • launch 1.49.20 Chromium: 109.0.5414.87 and run through onboarding
  • tap on Continue via the Brave notifications make it easier modal
  • tap on Allow via the Allow Brave to send you notifications modal to enable notifications
  • enable Privacy Report by tapping on the Brave Stats and running through onboarding
  • once Privacy Report has been enabled, go into Settings -> Notifications
  • toggle/disable All "General" notifications from the Notifications page
  • tap on <- (back) to return into Settings and ensure that you get the You're not receiving Privacy Reports warning modal
    • ensure that tapping on X or Got it dismiss the modal without any issues
    • ensured that the same modal never appears again when going back from Notifications -> Settings again
  • close the Settings page and restart the browser
  • ensure that you get the You're not receiving Privacy Reports warning modal
  • ensured that taping on Not now dismisses the modal without any issues
  • ensured that tapping on Turn on Brave notifications takes you to the Settings -> Notifications page within Brave
  • ensured that once either Not now or Turn on Brave notifications has been tapped, the warning modal never appears again
    • also ensured that the privacy report warning modal doesn't re-appear if the browser was closed while the modal was still opened
Example Example Example Example Example Example Example
Screenshot_20230112-034834 Screenshot_20230112-034843 Screenshot_20230112-034854 Screenshot_20230112-034909 Screenshot_20230112-034914 Screenshot_20230112-034926 Screenshot_20230112-034936

Test Case #7 (Reward & Privacy Report/Warning) - Rewards enabled & Privacy Report enabled

With the following case, basically follow Test Case #5 & Test Case #6 in terms of enabling both features and then disabling All "Brave Ads" notifications & All "General" notifications and ensuring you get the combined warning modal as per the following:

Example Example Example Example Example Example Example Example
Screenshot_20230112-035938 Screenshot_20230112-035942 Screenshot_20230112-040012 Screenshot_20230112-040019 Screenshot_20230112-040032 Screenshot_20230112-040037 Screenshot_20230112-040043 Screenshot_20230112-040053

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI/skip-ios Do not run CI builds for iOS CI/skip-macos-x64 Do not run CI builds for macOS x64
Projects
None yet
4 participants