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

Cleanup on contribution view #22698

Merged
merged 6 commits into from
Feb 18, 2022
Merged

Cleanup on contribution view #22698

merged 6 commits into from
Feb 18, 2022

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Feb 3, 2022

Overview

This combines #22684 and #22588 and adds a couple of other consistency changes

  • Move acl check for contributionView to the extension #22684 is a cleanup that moves acl logic to the financialacls extension
  • Generate button links in PHP instead of smarty for ContributionView form #22588 moves button logic from the tpl to the php layer
  • In addition this pr makes the payment/submit credit card payment/refund options the same as in the payment details block - it uses the same code to generate the list as the payment block. Understanding the logic for what & where bent my mind & was inconsistent with what we had otherwise settled on. I would also be ok with REMOVING them - since they are elsewhere on the form
  • this pr makes the payment summary block always present and abov ethe payment details (it seemed confusing and based on things we've moved away from having them sometimes there)

Before

Pending
image

Completed
image

After

Pending
image

Completed
image

Technical Details

I feel like the is_template stuff should be a separate form class (which possibly extends or shares a parent with the contribution view for the bits of functionality that are shared) - but it's really the tpl which is going to be really hard to maintain if we don't separate them. That tpl separation can be done even if the template form is overloaded onto the same class

Comments

@civibot
Copy link

civibot bot commented Feb 3, 2022

(Standard links)

@civibot civibot bot added the master label Feb 3, 2022
@eileenmcnaughton eileenmcnaughton changed the title WIP PR on contribution view Cleanup on contribution view Feb 4, 2022
@eileenmcnaughton eileenmcnaughton force-pushed the matt branch 2 times, most recently from 1955f39 to 6b3ff76 Compare February 10, 2022 01:43
eileenmcnaughton and others added 6 commits February 10, 2022 15:04
- removes an unused variable.
- fixes to not show 'record Refund' for pending contributions (is displayed
on the expand payments section of a contribution
- removes an unused variable.
- fixes to not show 'record Refund' for pending contributions (is displayed
on the expand payments section of a contribution
Note this updates the code such that the buttons are consistent on this form with the payment info block
@mattwire
Copy link
Contributor

Tested and seems to be working. I agree there are probably refinements and we probably don't need the "payment" buttons twice on the same form - but we can refine in future / smaller PRs.

@eileenmcnaughton eileenmcnaughton deleted the matt branch February 18, 2022 21:45
@colemanw
Copy link
Member

This PR caused several regressions, documented here: https://lab.civicrm.org/dev/core/-/issues/3168

I think someone testing on Drupal with clean urls wouldn't notice that the urls are getting processed twice, but on other systems it causes the buttons & links to break.

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.

3 participants