strangerstudios / pmpro-series

This is the "drip feed content" module for Paid Memberships Pro.
https://www.paidmembershipspro.com/add-ons/pmpro-series-for-drip-feed-content/
25 stars 25 forks source link

pmpro_getMemberDays() UTC vs local TZ issue? #19

Closed eighty20results closed 4 years ago

eighty20results commented 10 years ago

Been debugging a problem in my Series(like) plugin where I have a member with a $wpdb->pmpro_memberships_users.startdate - '2014-02-03 13:00:00'. When running pmpro_getMemberDays() for that user at 16:30:00 (cron job), the value returned is a float (184.927500).

From my perspective (possibly incorrect, I admit), once the day-count is >184.0, we're technically on day 185. and as a result, the calculation in paid-memberships-pro/includes/function.php, on line 1530 is incorrect. At present, it reads as;

$days = ($now - $startdate)/3600/24;

But it would be more "correct" (I think) to have it defined as;

$days = round(abs($now - startdate) / (60*60*24)); In my limited testing, this will (always?) result in the correct day being returned.

strangerstudios commented 10 years ago

Hi there. I did some thinking about this, but am open to changes if it won't affect anything.

I'm not sure the function benefits by building the rounding into it. You are free to wrap the result of pmpro_getMemberDays in a round function when using it.

For some uses, I think people would like to know more specifically the partial days... especially the different between 0 days or false and 0.1 days. Some people might want to round up, other might want to round down, etc.

eighty20results commented 10 years ago

The more I think about it, the less I agree that a fraction of a day for a "days since X" calculation has any real meaning.

We simply do not function that way. The moment we wake up, we consider it "day X of 365" (or 366 during leap years). Technically, as you point out, that isn't correct since if we wake up at 6 am, it's really day (X-1) and 1/4 of the year and not (what we'll state), day X of the year. But we insist on using the whole day regardless.

No billing system I've ever been involved in has worked that way (using fractions of a day). Interest rates are all calculated based on whole days. Even the Fed uses a point in time as the (hard) limit for when the day rate is applied to loans and the whole days worth of interest is applied (before the clock strikes, it's "not yet today" and thus the loan is interest free). So I'm not seeing a single use case where a decimal value for a calculation of "days since start" has usefulness. What am I missing?

strangerstudios commented 10 years ago

There are a couple things that are making me lean toward keeping the result here decimal.

  1. That's the way it is now, and I don't want to change it unless we have to to avoid anyone who might be expecting a decimal.
  2. Being more precise here allows people using the function to figure out how they want to work with the result.

FWIW, the function isn't used in the core PMPro plugin. It's used in our pmpro-series addon and I think possibly in some of our code snippets.

The fact that the function is called getMemberDays, makes me want to actually return the number of days, which as you point out is usually expected to be a whole number. And we should at least wrap the line here to round the result:

https://github.com/strangerstudios/pmpro-series/blob/master/pmpro-series.php#L107

And here is where it checks that result to see if the user's account is old enough: https://github.com/strangerstudios/pmpro-series/blob/master/pmpro-series.php#L150

I think outside of PMPro Series (which we control here) there wouldn't be too much impact to changing the behavior of this function. I think we can add a "round" parameter that rounds down by default and can be adjust to round up or return a decimal answer. (http://php.net/manual/en/function.round.php)

Here is what that would look like:

` function pmpro_getMemberDays($user_id = NULL, $level_id = 0, $round = "down") { if(empty($user_id)) { global $current_user; $user_id = $current_user->ID; }

global $pmpro_member_days;
if(empty($pmpro_member_days[$user_id][$level_id]))
{       
    $startdate = pmpro_getMemberStartdate($user_id, $level_id);

    //check that there was a startdate at all
    if(empty($startdate))
        $pmpro_member_days[$user_id][$level_id] = 0;
    else
    {           
        $now = current_time('timestamp');
        $days = ($now - $startdate)/3600/24;

        $pmpro_member_days[$user_id][$level_id] = $days;
    }
}

if($round == "down" || $round == "floor")
    return floor($pmpro_member_days[$user_id][$level_id]);
elseif($round == "up" || $round == "ceil")
    return ceil($pmpro_member_days[$user_id][$level_id]);
elseif($round == "halfup")
    return round($pmpro_member_days[$user_id][$level_id], 0, PHP_ROUND_HALF_UP);
elseif($round == "halfdown")
    return round($pmpro_member_days[$user_id][$level_id], 0, PHP_ROUND_HALF_DOWN);
else
    return $pmpro_member_days[$user_id][$level_id];

} `

We would I think want to edit PMPro Series to use this function rounding up when showing "what day of the membership" someone is on and use the function with $round NULL or down when comparing.

Thoughts?

eighty20results commented 10 years ago

That option certainly leaves the flexibility in the consumer's hands. Though I think the default probably should be NULL for backwards compatibility?

function pmpro_getMemberDays($user_id = NULL, $level_id = 0, $round = null)

dparker1005 commented 4 years ago

You can customize exactly how long it will take for posts to become available by using the pmpro_member_startdate filter. I have just put together a code recipe that demonstrates a couple of strategies for changing when posts will unlock, including how to change the unlock time: https://gist.github.com/dparker1005/d88e422c576587c473c9bb42b1bb8817

I have also added a commit to ensure that the timezone set in SQL/on the server does not affect the startdate timestamp retrieved by PMPro: https://github.com/strangerstudios/pmpro-series/commit/4adf08aea5a004b3311a58b6b3c41e6979cfe4ba