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

[REF] extract buildClause from CRM_Report_Form_Event_Income #14098

Merged
merged 1 commit into from
May 28, 2019

Conversation

eileenmcnaughton
Copy link
Contributor

Overview

Function extraction towards bringing the event income details report under unit testing

Before

Report less stdised

After

Report more stdisied

Technical Details

This report is currently excluded from unit tests by virtue of it being declared as skipped in
api_v3_ReportTemplateTest

We have a PR #13928 to alter test logic.

I'm really not comfortable making any logic changes to the report until
it is under testing. Extracting buildQuery removes one of the blockers to
bringing it under testing (remaining are moving most / all logic from
postProcess to beginPostProcessCommon and fixing up the setPager function -
which probably just means removing it

Comments

This report is currently excluded from unit tests by virtue of it being declared as skipped in
api_v3_ReportTemplateTest

We have a PR civicrm#13928 to alter test logic.

I'm really not comfortable making any logic changes to the report until
it is under testing. Extracting buildQuery removes one of the blockers to
bringing it under testing (remaining are moving most / all logic from
postProcess to beginPostProcessCommon and fixing up the setPager function -
which probably just means removing it
@civibot
Copy link

civibot bot commented Apr 21, 2019

(Standard links)

@seamuslee001
Copy link
Contributor

Jenkins re test this please

@pradpnayak
Copy link
Contributor

@eileenmcnaughton just curious to know if these changes will invoke hook_civicrm_alterReportVar ?

@eileenmcnaughton
Copy link
Contributor Author

@pradpnayak I think this change won't change whether it is called - it's really just an extraction. Once it is under testing we could probably add it - although I would probably add unit tests at that point. It feels like a very fragile hook to me in general & it's overloaded by being called from different places with different meanings

@eileenmcnaughton
Copy link
Contributor Author

@lcdservices are you ok with this as a first step to getting the report under testing so we can review your PR?

@eileenmcnaughton
Copy link
Contributor Author

@pradpnayak should we merge this?

@lcdservices
Copy link
Contributor

@eileenmcnaughton I tested this and it works fine. My only comment is that the SQL formatting is old-style and ugly. Would benefit from the more condensed style we use now.

@eileenmcnaughton
Copy link
Contributor Author

@lcdservices OK - I'll merge & that can follow - you mean less white space?

@eileenmcnaughton eileenmcnaughton merged commit e345d7f into civicrm:master May 28, 2019
@eileenmcnaughton eileenmcnaughton deleted the event_income branch May 28, 2019 14:20
@lcdservices
Copy link
Contributor

Yes. The large indentations, etc.

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.

4 participants