Skip to content

Conversation

@ayushpahwa
Copy link
Contributor

@ayushpahwa ayushpahwa commented Aug 21, 2025

Description

Add the plugin version of WC shipping to the API call made to connect server. This will inform the connect server if WC shipping is also active along with the current plugin.

Related issue(s)

Fixes #WOOSHIP-1511

Steps to reproduce & screenshots/GIFs

  1. Checkout locally and ensure both WC shipping and services are installed and activated.
  2. Ensure connect server is running locally and it's logs are accessible.
  3. Add this console statement in any request handler. For eg: in lib/services/index.js in the function validationHandler, add console.log("Request: ", JSON.stringify(request.payload, null, 2));
  4. Now navigate to woocommerce>orders page and notice the logs. It should show values for both wcs_version and wcshipping_version in the settings object.
  5. Now go to Installed plugins and deactivate WC Shipping. Navigate back to woocommerce>orders page and notice that only value for both wcs_version is valid and wcshipping_version will be coming as undefined.

PS: ensure to test the similar PR in WC shipping for effective review.

Checklist

  • unit tests
  • changelog.txt entry added
  • readme.txt entry added

@ayushpahwa ayushpahwa changed the title chore: /wooship 1511/add active wc ship version []0hchore: /wooship 1511/add active wc ship version Aug 21, 2025
@ayushpahwa ayushpahwa changed the title []0hchore: /wooship 1511/add active wc ship version [WOOSHIP-1511] chore: add active WC shipping plugin version to CS API calls Aug 21, 2025
@ayushpahwa ayushpahwa marked this pull request as ready for review August 21, 2025 18:15
@ayushpahwa ayushpahwa requested review from a team, Ferdev, samnajian and tpaksu and removed request for a team August 21, 2025 18:15
@ayushpahwa ayushpahwa self-assigned this Aug 21, 2025
'dimension_unit' => strtolower( get_option( 'woocommerce_dimension_unit' ) ),
'weight_unit' => strtolower( get_option( 'woocommerce_weight_unit' ) ),
'wcs_version' => WC_Connect_Loader::get_wcs_version(),
'wcshipping_version' => WC_Connect_Loader::get_wc_shipping_version(),
Copy link
Contributor

@samnajian samnajian Aug 22, 2025

Choose a reason for hiding this comment

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

I guess it's worth checking how currently wcshipping_version is used on connect-server side, I guess in some cases it's used to determine if the request is coming from WC Shipping or not.
Maybe simply passing active_plugins => get_option( 'active_plugins' ) or get_plugins(with a plugin's folder as argument) would be a safer alternative.

Copy link
Contributor Author

@ayushpahwa ayushpahwa Aug 22, 2025

Choose a reason for hiding this comment

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

I would actually argue to remove this ambiguous way. We should send the plugin_source key along with the API and if it is defined, that's the one we should be using. What do you think?
CleanShot 2025-08-22 at 20 40 13@2x

Copy link
Contributor

Choose a reason for hiding this comment

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

We'd anyways need the version as well, wcshipping_version currently has 2 use cases:

  • The actual version number of the plugin sending the request.
  • The existence of wcshipping_version, implies request is coming from wcshipping_version
    While the above is not perfect, we shouldn't refactor it at this stage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The idea was not to remove the wcshipping_version but to add a better plugin source check. I worked on a draft over the weekend for this but it needs a lot of test fixes. I have pushed it here and will fix in the cooldown week. Till then I guess both of these PRs can be parked.

@ayushpahwa ayushpahwa marked this pull request as draft August 25, 2025 13:22
@samnajian
Copy link
Contributor

@ayushpahwa do you want to keep this PR or should we close it?

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.

3 participants