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

Reporting #18: Fix sorting with > 10 participant roles #21975

Merged
merged 1 commit into from
Nov 7, 2021

Conversation

MegaphoneJon
Copy link
Contributor

@MegaphoneJon MegaphoneJon commented Nov 4, 2021

https://lab.civicrm.org/dev/report/-/issues/18

Overview

This is based on the feedback in #14982.

To replicate this bug, you need at least ten participant roles. The first one's value should be 1. Searching on this value will return any participant whose role BEGINS with a 1 (i.e. 10, 11, 100, etc.) rather than just records whose participant role value IS 1.

Before

Searching for a participant role will return all participants whose value begins with the value you're searching for.

After

Participant list is filtered correctly.

Technical Details

This report had an overridden where() method to filter is_test, but as @yashodha pointed out on #14982, this isn't a very good way to handle this.

Comments

I've been carrying #14982 as a custom patch for 2 years, I realized that the proper fix wasn't all that hard after all. The parent where() has been refactored a couple of times so it was a bit of work to trace through, but it all looks correct.

@civibot
Copy link

civibot bot commented Nov 4, 2021

(Standard links)

@civibot civibot bot added the master label Nov 4, 2021
@colemanw
Copy link
Member

colemanw commented Nov 7, 2021

I'm gonna merge this because as @eileenmcnaughton pointed out in #14982 the overridden function you're removing has no test cover whereas the parent function does, and 2 years is a long time to use it in production with no problems!

@colemanw colemanw merged commit 44f7813 into civicrm:master Nov 7, 2021
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