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

Add warning when url_site is not specified for paypal (it won't work if not set) #20082

Merged
merged 1 commit into from
May 15, 2021

Conversation

mattwire
Copy link
Contributor

Overview

I tracked down an issue with the Stripe extension that was hiding URL fields for processors other than itself and meant that configuring paypal standard after Stripe was enabled caused the configuration to silently be broken - the fix for Stripe is here: https://lab.civicrm.org/extensions/stripe/-/merge_requests/153

This "fix" adds a UI warning for Paypal standard that will be triggered if url_site is not set. If it is not set the system redirects back to the contribution page with no warnings or errors and never redirects to Paypal.

Before

No warning if url_site not set.

After

Error if url_site not set when configuring paypal standard

Technical Details

Comments

@civibot
Copy link

civibot bot commented Apr 16, 2021

(Standard links)

@civibot civibot bot added the master label Apr 16, 2021
@demeritcowboy
Copy link
Contributor

This seems straightforward except I can't get to the point where it displays the message. I can see that stripe is hiding the usual fields on the form, but then I already get this message first. I don't use paypal - is this the right paypal?

Untitled

@demeritcowboy
Copy link
Contributor

Oh ok I need to fill in an email. But then if I do that I still don't get to the message because I get this: CRM_Core_Exception: Could not find valid value for pp in .../CRM/Admin/Form/PaymentProcessor.php on line 511

@mattwire
Copy link
Contributor Author

@demeritcowboy I've seen CRM_Core_Exception: Could not find valid value for pp in ./CRM/Admin/Form/PaymentProcessor.php on line 511 as well with and without this PR - I think it's a core bug but don't think it's related. If you use Stripe master (6.6) you'll see the URL fields are no longer hidden for paypal.

@demeritcowboy
Copy link
Contributor

demeritcowboy commented Apr 20, 2021

Ok. It's weird. I'm not able to reproduce the 511 error on dmaster.demo or another site, but on the PR test site here and another site with the PR applied I do consistently get it when adding a new processor for paypal standard and then trying to trigger this error by filling it all out except leaving site_url blank. So yes it might be a core bug but it seems like it might be related to what happens when something in checkConfig errors out. So the only way I'm able to get the message without triggering the error first is to create a new processor normally, then go back and edit it and remove the site_url. So

If editing an existing one and you blank out the field, it works as intended,
else if stripe is older then
field is missing and it triggers this line 511 error when adding a new processor.
else if stripe is latest then
field is filled in by default when adding a new processor, and if you for some reason blank it out you get 511, but that would be an odd thing to do.

So it improves the situation? You do see the error on the next page you load. And if the suspected core bug is fixed it should work well.

@eileenmcnaughton
Copy link
Contributor

@demeritcowboy looking at this & your comments it seems to me like it's better in than out - even if there might be a related issue not fixed

@demeritcowboy
Copy link
Contributor

Yes I think so. It's basically that when you're configuring a new processor, with the PR you get a crash that doesn't tell you the problem, and without the PR you get silent fails later.

Jenkins retest this please.

@demeritcowboy demeritcowboy merged commit 0a14292 into civicrm:master May 15, 2021
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.

3 participants