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

Convert civicrm_note.modified_date to timestamp #16338

Merged
merged 1 commit into from
Feb 24, 2020

Conversation

mattwire
Copy link
Contributor

Overview

civicrm_note.modified_date is currently a "date" format and does not store the time. This is problematic because sorting in the UI by the default "Modified Date" doesn't necessarily put notes in the right order.

Before

civicrm_note.modified_date does not store time.

After

civicrm_note.modified_date stores date and time.

Technical Details

As the default for a timestamp field is "now" we don't need to pass in a default to the BAO.

Comments

@civibot
Copy link

civibot bot commented Jan 19, 2020

(Standard links)

@civibot civibot bot added the master label Jan 19, 2020
@eileenmcnaughton
Copy link
Contributor

Test fail relates.

I should note I can't think of a situation where this would cause a problem but I'd like to see a couple more people weigh on on the concept-approved side of this

@mattwire
Copy link
Contributor Author

@colemanw This seems to be failing on the API4 test - any ideas why? With API3 removing the modified_date parameter means it uses the database default which is current_timestamp() but it looks like api4 is not doing that here?

@simonjohnparker
Copy link

Hi - this appears to be working correctly on our CiviCRM instance.

Thanks,

@mattwire mattwire added the merge ready PR will be merged after a few days if there are no objections label Feb 20, 2020
@irritatie
Copy link

We tested this in the latest CiviCRM release and found no issues. Also did an automated full test of basic CiviCRM functionality and everything worked fine. Thanks!

@eileenmcnaughton
Copy link
Contributor

Code & test look OK - merging based on described testing

@eileenmcnaughton eileenmcnaughton merged commit bc19d24 into civicrm:master Feb 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
master merge ready PR will be merged after a few days if there are no objections
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants