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

Update system status info about WC Admin. #25830

Merged
merged 3 commits into from
Mar 5, 2020

Conversation

peterfabian
Copy link
Contributor

@peterfabian peterfabian commented Mar 5, 2020

The check now also tests whether wc admin is active to prevent notice about undefined constant when running on WP 5.2 and older, or with inactive WC Admin.

All Submissions:

Changes proposed in this Pull Request:

Closes # .

How to test the changes in this Pull Request:

  1. Get a testing machine with WP 5.2

  2. Install WC 4.0-rc.1

  3. Go to system status and observe the error
    Screenshot 2020-03-05 at 15 48 03

  4. apply this branch

  5. check again, it should show the WC Admin is inactive:
    Screenshot 2020-03-05 at 17 39 30

  6. Update to WP 5.3

  7. Check that WC Admin becomes active:
    Screenshot 2020-03-05 at 17 39 16

  8. Try installing & activating the WC Admin plugin, check that the version and path are correctly pointing to the plugin:
    Screenshot 2020-03-05 at 17 40 04

  9. Deactivate the plugin, it should, again, show the included package info.

  10. Use a snippet to deactivate WC Admin package (add_filter( 'woocommerce_admin_disabled', '__return_true' ); to wc-core-functions.php)

  11. It should show Inactive package in the system status report.

Other information:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes, as applicable?
  • Have you successfully run tests with your changes locally?

Changelog entry

Fix - System status report should correctly identify inactive package.

The check now also tests whether wc admin is active to prevent notice about undefined constant when running on WP 5.2 and older, or with inactive WC Admin.
@peterfabian peterfabian added this to the 4.0.0 milestone Mar 5, 2020
@peterfabian
Copy link
Contributor Author

cc @rrennick to check the logic for WC Admin.

@rodrigoprimo rodrigoprimo self-requested a review March 5, 2020 17:19
Copy link
Contributor

@rodrigoprimo rodrigoprimo left a comment

Choose a reason for hiding this comment

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

Looks good to me and I could confirm that it fixes the warning message. I left a comment with a nitpick suggestion.

Besides that, related to the part of the code that is changed in this PR but not directly related to this PR, I found a bit confusing that when WooCommerce Admin is inactive we show the string "Inactive" together with the version number and package path in green. It took me a while to realize what was the difference between when the plugin is inactive or active. I imagine that changing the font color to red is too drastic, but maybe we could change it to black and display a X when the plugin is inactive? That is what we do, for example, when there is no child theme installed. I'm not sure if we want to address this in this release cycle.

$path = \Automattic\WooCommerce\Admin\Composer\Package::get_path(); // phpcs:ignore WordPress.WP.GlobalVariablesOverride.Prohibited
} elseif ( defined( 'WC_ADMIN_VERSION_NUMBER' ) ) {
// To prevent $path from previous package to leak into the conditions in this section.
$path = null; // phpcs:ignore WordPress.WP.GlobalVariablesOverride.Prohibited
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a nitpick, but what if instead of using this generic name $path, we use a more specific name like $wc_admin_path. This way, we don't need to worry about other places that are using the same variable name, and also, we don't need to add a phpcs:ignore comment whenever we use this variable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, wanted to do that already, but then wasn't sure if I should rename the vars in other sections. Let's update at least this one.

@rrennick
Copy link
Contributor

rrennick commented Mar 5, 2020

to check the logic for WC Admin.

@peterfabian Looks good.

@peterfabian
Copy link
Contributor Author

I found a bit confusing that when WooCommerce Admin is inactive we show the string "Inactive" together with the version number and package path in green. It took me a while to realize what was the difference between when the plugin is inactive or active. I imagine that changing the font color to red is too drastic, but maybe we could change it to black and display a X when the plugin is inactive? That is what we do, for example, when there is no child theme installed.

I was wondering about changing it, but haven't noticed we have this neutral option available. Updated to the below in a9e262c:
Screenshot 2020-03-05 at 19 18 09

@rodrigoprimo rodrigoprimo self-requested a review March 5, 2020 18:30
Copy link
Contributor

@rodrigoprimo rodrigoprimo left a comment

Choose a reason for hiding this comment

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

Thanks for the changes.

@rodrigoprimo rodrigoprimo merged commit 5ef1378 into master Mar 5, 2020
@rodrigoprimo rodrigoprimo deleted the fix/wc-admin-system-status-logic branch March 5, 2020 18:31
@woocommercebot woocommercebot added release: add changelog Mark all PRs that have not had their changelog entries added. [auto] release: add testing instructions PRs that have not had testing instructions added to the wiki. [auto] labels Mar 5, 2020
@rodrigoprimo rodrigoprimo removed the release: add testing instructions PRs that have not had testing instructions added to the wiki. [auto] label Mar 5, 2020
@peterfabian peterfabian added cherry picked and removed release: add changelog Mark all PRs that have not had their changelog entries added. [auto] labels Mar 6, 2020
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.

4 participants