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

Change Setup Wizard redirect to work on all relevant pages #7581

Merged
merged 10 commits into from
Apr 9, 2024

Conversation

renatho
Copy link
Contributor

@renatho renatho commented Apr 5, 2024

Proposed Changes

There was a change on WP 6.5, where it doesn't reload the page when installing a plugin from the plugins directory through "Plugins > Add New". If the plugin is uploaded from a zip file the behavior continues the same as previously.

The Sensei Setup Wizard used to work immediately after we activate the plugin. With the recent change, our logic doesn't work anymore.

After a discussion (p1712321247992169-slack-C02NWDZBL0H), we decided to keep the intended behavior from WordPress core, and the Setup Wizard will only open after the user navigate in the WP admin.

Considered alternative

I thought of using this JS event https://github.com/WordPress/WordPress/blob/9bb63fd66503d139e0b3e8498c2a5a80cb3507a1/wp-admin/js/updates.js#L1123 and force a redirect when it's the Sensei activation, but it doesn't align with what WP core is proposing.

Testing Instructions

  1. Deactivate Sensei LMS plugin.
  2. Remove the option sensei_installed from the database.
  3. Navigate to WP Admin > Plugins > Add New Plugin.
  4. Search for "Sensei LMS".
  5. Click on "Activate" (notice the new behavior for WP 6.5, where it's an ajax request and won't redirect to the setup wizard).
  6. Click on "Users" in the menu.
  7. Check that you are not redirected to the Setup Wizard.
  8. Click on Sensei LMS, and check that you are redirected to the Setup Wizard (the same should happen when clicking on Sensei pages, Dashboard page and Plugins page).
  9. Repeat the test in WP 6.4, and make sure it continues working properly when clicking on "Activate", as it used to work.

Pre-Merge Checklist

  • PR title and description contain sufficient detail and accurately describe the changes
  • Acceptance criteria is met
  • Decisions are publicly documented
  • Adheres to coding standards (PHP, JavaScript, CSS, HTML)
  • All strings are translatable (without concatenation, handles plurals)
  • Follows our naming conventions (P6rkRX-4oA-p2)
  • Hooks (p6rkRX-1uS-p2) and functions are documented
  • New UIs are responsive and use a mobile-first approach
  • New UIs match the designs
  • Different user privileges (admin, teacher, subscriber) are tested as appropriate
  • Legacy courses (course without blocks) are tested
  • Code is tested on the minimum supported PHP and WordPress versions
  • User interface changes have been tested on the latest versions of Chrome, Firefox and Safari
  • "Needs Documentation" label is added if this change requires updates to documentation
  • Known issues are created as new GitHub issues

Context

  • See p1712321247992169-slack-C02NWDZBL0H

@renatho renatho added this to the 4.23.1 milestone Apr 5, 2024
@renatho renatho self-assigned this Apr 5, 2024
@renatho renatho changed the title Change/setup wizard redirect Change Setup Wizard redirect to work on all relevant pages Apr 5, 2024
Copy link

github-actions bot commented Apr 5, 2024

Test the previous changes of this PR with WordPress Playground.

