Show email receipt status on view recurring contribution #17177
Conversation
|
(Standard links)
|
| @@ -107,7 +100,7 @@ public function preProcess() { | |||
| public function run() { | |||
| $this->preProcess(); | |||
|
|
|||
| if ($this->_action & CRM_Core_Action::VIEW) { | |||
| if ($this->isViewContext()) { | |||
There was a problem hiding this comment.
@mattwire outside the scope of this PR but I'd like to consider moving this function up to CRM_Core_Form / CRM_Core_Page level as it's much more readable
|
|
||
| $this->assign('action', $this->_action); | ||
|
|
||
| if ($this->_permission == CRM_Core_Permission::EDIT && !CRM_Core_Permission::check('edit contributions')) { |
There was a problem hiding this comment.
This isn't in the parent function that I can see as yet....
There was a problem hiding this comment.
Well spotted. In taking a first look at this (so apols if any of the below comments are wrong headed...)
That code looks like logic that is going to apply to a number of entities where there can be a distinction between edit and view permissions. I suppose the code is there to catch situations where someone was offered an Edit link when they should not have been; and when someone has hackishly constructed an edit link to try to avoid permissions.
Presumably it's not specific to contributions. Presumably it's not specific to editing - what about create/delete too? And what if the user doesn't have view permission anyway?
So it seems to me that the logic is a bit flawed/limited anyway.
Looking at the used trait, I'm also unclear how the _action property is declared as an integer and compared using binary & operator is apparently set to a string like browse:
$this->_action = CRM_Utils_Request::retrieve('action', 'String', $this, FALSE, 'browse');
(There's also no validation on the user input there; we import the user value as-is.)
If we must have two versions of action - a word and an integer/binary constant, then I think any functions that get or set them should be named to differentiate clearly.
Back on track though, surely this logic should be at the point that $this->_permission is set?
There was a problem hiding this comment.
I have to say I can't see where
$this->assign('permission', 'view');
would be picked up - or even $this->_permission - perhaps that code just does nothing anyway?
There was a problem hiding this comment.
🤣 I have always had trouble understanding where anything comes from in the quickform side of things!!
There was a problem hiding this comment.
@artfulrobot Re $this->_action there is some magic in CRM_Utils_Request::retrieve that converts from string to integer. That has caught me out before but if you pass a string for action to the function it comes back as an integer!
Re. the permission code, I'm pretty sure the existing check (that I've removed) did nothing sensible. You only need view permission to view a recurring contribution, and if you have edit you need view anyway.
There was a problem hiding this comment.
some magic
AAAAAAAAAArgh! nooooooooo.
|
@mattwire this is a good change & I loaded the page fine on the demo site - so r-run worked. My only concern is the removal of the permission assignment - it seems that would change behaviour from my understanding so far |
|
@mattwire Thanks for this, it's definitely an improvement and something users (including me) don't understand - why's this missing when it's such important information?! (and why can't you search on it, either, is related) So that part looks great and I think that commit should be merged. As you can see from my coments above, the other refactors have thrown up some issues that I haven't understood (or that don't work but aren't part of this PR). |
|
Hey @artfulrobot @eileenmcnaughton Thanks for review - I've commented on the comments re conversion to |
|
OK - I'm going to merge this. @mattwire has responded to the comments & none of us determined any of the issues to be blocking - and none of us found any evidence of actual impact so I think our level of digging is appropriate |
|
This is great, thanks. re: iATS Payments, we have a site-wide configuration flag that allows an installation to override the "Send a receipt for each contribution" schedule-specific setting, so in that case, we'd actually prefer to not show that field, since it's not true. In our experience, most installs don't want to be sending a receipt with each payment, and the challenge is that CiviCRM doesn't have an option to only send a receipt with the first payment. |
|
@adixon - perhaps you could add a hook to that screen that unsets the field or always presents the site wide value. |
Overview
Before
You have to use API/Database to see if email receipts will be sent for a recurring contribution.
After
You can see like this:
Technical Details
There are two commits. The first converts the "View recurring contribution" page to use EntityPageTrait. This makes the code simpler and more maintainable but is not actually required for the UI change.
The second commit is a one line change to ContributionRecur.tpl to expose the value of is_email_receipt.
Comments
@adixon Does this work ok with IATs?