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.

Sign up for GitHub

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

Jump to bottom

SearchKit - Add display of type entity #25871

Merged
merged 1 commit into from
May 28, 2023
Merged

SearchKit - Add display of type entity #25871

merged 1 commit into from
May 28, 2023

Conversation

colemanw
Copy link
Member

@colemanw colemanw commented Mar 19, 2023 •
edited
Loading

Overview

Note: Description is updated to reflect latest status of the PR. Discussion thread references earlier drafts.

Adds a new display type "entity" which does not produce user-facing output, instead it writes to a SQL table which can then be queried from SearchKit, the API, or other SQL-based tools like Drupal Views.

Technical Details

  • Created SQL table is static.
  • Table includes indexes, FKs and pseudoconstants to facilitate searching.
  • Generated API entity includes a refresh action and a getRefreshDate action for checking the last refresh date
  • SearchKit will automatically create a scheduled job (disabled by default) to refresh the entity. The job can be enabled and configured directly from SearchKit.

image

@civibot
Copy link

civibot bot commented Mar 19, 2023

(Standard links)

@civibot civibot bot added the master label Mar 19, 2023
@eileenmcnaughton
Copy link
Contributor

That is super powerful. One thing that will catch us out if we use it is indexes, or lack thereof - I'd probably err on adding indexes to all fields over none given the search context

@eileenmcnaughton
Copy link
Contributor

"missing some UI niceities like a button to fill or refresh the table" - I would add to that list a date when the table was last refreshed

@colemanw colemanw mentioned this pull request Mar 20, 2023
Merged
@colemanw
Copy link
Member Author

I'm not sure what to make of the test failures, every single one of them passes for me locally 🤷

@colemanw colemanw force-pushed the searchKitEntity branch 3 times, most recently from 53abb14 to 06f5950 Compare March 22, 2023 12:51
@colemanw
Copy link
Member Author

retest this please

@colemanw colemanw force-pushed the searchKitEntity branch 2 times, most recently from fdb2ecd to 1626676 Compare March 22, 2023 13:49
@colemanw
Copy link
Member Author

Trying to figure out what's wrong here... retest this please

@colemanw colemanw force-pushed the searchKitEntity branch 4 times, most recently from 74085eb to 8ab2a43 Compare March 22, 2023 16:24
@aydun
Copy link
Contributor

aydun commented Mar 24, 2023

Just noting that snapshot groups are a baby version of this - a realised form of smart groups.

@colemanw
Copy link
Member Author

retest this please

@colemanw
Copy link
Member Author

Yay! Tests pass! However this PR still feels like a draft because of limitations and usability issues. I've updated the description with a todo list.

@vingle
Copy link
Contributor

vingle commented Mar 30, 2023

Great

  • Filling the table requires manually calling SearchDisplay::run from the API Explorer

As I'm assuming a Scheduled Job can be setup to make that call, it feels ok as a first proof-of-concept. Given the amount of data this exposes, and permissions it bypasses, perhaps there's an argument it shouldn't be too easy to use yet?!

  • I think saving the display ought to immediately fill the table with data

That would be good.

  • As @eileenmcnaughton noted, we need a way to store "last refresh date" and I can't currently think of anywhere to put that data.

Could there be a new date column in civicrm_search_display? For other displays maybe it could (one day) store the last modified date to help distinguish two similarly named old displays?

@colemanw colemanw mentioned this pull request May 19, 2023
Merged
@colemanw colemanw marked this pull request as ready for review May 23, 2023 01:50
@colemanw
Copy link
Member Author

colemanw commented May 23, 2023 •
edited
Loading

All outstanding issues have been addressed. Indexes, preview, refresh, scheduled job & last-refresh-date are all working now. Unit test added.
This is ready to review and merge :)

@eileenmcnaughton
Copy link
Contributor

@colemanw not so fast

api\v4\SearchDisplay\EntityDisplayTest::testEntityDisplay
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'int unsigned'
+'int(10) unsigned'

/home/homer/buildkit/build/build-0/web/sites/all/modules/civicrm/ext/search_kit/tests/phpunit/api/v4/SearchDisplay/EntityDisplayTest.php:81
/home/homer/buildkit/extern/phpunit8/phpunit8.phar:1721

@colemanw
Copy link
Member Author

Ah the joys of SQL dialects and their subtle differences. In Canadian SQL it would be int unsigned (eh)

@colemanw
SearchKit - Add display of type entity
ec43b66 
An entity display does not produce user-facing output, instead it writes to a SQL table
which can then be queried from SearchKit, the API, or other SQL-based tools like Drupal Views.

The new table is static; this includes a scheduled job to refresh it (disabled by default).
@eileenmcnaughton eileenmcnaughton added the merge ready PR will be merged after a few days if there are no objections label May 26, 2023
@eileenmcnaughton
Copy link
Contributor

OK - I've given it merge-ready to see if @aydun or @vingle have any last comments....

@aydun
Copy link
Contributor

aydun commented May 26, 2023

That looks really powerful.

I could successfully create a 'DB Entity' from a search, and then I could build a search based on that entity and create a table, list and grid. However, trying to create a smart group produces Unable to create smart group because this search does not include any contacts. even though it was a listing of contacts, and an autocomplete failed to load any entries. But I'm not sure how much sense it makes to create a smart group from it, or an autocomplete compared to doing it on the original search. It may be worth waiting for a real use-case to see whether those are worth investigating.

@eileenmcnaughton
Copy link
Contributor

ok let's merge it then!

@eileenmcnaughton eileenmcnaughton merged commit 8256036 into civicrm:master May 28, 2023
@eileenmcnaughton eileenmcnaughton deleted the searchKitEntity branch May 28, 2023 22:20
@vingle
Copy link
Contributor

vingle commented Jun 16, 2023

OK - I've given it merge-ready to see if @aydun or @vingle have any last comments....

Sorry I couldn't review this at the time - great it's now in core.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
has-test master merge ready PR will be merged after a few days if there are no objections
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@colemanw @eileenmcnaughton @aydun @vingle