dev/core#2243 Add created_date column to the civicrm_note table

[jitendrapurohit]

Overview

Add created_date column to the civicrm_note table

https://lab.civicrm.org/dev/core/-/issues/2243

Before

No date field to know when the note was added. Also, there's no UI field to control/update the date manually.

After

Date field added.

Is available on view -

Comments

API test added.

[civibot]

(Standard links)

• If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[seamuslee001]

@jitendrapurohit can you fix the style issues https://test.civicrm.org/job/CiviCRM-Core-PR/39602/checkstyleResult/new/

[sunilpawar]

@jitendrapurohit Tested the created date functionality, its working fine.

[mattwire]

@jitendrapurohit Looks like test fail related? CRM_Dedupe_BAO_RuleGroupTest::testSupportedFields()

[jitendrapurohit]

Yep. Test build is passing now 👍

[eileenmcnaughton]

@jitendrapurohit I agree with adding this field - but we have a pretty strong convention that the field created_date is a DB populated field normally and it it would normally hold the timestamp - I just added a note and having chosen not to enter the field it winds up empty - which is really against our convention.

I guess it's OK to expose if for override but I don't think it should be OK for it to be blank for new entries (historical ones will be like the same field on civicrm_contact) - @mattwire - @seamuslee001 what do you think here?

[jitendrapurohit]

ok, agree with the naming of created_date. Probably, we should -

• Rename created_date to note_date. So it is just the date attached to the note, not necessarily the date when it was created.
• On create, if it is left empty, it defaults to today's date. No extra handling on edit form.
• Write an upgrade sql after ALTER stmt -UPDATE civicrm_note SET note_date = modified_date so that no NULL entries are existing.

Does the above make sense? @eileenmcnaughton @sunilpawar @mattwire

[seamuslee001]

Should this not be a timestamp?

[eileenmcnaughton]

@sunilpawar so we chatted on chat & I think the agreement (with @seamuslee001 & @kcristiano ) was

1. yes- change the name to note_date if you are going to expose it for editing
2. yes - use mysql to default to now() if not entered - ie <default>CURRENT_TIMESTAMP</default> - it's probably clearer on the form to also default to now & make the field required so people don't think they are saving 'empty' and then it isn't
3. yes - your update script looks correct.

Normally we try to be standardised about having both modified_date and created_date where one is present. In this case modified_date is present & we should probably add created_date for consistency - but since you are not in the end touching either it's OK not to consider that in scope - the xml for the modified_date & created_date should look like the below (when both are present)

 <field>
    <name>created_date</name>
    <type>timestamp</type>
    <comment>When the search was created.</comment>
    <required>true</required>
    <default>CURRENT_TIMESTAMP</default>
    <add>5.36</add>
  </field>
  <field>
    <name>modified_date</name>
    <type>timestamp</type>
    <comment>When the search was last modified.</comment>
    <required>true</required>
    <default>CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP</default>
    <add>5.36</add>
    <readonly>true</readonly>
  </field>

[jitendrapurohit]

Based on the discussion with @eileenmcnaughton the latest update to the PR does the following -

• Add note_date and created_date column to the note table.
• Both these columns are of type = timestamp (default to CURRENT_TIMESTAMP).
• Note date is exposed to UI and can be modified by the user.
• Upgrade handling - so the existing value of modified_date is copied to both the date columns.
• Test fix to assert the values in both the dates on create + update.

[eileenmcnaughton]

I'm not sure about adding required here - that would impact what needs to be submitted at the apiv4 level I think (@colemanw to confirm)

I think it's OK just relying on the default

[eileenmcnaughton]

The code looks good - just needs r-run testing & clarification on the required

[eileenmcnaughton]

@jitendrapurohit I was just thinking I should merge this before 5.37 is cut & then do the one fix that stalled this (#19738 (review)) as a follow up - but I discovered this is actually stale

[jitendrapurohit]

@eileenmcnaughton Fixed the rebase conflicts and removed the required attribute for modified_date field 👍

[eileenmcnaughton]

OK - that looks like the same code I tested minus the required - merging

[J0WI]

Hi there,

With MariaDB 10.4 this breaks with the error: Unknown column 'note_date' in 'field list'. Maybe some quotes or quotations or backticks are missing?

[eileenmcnaughton]

@J0WI under what circumstance does it break? It is possibly on upgrade because it is trying to access the field before it exists?

[J0WI]

At this point they already exist. I get the same error when I execute this manually:

ALTER TABLE civicrm_note ADD COLUMN `note_date` timestamp NULL  DEFAULT CURRENT_TIMESTAMP COMMENT 'Date attached to the note';
ALTER TABLE civicrm_note ADD COLUMN `created_date` timestamp NULL  DEFAULT CURRENT_TIMESTAMP COMMENT 'When the note was created';
UPDATE civicrm_note SET note_date = modified_date, created_date = modified_date, modified_date = modified_date;

[eileenmcnaughton]

So there are 3 statements there - do the first 2 run OK & not the last one?

[J0WI]

do the first 2 run OK & not the last one?

Exactly.

[eileenmcnaughton]

@J0WI - do you have data base logging enabled? ie is there a log_civicrm_note table?

[J0WI]

Yes

[eileenmcnaughton]

@J0WI ok - so it will be the log_civicrm_note table that is missing the new field - if you can disable logging & re-enable it should create it

[J0WI]

Can this be included/fixed in the upgrade procedure?

[eileenmcnaughton]

@J0WI I found someone else had reported what sounded like the same issue https://lab.civicrm.org/dev/core/-/issues/2961 - I put up a patch here #22150 - which I think would fix it - but I haven't tested the patch