fix contribution summary report's statistics when grouping and having #16467
Conversation
|
(Standard links)
|
|
this test has failed... not sure if it's related or not.. |
|
test this please |
|
@sluc23 I agree the test fail is unrelated - I tested this & could replicate the bug & the fix However, it did create some e-notices
Also note that we are switching array() to [] where we touch relevant code |
|
ok I didn't get that notices, i'll re-work on it
tx
…On Wed, Feb 5, 2020 at 4:42 AM Eileen McNaughton ***@***.***> wrote:
@sluc23 <https://github.com/sluc23> I agree the test fail is unrelated -
I tested this & could replicate the bug & the fix
However, it did create some e-notices
[image: Screen Shot 2020-02-05 at 4 39 47 PM]
<https://user-images.githubusercontent.com/336308/73808871-369e4f80-4836-11ea-8f3e-b80f6553f337.png>
Also note that we are switching array() to [] where we touch relevant code
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#16467?email_source=notifications&email_token=AATYIZZJQV4AT2LKKLWK3ELRBIYRBA5CNFSM4KPYXQKKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEK2BGEI#issuecomment-582226705>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AATYIZ4OULKVRUF22MKROH3RBIYRBANCNFSM4KPYXQKA>
.
--
*Luciano Spiegel*
iXiam Global Solutions
[email protected]
www.ixiam.com
@ixiam <https://twitter.com/ixiam>
T: +34 902 750 682
-----
Este mensaje y sus archivos adjuntos son confidenciales y previstos
unicamente para el uso de la persona o entidad a quien haya sido dirigida.
Su uso, difusión, distribución, impresión o copia no autorizados está
prohibido legalmente. Si usted ha recibido este mensaje por error, por
favor, bórrelo y notifíquelo al remitente.
El contingut d'aquest missatge i els arxius adjunts són confidencials i
previstos sols per a ser utilitzats per les persones i entitats a qui han
estat tramesos. El seu ús, difusió, distribució, impressió o còpia no
autoritzats està prohibit legalment. Si vostè ha rebut aquest missatge per
error si us plau esborri'l i notifiqui-ho al remitent.
This e-mail is for the sole use of the intended recipient and contains
information that may be privileged and/or confidential. If you are not an
intended recipient, please notify the sender by return e-mail and delete
this e-mail and any attachments.
|
|
@eileenmcnaughton what's the rule in this case, should I use array This Form still has plenty of |
|
test this please |
|
@sluc23 yeah - generally I replace all the arrays in a file before I start working on it - I generally push up a commit just doing that & by the time my bug is fixed that is normally merged (if you make it clear in the header what it is it moves quickly). I think the minimum is no new instances of array() |
|
@eileenmcnaughton looks good now? any other req to merge this PR? tx! |
|
I retested & the enotices are gone now - fix still works - merging |
|
@sluc23 might be this caused https://lab.civicrm.org/dev/core/issues/1596 |
Overview
Fixes Contribution Summary Report statistics values, when grouping by any other field than currency, and filtering by any aggregate criteria (HAVING)
Before
Report statistics shows wrong amounts (total amount, average, count)
After
Report statistics shows correct amounts (total amount, average, count)
Technical Details
In
public function statistics(&$rows)the query generated to get the values is not using the samegroupBythan the main query, it is hardcoded the groupBy only by currency, so the results might be different when an aggregate (HAVING) filter is selected in the reportComments
Gitlab issue here 1110