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 data segmentation functionality #23059

Merged
merged 8 commits into from
Apr 20, 2022
Merged

SearchKit - Add data segmentation functionality #23059

merged 8 commits into from
Apr 20, 2022

Conversation

colemanw
Copy link
Member

@colemanw colemanw commented Mar 30, 2022 •
edited
Loading

Overview

Creates virtual fields based on flexible segmentation criteria.

Creating a Segment:
image

Example use:
image

@civibot
Copy link

civibot bot commented Mar 30, 2022

(Standard links)

@civibot civibot bot added the master label Mar 30, 2022
@colemanw colemanw force-pushed the pivotTables branch 3 times, most recently from 59f06e0 to d413a1b Compare March 30, 2022 15:53
@totten
Copy link
Member

totten commented Mar 31, 2022 •
edited
Loading

I've been reading the unit-test, and it looks like neat functionality. It almost looks like a custom computed (on-demand) field. In the example:

  • The schema for Contribution is expanded to include another field pivot_Giving_Tier.
  • The content of that field is computed on-demand (by a custom expression/function).
  • The custom expression/function is (roughly) a case expression (based on the items list).

Describing this with the word "pivot" is confusing to me. As I understand it, the "pivot" in "pivot table" refers to transposing the placement of rows/columns/values. The functionality here seems independent in two ways:

  • You may use these custom classifications in other contexts (without a pivot table).
  • You may do pivot tables with different fields (without necessarily needing custom classifications).

To make this concrete, let me offer a few examples that seem like plausible use-cases:

  • Regions of the US (eg "Midwest", "New England", "South")
  • Time of Day (eg "Morning", "Late Night")
  • Seasons of Year (eg "Summer", "Winter", "The Holidays")

These classifications are all a bit futzy. (Is "Virginia" in the "South" or "Mid-Atlantic"? Does 4:45am qualify as "Late Night" or "Morning"? Is November 15 in the "Winter"?) So if you're using these classifications, then the functionality here seems quite useful. (You can twiddle the classifications to match the organization's standards.)

The classifications certainly could appear in a pivot table (eg "Show me a list of regions, with the total-donation-amount from each"; or "Show me the times-of-day, with the average donation amount for each"). But you would also the same classification for a drill-down view ("List all cancellations from the Mid-Atlantic region"). It would also be useful in record-views. ("Alice Alison's home address is Virginia (Mid-Atlantic), so her email request should be directed to the "Mid-Atlantic" case-manager.")

So just as a labeling thing, PivotSet is confusing to me. But functionally, it seems pretty neat -- by attaching these custom fields to the model, you can use the classifications in other contexts.

@colemanw colemanw changed the title SearchKit - Add pivot tables feature SearchKit - Add data segmentation functionality Apr 1, 2022
@colemanw
Copy link
Member Author

colemanw commented Apr 1, 2022 •
edited
Loading

Based on extensive discussion our committee has reached a compromise and renamed the new entity VirtualComputedDataClassificationTranspositionPivotSegmentationMagicCategoryField.

@colemanw colemanw force-pushed the pivotTables branch 3 times, most recently from cee2c0d to 5cf08c2 Compare April 17, 2022 19:10
@colemanw colemanw marked this pull request as ready for review April 17, 2022 19:10
@colemanw
SearchKit - Add basic SearchSegment functionality for numeric range g…
13b73d7 
…roups
@colemanw
SearchKit - Fix ORDER BY calculated fields when using GROUP BY
ff2a73c 
Before: Calculated field functions were repeated verbabim in the SELECT, ORDER BY & GROUP BY clause,
this would cause mySql error when using FULL GROUP BY mode.

After: Calculated field function defined in SELECT clause, alias used in ORDER BY & GROUP BY.
No error when grouping by an ordering by the same calculated field.

Side-benefit: When selecting and ordering by RAND(), the selected random numbers will be in order,
as it's now the same random function used for both instead of a different one.
@colemanw
APIv4 - Pass entire Api4SelectQuery object to expression when rendering
3dbd392 
This permits SearchSegment pseudo-fields to call $query->getField() on multiple
fields, including custom fields, while rendering.
@colemanw
SearchKit - Support operators, custom fields & multiple fields in a s…
b69cbf9 
…earchSegment clause.
@colemanw
SearchKit - Move loadFieldOptions function to searchMeta service for …
f07136b 
…reusability
@colemanw
SearchKit - Add UI for editing SearchSegments
dbe1b6d
@colemanw
SearchKit - Refresh fields after saving a SearchSegment
5e5ac99
@samuelsov
Copy link
Contributor

Probably not ready for validation yet but just in case, I found that search segment with option list value doesn't work.
Contact subtypes works but gender, activity types or contribution status doesn't.

Example that always gives other:

ksnip_20220420-122457

@colemanw
Copy link
Member Author

Thanks for testing this @samuelsov. Yes this PR is ready for review.
That's strange it doesn't work for you because there is a unit test included with this PR for that same scenario and it passes.
I'll check...

@eileenmcnaughton
Copy link
Contributor

@colemanw I had a similar experience to @samuelsov - recreated here http://core-23059-10nxw.test-3.civicrm.org:8001/civicrm/admin/search#/edit/2

@eileenmcnaughton
Copy link
Contributor

@colemanw Based on my testing this is working - with the exception of the above issue - which you are working on a patch for.

I think on balance it is better to merge this and then keep testing / improving at this stage since I think we will have incremental improvements / requests from here

The 2 things I noticed were

  1. overlapping criteria - it's tricky with 'BETWEEN' to be clear about how the limit values will be treated - so it is easy to configure like this without being clear what the impact is - in fact $10 fits into 2 criteria but in search kit it will show up in the FIRST matching criteria - I suspect some UI text to clarify might be the best approach

image

  1. I can't add the segments as a filter when I get as far as formbuilder (which would also be necessary to manually build up a link-through I think)

@eileenmcnaughton
Copy link
Contributor

I would also note an advantage of merging is that it can be put though the hoops better on your Europe sojourn

@eileenmcnaughton eileenmcnaughton merged commit e16bbd5 into civicrm:master Apr 20, 2022
@eileenmcnaughton eileenmcnaughton deleted the pivotTables branch April 20, 2022 21:51
@colemanw colemanw mentioned this pull request Apr 20, 2022
Merged
@colemanw
Copy link
Member Author

@samuelsov @eileenmcnaughton this fixes the bug you noticed. It was a bug in the Angular UI which is why the PHPUnit tests didn't catch it. #23265

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
master
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@colemanw @totten @samuelsov @eileenmcnaughton