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.

Sign up for GitHub

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

Calculate last_payment for imported subscriptions #146

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Calculate last_payment for imported subscriptions #146

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
cb9556f
Calculate last_payment for imported subscriptions
duttonw Oct 9, 2016
858f0ec
correct whitespace removal
duttonw Oct 9, 2016
c4f145f
#146 code review corrections, whitespace change and clean up
duttonw Oct 11, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions wcs-importer-exporter.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,9 @@ public static function init() {
*/
public static function setup_importer() {

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

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good to know, have removed.

add_filter('woocommerce_subscription_get_' . 'last_payment' . '_date', array(WCS_Importer_Exporter::class, 'last_payment_calculation'), 10, 3);
Copy link
Contributor

@thenbrent thenbrent Oct 10, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

}
if ( is_admin() ) {
if ( class_exists( 'WC_Subscriptions' ) && version_compare( WC_Subscriptions::$version, '2.0', '>=' ) ) {
self::$wcs_exporter = new WCS_Export_Admin();
Expand All @@ -60,11 +63,32 @@ public static function setup_importer() {
}
}

/**
* This is to calculate last_payment when the last payment not set by import
* this is done by taking next interval and removing twice.
* Only change from 0 if it was an imported subscription
*
* @param $date
* @param $subscription @class WC_Subscription
* @param $timezone
* @return $date or date of last payment calulation
*/
public static function last_payment_calculation($date, $subscription, $timezone ) {
if ($date == 0 && "importer" === $subscription->__get("created_via")) {
Copy link
Contributor

Choose a reason for hiding this comment

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

$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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

$next_payment = strtotime($subscription->get_date('next_payment'));
$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);
}
return $date;
Copy link
Contributor

Choose a reason for hiding this comment

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

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 )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

}

/**
* Include Docs & Settings links on the Plugins administration screen
*
* @since 1.0
* @param mixed $links
* @return array
*/
public static function action_links( $links ) {

Expand Down