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

Workaround to use get_date_from_gmt to format into 'U' #3069

Merged

Conversation

mircobabini
Copy link
Member

All Submissions:

Changes proposed in this Pull Request:

Fix the next payment date TIME (and the cancel date TIME if using CONPD in core) to consider the timezone.
When usign get_date_from_gmt with format = 'U', the result does not consider the timezone.
Then, a solution is to call that function twice. One to add the timezone, then to format the datetime.

How to test the changes in this Pull Request:

  1. Create a new subscription
  2. Check the TIME of the next payment/cancellation

It will be a cycle ahead (MINUS or PLUS the timezone).

Other information:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you successfully run tests with your changes locally?

Copy link
Member

@dparker1005 dparker1005 left a comment

Choose a reason for hiding this comment

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

Overall, looks good to me.

I hesitated a bit when initially reviewing this PR since 'U' is, by definition, the seconds since the Unix Epoch (January 1 1970 00:00:00 GMT). Altering the date to a "local timestamp" like this PR breaks that definition.

With that being said, creating a "local timestamp" like this is something that we already do elsewhere in PMPro, so this change would bring the PMPro Subscriptions class more in line with other PMPro code.

I think we should merge this change and, down the line, consider setting stronger standards about how we handle timestamps in PMPro.

@dparker1005 dparker1005 merged commit c36558e into strangerstudios:dev Jul 29, 2024
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.

None yet

2 participants