Minor OverDrive bug fixes. #3877
Conversation
| @@ -1,5 +1,5 @@ | |||
| <?php | |||
| $previews = $this->driver->getPreviewLinks(); | |||
| if ($previews->url): ?> | |||
| if (isset($previews->url)): ?> | |||
There was a problem hiding this comment.
I think there's a bigger problem here, because getPreviewLinks always returns an array, so $previews->url is never going to be set. I wasn't sure what was intended, though, so this was my temporary fix to avoid fatal errors. I'm pretty sure more work is needed!
There was a problem hiding this comment.
I see that the function doc says it returns an array, and links implies plural, but it doesn't; it returns a struct with properties source, formatType and url.
There was a problem hiding this comment.
In that case, getPreviewLinks may need to be renamed and/or have its doc comment revised. Do you have any suggestions, or should we wait and see what @bpalme says?
There was a problem hiding this comment.
I think this is an incremental improvement, but @bpalme may have better ideas for how to handle these errors that doesn't provide incorrect data in the UI. It does run fine!
| if ($res) { | ||
| $res->copiesAvailable ??= 0; | ||
| $res->copiesOwned ??= 0; | ||
| $res->numberOfHolds ??= 0; | ||
| $res->code = 'od_none'; | ||
| } |
There was a problem hiding this comment.
This has the effect of giving incorrect information in the UI (saying 0/0 copies are available) regardless of the truth. That is better than the ugly error output that happens without the if ($res) check, but still not ideal.
| <?=$this->transEsc('od_copies_available', ['%%copies%%' => $copiesAvailable . '/' . ($avail->data->copiesOwned ?? 0)])?> | ||
| <?php endif; ?> | ||
|
|
||
| <?php if ($avail->data->numberOfHolds > 0): ?> | ||
| <?='; ' . $this->transEsc('od_waiting', ['holds' => $avail->data->numberOfHolds], null, true)?> | ||
| <?php if (($numHolds = $avail->data->numberOfHolds ?? 0) > 0): ?> | ||
| <?='; ' . $this->transEsc('od_waiting', ['holds' => $numHolds], null, true)?> |
There was a problem hiding this comment.
Same concern here, I'm not sure that saying zero is better than a clean error, although it's certainly better than the ugly current error.
There was a problem hiding this comment.
This block should only render if the result is greater than zero. Are you seeing otherwise?
| <?php if ($copiesAvailable == 0): ?> | ||
| <p> | ||
| <a class="btn btn-primary placehold" data-lightbox title="<?=$this->transEsc('request_place_texts')?>" | ||
| href="<?=$supportsPatronActions && count($urls) ? | ||
| <?php | ||
| $holdLink = $supportsPatronActions && count($urls) ? | ||
| "$hold_url?od_id=" . urlencode($od_id) . '&rec_id=' . urlencode($rec_id) . '&action=holdConfirm' : | ||
| $urls[0]['url'] | ||
| ?>"> | ||
| <?=$this->icon('place-hold', 'icon-link__icon')?> | ||
| <?=$this->transEsc('od_but_hold')?> | ||
| </a> | ||
| $urls[0]['url'] ?? null; | ||
| ?> | ||
| <?php if ($holdLink): ?> | ||
| <a class="btn btn-primary placehold" data-lightbox title="<?=$this->transEsc('request_place_texts')?>" | ||
| href="<?=$this->escapeHtmlAttr($holdLink)?>"> |
There was a problem hiding this comment.
I can't test this part as our institution does not $supportsPatronActions, but the code looks reasonable.
| @@ -1,5 +1,5 @@ | |||
| <?php | |||
| $previews = $this->driver->getPreviewLinks(); | |||
| if ($previews->url): ?> | |||
| if (isset($previews->url)): ?> | |||
There was a problem hiding this comment.
I see that the function doc says it returns an array, and links implies plural, but it doesn't; it returns a struct with properties source, formatType and url.
| if ($res) { | ||
| $res->copiesAvailable ??= 0; | ||
| $res->copiesOwned ??= 0; | ||
| $res->numberOfHolds ??= 0; | ||
| $res->code = 'od_none'; | ||
| } |
There was a problem hiding this comment.
Zero is better than an ugly debug error trace, but it's probably wrong information. Ideal would be a clean error message?
| @@ -44,16 +45,16 @@ | |||
| <?php if (($avail->data->availabilityType ?? 0) == 'AlwaysAvailable'): ?> | |||
| <?=$this->transEsc('od_unlimited')?> | |||
| <?php else: ?> | |||
| <?=$this->transEsc('od_copies_available', ['%%copies%%' => $avail->data->copiesAvailable . '/' . $avail->data->copiesOwned])?> | |||
| <?=$this->transEsc('od_copies_available', ['%%copies%%' => $copiesAvailable . '/' . ($avail->data->copiesOwned ?? 0)])?> | |||
There was a problem hiding this comment.
@maccabeelevine, do you think we should change the else to else if ($avail->data->copiesOwned ?? 0 > 0): so we display nothing if no copies are owned, instead of the weird 0/0 thing?
While testing some OverDrive templates with the fake connector (since I don't have access to real data), I ran into a number of errors and warnings. This PR improves type/error checking to eliminate those problems. It's possible that these scenarios will never arise with real data, but I don't think it hurts to be error-tolerant in any case!
Could @bpalme and/or @maccabeelevine please review and confirm that I haven't broken anything in the real integration?