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

closes #3745 Fixes Rewrite of Multi-Select Token in Search Kit #24393

Merged

Conversation

brienneK
Copy link
Contributor

@brienneK brienneK commented Aug 26, 2022

Overview

The Rewrite option for editing a display in a saved Search Kit search had a bug so that the token would only return the first value of a multi-select (i.e. multiple values) field.

Replication steps are available on the lab.civicrm issue

Before

Before this fix, if a token was used on the Rewrite option for a multi-select custom field, the display would only return the first value .

Selection_049

Selection_046

After

With the fix, the Search Kit display returns all values of the multi-select field when a token is used via the Rewrite option.

Selection_048

Selection_047

Technical Details

The bug was rooted in the replaceToken() function. $index had a default argument of 0 in the function's parameters and with the way that $replacement was being set on line 738, the function was only returning the first index of an array. This fix replaces the default argument with NULL and puts the assignment of $replacement as it was originally written within an if statement that accounts for an $index that is not NULL.

Comments

A unit test is included as part of this PR.

@civibot
Copy link

civibot bot commented Aug 26, 2022

(Standard links)

@civibot civibot bot added the master label Aug 26, 2022
@colemanw
Copy link
Member

@briennekordis thanks for the PR. Apologies that I am just now seeing it.
Your analysis is good and the fix looks correct, and it's great that you've added a unit test.

General note: the unit test is difficult to read & understand due to being a bit verbose (a lot of irrelevant array data in there not strictly necessary to construct a simple search display) and due to the arrays being formatted in an older style. In the future consider using the APIv4 explorer to format entity arrays for you, like this:
image

@colemanw colemanw merged commit 039403c into civicrm:master Sep 21, 2022
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.

2 participants