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

CRM-21157 Convert civicrm_subscription_history.date column to be time… #10954

Merged
merged 2 commits into from
Sep 10, 2017

Conversation

seamuslee001
Copy link
Contributor

…stamp rather than datetime to help with timezone issues

Overview

DateTime mysql columns do not handle timezones well, so lets convert this column for new installs so that timezone issues aren't a problem for group subscription history information

Comments

@eileenmcnaughton @totten

@eileenmcnaughton
Copy link
Contributor

@totten is this enough to trigger your new warning system?

@seamuslee001
Copy link
Contributor Author

@eileenmcnaughton needed to manually add it which i have done now

@totten
Copy link
Member

totten commented Sep 7, 2017

Good idea.

Regarding "System Status" and "Doctor When", those changes should be enough to have the prod the admin to change the data-type. However, I don't think Doctor When will update the default. It probably needs a patch around here and here.

For general QA... ideally, what we'd do is identify the use cases where SubscriptionHistory is create/updated and try them (for old/new sites). After grepping for SubscriptionHistory and civicrm_subscription_history, these items in particular stick out:

  • CRM_Contact_BAO_SubscriptionHistory::create() seems to be used most of the time
  • CRM_Contact_BAO_GroupContact::bulkAddContactsToGroup()
  • sql/GenerateData.php

IMHO, we don't need unit-tests for these things, but someone should give a quick/manual test with use-cases involving them.

@eileenmcnaughton
Copy link
Contributor

I tested this on a local install creating a clean build before & after applying this patch. I tested adding an individual contact to a group from their group tab and bulk add & removing contacts from a search & made the following observations

  1. test coverage of the function is quite good (numerous tests pass through it) but I couldn't find a specific test that checks the subscription_history & the fact no api exists for it is probably evidence no-one has done one (and the reason I stopped short of adding one this time as it was a bit more than I wanted to do in this review)
  2. before the patch the dates were inaccurate on my local, incorrect by 10 hours. After they were correct
  3. when doing a bulk remove from a group remove subscription history records are created even if the contact was not in the group to start with.

Conclusion - this does what it says & is an improvement

@eileenmcnaughton eileenmcnaughton merged commit b448aa6 into civicrm:master Sep 10, 2017
@eileenmcnaughton eileenmcnaughton deleted the CRM-21157 branch September 10, 2017 22:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants