Bug 15288 - Error pages: Code duplication removal and better translatability
Summary: Error pages: Code duplication removal and better translatability
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Marc Véron
QA Contact: Marcel de Rooy
URL:
Keywords:
: 14573 (view as bug list)
Depends on:
Blocks:
 
Reported: 2015-12-03 09:10 UTC by Marc Véron
Modified: 2017-06-14 22:05 UTC (History)
10 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 15288: Staff error pages: Code duplication removal and better translatability (12.92 KB, patch)
2015-12-03 14:48 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 15288: Staff error pages: Code duplication removal and better translatability (14.09 KB, patch)
2015-12-03 14:55 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 15288: Staff error pages: Code duplication removal and better translatability (17.05 KB, patch)
2015-12-03 15:07 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 15288: Staff error pages: Code duplication removal and better translatability (17.08 KB, patch)
2015-12-03 15:17 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 15288: Error pages: Code duplication removal and better translatability (27.77 KB, patch)
2015-12-03 23:37 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 15288: Error pages: Code duplication removal and better translatability (27.77 KB, patch)
2015-12-04 19:27 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 15288: (followup) Remove obsolete *.tt files (12.07 KB, patch)
2015-12-04 20:13 UTC, Marc Véron
Details | Diff | Splinter Review
[SIGNED-OFF]Bug 15288: Error pages: Code duplication removal and better translatability (27.85 KB, patch)
2015-12-04 21:20 UTC, Héctor Eduardo Castro Avalos
Details | Diff | Splinter Review
[SIGNED-OFF]Bug 15288: (followup) Remove obsolete *.tt files (12.18 KB, patch)
2015-12-04 21:20 UTC, Héctor Eduardo Castro Avalos
Details | Diff | Splinter Review
Bug 15288: (followup) Better wording for OPAC error page (3.70 KB, patch)
2016-01-06 05:54 UTC, Marc Véron
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 15288: (followup) Better wording for OPAC error page (3.79 KB, patch)
2016-01-06 11:27 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 15288: Error pages: Code duplication removal and better translatability (27.94 KB, patch)
2016-01-06 13:03 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 15288: (followup) Remove obsolete *.tt files (12.26 KB, patch)
2016-01-06 13:03 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 15288: (followup) Better wording for OPAC error page (4.20 KB, patch)
2016-01-06 13:03 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 15288: (follow-up) Make t/db_dependent/Auth.t pass (1.23 KB, patch)
2016-01-29 13:22 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 15288: (QA followup) Tests still point to old 500.tt file (1.51 KB, patch)
2016-02-29 17:32 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marc Véron 2015-12-03 09:10:46 UTC
On the Koha Error 404 page the following sentence is not properly translatable due to string splitting:

To report a broken link or any other issue, please send an email to the Koha Administrator

