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/report#5 - Fix mailing report unique count issue #13322

Merged
merged 3 commits into from
Dec 20, 2018

Conversation

monishdeb
Copy link
Member

Overview

Steps to replicate:

  1. Go to Mailing >> Scheduled and Sent Mailings >> Report
  2. Unique Opens -> (click) Report
  3. Total Opens -> (click) Report

Both Report show same count.

Before

Both Report showing same count (Unique report is in-correct)

After

Unique Opens Report has less value as compared to Total Opens

@civibot
Copy link

civibot bot commented Dec 19, 2018

(Standard links)

@monishdeb
Copy link
Member Author

monishdeb commented Dec 19, 2018

@sunilpawar I have cherry-picked your commit and on top of the patch I made the following changes:

  1. Modified UT to work with test data
  2. Minor optimization in code
  3. Removed the FGB handling in the groupBy() as it is no longer required ($this->optimisedForOnlyFullGroupBy = FALSE is declared in report)

and this is the last commit

I realized later why you have to bring a separate parameter 'distinct' instead of using the unique_opens_value=Yes/Any, as not to change the core behavior of MailingOpened Report.

@eileenmcnaughton @sunilpawar do you agree with the final patch?

@monishdeb monishdeb changed the title Dev report 5 dev/report#5 - Fix mailing report unique count issue Dec 19, 2018
@sunilpawar
Copy link
Contributor

@monishdeb i am ok with changes.

@eileenmcnaughton
Copy link
Contributor

I'm merging this as it is a reviewer's commit by @monishdeb & the original submitter @sunilpawar has confirmed the changes - @sunilpawar if you haven't retested it would be good if you could double check on master once this is merged or commit to testing on the rc when it is cut at the start of Jan

@eileenmcnaughton eileenmcnaughton merged commit 8910a00 into civicrm:master Dec 20, 2018
yashodha referenced this pull request in cividesk/civicrm-core May 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants