KohaRest: add support for translation prefix#4787
KohaRest: add support for translation prefix#4787rajaro wants to merge 7 commits intovufind-org:devfrom
Conversation
demiankatz
left a comment
demiankatz
left a comment
There was a problem hiding this comment.
Thanks, @rajaro, this is a good idea. However, it seems to only be implemented for holds errors at present, and there are other places where it might also apply (storage retrieval requests, for example, or things like availability status messages). Should the scope be broadened before this is merged?
demiankatz
left a comment
demiankatz
left a comment
There was a problem hiding this comment.
Thanks, @rajaro -- I think this could be made a little more readable with the help of a support method; see below for more details.
demiankatz
left a comment
demiankatz
left a comment
There was a problem hiding this comment.
Thanks for the progress, @rajaro, this is a lot more readable now!
See below for one minor suggested optimization, and a couple of questions about the application. I'm not at all familiar with Koha, so maybe my questions are misinformed -- I'm just asking based on a naive interpretation of what I see. Maybe @EreMaijala could offer a better-informed perspective on this.
| ); | ||
| if ($result['code'] >= 300) { | ||
| $msg = empty($result['data']['error']) | ||
| ? $result['code'] |
There was a problem hiding this comment.
Should we also consider prefixing the code? I'm not sure what this value is going to look like, but maybe translating it (possibly in combination with a contextualizing prefix) would have some value. Sending an out-of-context number as the error message doesn't seem like it would be helpful.
There was a problem hiding this comment.
Sure, added prefix to result code as well
| return $this->itemStatusMappings[$statusKey] | ||
| ?? $data['code'] ?? str_replace(':', '_', $statusKey); | ||
| ?? $data['code'] | ||
| ?? $this->getPrefixedMessage(str_replace(':', '_', $statusKey)); |
There was a problem hiding this comment.
I don't know the code well enough to fully understand what's going on here, but this seems like it might potentially be problematic (how do item status mappings interact with translation prefixes? Why is code prioritized over status keys?). Just want to be sure that we don't have two conflicting mechanisms in play, and that we apply rules consistently.
There was a problem hiding this comment.
The idea was to leave already mapped statuses as is, and only add translation prefix to non-mapped statuses. I don't know why code is prioritized over status key here.
Co-authored-by: Demian Katz <demian.katz@villanova.edu>
demiankatz
left a comment
demiankatz
left a comment
There was a problem hiding this comment.
Thanks for the progress, @rajaro. I'm nearing the end of what I feel confident reviewing (and I'm hoping @EreMaijala can lend us his wisdom on the rest), but I did spot one area where I think some simplification might be justified.
| return [ | ||
| 'success' => false, | ||
| 'error' => $result['data']['error'] ?? $result['code'], | ||
| 'error' => $msg, |
There was a problem hiding this comment.
If we're going to prefix both the code and the error message, we don't really need the more complex $msg assignment above, and we can just do this:
| 'error' => $msg, | |
| 'error' => $this->getPrefixedMessage($result['data']['error'] ?? $result['code']), |
| return $this->itemStatusMappings[$statusKey] | ||
| ?? $data['code'] ?? str_replace(':', '_', $statusKey); | ||
| ?? $data['code'] | ||
| ?? $this->getPrefixedMessage(str_replace(':', '_', $statusKey)); |
2 participants
No description provided.