Skip to content

Conversation

@Konamiman
Copy link
Contributor

@Konamiman Konamiman commented Sep 29, 2022

All Submissions:

Changes proposed in this Pull Request:

After the features engine is completed (#34727) it's time to actually do something with the gathered compatibility info. This pull request implements UI changes in two places:

The feature settings page

If there are one or more active plugins that haven't declared positive compatibility with a given feature, a warning notice will be displayed below the checkbox for the feature. Additionally, if the feature is disabled, the checkbox will be disabled too:

image

The legacy features ("Analytics" and "New navigation") are excluded from this check and don't display any warning: these features have been around for a long time and it doesn't make much sense to start bugging plugin developers about declaring compatibility at this point.

The plugins page

The "Manage incompatible plugins" link in the feature notice targets a special view of the plugins page, with ?plugin_status=incompatible_with_feature&feature_id=<feature_id>:

image

This new view includes all the plugins that haven't declared positive compatibility with the given feature, regardless of whether the plugin is currently active or not. The link to the view will be visible only if plugin_status=incompatible_with_feature is present in the URL.

We are only showing the link to the All view together with our new view, because the counters would be wrong anyway (we are filtering the list returned by the all_plugins hook); the counter for All is explicitly recalculated here.

Regardless of the current view, for plugins that haven't declared positive compatibility with at least one of the currently enabled features (again, excluding "Analytics" and "New navigation"), a warning will be shown:

image

Additionally, the Activate link for the plugin will be disabled.

Enabling/activating via code

It's still possible to activate plugins and enable features via code (wp cli tool) and that's on purpose: at least initially there must be a mechanism to bypass the UI constraints for the cases where the admins know what they are doing.

Additionally, the FeatureUtil class gets two new methods that disable the UI restrictions. So the admin can add this code to their site:

add_action('before_woocommerce_init', function() {
	\Automattic\WooCommerce\Utilities\FeaturesUtil::allow_activating_plugins_with_incompatible_features();
	\Automattic\WooCommerce\Utilities\FeaturesUtil::allow_enabling_features_with_incompatible_plugins();
});

Then the warning notices will still be displayed, but it will be possible to activate the plugins/enable the features from the respective UI pages.

Closes #34861.

How to test the changes in this Pull Request:

You may want to add or remove features during your testing. To do so, just manually modify the $features array that is initialized at tthe beginning of the constructor in the FeaturesController class.

Testting UI

You need to test that the features page (WooCommerce - Settings - Advanced - Features) and the plugins page work as expected by trying various combinations of enabling/disabling features and activating/deactivation plugins. Thinks to look for:

  • Features can't be enabled from the settings page if there's at least one active plugin that isn't compatible (but they can be disabled if they are already enabled).
  • Plugins can't be activated from the plugins page if there's at least one enabled feature that isn't compatible.
  • The "Manage incompatible plugins" link redirects to a special view in the plugins page with only two views, "All" and "Incompatible with..."
  • The special plugins view displays all the plugins that are incompatible with the feature, not only the active ones.
  • The warning text for plugins only mentions/counts enabled features, and the warning text for features only mentions/counts active plugins.
  • The text of the incompatibility warnings for both features and plugins renders appropriately for the cases of one, two and tthree or more incompatible items ("incompatible with X", "incompatible with X and Y", "incompatible with X, Y and N more")
  • The plugin information rows renders nicely with all of these combinations:
    • Compatible and incompatible plugins
    • With and without compatibility warning notice
    • With and without plugin update available notice

Testing code

  1. Add the snippet indicated above (in "Enabling/activating via code") and verify that the warnings are still displayed, but features can always be enabled and plugins can always be activated, even with incompatibilities.

  2. Verify that you can still enable and disable features via code, even features with incompatibilities, and even without the above snippet, e.g: wp option set woocommerce_feature_high_performance_order_storage_enabled yes

  3. Verify that you can still activate and deactivate plugins, even plugins with incompatibilities, and even without the above snippet, e.g: wp plugin activate woocommerce-bookings

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?
  • Have you created a changelog file for each project being changed, ie pnpm --filter=<project> run changelog add?

FOR PR REVIEWER ONLY:

  • I have reviewed that everything is sanitized/escaped appropriately for any SQL or XSS injection possibilities. I made sure Linting is not ignored or disabled.

The methods that return compatibility info now have an extra
'uncertain' part with information regarding plugins that are
WooCommerce-aware but haven't declared compatibility.
@Konamiman Konamiman self-assigned this Sep 29, 2022
@github-actions github-actions bot added the plugin: woocommerce Issues related to the WooCommerce Core plugin. label Sep 29, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Sep 29, 2022

Test Results Summary

Commit SHA: bd86907

Test 🧪Passed ✅Failed 🚨Broken 🚧Skipped ⏭️Unknown ❔Total 📊Duration ⏱️
API Tests18800201900m 43s
E2E Tests563140647m 2s

⚠️ Warning

Please address the following issues prior to merging this pull request:
  • FAILED/BROKEN TESTS. There were failed and/or broken API and E2E tests.
  • INCOMPLETE E2E TEST RUN. We have a total of 189 E2E tests, but only 64 were executed. Note that in CI, E2E tests automatically end when they encounter too many failures.

To view the full API test report, click here.
To view the full E2E test report, click here.
To view all test reports, visit the WooCommerce Test Reports Dashboard.

Includes a link to the list of incompatible plugins.
Plugins that are incompatible with at least one enabled feature
will show a warning in the WooCommerce plugins page, and will
have their "Activate" link disabled.
@Konamiman Konamiman force-pushed the add/feature-incompatible-plugins-management branch from 3b2fcd0 to faad9ee Compare October 5, 2022 15:12
  "All" and "Incompatible with feature X"

- Exclude the legacy Analytics features from the feature and plugins
  activation restrictions

- Allow disabling a feature from the settings pages if it's enabled
  and is incompatible with at least one plugin (it won't be possible
  to re-enable it once the settings page reloads)
(which uses \ instead of / as directory separator)
- Fix: the incompatible plugins count in the feature settings page
  now only counts active plugins.
@Konamiman Konamiman force-pushed the add/feature-incompatible-plugins-management branch from 84d12de to 5ed89cf Compare October 7, 2022 08:10
@github-actions github-actions bot added the release: highlight Issues that have a high user impact and need to be discussed/paid attention to. label Oct 7, 2022
…les"

- Add an extra parameter to FeaturesController::get_compatible_plugins_for_feature
  to retrieve all matching plugins or only active plugins.
@github-actions github-actions bot removed the release: highlight Issues that have a high user impact and need to be discussed/paid attention to. label Oct 7, 2022
@Konamiman Konamiman marked this pull request as ready for review October 7, 2022 15:05
@Konamiman Konamiman requested review from a team and jorgeatorres and removed request for a team October 7, 2022 15:05
Copy link
Member

@jorgeatorres jorgeatorres left a comment

Choose a reason for hiding this comment

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

Hey @Konamiman! 👋

Great job here 🥇. I left some comments but overall this works and looks great.

My main concern (with the code) has to do with the renaming of custom_order_tables to high_performance_order_storage. I added some notes about this directly as part of the review.

There is another thing we feel should be discussed, which didn't really fit into the code review per se:

  • Even though there's support for allowing enabling of features/plugins with incompatible features, there's really no UI for this at the moment. I might've missed whether this is planned or not, but it'd be very useful.
  • "Uncertain" compatibility shouldn't prevent activation of plugins, or at least not if the uncertain compatibility is with a legacy feature. For example, "Analytics" is enabled by default but then because all plugins have "uncertain" compatibility with it, it prevents them from being activated. This effectively means no one can activate current version of WC plugins even if they aren't using HPOS or any other experimental feature.

Please let me know what you think.

* @return string Printable plugin name, or the plugin id itself if printable name is not available.
*/
public function get_plugin_name( string $plugin_id ): string {
$plugin_data = $this->proxy->call_function( 'get_plugin_data', WP_PLUGIN_DIR . DIRECTORY_SEPARATOR . $plugin_id );
Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to also cache the result of this function call? get_plugin_data() doesn't seem to cache things on its own, so this might result in unnecessary I/O if the function is called a few times during a request.

);
} else {
/* translators: %1\$s, %2\$s = printable plugin names, %3\$d = plugins count */
$message = sprintf(
Copy link
Member

Choose a reason for hiding this comment

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

Also needs to be passed through _n().

Copy link
Contributor

Choose a reason for hiding this comment

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

This is probably not required, the messaging seems correct for both 1 or more cases.

Copy link
Member

Choose a reason for hiding this comment

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

That's true in English, but we don't necessarily know how pluralization works for other languages. This seems like a minor inconvenience though, so I'm not holding off merging for that.

@peterfabian
Copy link
Contributor

@Konamiman We discussed this with @beaulebens now and since HEs are not available during the rollout of 7.1 and if I understand correctly this paragraph

Regardless of the current view, for plugins that haven't declared positive compatibility with at least one of the currently enabled features (again, excluding "Analytics" and "New navigation"), a warning will be shown

...it means that the default plugins list will show the warnings. Can we disable those until later please (maybe we'll put them back in 7.2 or when a higher % of plugins are compatible)? So that we'll still have the extra view, but the default view won't be flooded by the notices for everyone?

@vedanshujain
Copy link
Contributor

Can we disable those until later please (maybe we'll put them back in 7.2 or when a higher % of plugins are compatible)?

Addressed in 5a3e897

Copy link
Member

@jorgeatorres jorgeatorres left a comment

Choose a reason for hiding this comment

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

LGTM.Thanks @Konamiman & @vedanshujain!

@jorgeatorres jorgeatorres merged commit edc1c6c into trunk Oct 12, 2022
@jorgeatorres jorgeatorres deleted the add/feature-incompatible-plugins-management branch October 12, 2022 14:08
@github-actions github-actions bot added this to the 7.1.0 milestone Oct 12, 2022
@github-actions
Copy link
Contributor

Hi @jorgeatorres, thanks for merging this pull request. Please take a look at these follow-up tasks you may need to perform:

  • Add the release: add testing instructions label

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

plugin: woocommerce Issues related to the WooCommerce Core plugin.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Management of plugins (in)compatibility with features

5 participants