CRM-20879: Extend self-service event registration transfer to backend #10695
Conversation
monishdeb
commented
Jul 18, 2017
•
- CRM-20879: Extend self-service event registration transfer to backend
|
As per the change:
|
|
Tested, working as expected. @lcdservices can you please confirm? |
|
Tested and confirmed this is working as expected. Transfer/Cancel action is present from contrib row and button available on contrib view. Transfer and cancel actions work correctly. The action is no longer available once the contrib is in cancelled or transferred status. |
|
Thanks @lcdservices for your feedback. @eileenmcnaughton @mattwire can you review my PR. |
|
@monishdeb I've now reviewed this and I think it's good to merge. The changes are mostly adding conditionals into those two forms around the $isBackOffice flag so they can operate on both frontend and backend. It's a useful addition for a rather hidden admin function that probably has more use on the backend. |
CRM/Event/Form/ParticipantView.php
Outdated
| if (CRM_Core_Permission::check('edit event participants') && !in_array($status, array('Cancelled', 'Transferred'))) { | ||
| $this->assign('transferOrCancelLink', CRM_Utils_System::url( | ||
| 'civicrm/event/selfsvcupdate', | ||
| sprintf("reset=1&pid=%d&is_backoffice=1&cs=%s", $participantID, CRM_Contact_BAO_Contact_Utils::generateChecksum($contactID, NULL, 'inf')) |
There was a problem hiding this comment.
It might be clearer to pass the $query as an array rather than sprintf (which I can't quite figure the purpose of. In general I think the array format is cleaner in 80% of cases.
CRM/Event/Form/SelfSvcTransfer.php
Outdated
| @@ -364,6 +390,9 @@ public function postProcess() { | |||
| $statusMsg = ts('Event registration information for %1 has been updated.', array(1 => $displayName)); | |||
| $statusMsg .= ' ' . ts('A confirmation email has been sent to %1.', array(1 => $email)); | |||
| CRM_Core_Session::setStatus($statusMsg, ts('Registration Transferred'), 'success'); | |||
| if (!empty($this->_isBackoffice)) { | |||
There was a problem hiding this comment.
you can just use if ($this->isBackOffice) - since it's defined on the form it won't e-notice
CRM/Event/Form/SelfSvcUpdate.php
Outdated
| * @array bool | ||
| */ | ||
| protected $_isBackoffice = FALSE; | ||
| /** |
There was a problem hiding this comment.
if there is not another pattern we are trying to match $isBackOffice (caps on Office, no _) is preferable
|
I added my minor comments but my main concern is whether the 2 forms are consistent enough with how the contribution page & event pages work (they are also front end forms that can be used by a back office user). They use the function $this->addCIDZeroOptions() to pass allow for contact_id to be edited IF the logged in contact has access to any other contact. I haven't gone through this in depth but it feels like a different approach is being used & I wonder if it could be more aligned? |
1433a1f to
d75f046
Compare
|
@eileenmcnaughton I have made the minor changes and updated the PR. Also, I tried to align the 2 form codes like contribution & event page. But it isn't simple and need some more time. Since this is a paid issue for IIDA, I think we need to do this in a separate issue? |
|
@monishdeb @mattwire @lcdservices this stalled a long time ago based on my comments & I see updates were made in response. I also note that that code it touches on is fairly contained within the self-service subsystem, has not changed much since it was submitted & it had good feedback. I feel like I would be happy to merge this if one of you is prepared to re-test to check that it still works & if @monishdeb squashes the commits. I'm happy for the reviewer here to be @monishdeb since it's just a final-re-review & enough time has gone by for him to have fresh eyes on it |
|
@eileenmcnaughton I have squashed the commit and retested on my local and here's the screencast: And this screencast shows that participant with Transferred/Cancelled status won't see the option 'Transfer or Cancel': |
|
Thanks @eileenmcnaughton :) |
I don't know what purpose this serves, but it was added in civicrm#10695 so this keeps the new screen true to the old one.