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

CIVICRM-706: Making front-end CVV always required, regardless of the CVV required for back office option. #11227

Merged
merged 1 commit into from
Jun 4, 2018

Conversation

agilewarealok
Copy link
Contributor

Overview

Setting "CVV required for backoffice" to NO results in it setting all front facing forms to 'not require' CVV site wide, ie on all public facing contribution pages.

Before

A single setting was there to disable/enable CVV on front office and back office pages.

After

Now we don't consider back-office setting for front-end contribution pages by making CVV always required for front-end pages.

Agileware Ref: CIVICRM-706

@jusfreeman
Copy link
Contributor

ping @jackrabbithanna

@agilewarealok
Copy link
Contributor Author

We've added Unit test for this PR.

@mlutfy
Copy link
Member

mlutfy commented Nov 15, 2017

For reference, the 4.7 PR (merged): #11205

@jackrabbithanna
Copy link
Contributor

@KarinG Can you review this?

@jackrabbithanna
Copy link
Contributor

Jenkins test this please

@KarinG
Copy link
Contributor

KarinG commented Dec 16, 2017

Hey @jackrabbithanna - considering lots of non-profits are year-end fundraising - best not to make any non-critical edits to payment processing until into January; ping me then - and we'll get this in.

@jusfreeman
Copy link
Contributor

Ping :)

@jusfreeman
Copy link
Contributor

@jackrabbithanna can you review this again with a view to merging. For reference, the 4.7 PR (merged): #11205

@jackrabbithanna
Copy link
Contributor

@KarinG I'd like your technical opinion on this as you have tons of expertise on payment processors, making sure as far as is the code good and would not cause problems. If you could look at it and tell me if you forsee any problems I'd appreciate it. You seemed to be in favor of this change for 4.7 as it could improve security of the forms. Any reason NOT to do it in 4.6? Did your experience with the related PR for 4.7 pan out well?

This feels like a new feature, not so much a bug fix, so I feel like it's a "nice to have", and not critical. I'm very risk adverse for the LTS, so I'd want to feel good about it. Tests have passed, and the contributor has written a test. So all good there.

@KarinG
Copy link
Contributor

KarinG commented May 10, 2018

Sorry still have not had a chance to put this in our 4.6 repo; all our clients know to always use CVV on front end forms; what this does is not so much a new feature - it’s preventing people from being able to set their config such that they become easy targets for credit card fraud; so for sure this is a pass on the merits - but we (or someone else) needs to confirm this is not throwing unexpected errors on 4.6 before merging; i.e. have it in production for some time;

@jusfreeman
Copy link
Contributor

jusfreeman commented May 10, 2018

but we (or someone else) needs to confirm this is not throwing unexpected errors on 4.6 before merging; i.e. have it in production for some time

@KarinG this change has been in production for the customer that requested this change since 14/11/2017 and is included in our custom CiviCRM build (for both CiviCRM 4.6 and 4.7) since 18/1/2018. As a result, all Agileware, CiviCRM customers are using this change in production.

Agileware custom CiviCRM builds are available from https://github.com/agileware/civicrm-core/releases

If you have any other questions about it, just ask - or give it a go!

PS. I've been shepherding this PR for the community's benefit, not our own. We do not need this PR merged to use it ourselves.

@KarinG
Copy link
Contributor

KarinG commented May 10, 2018

@jusfreeman - excellent; I think all this needed was some live production experience. @jackrabbithanna - this is ready for merge.

@KarinG
Copy link
Contributor

KarinG commented May 10, 2018

PS and thank you both!

@jusfreeman
Copy link
Contributor

jusfreeman commented May 10, 2018

@KarinG You're so polite! :)

@jackrabbithanna
Copy link
Contributor

Thanks for the review and recommendation KarinG, Merging #11227

@jackrabbithanna jackrabbithanna merged commit 9bb1e36 into civicrm:4.6 Jun 4, 2018
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.

6 participants