-
-
Notifications
You must be signed in to change notification settings - Fork 825
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
Afform - typo when getting options from SavedSearch entity #29662
Conversation
🤖 Thank you for contributing to CiviCRM! ❤️ We will need to test and review this PR. 👷 Introduction for new contributors...
Quick links for reviewers...
|
@ufundo let's go ahead and merge this part now. It's a typo fix and causes bugs so IMO should be against the RC branch. |
ok, have undrafted! |
when you say RC branch, is that 5.71 or 5.72? |
I was slightly worried the fix might cause bugs if anything has been working around this not working? |
@ufundo looks like you need to rebase & force-push this branch to take out the |
@ufundo I don't think there's any chance this would break something. |
@ufundo to the other point about fixing the filtering based on case type. See this relevant PR and my comment: At the time I felt it unnecessary to "go nuts" and load lots of case type definitions... and I'm still a little worried about the performance impact - unserializing xml is notoriously slow and if we do it once or more per row for 50 rows in a SK display... I think we should look at caching. |
@colemanw - thanks, have rebased properly now (was on my phone before...). makes sense re-chain selects being quite rare. I'll comment on the other PR on the other PR 👍 |
Oh I didn't actually make the other PR yet... |
Overview
Fix a typo that crashes chain select filters on saved searches.
Example form:
Create a saved search of Cases
Create a Search Afform and add filters for Case Type and Case Status. Make the Case Status a chain-select based on the Case Type (because Case Types can define available statuses)
Before
The Case Status selector crashes based on failed Afform/getOptions AJAX call for the Case Status
After
It crashes because the Case BAO can't handle being passed an array of possible case_type_id values, which is an improvement...
TODO
Add test
Find a non-case example where the chain select actually works!