PAIA-driver: add better error handling and scopes #924
Conversation
* PAIA error messages are now passed to the user as exceptions (displayed through flashmessenger) * allow user actions based on scopes (e.g. do not allow renew if user has not scope write_items)
|
Have you tested how this interacts with the translation system? If we're displaying new messages to end users, do we need to add translations and/or map language to existing strings? |
|
The messages are from the PAIA service which should be "Human-readable error description". PAIA supports a "Accept-Language" parameter which could enable the service to return only localized messages based on the header parameter. Though current PAIA ILS-driver does not support this parameter. Does this answer your question? |
|
Ah, btw. I did test this with the translation system: we added a translation for one error message returned by our ILS to support English language. This works fine as the flashmessenger attempts to translate any given string iirc. |
|
I definitely understand that you can't translate unpredictable strings coming out of the API... but my question was actually in reference to some of the hard-coded strings you seem to have added, like |
|
Oh, yes - you are right. I forgot to add translations for these strings, will do so asap (and check if we do have any strings that are more or less equivalent in meaning). |
* added English translation for exceptions (introduced a new translation domain -- suggestion)
|
I added translations for the exception strings - the "Exception" translation-domain is a suggestion, maybe an even more general term would be better to put all the error messages and exception strings into those language files. Apart from the translations I added a little bit more coherent exception-handling to the PAIA driver which will now pass exceptions to the controller in case the PAIA service reports that the logged in patron has not sufficient rights (i.e. missing scopes) to do certain actions. Unfortunately the controller is kind of a dead end for these exceptions and will result in an uncaught exception which is not very pleasant to view for the user. I started to look at the MyResearchController and saw that #815 introduced some blocking-report-logic to the controller which aims at a similar direction. At least it seems appropriate for the PAIA scope Which brings me to the question if we should introduce ILSException handling in the MyResearchController in order to allow the ILS driver to pass messages to the user if certain actions fail. What do you think? |
|
@lahmann, have you seen #917? This introduces ILSException handling to the MyResearchController, as you suggest. However, that PR has currently hit a dead end because we're trying to figure out exactly which use cases it is useful for, particularly due to possible interactions with the "fail over to NoILS driver" functionality. Perhaps this PR adds some relevant use cases. Anyway, that's just one of several issues you bring up -- I'll try to review the rest of the new code before too long. Just wanted to get this idea out as soon as possible in case it is helpful. |
|
@demiankatz thanks for pointing me to #917! I tested it locally and it works fine for passing the exceptions to the frontend if actually passing the exception message to the flash messenger instead a generic string (but this is a small detail). So I guess #917 would be a great addition for the PAIA driver in this pull request or the other way round, this pull request seems to be a good use case for #917 ;) |
|
@lahmann, as I alluded to on #917, we have some issues regarding end-user-readable vs. admin-readable exception messages. In the current implementation, ILSException messages contain administrative details that may be useful to log, but which should not be displayed to a public user. That's why we always catch the exceptions but display a flat, standard flash message. By contrast, you are using human-readable messages in your implementation here. I think we may need to either develop a new exception type for these situations, or else add a custom method to the existing exception to extract a human-readable message. Adding a custom method might be the simpler solution, but my gut feeling is that it's not the right one. I think the core issue here is that right now, the current ILSException means "something has gone wrong with the ILS" while the way you are using it here appears to indicate "the user is not authorized to do something in the ILS." I wonder if a Forbidden exception might actually be better here, with a try..catch in the appropriate spot to offer cleaner feedback. Any thoughts? |
|
@demiankatz I think you are right! The way I use most of ILSExceptions in the PAIA driver could be framed more narrowly as ForbiddenException whereas ILSExceptions should be more technical ones resulting in more generic messages for the user. What do you think? |
|
@lahmann, VuFind already has a ForbiddenException. Are you proposing to create an ILS-oriented subclass, change the error handling, or something else? In any case, I think we are on the right track; just want to be sure about the details. |
|
@demiankatz no you are right, I simply wasn't aware of |
|
@lahmann, glad you've been able to make some progress on this. I'll wait to hear more about your testing progress before proceeding, but in the meantime, one more question: I notice that you now have some try..catch blocks where the catch clause does nothing but re-throw the exception. I don't think these serve any purpose (not catching an exception has the same result as catching and re-throwing it without processing); is there a reason they are there? |
|
I have merged the latest master into this branch and have resolved conflicts. However, I am not in a position to test this. Is anyone interested in moving this forward in @lahmann's absence? |
|
As I'm just working with this PAIA driver I'm going to have a look at it; but no promisses. |
|
Sorry, |
…or_handling_and_scopes
|
Thanks, @hajoseng -- even if there are parts of this that you think are worth merging, I'm happy to move some parts over to master to reduce the complexity of the remaining diffs on this side. It also looks like in some cases there are unnecessary |
… at change password view * concerns \finc\Driver\ILS\PAIA::changePassword w/ ILS authentification
* add return null by invalid login attempt * concerns \finc\Driver\ILS\PAIA::patronLogin * aids for better messaging if login failed due to invalid credentials
|
Hi Demian, I add some minor AuthException handling fixes of Sebastian for avoid misleading error messages by invalid credentials at patronLogin and changePassword action. Hope it will be fine to integrate it. Indeed, some exceptions seems to be useless and comparing to our original a few seems to be removed. I would like to analyse and review this in detail if I had have a good bunch of plenty time. For new content at PAIA specification we've already implemented I think me or any team member can place a new pull-request in the near future. |
|
Thanks, @ndege, this is great! What do you think is the best way to move forward? I imagine it will be easier for your team to open new pull requests if we first get this work merged. There are also two changes I would like to make, but I want to be sure that making these adjustments will not make things harder for you: 1.) I'd like to remove the unnecessary try..catch blocks we discussed. 2.) I'd like to rename all the |
|
Thanks @demiankatz! I agree with all your points and haven't any objections so far. If this merge will be finished we put a new pull-request with additions of PAIA driver we've already implemented. |
|
Thanks, @ndege. I'm on vacation this week but will take care of this when I return to the office. |
|
Superseded by #1319. |
These changes add better error handling as defined in PAIA specification (cf. http://gbv.github.io/paia/paia.html#request-errors). Errors are handled in a single place (
paiaHandleErrors()) and throw Exceptions by default which will be passed up to ILS-Connection with the error message provided by the PAIA service. This should result in more detailed error messages (via flashmessenger) for the user.According to the scopes reported for the logged in user by PAIA the user will now have limited actions available in MyResearch views (e.g. do not allow renew if user has not scope write_items).