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#578 follow up fix on activity summary report #14745

Merged
merged 1 commit into from
Jul 9, 2019

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Jul 7, 2019

Overview

Completes #13540 by getting us past the full group by error

Before

Steps to replicate:

From the standard pre-defined Contact Reports, go to Activity Summary.
Leave Columns as default, with Contact Name not checked.
Leave Grouping as default (Activity Type + Activity Status).
In Sorting, select Contact Name.
Click "Refresh results".

results in error when full group by enabled

After

No error

Technical Details

In #13540 @davejenx tidied up the report to support unit tests and fixed another bug. This completes that by fixing the remaining FGB bug. Note that test coverage does not exactly address this but we have a standard that when fixing a bug in an area of code with poor test coverage it's OK to take us towards having test cover (which Dave did) rather than to cover the specific code break down. This reflects the fact that multiple technical hurdles often need to be over come to bring a report / code path under testing

Comments

https://lab.civicrm.org/dev/core/issues/578

Dave J did a tonne of work fixing up this report & adding a unit test - this complete by just handling FGB very specifically
on one query
@civibot civibot bot added the master label Jul 7, 2019
@civibot
Copy link

civibot bot commented Jul 7, 2019

(Standard links)

@eileenmcnaughton
Copy link
Contributor Author

test this please

@davejenx
Copy link
Contributor

davejenx commented Jul 9, 2019

Thanks @eileenmcnaughton ! Tested and working here.

  • General standards
    • Explain (r-explain)
      • PASS : The goal/problem/solution have been adequately explained with a link (Github).
    • User impact (r-user)
      • PASS: The change would be intuitive or unnoticeable for a majority of users who work with this feature.
    • Documentation (r-doc)
      • PASS: The changes do not require documentation.
    • Run it (r-run)
      • PASS: Ran report with the specified settings, first replicating the error without the patch, then verifying that the patch resolves the error.
  • Developer standards

@eileenmcnaughton
Copy link
Contributor Author

Thanks @davejenx - merging!

@eileenmcnaughton eileenmcnaughton merged commit 80db43b into civicrm:master Jul 9, 2019
@eileenmcnaughton eileenmcnaughton deleted the fgb branch July 9, 2019 19:47
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