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

Enabled diagnostic reporting by default #16208

Merged
merged 1 commit into from
Dec 9, 2022

Conversation

nullhook
Copy link
Contributor

@nullhook nullhook commented Dec 2, 2022

Resolves brave/brave-browser#27126
Resolves brave/brave-browser#27253

Security review: https://github.com/brave/security/issues/1132

We enable diagnostic reporting by default on the new Welcome UI

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:

@github-actions github-actions bot added the CI/storybook-url Deploy storybook and provide a unique URL for each build label Dec 2, 2022
@rebron rebron requested a review from fmarier December 2, 2022 20:21
@brave-builds
Copy link
Collaborator

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

Copy link
Member

@fmarier fmarier left a comment

Choose a reason for hiding this comment

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

I'm a bit confused. The PR is for changing the P3A default, but the linked issue is for changing the crash reporting default.

Which setting is this PR meant to change?

@nullhook
Copy link
Contributor Author

nullhook commented Dec 3, 2022

Good catch, looks like the callbacks were swapped. Just to confirm, the first checkbox handles metrics reporting, and the second checkbox handles P3A.

The PR is intended to change the metrics reporting default. Both options should now be checked by default and also should call the correct callbacks.

@nullhook nullhook requested a review from fmarier December 3, 2022 22:51
@brave-builds
Copy link
Collaborator

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

@fmarier
Copy link
Member

fmarier commented Dec 5, 2022

I was confused about this initially, but @nullhook cleared it up for me in a Slack DM:

"metrics reporting" refers to automatic crash reporting

This PR does two things:

  • It corrects the naming of the checkboxes in the code (the crash report setting was controlled by the p3a variable and vice-versa) -- no user-visible changes.
  • It changes the default for crash reporting to ON by default (i.e. opt out) -- covered by the linked security review.

Copy link
Member

@fmarier fmarier left a comment

Choose a reason for hiding this comment

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

The part of the changes which corrects the naming of the variables looks great.

I'll put my review on hold because of the second part of this change (changing the crash reporting default) until the security review is complete.

@brave-builds
Copy link
Collaborator

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

@nullhook nullhook merged commit be75539 into master Dec 9, 2022
@nullhook nullhook deleted the onboarding/default-reporting branch December 9, 2022 05:53
@github-actions github-actions bot added this to the 1.48.x - Nightly milestone Dec 9, 2022
nullhook added a commit that referenced this pull request Dec 10, 2022
Enabled diagnostic reporting by default and swapped handles of the two checkboxes on help improve screen
@kjozwiak
Copy link
Member

kjozwiak commented Dec 15, 2022

Verification PASSED on Win 11 x64 using the following build(s):

Brave | 1.48.57 Chromium: 108.0.5359.128 (Official Build) nightly (64-bit)
-- | --
Revision | 1cd27afdb8e5d057070c0961e04c490d2aca1aa0-refs/branch-heads/5359@{#1185}
OS | Windows 11 Version 22H2 (Build 22621.963)

Test Case #1 - brave/brave-browser#27126

Using the STR/Cases outlined via brave/brave-browser#27126 (comment), ensured that Send diagnostic reports if Brave crashes or freezes is set is enabled (opt-out) as per #16208 (comment). Also ensured that it appeared enabled via brave://settings/privacy.

Example Example
image image

Test Case #2 - brave/brave-browser#27253

Using the STR/Cases outlined via brave/brave-browser#27253 (comment), ensured that Allow privacy-preserving product analytics (P3A) is enabled via brave://settings/privacy as per the following:

Example Example
image image

nullhook added a commit that referenced this pull request Dec 15, 2022
Enabled diagnostic reporting by default and swapped handles of the two checkboxes on help improve screen
@@ -34,8 +34,8 @@ function InputCheckbox (props: InputCheckboxProps) {
}

function HelpImprove () {
const [isP3AEnabled, setP3AEnabled] = React.useState(false)
Copy link
Collaborator

Choose a reason for hiding this comment

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

this old version was wrong because it should be true for nightly and beta, but the new version is also implemented incorrectly. If we want to default to true, we should change the default pref value, not the UI

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI/storybook-url Deploy storybook and provide a unique URL for each build
Projects
None yet
5 participants