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

Export Add unit test + minor refactor covering specifiable payment output fields for participant export. #12535

Merged
merged 2 commits into from
Jul 30, 2018

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Jul 23, 2018

Overview

Unit test covering currently specifiable fields for participant export + refactor out $selectedPaymentFields & paymentTableId variables

Before

No test. Parameter passed via multiple functions

After

Parameter calculated by processor class, significant additional tests

Technical Details

The $selectedPaymentFields & paymentTableId variables are set when payment fields ( like contribution status) are specified on participant export. This test + refactor makes that more sane.

The goal here is to have a sensible separation of concerns in the various processing actions

Comments

@civibot
Copy link

civibot bot commented Jul 23, 2018

(Standard links)

@eileenmcnaughton eileenmcnaughton changed the title Add unit test covering specifiable output fields for participant export. Export Add unit test + minor refactor covering specifiable payment output fields for participant export. Jul 23, 2018
@eileenmcnaughton eileenmcnaughton force-pushed the export_participant_test branch from c41bd1e to 096e72c Compare July 23, 2018 10:05
@eileenmcnaughton eileenmcnaughton force-pushed the export_participant_test branch from 096e72c to 876a47f Compare July 24, 2018 19:52
The selected payments field variable is primarily used when exporting participants
with contributions. This gets the definition of it out of the returnProperties definition
and calculates it more sensibly.
@eileenmcnaughton eileenmcnaughton force-pushed the export_participant_test branch from 876a47f to c66a574 Compare July 24, 2018 20:17
@stesi561
Copy link
Contributor

Reviewing this now

@stesi561
Copy link
Contributor

(CiviCRM Review Template WORD-1.1)

  • The rest to follow.

@eileenmcnaughton
Copy link
Contributor Author

@stesi561 is this still pending you testing in the UI?

@stesi561
Copy link
Contributor

Yes sorry I ran out of time last night. Will have a run at it tonight/tomorrow. If that's okay?

@seamuslee001
Copy link
Contributor

@stesi561 perfectly fine :-) i think Eileen was just trying to check where things were at

@eileenmcnaughton
Copy link
Contributor Author

@stesi561 much appreciated!

@eileenmcnaughton
Copy link
Contributor Author

@stesi561 if you get a chance to check this today it would be fantastic - I have a couple more PR that will need rebasing once this is merged

Copy link
Contributor

@jitendrapurohit jitendrapurohit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested on different component export - works well. Also checked the payment fields in participant export and it works without any issues. +1 to merge this change.

@seamuslee001
Copy link
Contributor

Merging as per review by @stesi561 and testing by @jitendrapurohit

@seamuslee001 seamuslee001 merged commit 87e636d into civicrm:master Jul 30, 2018
@eileenmcnaughton eileenmcnaughton deleted the export_participant_test branch July 30, 2018 10:29
@eileenmcnaughton
Copy link
Contributor Author

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.

5 participants