Bug 15288

The problem is related to all error pages (400, 401, 402, 403, 404 500).

Changing Bug title to "Staff error pages: Code duplication removal and better translatability"

Why you leave 405.tt even though you don't touch 405.pl?


><li>To report a broken link or any other issue, please contact the Koha >Administrator <a href="mailto:[% admin %]">Email</a></li>

I think (IMHO) that you can change "Email" for "Email the administrator" or something like that.

Another thing is that the OPAC has the same error pages.

This patch could annoy people who have customized their template error pages.
I don't know how popular is it, but I am not confident to pass QA on it without any feedbacks.

Could you ask on koha[-devel] or irc please?

(In reply to Jonathan Druart from comment #13)
> Could you ask on koha[-devel] or irc please?

http://irc.koha-community.org/koha/2015-12-10#i_1766309
http://irc.koha-community.org/koha/2015-12-10#i_1766411

Hm, I think we will always have changes happen that will have people rework their customizations a bit. Changes make sense to me on first glance, so in my opinion I wouldn't block it. Jonathan, do you have a specific change/use case in mind?

(In reply to Katrin Fischer from comment #15)
> Hm, I think we will always have changes happen that will have people rework
> their customizations a bit. Changes make sense to me on first glance, so in
> my opinion I wouldn't block it. Jonathan, do you have a specific change/use
> case in mind?

If a sys admin has customized the different error pages, the changes will be erased. I agree that it's the same for all the specific changes made in templates, but it is much more easier to customize error pages.
With only 1 template to drive the different error pages, it will be harder to customize them.
If nobody used to customize them, so let's refactore them.
I have just told that I am not aware of the different practices and maybe it's not worth doing this change if it will overload sys admins in the next version.

I think I don't quite understand since it would have never occurred to me to customize them (apart from CSS/Javascript). I have checked the code - they will still show a unique class for the different errors in the body tag - won't that be enough to customize?

(In reply to Marc Véron from comment #14)
> (In reply to Jonathan Druart from comment #13)
> > Could you ask on koha[-devel] or irc please?
> 
> http://irc.koha-community.org/koha/2015-12-10#i_1766309
> http://irc.koha-community.org/koha/2015-12-10#i_1766411

I asked on IRC devel list as well:
http://lists.koha-community.org/pipermail/koha-devel/2015-December/042131.html

Best practice would probably be to use the explicitly numbered error code page
template if it exists.

Eg. If errors/400.tt exists, then use that for error 400, otherwise use errors/errorpage.tt

This way it will not clobber any local modifications.

I am not sure we should build in hooks for template modifications like this - we have a template system if people really really want to do this, with a fallback to the standard template, already.

I agree, if people have modified template files (which the error pages are) and haven't either 1, used any version control, or 2 backed up a copy I don't think we need to care. It's the same as editing any of the files IMHO.

What is now the status of this bug - can we switch back to Signed off?

imo the argument of adjusted error templates is not really valid; as Chris and Katrin already pointed out, this holds for any template.
So I would not oppose these changes.
But I think the new template text needs more attention. Two examples. Perhaps check text changes with a native speaker?

400: Koha is pointed an invalid link.
Is this valid English?
402: Same string.
These are separate http status codes. They should not have the same message.

Could you try to keep close to the original messages for http statuses?

Search for better wording for 400 / 402:

See: https://tools.ietf.org/html/rfc2616#section-10.4

10.4.1 400 Bad Request
   The request could not be understood by the server due to malformed
   syntax. The client SHOULD NOT repeat the request without
   modifications.

10.4.2 401 Unauthorized
   The request requires user authentication. (...)

10.4.3 402 Payment Required
   This code is reserved for future use.

10.4.4 403 Forbidden
   The server understood the request, but is refusing to fulfill it.
   Authorization will not help (...)


10.4.5 404 Not Found
   The server has not found anything matching the Request-URI. (...)
   This status code is commonly used when the server does not wish to
   reveal exactly why the request has been refused, or when no other
   response is applicable.


10.5.1 500 Internal Server Error
   The server encountered an unexpected condition which prevented it
   from fulfilling the request.


Porposals for the template:

400 (New wording):
<li>The request could not be understood by the server due to malformed syntax.</li>

401 (no changes to patch):
<li>You are trying to access a link that you're not authorized to see.</li>
<li>Try logging in to the catalog.</li>

402 (remove):
It seems that 402 is not used (git grep '402.pl')
Can be removed.

403 (no changes to patch):
<li>You are forbidden for some reason to see this page.</li>

404 (no changes to patch):
<li>You made use of an external link to a catalog item that is no longer available.</li>
<li>You followed an outdated link e.g. from a search engine or a bookmark.</li>
<li>You tried to access a page that needs authentification.</li>
<li>An internal link in our catalog home page is broken and the page does not exist.</li>

405:
<li>An internal error occurred while processing your request.</li>

Hi Marc you can use the paragraphs from bug 14573, like Aleisha proposes. What do you think about it, Marcel and Aleisha?

Regards

(In reply to Héctor Eduardo Castro Avalos from comment #25)
> Hi Marc you can use the paragraphs from bug 14573, like Aleisha proposes.
> What do you think about it, Marcel and Aleisha?
> 
> Regards

I think it is a good idea to use the messages from Bug 14573 because then they will be consistent with the Intranet error messages (see Bug 14574)

Hi Aleisha, as a native speaker, would you have time to do a follow up with your wording? - That would be great.

(In reply to Marc Véron from comment #27)
> Hi Aleisha, as a native speaker, would you have time to do a follow up with
> your wording? - That would be great.

Have you had a look at my error messages from Bug 14573? I think this bug is basically a duplicate of 14573.

Needs sign-off for the followup (Better wording).

QA: Having a look now

Created attachment 46322 [details] [review]
Bug 15288: Error pages: Code duplication removal and better translatability

This patch changes the 400, 401, 402, 403, 404, 500 error pages to be handled
by only one template for staff and only one template for OPAC.

Additionally it removes a translatability issues due to sentence splitings by html tags.

To test:
- Apply patch
- Trigger 404 error for staff and for OPAC by calling a page that does not exist
- Try to trigger other error pages and/or carefully review code changes
  in the *.pl files
- Review koha-tmpl/opac-tmpl/bootstrap/en/modules/errors/errorpage.tt and
  koha-tmpl/intranet-tmpl/prog/en/modules/errorpage.tt

(Amended to fix a typo)
(Amended for comment #6)
(Amended to cover OPAC error pages as well)

Signed-off-by: Hector Castro <hector.hecaxmmx@gmail.com>
Works as advertised

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 46323 [details] [review]
Bug 15288: (followup) Remove obsolete *.tt files

Follwoing files were not yet deleted:

koha-tmpl/opac-tmpl/bootstrap/en/modules/errors/400.tt
koha-tmpl/opac-tmpl/bootstrap/en/modules/errors/401.tt
koha-tmpl/opac-tmpl/bootstrap/en/modules/errors/402.tt
koha-tmpl/opac-tmpl/bootstrap/en/modules/errors/403.tt
koha-tmpl/opac-tmpl/bootstrap/en/modules/errors/500.tt

Note: the 404.tt file was renamed to errorpage.tt

To test: Verify that files are deleted.

Signed-off-by: Hector Castro <hector.hecaxmmx@gmail.com>
Files removed, new errorpage.tt added

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 46324 [details] [review]
Bug 15288: (followup) Better wording for OPAC error page

This patch changes the wording of the OPAC error page (See Bug 14573).

Additionally, it displays the link to the login page only if the system
preference 'opacuserlogin' is set.

To test:
- Apply patch
- Review koha-tmpl/opac-tmpl/bootstrap/en/modules/errors/errorpage.tt

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
No errors

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Amended: Replaced following reasons by 'the following reasons'. Reads better.

QA Comment:
Thanks for the follow-up, Marc.
Looks good to me now.

No blockers, but maybe for future improvement:
[1] For more consistency, it would be nice to have the same reasons for an error listed on opac and intranet. Perhaps from a shared include or so? At this time the opac is more specific.
[2] I would not mind listing the original status message after Error <number> on the same line. 
Something like Error 403 Forbidden or Error 404 Not Found
[3] Perhaps removing 402 is better than saying Invalid link. We do not use it in Koha, it is supposed to mean Payment required.

Passed QA

(In reply to Marcel de Rooy from comment #36)

> [1] For more consistency, it would be nice to have the same reasons for an
> error listed on opac and intranet. Perhaps from a shared include or so? At
> this time the opac is more specific.

This is exactly why I suggested using the error messages from Bug 14573 which needed signing off, because the error messages would then be consistent with those on Bug 14574, which has been pushed

*** Bug 14573 has been marked as a duplicate of this bug. ***

Pushed to Master - Should be in the May 2016 release.  Thanks!

Please fix t_db_dependent_Auth_t.The file errors/500.tt should be accessible (contains integers) (734)

http://jenkins.koha-community.org/job/Koha_Master_D8/lastBuild/testReport/junit/%28root%29/t_db_dependent_Auth_t/The_file_errors_500_tt_should_be_accessible__contains_integers___734_/

Created attachment 47464 [details] [review]
Bug 15288: (follow-up) Make t/db_dependent/Auth.t pass

Changed test from (deleted) 500.tt to erroroage.tt

Created attachment 48476 [details] [review]
Bug 15288: (QA followup) Tests still point to old 500.tt file

This patch fixes the t/db_dependent/Auth.t so they don't look for the removed
file and use the new one instead.

To test:
- On master, run
  $ prove t/db_dependent/Auth.t
=> FAIL: You get something similar to this:

- Apply the patch
- Re-run the tests
=> SUCCESS: Tests now pass.

Signed-off-by: Tomas Cohen Arazi <tomascohen@unc.edu.ar>

Sending to the RM queue. Please feel free to adjust it. The old test seems to be testing for the file's existence. Not sure how good that test was, but needed updating for the new file name.

Didn't apply cleanly - I know because of the moving target that master is.  Please rebase.  Once it's done - leave me an @later in #koha and I'll jump right on it.  Thank you so much for the time and bother of rebasing.

Brendan

(In reply to Brendan Gallagher from comment #44)
> Didn't apply cleanly - I know because of the moving target that master is. 
> Please rebase.  Once it's done - leave me an @later in #koha and I'll jump
> right on it.  Thank you so much for the time and bother of rebasing.
> 
> Brendan

Sorry - my mistake - need the last patch - that applies :)

Pushed last patch - thanks!