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 - Fix Campaign, State, Country selectors (again) #25053

Merged
merged 1 commit into from
Nov 30, 2022
Merged

SearchKit - Fix Campaign, State, Country selectors (again) #25053

merged 1 commit into from
Nov 30, 2022

Conversation

colemanw
Copy link
Member

Overview

This fixes the attempted fix from 7deb8c3 which broke more than it fixed.

Before

Lots of SearchKit field no longer showing option value correctly. E.g. create a search for Contributions and add "Financial Type" as a column. It will show as an integer rather than showing the label.

After

Fixed.

Technical Details

The goal was to use ajax to load state, country & county lists because they are too long to prefetch. But switching all FK fields to use ajax instead of option suffixes was too heavy-handed and would have broken a lot of existing searches by no longer supporting those pseudoconstants in the UI.

This restores all previous option list fields, and targets only address country/county/state fields. Ideally we'd turn the prefetch flag off on those fields but that would have major consequences for formBuilder and existing searches.
So this uses a subtler approach and tweaks their "suffixes" metadata to remove name which didn't really make sense for those fields anyway since they don't actually have machine names. Then it teaches SearchKit to use ajax for selecting from fields with no :name, while still using :label for their display value in the table.

@colemanw
SearchKit - Fix Campaign, State, Country selectors (again)
5511ef1 
This fixes the attempted fix from 7deb8c3 which broke more than it fixed.

The goal was to use ajax to load state, country & county lists because they are too long to prefetch.
But switching ALL FK fields to use ajax instead of option suffixes was too heavy-handed and would
have broken a lot of existing searches by no longer supporting those pseudoconstants in the UI.

This restores all previous option list fields, and targets only address country/county/state fields.
Ideally we'd turn the prefetch flag off on those fields but that would have major consequenses for
formBuilder and existing searches.
So this uses a subtler approach and tweaks their "suffixes" metadata to remove :name which didn't really
make sense for those fields anyway since they don't actually have machine names.
Then it teaches SearchKit to use ajax for selecting from fields with no :name, while still using :label
for their display value in the table.
@civibot
Copy link

civibot bot commented Nov 25, 2022

(Standard links)

@civibot civibot bot added the master label Nov 25, 2022
@eileenmcnaughton
Copy link
Contributor

@colemanw if this re-enables the entire pre-load of all options on campaign_id - that was actually the field that WMF and Greenpeace DE experienced significant performance issues on (for other sites it could be events or groups) - so narrowing it down to being performance on country \county\ State is too narrow

@colemanw
Copy link
Member Author

@eileenmcnaughton no it does not re-enable pre-loading of Campaigns. To clarify that aspect:

Before

No option lists were loaded for any field with an FK.

After

All fields with an option list get loaded except for Campaign, Country, County and StateProvince - those all use ajax.

@eileenmcnaughton
Copy link
Contributor

@colemanw & the way in which campaign_id differs from event_id is that event_id doesn't have a a pseudoconstant defined? So it would never pre-load anyway?

  <field>
    <name>campaign_id</name>
    <component>CiviCampaign</component>
    <uniqueName>participant_campaign_id</uniqueName>
    <type>int unsigned</type>
    <title>Campaign ID</title>
    <import>true</import>
    <comment>The campaign for which this participant has been registered.</comment>
    <html>
      <type>EntityRef</type>
      <label>Campaign</label>
    </html>
    <add>3.4</add>
    <pseudoconstant>
      <table>civicrm_campaign</table>
      <keyColumn>id</keyColumn>
      <labelColumn>title</labelColumn>
      <prefetch>FALSE</prefetch>
    </pseudoconstant>
  </field>

  <field>
    <name>event_id</name>
    <type>int unsigned</type>
    <title>Event ID</title>
    <import>true</import>
    <headerPattern>/event id$/i</headerPattern>
    <required>true</required>
    <comment>FK to Event ID</comment>
    <html>
      <label>Event</label>
      <type>EntityRef</type>
    </html>
    <add>1.7</add>
  </field>

@eileenmcnaughton
Copy link
Contributor

ie - the same performance issue would affect event_id on some sites...

@colemanw
Copy link
Member Author

@eileenmcnaughton fields that don't have a pseudoconstant by definition have no option list, so there's nothing to pre-load. Those fields are unaffected by any of this as they would be

Before

Use ajax

After

Use ajax

@eileenmcnaughton
Copy link
Contributor

ok

@eileenmcnaughton eileenmcnaughton merged commit 66daecc into civicrm:master Nov 30, 2022
@eileenmcnaughton eileenmcnaughton deleted the fixStateCampaignSelectors branch November 30, 2022 23:10
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 sig:unreleased-regression
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@colemanw @eileenmcnaughton