@@ -81,4 +81,13 @@
<exclude-pattern>**/views/*</exclude-pattern>
<exclude-pattern>tests/bootstrap.php</exclude-pattern>
</rule>

<rule ref="WordPress.WP.Capabilities">
Copy link
Contributor Author

@renatho renatho Apr 5, 2024

Choose a reason for hiding this comment

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

It adds the Sensei custom capability, so PHPCS issues don't happen when it's used.

@@ -446,7 +448,7 @@ public function get_woocommerce_connect_data() {
*
* @return stdClass Extension with status.
*/
private function get_feature_with_status( $extension, $installing_plugins, $selected_plugins ) {
private function get_feature_with_status( $extension, $installing_plugins, $selected_plugins ) { // phpcs:ignore Generic.CodeAnalysis.UnusedFunctionParameter.FoundAfterLastUsed -- Called by a public deprecated method.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just a PHPCS fix.

@@ -545,7 +547,7 @@ public static function close_wccom_install() {

if (
isset( $_SERVER['HTTP_REFERER'] ) &&
0 === strpos( $_SERVER['HTTP_REFERER'], 'https://woocommerce.com/checkout' ) && // phpcs:ignore sanitization ok.
0 === strpos( $_SERVER['HTTP_REFERER'], 'https://woocommerce.com/checkout' ) && // phpcs:ignore -- sanitization ok.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just a PHPCS fix.

@@ -496,7 +498,7 @@ public function get_sensei_extensions( $clear_active_plugins_cache = false ) {
}

$extensions = array_map(
function( $extension ) use ( $installing_plugins, $selected_plugins ) {
function ( $extension ) use ( $installing_plugins, $selected_plugins ) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just a PHPCS fix.

@@ -93,7 +93,7 @@ public function __construct() {
add_action( 'current_screen', [ $this, 'remove_notices_from_setup_wizard' ] );
add_action( 'admin_notices', [ $this, 'setup_wizard_notice' ] );
add_action( 'admin_init', [ $this, 'skip_setup_wizard' ] );
add_action( 'admin_init', [ $this, 'activation_redirect' ] );
add_action( 'current_screen', [ $this, 'activation_redirect' ] );
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's changed because now we want to use the get_current_screen, and it's not ready in the admin_init hook.

@@ -221,6 +220,8 @@ protected function redirect_to_setup_wizard() {
* Render app container for setup wizard.
*/
public function render_wizard_page() {
// Delete option when the Setup Wizard is loaded, so it doesn't redirect anymore.
delete_option( 'sensei_activation_redirect' );
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved from the redirect method to here because we want it removed anytime the Setup Wizard is loaded.

I don't think it would happen in a normal flow, but this makes more sense in case some logic is changed in the future.

@@ -1166,7 +1166,7 @@ public function activate_sensei() {
// Do not enable the wizard for sites that are created with the onboarding flow.
if ( 'sensei' !== get_option( 'site_intent' ) ) {

set_transient( 'sensei_activation_redirect', 1, 30 );
update_option( 'sensei_activation_redirect', 1 );
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to option, so if the user delays more than 30 seconds to click around, the Setup Wizard will still open in the relevant pages.

Copy link

github-actions bot commented Apr 5, 2024

Test the previous changes of this PR with WordPress Playground.

Copy link

github-actions bot commented Apr 5, 2024

Test the previous changes of this PR with WordPress Playground.

Copy link

codecov bot commented Apr 5, 2024

Codecov Report

Attention: Patch coverage is 89.18919% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 51.81%. Comparing base (7742f33) to head (564fdde).
Report is 4 commits behind head on trunk.

❗ Current head 564fdde differs from pull request most recent head 693851e. Consider uploading reports for the commit 693851e to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##              trunk    #7581      +/-   ##
============================================
+ Coverage     51.76%   51.81%   +0.04%     
- Complexity    11323    11326       +3     
============================================
  Files           641      641              
  Lines         48185    48208      +23     
  Branches        470      470              
============================================
+ Hits          24943    24978      +35     
+ Misses        22861    22849      -12     
  Partials        381      381              
Files Coverage Δ
includes/class-sensei-data-cleaner.php 87.75% <ø> (ø)
includes/class-sensei.php 24.89% <0.00%> (ø)
includes/admin/class-sensei-setup-wizard.php 55.00% <91.66%> (+11.34%) ⬆️

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0132d5c...693851e. Read the comment docs.

Copy link

github-actions bot commented Apr 5, 2024

Test the previous changes of this PR with WordPress Playground.

// Only redirects for admin users.
|| ! current_user_can( 'manage_sensei' )
// Check if it's an admin screen that should redirect.
|| ! $this->should_current_page_redirect_to_wizard()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now the redirect runs when the user accesses the pages we used to display the Setup Wizard notice (Dashboard, Plugins, and Sensei pages).

*
* @return boolean
*/
private function should_current_page_redirect_to_wizard() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I separated the checks for the notice and the redirect because the notice is very ugly in Sensei Home and we want the redirect if the user navigates to the Sensei Home.

@renatho renatho requested a review from a team April 5, 2024 19:12
@renatho renatho marked this pull request as ready for review April 5, 2024 19:12
Copy link

github-actions bot commented Apr 5, 2024

Test the previous changes of this PR with WordPress Playground.

Imran92
Imran92 previously approved these changes Apr 8, 2024
Copy link
Contributor

@Imran92 Imran92 left a comment

Choose a reason for hiding this comment

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

LGTM! Just left some minor suggestions

/**
* Testing if redirect option is cleared on setup wizard rendering.
*/
public function testRenderWizardPageClearsRedirectOption() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we name the test function like we do for our other tests? Like this p6rkRX-35r-p2 (test**_When**_Does**)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure! I didn't do that to follow the old standard of this file, but to make it write I changed the whole file here now: 2d7b8eb

Comment on lines 295 to 298

Sensei()->setup_wizard->render_wizard_page();

$this->assertFalse( get_option( 'sensei_activation_redirect', false ) );
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can divide these in Arrange Act Assert sections?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -247,8 +267,6 @@ public function testActivationRedirectNoAdmin() {
->method( 'redirect_to_setup_wizard' );

$setup_wizard_mock->activation_redirect();

$this->assertNotFalse( get_transient( 'sensei_activation_redirect' ), 'Transient should not be removed' );
Copy link
Contributor

Choose a reason for hiding this comment

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

The next test still mentions Transient even though we are no longer using transients, should we rename?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed with the 2d7b8eb


set_transient( 'sensei_activation_redirect', 1, 30 );
update_option( 'sensei_activation_redirect', 1 );
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of using mock here, is it possible to test it using the Sensei_Test_Redirect_Helpers trait? You can see an example here

public function testMarkCompleteAndRedirect_WhenCalled_TriesToRedirectToRightPage() {
/* Arrange. */
$this->login_as_admin();
$this->prevent_wp_redirect();
$redirect_location = '';
/* Act. */
try {
Sensei_Home_Task_Pro_Upsell::mark_completed_and_redirect();
} catch ( Sensei_WP_Redirect_Exception $e ) {
$redirect_location = $e->getMessage();
}
/* Assert. */
$this->assertSame( 'https://senseilms.com/sensei-pro/?utm_source=plugin_sensei&utm_medium=upsell&utm_campaign=sensei-home', $redirect_location );
}
. Maybe it can farther improve the reliability of the test? Just a suggestion, not necessary at all

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure! Thank you for the suggestion! Updated here: 693851e

Copy link
Contributor

Choose a reason for hiding this comment

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

Very nice! <3 Now they are covering much more realistically than the mock!

Copy link

github-actions bot commented Apr 8, 2024

Test the previous changes of this PR with WordPress Playground.

Copy link
Contributor

@Imran92 Imran92 left a comment

Choose a reason for hiding this comment

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

LGTM! 🚀

@renatho renatho merged commit 9410aa2 into trunk Apr 9, 2024
23 checks passed
@renatho renatho deleted the change/setup-wizard-redirect branch April 9, 2024 14:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants