Calculate last_payment for imported subscriptions #146

duttonw · 2016-10-09T06:53:08Z

A workaround and viable fix for issue #142
added a filter to make up a date when the date return from the parent
function is 0 and the subscription was imported. This will then allow
switch subscriptions to work.

Note: this may cause issues with free trials where no payment has been
paid, testing is required to ensure it does not cause interference on
those imported subscriptions.

A workaround and viable fix for issue woocommerce#142 added a filter to make up a date when the date return from the parent function is 0 and the subscription was imported. This will then allow switch subscriptions to work. Note: this may cause issues with free trials where no payment has been paid, testing is required to ensure it does not cause interference on those imported subscriptions.

mattallan · 2016-10-10T10:35:00Z

Thanks @duttonw, I wanted to try avoid a change like this mainly because it requires you to keep the importer installed and activated so that upgrades/downgrades to properly process.
I'm thinking a lot of store managers will most likely remove the importer/exporter once they're done with it, hence breaking switching.

If this works for you solution, feel free to pull it out into a separate plugin while we work on another workaround.

duttonw · 2016-10-10T10:49:15Z

Hi @mattallan,

Yep, i've put together a workaround branch for this site i'm releasing which will tide me over.

This only needs to be installed until all subscriptions have renewed. Also removing the plugin does not really break the site, just gracefully reduces the functionality. {another note like the paypal gotchas that this needs to stay installed for upgrade/downgrade to work the first time before renewals come up}

On the pull request i have added a comment that could be a speed improvement (reduce db calls on long running subscriptions) on the main plugin by storing the last_payment on the subscription (with backward compatibility to look up orders if needed) (could also be a tools option to recalculate all last payments and next payment dates on subscriptions if woocommerce subscriptions is run on shared hosting and the php thread crashes)

thenbrent

Thanks for the PR @duttonw. I've given it a quick review. I've left comments about a number of issues that would need to be addressed before we could merge it. It's also important all the code be updated to conform to the WordPress Coding Standards. At the moment, the whitespace and variables names are often contradicting those standards.

