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

WooCommerce API Manager migration #7

Open
wants to merge 26 commits into
base: feature/woocommerce
Choose a base branch
from

Conversation

shramee
Copy link

@shramee shramee commented Nov 11, 2016

@shramee
Copy link
Author

shramee commented Nov 11, 2016

Tested with WCAM v1.4.6.3


return $billing_cycle;
private function get_billing_cycle( $id ) {
return 'annual';
Copy link
Contributor

Choose a reason for hiding this comment

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

@shramee does WooCommerce only allow annual billing cycles? Or is that only the case for your store?

Copy link
Author

Choose a reason for hiding this comment

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

Well WooCommerce/WCAM doesn't support billing cycles... only one time payment AFAIK. I think WC Subscriptions plugin allows that...

"url" => $url,
"plugin_version" => '2.5.0',
"is_premium" => true,
"site_uid" => 'MoouenVlGG45',
Copy link
Contributor

Choose a reason for hiding this comment

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

@shramee you need to use $this->get_anonymous_id( $url ), the site_uid must be a unique 32 chars identifier.

"charset" => 'UTF-8',
"platform_version" => '4.6.1',
"php_version" => '7.0.0',
"module_id" => 'Storefront Pro',
Copy link
Contributor

Choose a reason for hiding this comment

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

@shramee the module_id have to be the product ID, not the title.

"platform_version" => '4.6.1',
"php_version" => '7.0.0',
"module_id" => 'Storefront Pro',
"email" => $email,
Copy link
Contributor

Choose a reason for hiding this comment

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

@shramee there's no point adding new params since maybe_process_api_request() is part of the abstract class and it doesn't expect that argument, and won't do anything with it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I do see that you added some logic that checks email in validate_params() which is fine. But I don't see that you send the email as part of the migration request in my_edd_activate_license() at samples/module-migration.php.

* @param string $name Property to return
* @return mixed|null Value if property exists else null
*/
public function __get( $name ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@shramee why do you need external "magic" access? It's not a good practice in general.

Copy link
Author

Choose a reason for hiding this comment

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

Well, that doesn't expose private values... Just order (protected property) and requests parameters...

Copy link
Contributor

Choose a reason for hiding this comment

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

In general, if you want to expose specific params, the best practice is adding the relevant getters. In the current context, if you read my comment about the dependency of FS_WC_Migration and FS_WC_Migration_Endpoint (that should be removed), I think you can get rid of this code at all.

0,
32
);
return $this->ep->site_uid;
Copy link
Contributor

Choose a reason for hiding this comment

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

@shramee site_uid and install_id are NOT the same things. Please generate the install_id based on a similar structure I created the EDD insatll_id.

protected static $_edd_sl;

/** @var FS_WC_Migration_Endpoint Instance */
protected $ep;
Copy link
Contributor

Choose a reason for hiding this comment

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

@shramee FS_WC_Migration and FS_WC_Migration_Endpoint were intentionally designed to be isolated classes. Based on reviewing your code, you are using $ep to access the order information. I recommend removing this dependency and just passing the minimal arguments that you need to access the order data (based on what I've seen it's the customer email and license key).

$purchase['payment_method'] = $this->get_local_purchase_gateway();
$purchase['customer_external_id'] = 'edd_customer_' . $this->_edd_customer->id;
$purchase['license_key'] = self::$_edd_sl->get_license_key( $this->_edd_license->ID ); // Preserve the same keys.
$purchase['payment_method'] = $this->_wc_order->payment_method;
Copy link
Contributor

Choose a reason for hiding this comment

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

@shramee what are the optional values of $this->_wc_order->payment_method? Because FS only accepts paypal and cc (for credit cards).


// Set license expiration if not a lifetime license via a purchase.
$license_expiration = $this->get_local_license_expiration();
if ( null !== $license_expiration ) {
$purchase['license_expires_at'] = $license_expiration;
}

if ( $this->local_is_sandbox_purchase() ) {
$purchase['is_sandbox'] = true;
if ( ! $purchase['payment_method'] ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@shramee in what scenario $this->_wc_order->payment_method can be empty?

Copy link
Author

Choose a reason for hiding this comment

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

When payment is not made (100% coupon)...

I used 100% coupon and FS api threw an error saying it can't be empty...

# Conflicts:
#	includes/entities/class-fs-entity-map.php
#	includes/migration/edd/class-fs-edd-download-migration.php
#	samples/module-migration.php
and…
`get_local_license_id` now uses download `permission_id`
and fixing `FS_WC_Migration_Endpoint::get_local_paid_plan_id()`
should be either `paypal` or `cc`
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