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

CRM-20915: Creating credit card registration for event stores payment method as check #10701

Merged
merged 2 commits into from
Jul 28, 2017

Conversation

monishdeb
Copy link
Member

@monishdeb monishdeb commented Jul 19, 2017

@Edzelopez
Copy link
Contributor

Works as expected after patch.

@eileenmcnaughton
Copy link
Contributor

eileenmcnaughton commented Jul 23, 2017

@monishdeb @Edzelopez this works & makes sense but I really wanted to add tests on this. Unfortunately there were no existing tests on this form :-(

However, I created a test, (which required some extraction) here #10725 for that class.

The test DOES demonstrate this bug with the addition of one line to the bottom of the test class

$this->assertEquals('Debit Card', $contribution['payment_instrument']);

I have left that line out because I wanted to submit it passing. If you can review & confirm the extraction & get that merged and then add that line to this PR then I will be happy to merge this pr

monishdeb and others added 2 commits July 24, 2017 00:12
…class.

Although the failing assertion is not present, this test demonstrates the bug described in
CRM-20915. It also adds initial test to the form
@monishdeb
Copy link
Member Author

@eileenmcnaughton I have reviewed and cherry-picked the commit from #10725. Also added that extra line. Please have a look!

@eileenmcnaughton
Copy link
Contributor

@monishdeb great - I'm happy to merge this - as long as you confirm that you did take a look at my patch & make sure I didn't do anything dumb (now this PR has both our code & I can confirm I am happy with & have reviewed the code you wrote)

@eileenmcnaughton
Copy link
Contributor

@monishdeb this is ok to merge from my pov - do you want to confirm my part looks ok to you?

@monishdeb
Copy link
Member Author

monishdeb commented Jul 26, 2017

@eileenmcnaughton yup just did a quick test on my local. Did event registration w/o live mode and it works fine, didn't cause any unintended regression. Ok to merge 👍

@eileenmcnaughton eileenmcnaughton merged commit dc567c9 into civicrm:master Jul 28, 2017
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