In German, it results in:
Um diesen Fehler zu melden, können Sie E-Mail an den Koha-Administrator.
Comment 1 Marc Véron 2015-12-03 14:23:37 UTC
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"
Comment 2 Marc Véron 2015-12-03 14:48:40 UTC Comment hidden (obsolete)
Comment 3 Marc Véron 2015-12-03 14:55:02 UTC Comment hidden (obsolete)
Comment 4 Marc Véron 2015-12-03 15:07:12 UTC Comment hidden (obsolete)
Comment 5 Marc Véron 2015-12-03 15:17:03 UTC Comment hidden (obsolete)
Comment 6 Héctor Eduardo Castro Avalos 2015-12-03 20:33:11 UTC
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.
Comment 7 Marc Véron 2015-12-03 23:37:52 UTC Comment hidden (obsolete)
Comment 8 Marc Véron 2015-12-04 19:27:04 UTC Comment hidden (obsolete)
Comment 9 Marc Véron 2015-12-04 20:13:41 UTC Comment hidden (obsolete)
Comment 10 Héctor Eduardo Castro Avalos 2015-12-04 21:20:44 UTC Comment hidden (obsolete)
Comment 11 Héctor Eduardo Castro Avalos 2015-12-04 21:20:54 UTC Comment hidden (obsolete)
Comment 12 Jonathan Druart 2015-12-10 10:44:39 UTC
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.
Comment 13 Jonathan Druart 2015-12-10 10:45:19 UTC
Could you ask on koha[-devel] or irc please?
Comment 14 Marc Véron 2015-12-11 08:54:52 UTC
(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
Comment 15 Katrin Fischer 2015-12-13 22:00:28 UTC
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?
Comment 16 Jonathan Druart 2015-12-14 10:38:11 UTC
(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.
Comment 17 Katrin Fischer 2015-12-14 10:49:27 UTC
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?
Comment 18 Marc Véron 2015-12-15 08:02:20 UTC
(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
Comment 19 paxed 2015-12-16 07:13:08 UTC
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.
Comment 20 Katrin Fischer 2015-12-16 07:21:52 UTC
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.
Comment 21 Chris Cormack 2015-12-16 07:34:33 UTC
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.
Comment 22 Marc Véron 2015-12-24 12:40:28 UTC
What is now the status of this bug - can we switch back to Signed off?
Comment 23 Marcel de Rooy 2015-12-28 09:41:51 UTC
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?
Comment 24 Marc Véron 2015-12-28 10:37:23 UTC
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>
Comment 25 Héctor Eduardo Castro Avalos 2015-12-29 02:51:07 UTC
Hi Marc you can use the paragraphs from bug 14573, like Aleisha proposes. What do you think about it, Marcel and Aleisha?

Regards
Comment 26 Aleisha Amohia 2015-12-29 19:28:42 UTC
(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)
Comment 27 Marc Véron 2015-12-30 09:01:25 UTC
Hi Aleisha, as a native speaker, would you have time to do a follow up with your wording? - That would be great.
Comment 28 Aleisha Amohia 2016-01-06 00:28:29 UTC
(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.
Comment 29 Marc Véron 2016-01-06 05:54:30 UTC Comment hidden (obsolete)
Comment 30 Marc Véron 2016-01-06 05:56:34 UTC
Needs sign-off for the followup (Better wording).
Comment 31 Bernardo Gonzalez Kriegel 2016-01-06 11:27:47 UTC Comment hidden (obsolete)
Comment 32 Marcel de Rooy 2016-01-06 12:40:33 UTC
QA: Having a look now
Comment 33 Marcel de Rooy 2016-01-06 13:03:42 UTC
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>
Comment 34 Marcel de Rooy 2016-01-06 13:03:48 UTC
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>
Comment 35 Marcel de Rooy 2016-01-06 13:03:52 UTC
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.
Comment 36 Marcel de Rooy 2016-01-06 13:10:04 UTC
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
Comment 37 Aleisha Amohia 2016-01-06 23:32:49 UTC
(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
Comment 38 Aleisha Amohia 2016-01-11 22:00:13 UTC
*** Bug 14573 has been marked as a duplicate of this bug. ***
Comment 39 Brendan Gallagher 2016-01-27 05:58:25 UTC
Pushed to Master - Should be in the May 2016 release.  Thanks!
Comment 40 Jonathan Druart 2016-01-29 12:45:37 UTC
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_/
Comment 41 Marc Véron 2016-01-29 13:22:20 UTC
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
Comment 42 Tomás Cohen Arazi 2016-02-29 17:32:07 UTC
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>
Comment 43 Tomás Cohen Arazi 2016-02-29 17:34:40 UTC
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.
Comment 44 Brendan Gallagher 2016-03-02 22:47:07 UTC
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
Comment 45 Brendan Gallagher 2016-03-02 23:14:07 UTC
(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 :)
Comment 46 Brendan Gallagher 2016-03-02 23:20:01 UTC
Pushed last patch - thanks!