Skip to content

Conversation

@regevbr
Copy link
Contributor

@regevbr regevbr commented Jul 14, 2019

User timezone should also be evaluated from user details #18

@regevbr
Copy link
Contributor Author

regevbr commented Jul 14, 2019

@collinkrawll can you please review?

@collinkrawll
Copy link
Contributor

@regevbr This looks great! I should have time to test and merge it early this week. Thanks!

@collinkrawll collinkrawll self-assigned this Jul 14, 2019
@regevbr
Copy link
Contributor Author

regevbr commented Jul 14, 2019

@collinkrawll sure thing!
I'm already using my fork in production and it works well!

Maybe a better explanation about the "user timezone" in the README is in order as well? I thought that what I changed here was obvious but after I saw the emails were sent in wrong times, I investigated your code and realized my assumption was wrong.

@collinkrawll
Copy link
Contributor

collinkrawll commented Jul 14, 2019

@regevbr Feel free to add an update to the README.md to this PR or a separate PR as well if you have an idea for how to improve it. Thanks!

@regevbr
Copy link
Contributor Author

regevbr commented Jul 15, 2019

@collinkrawll I updated the readme and added a unit test and fixed the tests to run with mautic 2.15.1.

My change is a breaking change, so please consider releasing a major version for it.

Also, since mautic 2.15.1 is the latest stable version, can you please test against it and update the readme for the supported version? I tested and used this fix in 2.15.1 already at it works well for me.

@collinkrawll collinkrawll merged commit 6fd5308 into thirdset:master Jul 16, 2019
@collinkrawll collinkrawll added this to the 2.0.0 milestone Jul 16, 2019
@collinkrawll
Copy link
Contributor

@regevbr I just merged this and released your changes in v2.0.0 of the plugin.

A couple of notes:

  • It looks like the timezone property was added to the Contact/Lead in v2.6.1 of Mautic (see Mautic 82fe3286fa8f789f8da9a5e98c41a04658b3a328). I wrapped some of your code in if statements to keep things working on older versions of Mautic.
  • I only bumped the 'tested up to' version to 2.15.0 because I'm testing using Docker and it doesn't look like they've released an image for 2.15.1 yet.

Thanks again.

@regevbr
Copy link
Contributor Author

regevbr commented Jul 16, 2019

@collinkrawll thanks for the swift handling :-)

Yes I already looked at your code and makes sense - good thing you noticed!

As to the docker - I already have a pending PR there to upgrade it to 2.15.1 - mautic/docker-mautic#117 I will mention you there so you can get notified when they release it so you can test 2.15.1 as well if you desire.

@regevbr
Copy link
Contributor Author

regevbr commented Jul 16, 2019

@collinkrawll the new docker image is out, can you test it on 2.15.1?

@collinkrawll
Copy link
Contributor

@regevbr done. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants