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

dev/core#4189 Fix indiscriminate display of direct debit agreement #25910

Merged
merged 1 commit into from
Mar 25, 2023

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Mar 23, 2023

Overview

dev/core#4189 Fix indiscriminate display of direct debit agreement

Associated documentation update to encourage extension writers to provide their own text https://lab.civicrm.org/documentation/docs/dev/-/merge_requests/1065

Before

Direct debit text showing when it should not...

After

No longer showing for Dummy processor (I also tested in paylater mode but didn't screenshot)

Main Page
image

image

Confirm Page
image

Thank you page
image

Note to test Direct Debit I updated the Dummy Processor row to have payment_type = 2

Main Page

image

image

Confirm Page

image

Thank you (there is no agreement text on thank you)

image

Technical Details

Comments

@civibot
Copy link

civibot bot commented Mar 23, 2023

No issue was found matching the number given in the pull request title. Please check the issue number.

@civibot
Copy link

civibot bot commented Mar 23, 2023

(Standard links)

@civibot civibot bot added the 5.60 label Mar 23, 2023
@mlutfy
Copy link
Member

mlutfy commented Mar 25, 2023

Works for me.

I tested most of the scenarios as well, on 5.60rc:

  • dummy processor
  • Stripe
  • direct debit processor
  • all of them enabled on the same form
  • changing which one was the default processor

There is a non-regression bug, which happened before: if there are multiple processors on the same form, the display of the "agreement" text depends on whether Direct Debit is set as the default processor. I think I've never noticed this agreement text before, because DD is never my default processor (global PP setting)

@mlutfy mlutfy merged commit 425aeaf into civicrm:5.60 Mar 25, 2023
@eileenmcnaughton eileenmcnaughton deleted the regression_dd branch March 25, 2023 01:01
@eileenmcnaughton
Copy link
Contributor Author

I guess the non-regression bug might not be hit IRL too much

@mlutfy
Copy link
Member

mlutfy commented Mar 25, 2023

#25918 WIP for the non-reggresion

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.

2 participants