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

dev/core#2422 Add created_id, modified_id, expires_date to saved search table #19709

Merged
merged 3 commits into from
Mar 4, 2021

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Mar 2, 2021

Overview

Per https://lab.civicrm.org/dev/core/-/issues/2422 sites using search kit will rapidly start to accumulate a large number of saved searches and will want better filtering (& cleanup). This adds created_id and modified_id and starts populating them as well as expires_date (which won't start populating until we expose it in the search kit UI).

Starting to populate this data in 5.36 (before we've had a chance to expose it) means that when we do people will see data not just empty fields.

Before

id of saved_search creator / modifier not stored. No field for expires_date

After

tada

Technical Details

@colemanw

Comments

@civibot
Copy link

civibot bot commented Mar 2, 2021

(Standard links)

@civibot civibot bot added the master label Mar 2, 2021
@eileenmcnaughton eileenmcnaughton changed the title dev/core#2422 Add created_id, modified_id, expires_date to saved sear… dev/core#2422 Add created_id, modified_id, expires_date to saved search table Mar 2, 2021
@eileenmcnaughton eileenmcnaughton force-pushed the ss branch 4 times, most recently from 9a44022 to 0d04d94 Compare March 2, 2021 10:08
@mattwire
Copy link
Contributor

mattwire commented Mar 2, 2021

@eileenmcnaughton It seems to me if we are adding created_id and modified_id that it would also make a lot of sense to add created_date and modified_date at the same time? Extremely useful for troubleshooting and pretty useful if exposed to the UI as well.

@eileenmcnaughton
Copy link
Contributor Author

@mattwire I did consider those - I'm happy to add them in if you want me to (I wavered because they aren't in the civicrm_group table yet I think)

@mattwire
Copy link
Contributor

mattwire commented Mar 2, 2021

I think it would be good. I've generally found wherever we're recording a create/modify contact_id the date is useful too.

@eileenmcnaughton
Copy link
Contributor Author

@mattwire ok - I've added them in

@totten
Copy link
Member

totten commented Mar 3, 2021

I haven't r-run - but just want to say it looks like this is generally good practice - and we should merge before branching.

@eileenmcnaughton
Copy link
Contributor Author

after spending more time playing with saved search I think we also want a description field to elaborate on what the searches are (also in the group table).

I realise my biggest challenge now is getting this through review so not sure whether to attempt to add that or wait for this to hopefully get merged

@colemanw
Copy link
Member

colemanw commented Mar 3, 2021

I'm happy to merge this after you add a description field @eileenmcnaughton

…ch table

Per

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

These can be exposed in the search kit saved search listing as filters when
we get to that point but the earlier we start
saving them the better the data will be when we do expose
@eileenmcnaughton
Copy link
Contributor Author

@colemanw should I go whole hog & add frontend_description per Group.xml

@eileenmcnaughton
Copy link
Contributor Author

Well I've added in description now


/**
* When the search was lase modified.
*
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: "lase"

'name' => 'modified_date',
'type' => CRM_Utils_Type::T_TIMESTAMP,
'title' => ts('Modified Date'),
'description' => ts('When the search was lase modified.'),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: "lase"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one is actually supposed to be modified with lasers.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dunno if we should add lasers as a hard-dependency. We want to support older systems that don't have built-in CD burners. IMO Civi should work on any box fast enough to run Win95.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK - you had your fun - all lasers updated now

<field>
<name>modified_date</name>
<type>timestamp</type>
<comment>When the search was lase modified.</comment>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: "lase"
At least it's consistent!

@colemanw
Copy link
Member

colemanw commented Mar 3, 2021

@colemanw should I go whole hog & add frontend_description per Group.xml

I wouldn't, no. SavedSearches are a backend thing. SearchDisplays are more front-endey.

@eileenmcnaughton
Copy link
Contributor Author

@colemanw all passing now without the lasers

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.

5 participants