Skip to content

/ ghostery-extension

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

Jump to bottom

GH-2258: Preserve user onboarding settings #681

Merged
merged 10 commits into from Mar 15, 2021
+218 −95
Merged

GH-2258: Preserve user onboarding settings #681

Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
c53210c
Preserve onboarding selections on back
leuryr Feb 8, 2021
45caa11
Simplify and fix default browser check
leuryr Feb 19, 2021
ccfb5dc
Remove antiSuite wrapper, update checkbox
leuryr Feb 19, 2021
a3c31a1
Refactor question blocks
leuryr Feb 22, 2021
1cadc46
Use Blocking Policy constants
leuryr Feb 22, 2021
04e7348
Update tests and PropTypes
leuryr Feb 22, 2021
06aa58e
Refactor blocking policy handling
leuryr Feb 22, 2021
ebbab47
Update recommended choices check
leuryr Mar 10, 2021
fe4c852
Remove recommendedChoices
leuryr Mar 11, 2021
ae7fdee
Merge in develop
wlycdgr Mar 11, 2021
File filter
Filter file types
Jump to
Jump to file
Failed to load files.

Always

Just for now

Next

Preserve onboarding selections on back

  • Loading branch information
@leuryr
leuryr committed Feb 8, 2021
commit c53210c3d7818c6b02c7d86d78bcb61e58707605
95 app/dawn-hub/Views/OnboardingViews/Step2_BlockSettingsView/BlockSettingsView.jsx
View file
Open in desktop
@@ -30,13 +30,70 @@ class BlockSettingsView extends Component {
super(props);
this.state = {
recommendedChoices: false,
enable_ad_block: null,
kindsOfTrackers: null,
enable_ad_block: null,
enable_anti_tracking: null,
enable_smart_block: null
};
}

componentDidMount() {
const { setupLifecycle: { blockSetupSeen } } = this.props;
if (blockSetupSeen) {
const {
antiSuite: {
enable_anti_tracking,
enable_ad_block,
enable_smart_block,
},
blockingPolicy
} = this.props;

const decodedPolicy = this.decodeBlockingPolicy(blockingPolicy);

this.setState({
kindsOfTrackers: decodedPolicy,
enable_ad_block,
enable_anti_tracking,
enable_smart_block
});
}
}

decodeBlockingPolicy = (blockingPolicy) => {
let decodedPolicy;
if (typeof blockingPolicy === 'number') {
switch (blockingPolicy) {
case 1:
decodedPolicy = 'BLOCKING_POLICY_EVERYTHING';
break;
case 2:
decodedPolicy = 'BLOCKING_POLICY_RECOMMENDED';
break;
case 3:
decodedPolicy = 'BLOCKING_POLICY_NOTHING';
break;
default:
break;
}
} else if (typeof blockingPolicy === 'string') {
switch (blockingPolicy) {
case 'BLOCKING_POLICY_EVERYTHING':
decodedPolicy = 1;
break;
case 'BLOCKING_POLICY_RECOMMENDED':
decodedPolicy = 2;
break;
case 'BLOCKING_POLICY_NOTHING':
decodedPolicy = 3;
break;
default:
break;
}
}
return decodedPolicy;
This conversation was marked as resolved by wlycdgr
Comment on lines +63 to +94

This comment has been minimized.

Sign in to view
@wlycdgr

wlycdgr Feb 9, 2021
Member

Let's use the constants from shared-hub\constants\BlockingPolicyConstants here (and anywhere else where we are using these string values)

This comment has been minimized.

Sign in to view
@wlycdgr

wlycdgr Feb 9, 2021
Member

Better yet, can we make do without doing these conversions in the first place? I think we are asking for trouble if kindsOfTrackers can sometimes be a string and sometimes a number. What if we just always use the string values? That's what background.js expects so no conversion would be necessary - only a modification to what values we are testing against in the checkbox rendering code (unless I'm missing somethin'?)

This comment has been minimized.

Sign in to view
@wlycdgr

wlycdgr Feb 9, 2021
Member

Also, while we're digging around in this file, could you factor out rendering of the BlockSettingsView_question lis to a helper like the renderAnswerBlock helper I made? It would be a nice bit of cleanup since they are all the same except for a couple strings

This comment has been minimized.

Sign in to view
@leuryr

leuryr Feb 23, 2021
Author Contributor

Better yet, can we make do without doing these conversions in the first place? I think we are asking for trouble if kindsOfTrackers can sometimes be a string and sometimes a number. What if we just always use the string values? That's what background.js expects so no conversion would be necessary - only a modification to what values we are testing against in the checkbox rendering code (unless I'm missing somethin'?)

While it looks like we still need the number representation for when we send over the setup_number string from onboarding, using the constants everywhere else would mean that we only need to convert to a number for that case in particular, so all the other code can be removed/refactored. Was able to drop quite a few lines with this change.

This comment has been minimized.

Sign in to view
@wlycdgr

wlycdgr Mar 8, 2021
Member

Sweet!
}