Similar to @mattallan, I've got reservations about including this in the Importer vs. in Subscriptions itself. I'm not 100% sure of the best approach for this just yet. You can safely use this code on your site in a custom plugin or elsewhere (at least once you've addressed the timezone issue I mentioned), but I'm not 100% sure this is the approach we'll take to address the issue, so maybe hold off fixing up the issues until @mattallan had has a chance to think through the best approach more fully.

thenbrent · 2016-10-10T17:00:10Z

wcs-importer-exporter.php

 	 */
 	public static function setup_importer() {

+		if ( class_exists( 'WC_Subscriptions' ) && version_compare( WC_Subscriptions::$version, '2.0', '>=' ) ) {


This check is unnecessary. The filter will only be called with Subscriptions 2.0, and when Subscriptions is active, so it's safe to just add the filter.

good to know, have removed.

thenbrent · 2016-10-10T17:02:19Z

wcs-importer-exporter.php

 	public static function setup_importer() {

+		if ( class_exists( 'WC_Subscriptions' ) && version_compare( WC_Subscriptions::$version, '2.0', '>=' ) ) {
+			add_filter('woocommerce_subscription_get_' . 'last_payment' . '_date', array(WCS_Importer_Exporter::class, 'last_payment_calculation'), 10, 3);


The filter name here would be changed 'woocommerce_subscription_get_last_payment_date'. Using 'woocommerce_subscription_get_' . 'last_payment' . '_date' is identical to 'woocommerce_subscription_get_last_payment_date', only the former unnecessarily adds string concatenation. That would only be necessary if 'last_payment' was a variable or some dynamic value. When it's static, 'woocommerce_subscription_get_last_payment_date' is identical and a more easily understood way of writing it.

That is true, but the number of times i've come across a theme which extended part of woocommerce but was not able to find the do_action/filter it was adding onto is many as the dynamic breakdown stops easy searching and then i need to work out where to place the wild card's. I've corrected this to your suggested wording as they should know how to find this if they have come this far.

thenbrent · 2016-10-10T17:03:34Z

wcs-importer-exporter.php

 	public static function setup_importer() {

+		if ( class_exists( 'WC_Subscriptions' ) && version_compare( WC_Subscriptions::$version, '2.0', '>=' ) ) {
+			add_filter('woocommerce_subscription_get_' . 'last_payment' . '_date', array(WCS_Importer_Exporter::class, 'last_payment_calculation'), 10, 3);


Instead of WCS_Importer_Exporter::class here, the __CLASS__ constant should be used to conform to the rest of the codebase.

doh, this was used elsewhere in the class so it's a silly mistake, am too use to using non static classes for adding to filters with class

thenbrent · 2016-10-10T17:05:14Z

wcs-importer-exporter.php

+	 * @param $timezone
+	 * @return $date or date of last payment calulation
+	 */
+	public static function last_payment_calculation($date, $subscription, $timezone ) {


$date == 0 here should be changed to 0 == $date so that it is a Yoda condition and conforms to the WP coding standards: https://make.wordpress.org/core/handbook/best-practices/coding-standards/php/#yoda-conditions

another silly mistake as i did the right thing on the next check....

thenbrent · 2016-10-10T17:07:21Z

wcs-importer-exporter.php

+	 * @param $timezone
+	 * @return $date or date of last payment calulation
+	 */
+	public static function last_payment_calculation($date, $subscription, $timezone ) {


The __get() magic method shouldn't be called directly on $subscription, that defeats the purpose of the magic method. Instead, just use $subscription->created_via and the magic method will work its magic in the background. :)

too use to java with having fully named getters and setters and an ide that can work it out for me. phpstorm let me down on this one :$.

if anyone stumbles upon this (including future me) and wondering about magic __get usage, it is http://php.net/manual/en/language.oop5.overloading.php#object.get

thenbrent · 2016-10-10T17:11:18Z

wcs-importer-exporter.php

+			$next_interval = wcs_add_time( $subscription->billing_interval, $subscription->billing_period, $next_payment );
+			$last_payment_cal = $next_payment - ($next_interval - $next_payment);
+			return  gmdate( 'Y-m-d H:i:s',$last_payment_cal);
+		}


Because the $timezone is being ignored here, if someone is requesting $subscription->get_date( 'last_payment', 'site' ), this will return an incorrect date. It needs to also check 'gmt' != strtolower( $timezone ) and when that's the case, it should change the $last_payment_cal value to be in the site's timezone using something like:

$last_payment_cal = get_date_from_gmt( $last_payment_cal )

corrected, the downgrade/upgrade looks at gmt time it seems, but its good to handle both, also updated documentation to match from other locations in subscriptions/woocommerce.

duttonw · 2016-10-11T10:51:02Z

@thenbrent thankyou for taking the time to code review this, i'll try and improve it for you, some of the things i've seen before other i have not. I put this together in about 1-2 hours.

seems phpstorm now includes the wordpress standard so i'll see how well that bring things into line. https://gist.github.com/Rarst/1370155

i've come from a java background and many of these tricks in php are very clever and wished phpstorm would give it in the drop down when i do that ->(check) all i get for the magic __get is can't find declaration.

duttonw added 2 commits October 9, 2016 17:44

correct whitespace removal

858f0ec

thenbrent suggested changes Oct 10, 2016

View reviewed changes

woocommerce#146 code review corrections, whitespace change and clean up

c4f145f

Calculate last_payment for imported subscriptions #146

Are you sure you want to change the base?

Calculate last_payment for imported subscriptions #146

Uh oh!

Conversation

duttonw commented Oct 9, 2016

Uh oh!

mattallan commented Oct 10, 2016

Uh oh!

duttonw commented Oct 10, 2016

Uh oh!

thenbrent left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thenbrent Oct 10, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

duttonw commented Oct 11, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

thenbrent Oct 10, 2016 •

edited

Loading