-
-
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
Fix CRM-20660: Total Opens should count without DISTINCT keyword. #10443
Fix CRM-20660: Total Opens should count without DISTINCT keyword. #10443
Conversation
Removing "WIP' prefix now that all checks have passed. This is ready for review. |
CRM/Report/Form/Mailing/Summary.php
Outdated
} | ||
else { | ||
$distinct = ''; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we remove else part and initialize $distinct
variable above if condition ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
CRM/Report/Form/Mailing/Summary.php
Outdated
// is the key in $this->_columns, and $fieldName is the key in that array's | ||
// ['fields'] array. | ||
// Reference: CRM-20660 | ||
$distinct_count_columns = array( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please change this to satisfy civi-coding standards(camelCase). It would also be nice to fix $count_tables
above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Funny, I wasn't aware of that part of the standard, somehow. Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested mailing summary report and it seems to show correct count for total, unique after the updated PR.
Thanks.
WIP until all checks pass.