Description
Martin Renvoize (ashimema)
2019-06-07 08:31:04 UTC
Created attachment 90440 [details] [review] Bug 23075: Better error handling in Mana config Currently, if you input an incorrect mana service URL in your config (http rather than https for example) the error message you are given is the direct output of a failed json parse. We should be able to catch such failures and display a more meaningful error to the end user. This patch makes it display a more friendly message. I wanted to test this, but couldn't figure it out! Any hints or direction would be appreciated. Is this talking about etc/koha-config.xml and the URL in there? For testing is this the URL to configure in koha-config.xml? https://mana-test.koha-community.org/ Or is it once you have it enabled and working, that is you have enabled and added a token? Up-to-date on master. I haven't been to get registration to work either, I get these two error messages when on /cgi-bin/koha/admin/share_content.pl: - An error occurred, mana server returned: URL must be absolute - Mana configuration is currently empty. This feature will not work. Please contact your site administrator. (In reply to David Nind from comment #2) > I wanted to test this, but couldn't figure it out! Any hints or direction > would be appreciated. > > Is this talking about etc/koha-config.xml and the URL in there? For testing > is this the URL to configure in koha-config.xml? > https://mana-test.koha-community.org/ > > Or is it once you have it enabled and working, that is you have enabled and > added a token? David, just enter a non valid URL in mana_config and try to register via cgi-bin/koha/admin/share_content.pl (request a mana token). > > Up-to-date on master. > > I haven't been to get registration to work either, I get these two error > messages when on /cgi-bin/koha/admin/share_content.pl: > > - An error occurred, mana server returned: URL must be absolute > > - Mana configuration is currently empty. This feature will not work. Please > contact your site administrator. As said, mana_config is empty. enter the following in your koha-conf.xml: <mana_config>https://mana-test.koha-community.org</mana_config> This makes we lost the response code ($result->{code} = $response->code;) We need it in some place to test the response. I.e: report search is broken Are you sure Alex? This patch did not remove the line you are referring to, it moved it above. Ha! my bad, got it. My explanation was not very clear, i admit :) Created attachment 90459 [details] [review] Bug 23075: Better error handling in Mana config Currently, if you input an incorrect mana service URL in your config (http rather than https for example) the error message you are given is the direct output of a failed json parse. We should be able to catch such failures and display a more meaningful error to the end user. This patch makes it display a more friendly message. I'm having difficulty getting mana enabled using koha-testing-docker. I assume I have to update the instance configuration file in /etc/koha/sites/kohadev/koha-conf.xml I'll try kohadevbox (but last time I tried this I had problems with getting a local git repository and nfs working). (In reply to David Nind from comment #9) > I'm having difficulty getting mana enabled using koha-testing-docker. Solved my problem! Needed to go flush_memcached and the restart_all to get changes to /etc/koha/sites/kohadev/koha-conf.xml recognised. Created attachment 90594 [details] [review] Bug 23075: Better error handling in Mana config Currently, if you input an incorrect mana service URL in your config (http rather than https for example) the error message you are given is the direct output of a failed json parse. We should be able to catch such failures and display a more meaningful error to the end user. This patch makes it display a more friendly message. Test plan: 1. Add <mana_config>http://mana-test.koha-community.org</mana_config> to /etc/koha/sites/<instancename>/koha-conf.xml (see etc/koha-conf.xml in the Koha code repositoty for an example of where to add this; <instancename> if using koha-testing-docker is kohadev). 2. Clear memcached and restart services so that the changes in your Koha instance configuration are recognised (if using koha-testing-docker enter flush_memcached and then restart_all). 3. From the staff client home page go to Koha administration > Additional parameters > Share content with Mana KB. 4. Enable content sharing: change 'Use Mana KB for sharing content' to Yes and press Save. 5. Enter your first name, last name and email address in the Configure Mana KB section of the page and then press Send to Mana KB. 6. An error message is displayed, this may be something like 'malformed JSON string, neither tag, array, object, number, string or atom, at character offset 0 (before "<html>\r\n<head><tit...") at /usr/share/perl5/JSON.pm line 171.' 7. Apply the patch (git so bz apply 23075). 8. Change the Mana config to the correct URL: <mana_config>https://mana-test.koha-community.org</mana_config> 9. Repeat steps 2,3 and 5. 10. A more friendly/human readable error message is now displayed: 'Error trying to connect or read response from the Mana server (Code: 301) Moved Permanently'. Signed-off-by: David Nind <david@davidnind.com> Ignore step 8 in the test plan! Created attachment 90595 [details] [review] Bug 23075: Better error handling in Mana config Currently, if you input an incorrect mana service URL in your config (http rather than https for example) the error message you are given is the direct output of a failed json parse. We should be able to catch such failures and display a more meaningful error to the end user. This patch makes it display a more friendly message. Test plan: 1. Add <mana_config>http://mana-test.koha-community.org</mana_config> to /etc/koha/sites/<instancename>/koha-conf.xml (see etc/koha-conf.xml in the Koha code repositoty for an example of where to add this; <instancename> if using koha-testing-docker is kohadev). 2. Clear memcached and restart services so that the changes in your Koha instance configuration are recognised (if using koha-testing-docker enter flush_memcached and then restart_all). 3. From the staff client home page go to Koha administration > Additional parameters > Share content with Mana KB. 4. Enable content sharing: change 'Use Mana KB for sharing content' to Yes and press Save. 5. Enter your first name, last name and email address in the Configure Mana KB section of the page and then press Send to Mana KB. 6. An error message is displayed, this may be something like 'malformed JSON string, neither tag, array, object, number, string or atom, at character offset 0 (before "<html>\r\n<head><tit...") at /usr/share/perl5/JSON.pm line 171.' 7. Apply the patch (git so bz apply 23075). 8. Change the Mana config to the correct URL: <mana_config>https://mana-test.koha-community.org</mana_config>9. Repeat steps 2,3 and 5. 9. A more friendly/human readable error message is now displayed: 'Error trying to connect or read response from the Mana server (Code: 301) Moved Permanently'. Signed-off-by: David Nind <david@davidnind.com> Created attachment 90596 [details] [review] Bug 23075: Better error handling in Mana config Currently, if you input an incorrect mana service URL in your config (http rather than https for example) the error message you are given is the direct output of a failed json parse. We should be able to catch such failures and display a more meaningful error to the end user. This patch makes it display a more friendly message. Test plan: 1. Add <mana_config>http://mana-test.koha-community.org</mana_config> to /etc/koha/sites/<instancename>/koha-conf.xml (see etc/koha-conf.xml in the Koha code repositoty for an example of where to add this; <instancename> if using koha-testing-docker is kohadev). 2. Clear memcached and restart services so that the changes in your Koha instance configuration are recognised (if using koha-testing-docker enter flush_memcached and then restart_all). 3. From the staff client home page go to Koha administration > Additional parameters > Share content with Mana KB. 4. Enable content sharing: change 'Use Mana KB for sharing content' to Yes and press Save. 5. Enter your first name, last name and email address in the Configure Mana KB section of the page and then press Send to Mana KB. 6. An error message is displayed, this may be something like 'malformed JSON string, neither tag, array, object, number, string or atom, at character offset 0 (before "<html>\r\n<head><tit...") at /usr/share/perl5/JSON.pm line 171.' 7. Apply the patch (git so bz apply 23075). 8. Repeat steps 2,3 and 5. 9. A more friendly/human readable error message is now displayed: 'Error trying to connect or read response from the Mana server (Code: 301) Moved Permanently'. Signed-off-by: David Nind <david@davidnind.com> Created attachment 90606 [details] [review] Bug 23075: Better error handling in Mana config Currently, if you input an incorrect mana service URL in your config (http rather than https for example) the error message you are given is the direct output of a failed json parse. We should be able to catch such failures and display a more meaningful error to the end user. This patch makes it display a more friendly message. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Nice work! Pushed to master for 19.11.00 Oops, this one slipped passed. I'm going to revert as I don't think the solution here is tangible unfortunately. The Mana server itself may return codes that are still meaningful even if they're not 200.. i.e. 201 has a meaning of it's own and as we're short-circuiting anything that's not 200 in this patch we'll be dropping OK responses of kinds. I have a plan and will post an alternative shortly. Created attachment 90682 [details] [review] Bug 23075: Report if mana-kb server is not found This patch adds a status check into the mana configuration page and prompts the user to ask an adminstrator to check the configured mana service url. I've submitted an alternative resolution to this bug above. This alternative checks for a known response for a mana server to identify if the URL appears to be correct. It relies on bug 22913 at the mana-kb serverside to match this known state.. Patch now fails to apply. Created attachment 94098 [details] [review] Bug 23075: Report if mana-kb server is not found This patch adds a status check into the mana configuration page and prompts the user to ask an adminstrator to check the configured mana service url. Still getting message like 'malformed JSON string, neither tag, array, object, number, string or atom, at character offset 0 (before "<html>\r\n<head><tit...") at /usr/share/perl5/JSON.pm line 171.' when URL is incorrect. (have run flush_memcached and restart_all). Confirmed. Maybe a change server-side? $result is: \ { code 200, description "Mana KB is a global knowledge base for library-centric data. It has been designed initially to interact with Koha, the Open Source ILS, but can be used by any other software.", title "Mana KB API", version 0.1 } (In reply to David Nind from comment #22) > Still getting message like 'malformed JSON string, neither tag, array, > object, number, string or atom, at character offset 0 (before > "<html>\r\n<head><tit...") at /usr/share/perl5/JSON.pm line 171.' when URL > is incorrect. (have run flush_memcached and restart_all). Hmm, where are you seeing this exactly.. I'm struggling to replicate it Created attachment 94363 [details]
screenshot
This is what I see with the patch applied.
(In reply to Martin Renvoize from comment #25) > Created attachment 94363 [details] > screenshot > > This is what I see with the patch applied. Try with http://mana-test.koha-community.org (In reply to Jonathan Druart from comment #26) > (In reply to Martin Renvoize from comment #25) > > Created attachment 94363 [details] > > screenshot > > > > This is what I see with the patch applied. > > Try with http://mana-test.koha-community.org But that is a 'good' URL.. so I don't expect to see any errors.. The https version works fine, the http version produces errors as it is not a 'valid' URL. Failing because of David's last comment, can you have a look Martin? Still confused.. it should warn you when using http and not https as that's an invalid mana url. http://neverssl.com/mana-test.koha-community.org What I see https://snipboard.io/bWxT4B.jpg with <mana_config>http://mana-test.koha-community.org</mana_config> Created attachment 95000 [details] [review] Bug 23075: Report if mana-kb server is not found This patch adds a status check into the mana configuration page and prompts the user to ask an adminstrator to check the configured mana service url. Pushed yet another rebased version.. but nothing has actually changed in the code.. I can't reproduce your issues, and can't see in the code how you would get that either. :( Created attachment 95003 [details] [review] Bug 23075: (follow-up) Dissallow transparent redirects GET request redirects were causing the http URL to 'pass' our 'good url' test for http => https redirected mana servers. However, on the subsquent POST request to get a mana security token we would fail as the POST was not transparently redirected. This patch changes the request to be a simple_request so we will catch bad urls earlier and it also prevents submissions for security tokens if the configured URL is identified as bad. Bad to needs signoff.. after a long chat with Jonathan on IRC I managed to replicate it. Created attachment 95009 [details] [review] Bug 23075: Do not 'Send' if the url is not correct Just in case. Created attachment 95074 [details] [review] Bug 23075: Report if mana-kb server is not found This patch adds a status check into the mana configuration page and prompts the user to ask an adminstrator to check the configured mana service url. Signed-off-by: David Nind <david@davidnind.com> Created attachment 95075 [details] [review] Bug 23075: (follow-up) Dissallow transparent redirects GET request redirects were causing the http URL to 'pass' our 'good url' test for http => https redirected mana servers. However, on the subsquent POST request to get a mana security token we would fail as the POST was not transparently redirected. This patch changes the request to be a simple_request so we will catch bad urls earlier and it also prevents submissions for security tokens if the configured URL is identified as bad. Signed-off-by: David Nind <david@davidnind.com> Created attachment 95076 [details] [review] Bug 23075: Do not 'Send' if the url is not correct Just in case. Signed-off-by: David Nind <david@davidnind.com> Thanks Jonathan and Martin for your perseverance in resolving this! Message if an incorrect URL in config now says 'Cannot detect mana server at http://mana-test.koha-community.org, please ask an administrator to check your configuration.' Created attachment 95078 [details] [review] Bug 23075: Report if mana-kb server is not found This patch adds a status check into the mana configuration page and prompts the user to ask an adminstrator to check the configured mana service url. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 95079 [details] [review] Bug 23075: (follow-up) Dissallow transparent redirects GET request redirects were causing the http URL to 'pass' our 'good url' test for http => https redirected mana servers. However, on the subsquent POST request to get a mana security token we would fail as the POST was not transparently redirected. This patch changes the request to be a simple_request so we will catch bad urls earlier and it also prevents submissions for security tokens if the configured URL is identified as bad. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 95080 [details] [review] Bug 23075: Do not 'Send' if the url is not correct Just in case. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 95081 [details] [review] Bug 23075: Correct indendation Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Nice work! Pushed to master for 19.11.00 |