toggleRecommendedChoices = (value) => {
if (value === true) {
this.setState({
@@ -94,35 +151,24 @@ class BlockSettingsView extends Component {
});

const {
setAdBlock, setAntiTracking, setSmartBlocking, setBlockingPolicy, setSetupStep
setAdBlock, setAntiTracking, setSmartBlocking, setBlockingPolicy, setSetupStep, setBlockSetupSeen
} = actions;
const { history } = this.props;

setAdBlock({ enable_ad_block });
setAntiTracking({ enable_anti_tracking });
setSmartBlocking({ enable_smart_block });

let blockingPolicy;
switch (kindsOfTrackers) {
case 1:
blockingPolicy = 'BLOCKING_POLICY_EVERYTHING';
break;
case 2:
blockingPolicy = 'BLOCKING_POLICY_RECOMMENDED';
break;
case 3:
blockingPolicy = 'BLOCKING_POLICY_NOTHING';
break;
default:
break;
}
setBlockingPolicy({ blockingPolicy });
const decodedPolicy = this.decodeBlockingPolicy(kindsOfTrackers);

setBlockingPolicy({ blockingPolicy: decodedPolicy });

setSetupStep({
setup_step: BLOCK_SETTINGS,
dawn_setup_number: this.buildSetupNumberString(),
origin: ONBOARDING
});
setBlockSetupSeen(true);
history.push('/onboarding/3');
} else {
setToast({
@@ -231,5 +277,20 @@ BlockSettingsView.propTypes = {
setBlockingPolicy: PropTypes.func.isRequired,
setToast: PropTypes.func.isRequired,
setSetupStep: PropTypes.func.isRequired,
setBlockSetupSeen: PropTypes.func.isRequired,
}).isRequired,
setupLifecycle: PropTypes.shape({
blockingPolicy: PropTypes.string.isRequired,
enable_anti_tracking: PropTypes.bool.isRequired,
enable_ad_block: PropTypes.bool.isRequired,
enable_smart_block: PropTypes.bool.isRequired,
blockSetupSeen: PropTypes.bool.isRequired,
searchSetupSeen: PropTypes.bool.isRequired,
}).isRequired,
antiSuite: PropTypes.shape({
enable_ad_block: PropTypes.bool.isRequired,
enable_anti_tracking: PropTypes.bool.isRequired,
enable_smart_block: PropTypes.bool.isRequired,
}).isRequired,
blockingPolicy: PropTypes.string.isRequired
};
5 ...wn-hub/Views/OnboardingViews/Step2_BlockSettingsView/__tests__/BlockSettingsView.test.jsx
View file
Open in desktop
@@ -22,6 +22,7 @@ jest.mock('../../../../../shared-components/Tooltip');

describe('app/dawn-hub/Views/OnboardingViews/Step2_BlockSettingsView/BlockSettingsView.test.jsx', () => {
const initialState = {
setupLifecycle: noop,
actions: {
logout: noop,
setAntiTracking: noop,
@@ -30,7 +31,8 @@ describe('app/dawn-hub/Views/OnboardingViews/Step2_BlockSettingsView/BlockSettin
setBlockingPolicy: noop,
setToast: noop,
setSetupStep: noop,
}
setBlockSetupSeen: noop,
},
};
describe('Snapshot tests with react-test-renderer', () => {
test('BlockSettings View is rendered correctly', () => {
@@ -55,6 +57,7 @@ describe('app/dawn-hub/Views/OnboardingViews/Step2_BlockSettingsView/BlockSettin
setBlockingPolicy: jest.fn(),
setToast: noop,
setSetupStep: jest.fn(),
setBlockSetupSeen: jest.fn(),
},
history: {
push: noop
5 app/dawn-hub/Views/OnboardingViews/Step2_BlockSettingsView/index.js
View file
Open in desktop
@@ -18,16 +18,17 @@ import { logout } from '../../../../Account/AccountActions';
import { setAntiTracking, setAdBlock, setSmartBlocking } from '../../../../shared-hub/actions/AntiSuiteActions';
import setBlockingPolicy from '../../../../shared-hub/actions/BlockingPolicyActions';
import setToast from '../../../../shared-hub/actions/ToastActions';
import { setSetupStep } from '../../../../shared-hub/actions/SetupLifecycleActions';
import { setSetupStep, setBlockSetupSeen } from '../../../../shared-hub/actions/SetupLifecycleActions';

const actionCreators = {
logout,
setAntiTracking,
setAdBlock,
setSmartBlocking,
setBlockingPolicy,
setBlockSetupSeen,
setToast,
setSetupStep,
};

export default withRouter(buildReduxHOC(null, actionCreators, BlockSettingsView));
export default withRouter(buildReduxHOC(['setupLifecycle', 'antiSuite', 'blockingPolicy'], actionCreators, BlockSettingsView));
21 app/dawn-hub/Views/OnboardingViews/Step3_ChooseDefaultSearchView/ChooseDefaultSearchView.jsx
View file
Open in desktop
@@ -78,6 +78,24 @@ class ChooseDefaultSearchView extends Component {
document.addEventListener('click', this.handleClickAway);

this.fetchSearchEnginesAsync();

const { setupLifecycle: { searchSetupSeen } } = this.props;

if (searchSetupSeen) {
const { defaultSearch } = this.props;
const searchChoices = [
SEARCH_GHOSTERY,
SEARCH_BING,
SEARCH_YAHOO,
SEARCH_STARTPAGE,
];
const isOther = searchChoices.reduce((accum, choice) => {
if (accum === false) return false;
return choice !== defaultSearch;
}, false);
This conversation was marked as resolved by wlycdgr
Comment on lines +92 to +95

This comment has been minimized.

Sign in to view
@wlycdgr

wlycdgr Feb 9, 2021
Member

You can simplify to
const isOther = searchChoices.includes(defaultSearch)

(Also I think your version will always return false? Since accum is initialized to false and so the condition in line 93 is always satisfied. Bigger picture, with the simpler includes version it's much easier to be confident that the code does what you intend)

This comment has been minimized.

Sign in to view
@leuryr

leuryr Feb 23, 2021
Author Contributor

Yea, you're right. Definitely messed up on the logic here, and .includes() is a much simpler implementation. I changed the check to const isOther = !searchChoices.includes(defaultSearch) as .includes(defaultSearch) returning true would mean that isOther should be false.

This comment has been minimized.

Sign in to view
@wlycdgr

wlycdgr Mar 8, 2021
Member

Great, thanks man!
const prevChosenSearch = isOther ? SEARCH_OTHER : defaultSearch;
this.setState({ chosenSearch: prevChosenSearch, otherSearchSelected: isOther ? prevChosenSearch : null });
}
This conversation was marked as resolved by wlycdgr
Comment on lines +97 to +98

This comment has been minimized.

Sign in to view
@wlycdgr

wlycdgr Feb 9, 2021
Member

This will set otherSearchSelected to SEARCH_OTHER if prevChosenSearch is SEARCH_OTHER. That's not quite right: otherSearchSelected should store which specific other search is selected, when chosenSearch is SEARCH_OTHER (e.g., SEARCH_GIBIRU, etc)

This comment has been minimized.

Sign in to view
@leuryr

leuryr Feb 23, 2021
Author Contributor

Yep messed up here as well. This should be setting otherSearchSelected as the defaultSearch value if isOther is true, which would carry the specific other search that the user had selected.

This comment has been minimized.

Sign in to view
@wlycdgr

wlycdgr Mar 8, 2021
Member

Woot woot, ty ty
}

componentWillUnmount() {
@@ -140,7 +158,7 @@ class ChooseDefaultSearchView extends Component {
handleSubmit = () => {
const { chosenSearch, otherSearchSelected } = this.state;
const { actions, history } = this.props;
const { setSetupStep, setDefaultSearch } = actions;
const { setSetupStep, setDefaultSearch, setSearchSetupSeen } = actions;

const chosenSearchName = chosenSearch === SEARCH_OTHER
? otherSearchSelected
@@ -173,6 +191,7 @@ class ChooseDefaultSearchView extends Component {
dawn_setup_number,
origin: ONBOARDING
});
setSearchSetupSeen(true);
history.push(`/${ONBOARDING}/${CHOOSE_PLAN}`);
}

5 app/dawn-hub/Views/OnboardingViews/Step3_ChooseDefaultSearchView/index.js
View file
Open in desktop
@@ -15,11 +15,12 @@ import { withRouter } from 'react-router-dom';
import ChooseDefaultSearchView from './ChooseDefaultSearchView';
import { buildReduxHOC } from '../../../../shared-hub/utils';
import setDefaultSearch from './ChooseDefaultSearchActions';
import { setSetupStep } from '../../../../shared-hub/actions/SetupLifecycleActions';
import { setSetupStep, setSearchSetupSeen } from '../../../../shared-hub/actions/SetupLifecycleActions';

const actionCreators = {
setDefaultSearch,
setSetupStep,
setSearchSetupSeen,
};

export default withRouter(buildReduxHOC(null, actionCreators, ChooseDefaultSearchView));
export default withRouter(buildReduxHOC(['setupLifecycle', 'defaultSearch'], actionCreators, ChooseDefaultSearchView));
18 app/shared-hub/actions/SetupLifecycleActions.js
View file
Open in desktop
@@ -15,7 +15,9 @@ import { makeDeferredDispatcher } from '../utils';
import {
INIT_SETUP_PROPS,
SET_SETUP_STEP,
SET_SETUP_COMPLETE
SET_SETUP_COMPLETE,
SET_BLOCK_SETUP_SEEN,
SET_SEARCH_SETUP_SEEN
} from '../constants/SetupLifecycleConstants';

export function initSetupProps(data) {
@@ -25,6 +27,20 @@ export function initSetupProps(data) {
};
}

export const setBlockSetupSeen = data => (
{
type: SET_BLOCK_SETUP_SEEN,
data,
}
);

export const setSearchSetupSeen = data => (
{
type: SET_SEARCH_SETUP_SEEN,
data,
}
);

export const setSetupStep =
actionData => makeDeferredDispatcher(SET_SETUP_STEP, actionData);

2 app/shared-hub/constants/SetupLifecycleConstants.js
View file
Open in desktop
@@ -14,3 +14,5 @@
export const INIT_SETUP_PROPS = 'INIT_SETUP_PROPS';
export const SET_SETUP_STEP = 'SET_SETUP_STEP';
export const SET_SETUP_COMPLETE = 'SET_SETUP_COMPLETE';
export const SET_BLOCK_SETUP_SEEN = 'SET_BLOCK_SETUP_SEEN';
export const SET_SEARCH_SETUP_SEEN = 'SET_SEARCH_SETUP_SEEN';
14 app/shared-hub/reducers/AntiSuiteReducer.js
View file
Open in desktop
@@ -17,21 +17,27 @@ import {
SET_SMART_BLOCK
} from '../constants/AntiSuiteConstants';

const initialState = {};
const initialState = {
antiSuite: {
enable_ad_block: false,
enable_anti_tracking: false,
enable_smart_block: false
}
};
This conversation was marked as resolved by wlycdgr
Comment on lines +20 to +26

This comment has been minimized.

Sign in to view
@wlycdgr

wlycdgr Feb 9, 2021
Member

I am realizing I have a strong suspicion we don't need the extra antiSuite and setupLifecycle wrappers at all after all, and that the code would be better and simpler without them....but I can fix that up at the end, don't worry about it unless you want to

This comment has been minimized.

Sign in to view
@wlycdgr

wlycdgr Mar 8, 2021
Member

(Based on our convo, you're right, let's leave it as is)

function AntiSuiteReducer(state = initialState, action) {
switch (action.type) {
case SET_AD_BLOCK: {
const { enable_ad_block } = action.data;
return { ...state, setup: { ...state.setup, enable_ad_block } };
return { ...state, antiSuite: { ...state.antiSuite, enable_ad_block } };
}
case SET_ANTI_TRACKING: {
const { enable_anti_tracking } = action.data;
return { ...state, setup: { ...state.setup, enable_anti_tracking } };
return { ...state, antiSuite: { ...state.antiSuite, enable_anti_tracking } };
}
case SET_SMART_BLOCK: {
const { enable_smart_block } = action.data;
return { ...state, setup: { ...state.setup, enable_smart_block } };
return { ...state, antiSuite: { ...state.antiSuite, enable_smart_block } };
}

default: return state;
6 app/shared-hub/reducers/BlockingPolicyReducer.js
View file
Open in desktop
@@ -13,13 +13,15 @@

import { SET_BLOCKING_POLICY } from '../constants/BlockingPolicyConstants';

const initialState = {};
const initialState = {
blockingPolicy: ''
};

function BlockingPolicyReducer(state = initialState, action) {
switch (action.type) {
case SET_BLOCKING_POLICY: {
const { blockingPolicy } = action.data;
return { ...state, setup: { ...state.setup, blockingPolicy } };
return { ...state, blockingPolicy };
}

default: return state;
23 app/shared-hub/reducers/SetupLifecycleReducer.js
View file
Open in desktop
@@ -12,10 +12,19 @@
*/

import {
INIT_SETUP_PROPS
INIT_SETUP_PROPS, SET_BLOCK_SETUP_SEEN, SET_SEARCH_SETUP_SEEN
} from '../constants/SetupLifecycleConstants';

const initialState = {};
const initialState = {
setupLifecycle: {
blockingPolicy: '',
enable_anti_tracking: false,
enable_ad_block: false,
enable_smart_block: false,
blockSetupSeen: false,
searchSetupSeen: false
}
};

function SetupLifecycleReducer(state = initialState, action) {
switch (action.type) {
@@ -29,14 +38,22 @@ function SetupLifecycleReducer(state = initialState, action) {
} = action.data;
return {
...state,
setup: {
setupLifecycle: {
blockingPolicy,
enable_anti_tracking,
enable_ad_block,
enable_smart_block,
blockSetupSeen: false,
searchSetupSeen: false
}
};
}
case SET_BLOCK_SETUP_SEEN: {
return { ...state, setupLifecycle: { ...state.setupLifecycle, blockSetupSeen: action.data } };
}
case SET_SEARCH_SETUP_SEEN: {
return { ...state, setupLifecycle: { ...state.setupLifecycle, searchSetupSeen: action.data } };
}

default: return state;
}
ProTip! Use n and p to navigate between commits in a pull request.