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

dev/core#1714 Fix mis-filtering on activity type #17215

Merged
merged 1 commit into from May 1, 2020

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented May 1, 2020

Overview

This fixes a bug where the activity_type_id was being compared against the label of the option value

Before

Using Search Builder -
Choose Activity - Activity Type - = - Tell a Friend.
Actual results - 0
Choose Activity - Activity Type ID - = - Tell a Friend. (Not listed as a ID just the label)
Actual results - 92

After

Filter correctly filters

Technical Details

Most of the code in the handling for the first bunch of fields was silly. I added a test for each field to ensure they would be non-weird without it all

Comments

https://lab.civicrm.org/dev/core/-/issues/1714

This fixes a bug where the activity_type_id was being compared against the label of the option value
@civibot
Copy link

civibot bot commented May 1, 2020

(Standard links)

@civibot civibot bot added the 5.25 label May 1, 2020
@colemanw
Copy link
Member

colemanw commented May 1, 2020

I love how this deletes 40 lines of garbage and adds 60 lines of tests.

@eileenmcnaughton
Copy link
Contributor Author

@demeritcowboy @kcristiano any chance one of you can do further UI testing on this - because it targets the rc. Note there are some weird things still happening but I think they pre-exist

@demeritcowboy
Copy link
Contributor

I can do some tomorrow.

@kcristiano
Copy link
Member

@eileenmcnaughton I id an r-run and it does solve the specific issue in https://lab.civicrm.org/dev/core/-/issues/1714

I also ran a number of other searches and could not find any issues. I would be more comfortable if we had more time, but I do think it is good to merge.

@colemanw colemanw merged commit d9d9e06 into civicrm:5.25 May 1, 2020
@colemanw colemanw deleted the act branch May 1, 2020 13:54
@colemanw
Copy link
Member

colemanw commented May 1, 2020

@demeritcowboy I've merged this but your testing would still be helpful. Thanks for offering.

@demeritcowboy
Copy link
Contributor

Agree with @kcristiano more time would be better but as noted there's a slew of existing weirdnesses so could spend forever on this. Activity Status appears twice in the dropdown, there's a chunk of E_NOTICEs when you edit the smart group, Find Activities has always been weird regarding target_contact, etc...

It does fix the stated problem. And adds some tests.

@eileenmcnaughton
Copy link
Contributor Author

@demeritcowboy the double activity status thing is because @colemanw renamed the title on id fields to 'Activity Status' not 'Activity Status ID'. In the search code it actually uses the title for the id field and the option group title for the psuedofield

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants