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

[REF] Afform - Code cleanup in LoadAdminData API action #21089

Merged
merged 1 commit into from
Aug 11, 2021

Conversation

colemanw
Copy link
Member

Overview

Minor code simplification and added docblocks in the loadAdminData Afform API.

@civibot
Copy link

civibot bot commented Aug 11, 2021

(Standard links)

@civibot civibot bot added the master label Aug 11, 2021
@colemanw colemanw changed the title Afform - Code cleanup in LoadAdminData API action [REF] Afform - Code cleanup in LoadAdminData API action Aug 11, 2021
@eileenmcnaughton
Copy link
Contributor

@colemanw looks simple - I can r-run if you confirm where I would see it

@colemanw
Copy link
Member Author

colemanw commented Aug 11, 2021

@colemanw looks simple - I can r-run if you confirm where I would see it

@eileenmcnaughton if you add a search display that has 1 or more field transformation in the search, you should still see the transformed field in the afform when creating a form for the display.

@eileenmcnaughton
Copy link
Contributor

My transform isn't working - but it's pre-existing - ie 'amount' is not getting MATH options - trying another field

@eileenmcnaughton
Copy link
Contributor

Possibly this is also pre-existing

image

image

@eileenmcnaughton
Copy link
Contributor

@colemanw I was able to add & select a date field & transform to YEAR - the transform options for date field also seemed a bit confusing - ie

image

but 'LOWER CASE still leaves the only cased thing in the date in upper case & I'm not sure what the MATH ones would mean in this context

image

@eileenmcnaughton
Copy link
Contributor

@colemanw so the summary is - I think that all the things I spotted testing this are unchanged by this & if you are happy that is the case then go ahead & merge

@colemanw colemanw merged commit 38edbd0 into civicrm:master Aug 11, 2021
@colemanw colemanw deleted the afformCleanup branch August 11, 2021 20:34
@colemanw
Copy link
Member Author

Hoo boy, those are a lot of issues with the transforms. I think a lot of the issue is that not all sql functions are appropriate for every field and the UI needs to do a better job of making that clear.
Another part is that pseudoconsant matching happens in the post-processing not the sql query so UPPERCASE(financial_type_id:label) isn't going to work as expected. If we worked at it a bit I think it could be fixed but yea that's currently broken.

@eileenmcnaughton
Copy link
Contributor

@colemanw I suspect once we can do them on combined fields the issues will change again so maybe it's worth figuring out that first

@colemanw
Copy link
Member Author

Well there is a way to do pseudoconstants in SQL, by rewriting the query to use FIELD() like this:

// Use FIELD() function to sort on pseudoconstant values
$suffix = strstr($item, ':');
if ($suffix && $expr->getType() === 'SqlField') {
$field = $this->getField($item);
$options = FormattingUtil::getPseudoconstantList($field, $item);
if ($options) {
asort($options);
$column = "FIELD($column,'" . implode("','", array_keys($options)) . "')";
}
}

I don't know how it would impact performance if we switched them ALL to work that way. We could switch the ones inside functions to do so though, and that would get things like UPPERCASE working.

@eileenmcnaughton
Copy link
Contributor

@colemanw I guess that's the question isn't it - should they all work in sql or should we do some parts in php. I guess that's why I think working on combining the fields makes sense - because whether you hit the limits of mysql in there might drive that answer

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