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-21079, CRM-9683 - Display advisories about DATETIME/TIMESTAMP columns #10874

Merged
merged 2 commits into from
Aug 29, 2017

Conversation

totten
Copy link
Member

@totten totten commented Aug 18, 2017

Overview

Some database columns were originally flagged as DATETIME when TIMESTAMP
would behave more appropriately. CRM-9683 changed the default type of
several CiviMail columns but did not provide an upgrade path for existing sites.

Before

Existing sites may be blissfully unaware of their problematic CiviMail
schema.

After

If a site has outdated CiviMail schema, then the system-status will display
an alert about "Timestamp Schema".

screen shot 2017-08-28 at 6 11 15 pm

Comments

Old Screenshot (A)

The screenshot is preserved for historical purposes. It's no longer current with the PR.

screen shot 2017-07-31 at 8 45 53 pm


@jmcclelland
Copy link
Contributor

Beautiful! I love it.

@mfb
Copy link
Contributor

mfb commented Aug 18, 2017

anyone know if MySQL plans to fix the year 2038 problem for TIMESTAMP anytime soon?

@davejenx
Copy link
Contributor

Thanks @totten for the detailed description and proposal. I think it looks good in general. Would be good to work out how best to present this to end users, who mostly won't understand the intricacies of timezone handling.

Timezones for Activities and Cases
A non-technical user reading the above message may well think, "sounds fine, why didn't you just do it; if it should be done, what am I supposed to do?"
Is the reason for not automatically back-filling these two timestamps during the upgrade, that this could take too long on large installations?
If there's a good reason for not doing this automatically, it would be good to guide the user towards the appropriate action. If the reason is duration, we could explain that, provide a Fix It link and advise doing this at a quiet time. If there's a judgement call to be made about whether to do it, it's good to minimise the number of cases where the user is left perplexed. So if there are scenarios where it's pretty clear-cut one way or the other, then describe those, so we're in effect giving a flowchart where most users end up at a clear terminus (Fix It link or Hide) and few are left in need of external support.

Timestamp Schema

The message shown in the screenshot above will make little sense to non-technical end users and few will know what they ought to do, so this is likely to generate a lot of support requests, for an issue that will make little difference to the majority. Again, a flowchart-like approach seems sensible, minimising the number of cases where the user is left perplexed.

@eileenmcnaughton
Copy link
Contributor

Looking at the code only the Timestamp Schema portion is included in this update so we should focus on getting that right to get it merged. I feel like it might be enough to say that it is an optional update and sites with no issues do not need to take any action.

@totten
Copy link
Member Author

totten commented Aug 29, 2017

Yes, @eileenmcnaughton is correct - the PR only touches the "Timestamp Schema" section.

Is the reason for not automatically back-filling these two timestamps during the upgrade, that this could take too long on large installations?

@davejenx - ah, no. The reason is that we're worried because (a) time handling has multiple moving parts and (b) downstream folks have diverse configurations/customizations/use-cases/workflows. Of course, we believe most folks would be well-served by a conversion. (IIRC, Fuzion did this for all their customers years ago... but that has a selection-bias.) We're not in a position to provide strong, clear statements or flowcharts for every possible user.

Producing a clearer flow-chart sounds quite nice... we just don't have the means to do that a-priori. (If someone disagrees, please prove me wrong by writing one!) The compromise in this PR is to soft-pedal: the changes are optional, the tooling is add-on/experimental, and feedback/revisions are welcome.

I do appreciate the critique, esp. since my original prose was really thin, and we can do a bit better. Here's a revision:

screen shot 2017-08-28 at 6 11 15 pm

(Note: To improve beyond that... I don't think discussing principles will help me. We'd need concrete prose/revisions/alternatives -- eg in comments here or in follow-up PRs. There's a lot more background in the JIRA and DoctorWhen links.)

@eileenmcnaughton
Copy link
Contributor

I'm giving this merge on pass because I do think the points have been addressed & because I agree that it would be better to make further improvements as follow-up PRs

@eileenmcnaughton eileenmcnaughton merged commit 2e23ec1 into civicrm:master Aug 29, 2017
@totten totten deleted the master-ts-warn branch August 29, 2017 03:06
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.

6 participants