Email the service desk after a hold is placed#3955
Conversation
| * | ||
| * @return void | ||
| */ | ||
| protected function emailRequestPlaced($holdDetails) |
There was a problem hiding this comment.
VuFind calls these user actions both holds (too specific) and requests (maybe too general). I use requests where possible but holds where the term is already used i.e. in that config section.
| $message | ||
| ); | ||
| } catch (\VuFind\Exception\Mail $e) { | ||
| return; |
There was a problem hiding this comment.
I don't want to throw an exception, after all the hold succeeded. Might be helpful to log the error but I wasn't sure if it was worth using LoggerAwareTrait for the one message.
There was a problem hiding this comment.
You could also use error_log as a last resort.
| * | ||
| * @return string The email address | ||
| */ | ||
| protected function getEmailRecipient($holdDetails) |
There was a problem hiding this comment.
The logic I'll be implementing locally is to look at the pickup location and do a mapping from that to a particular email address. Not sure if that is generic enough to share.
There was a problem hiding this comment.
Could that be generalized to something like a prioritized list of $holdDetails properties mapped to email addresses? This is pretty ugly, but something like:
[EmailNotificationMap]
property_name[value] = "email@example.com"
other_property_name[value2] = "email2@example.com"
default = "sharedbox@example.com"
(Obviously this would lend itself better to YAML or JSON configuration, but if we have to use .ini, it might work).
|
|
||
| <?=$this->translate('Comments')?>: <?=$hold_details['comment'] ?? ''?> | ||
|
|
||
| <?php /* Request Group: <?=$this->translate('request_group')?>: <?=$hold_details['requestGroupId'] ?? ''?> */ ?> |
|
@maccabeelevine, I haven't had a chance to look at this in depth yet, but I wonder if it's worth re-targeting it against the dev-11.0 branch; we've just switched from laminas-mail to symfony-mailer there, and so it may be good to test/develop this new feature against the library we'll be using going forward. (I assume this is unlikely to come out of draft mode in time for release 10.1 in any case). |
I think I'd like to see what happens with timing? The only thing holding this in draft is a change I'd like to make once #3930 is merged, and so I don't know how long either will take to review. Obviously I know we're close to the end so it's fine to push this if needed, but I'd rather wait to see. |
Sure, not a problem! |
I didn't realize when I responded above that we were past the 10.1 translation freeze. There is no point to doing this PR without the translations, so it should indeed wait for 11. That said, I hate re-basing branches so I'd rather just hold this until the |
Sure, no problem -- though rebasing to a future branch is a lot easier than rebasing to a past branch, so if this does get done before the dev-11.0 gets merged into dev, I'll happily volunteer to help with any necessary adjustments. :-) |
|
Actually, it was just a matter of clicking the "change base" button on this PR to re-target this against dev-11.0 without conflicts, so I went ahead and did that... helps prevent it from accidentally getting merged to the wrong place if I get sloppy. ;-) |
demiankatz
added
new feature
new language strings
demiankatz
left a comment
demiankatz
left a comment
There was a problem hiding this comment.
@maccabeelevine, just noticed that some minor conflicts have cropped up here, while I was reviewing outstanding work to prepare for tomorrow's Community Call. No rush from my end, but if you're still planning to finish this draft, you might want to sort that out when time permits.
|
Not ready to return to this yet, but at least I cleaned up the conflict. |
|
Reassigning milestone as per discussion on the 1 July Community Call. |
|
@maccabeelevine, I'm starting to evaluate PRs for inclusion in release 11.1 or 12.0 -- this is currently tagged for 11.1, but it hasn't moved in a while. Should I bump it forward to 12.0, or do you plan to get back to it soon? (No pressure either way from my end -- I just want to be sure our milestones are realistic). |
|
(I will note that, since this touches controller code, if we bump it forward, it will need to be significantly rewritten; on the other hand, if we want it in release 11.1, the sooner we get it done, the less likely it will be to create conflicts in the 12.0 branch as controller refactoring gets underway). |
|
I decided that this is too heavy an approach; the code doesn't belong in VuFind just to fill a gap in ILS behavior. Adding just a webhook is simpler, as folks on the community call yesterday agreed. Doing that in #5262. |
2 participants
Not all ILS's notify the circ staff after a request is placed (psst FOLIO). Staff have to monitor the requests queue. This allows VuFind to do the email instead.