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.

Sign up for GitHub

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

Stats: Fix stats setting role toggles not sticking #40853

Merged
merged 5 commits into from
Jan 6, 2025
Merged

Stats: Fix stats setting role toggles not sticking #40853

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
85b6b9e
fix toggles
kangzj Jan 6, 2025
78ac553
changelog
kangzj Jan 6, 2025
ad5841c
includes is not necessary
kangzj Jan 6, 2025
7ebf1ea
make the condition check more robust
kangzj Jan 6, 2025
22f546e
use a const array
kangzj Jan 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 16 additions & 35 deletions projects/plugins/jetpack/_inc/client/traffic/site-stats.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,9 +42,22 @@ class SiteStatsComponent extends React.Component {
wpcom_reader_views_enabled: props.getOptionValue( 'wpcom_reader_views_enabled' ),
};

if ( roles ) {
this.addCustomCountRolesState( countRoles );
this.addCustomRolesState( roles );
const defaultRoles = [ 'administrator', 'editor', 'author', 'contributor', 'subscriber' ];

if ( roles?.length > 0 ) {
roles.forEach( role => {
if ( ! defaultRoles.includes( role ) ) {
this.state[ `roles_${ role }` ] = true;
}
} );
}

if ( countRoles?.length > 0 ) {
countRoles.forEach( role => {
if ( ! defaultRoles.includes( role ) ) {
this.state[ `count_roles_${ role }` ] = true;
}
} );
}
}

Expand Down Expand Up @@ -107,38 +120,6 @@ class SiteStatsComponent extends React.Component {
return () => this.updateOptions( role, setting );
};

/**
* Allows for custom roles 'count logged in page views' stats settings to be added to the current state.
*
* @param {Array} countRoles - All roles (including custom) that have 'count logged in page views' enabled.
*/
addCustomCountRolesState( countRoles ) {
countRoles.forEach( role => {
if (
! [ 'administrator', 'editor', 'author', 'subscriber', 'contributor' ].includes(
countRoles
Copy link
Contributor

Choose a reason for hiding this comment

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

In addition to the setState() issue, testing for countRoles here would appear to be a bug.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ya it was a bug and I fixed it by the way 😄

)
) {
this.setState( { [ `count_roles_${ role }` ]: includes( countRoles, role, false ) } );
}
} );
}

/**
* Allows for custom roles 'allow stats reports' stats settings to be added to the current state.
*
* @param {Array} roles - All roles (including custom) that have 'allow stats reports' enabled.
*/
addCustomRolesState( roles ) {
roles.forEach( role => {
if (
! [ 'administrator', 'editor', 'author', 'subscriber', 'contributor' ].includes( role )
) {
this.setState( { [ `roles_${ role }` ]: includes( roles, role, false ) } );
}
} );
}

handleStatsOptionToggle( option_slug ) {
return () => this.props.updateFormStateModuleOption( 'stats', option_slug );
}
Expand Down
4 changes: 4 additions & 0 deletions projects/plugins/jetpack/changelog/fix-stats-setting-role-toggles
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Significance: patch
Type: bugfix

Fix custom roles settings are not sticking for Jetpack Stats
Loading