CRM-19752 - Unit Test for issue CRM-19752

[vedantrathore]

Extracted a function getSummaryQuery(), added unit Tests for checking the function with ACL's enabled and ACL's disabled.

---

• CRM-19752: Slow query created by financial type acls

[civicrm-builder]

Can one of the admins verify this patch?

[vedantrathore]

@eileenmcnaughton could you please review this?

[colemanw]

@civicrm-builder add to whitelist

[eileenmcnaughton]

test this please

[eileenmcnaughton]

If you click through to https://test.civicrm.org/job/CiviCRM-Core-PR/13768/console (click on the console link from the jenkins report) you can see there is a merge issue, perhaps because there are 4 similar looking commits

To fix it do a rebase - ie

git pull --rebase origin master

to push rebased changes back up to your repo you will need to use the -f switch (force)

[vedantrathore]

@eileenmcnaughton I have rebased and squashed them into a single commit, I think this should work now.

[eileenmcnaughton]

OK we are homing in on it - you need to fix some more style issues. You could try using https://github.com/civicrm/coder although TBH I used jenkins a lot to check style.

When you fix those style issues if you use git commit --amend (& then use the force switch while pushing) you can adjust your existing commit rather than creating a new one

[vedantrathore]

Ok cool, I'll see the jenkins report, will civilint do the same job?

[colemanw]

Yes, it's generally a good idea to run civilint before committing. It doesn't check every file in the tree, only the ones with uncommitted changes. I wrote a handy little script for frequent contributors, it will run civilint, checkout a branch, make a commit and then open a pull-request all in one command: https://gist.github.com/colemanw/01dae2a2937f6f39c2300b701636b4a8

…

On 02/07/2017 01:35 PM, Vedant Rathore wrote: Ok cool, I'll see the jenkins report, will civilint do the same job? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#9746 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACveILINY1f_Ao_8BFOmdfc2seXJyrlfks5raLmNgaJpZM4Lwc_m>.

[vedantrathore]

Okay, thanks @colemanw 😄

[vedantrathore]

@eileenmcnaughton All tests passed, I think It's mergeable now. :)

[mlutfy]

jenkins, test this please

[eileenmcnaughton]

this needs to be .=

[eileenmcnaughton]

nope it doesnt'

[eileenmcnaughton]

Sorry about the huge delay on reviewing this. I just took a look & my reservation was that I want to take this change a bit further. However, this PR moves us in the right direction & adds patches so I'm going to merge & then add more as a follow up