Bug 21299 - Move referer code from changelanguage to module in opac and staff
Summary: Move referer code from changelanguage to module in opac and staff
Status: Signed Off
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Marcel de Rooy
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-08-31 06:47 UTC by Marcel de Rooy
Modified: 2018-10-15 13:58 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 21299: Add localReferer function to Util module, add test (5.90 KB, patch)
2018-08-31 08:43 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21299: Introduce localReferer in [opac-]changelanguage.pl (3.16 KB, patch)
2018-08-31 08:43 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21299: Add localReferer function to Util module, add test (5.92 KB, patch)
2018-08-31 16:10 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21299: Introduce localReferer in [opac-]changelanguage.pl (3.18 KB, patch)
2018-08-31 16:11 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21299: (QA follow-up) Rename module and subroutine (6.68 KB, patch)
2018-09-13 06:44 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marcel de Rooy 2018-08-31 06:47:32 UTC
Originally posted on bug 19799. See also 21267.
Comment 1 Marcel de Rooy 2018-08-31 08:43:25 UTC
Created attachment 78324 [details] [review]
Bug 21299: Add localReferer function to Util module, add test

Before implementing the change in [opac-]changelanguage, we define a new
function (in Koha/Util.pm) and add tests.

Test plan:
Run t/Koha/Util/localReferer.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 2 Marcel de Rooy 2018-08-31 08:43:30 UTC
Created attachment 78325 [details] [review]
Bug 21299: Introduce localReferer in [opac-]changelanguage.pl

With the work from the former patch, we now have a nice and easy replace..

NOTE: The additional logic in opac-changelanguage.pl is now also applied
to the intranet counterpart (changelanguage.pl). This effectively makes
changelanguage.pl no longer refer to external URLs (as the opac already
did by testing the referer). Might be just theoretical, but consistent.

Test plan:
Test changing language from various OPAC and staff locations.
Include language=[some_language] in the URL before changing language.
[Bonus] Test calling changelanguage.pl from another server. You should no
longer be referred back to the calling server. (See below.)

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Added link <a href="[mystaffserver]/cgi-bin/koha/changelanguage.pl?language=en">link</a> somewhere on external server.
Verified that without this patch, it hits the staff server and refers back to the caller. But with this patch, it refers to staff homepage.
Comment 3 Marcel de Rooy 2018-08-31 08:48:08 UTC
Tomas, this begs for your signoff now :) Note that I found some extra argument for this code in the staff script still allowing external referrals.. And more consistency.
Comment 4 Tomás Cohen Arazi 2018-08-31 16:10:56 UTC
Created attachment 78334 [details] [review]
Bug 21299: Add localReferer function to Util module, add test

Before implementing the change in [opac-]changelanguage, we define a new
function (in Koha/Util.pm) and add tests.

Test plan:
Run t/Koha/Util/localReferer.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 5 Tomás Cohen Arazi 2018-08-31 16:11:01 UTC
Created attachment 78335 [details] [review]
Bug 21299: Introduce localReferer in [opac-]changelanguage.pl

With the work from the former patch, we now have a nice and easy replace..

NOTE: The additional logic in opac-changelanguage.pl is now also applied
to the intranet counterpart (changelanguage.pl). This effectively makes
changelanguage.pl no longer refer to external URLs (as the opac already
did by testing the referer). Might be just theoretical, but consistent.

Test plan:
Test changing language from various OPAC and staff locations.
Include language=[some_language] in the URL before changing language.
[Bonus] Test calling changelanguage.pl from another server. You should no
longer be referred back to the calling server. (See below.)

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Added link <a href="[mystaffserver]/cgi-bin/koha/changelanguage.pl?language=en">link</a> somewhere on external server.
Verified that without this patch, it hits the staff server and refers back to the caller. But with this patch, it refers to staff homepage.
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 6 Marcel de Rooy 2018-09-03 06:29:52 UTC
(In reply to Tomás Cohen Arazi from comment #5)
> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Thanks!
Comment 7 Jonathan Druart 2018-09-12 23:23:17 UTC
Quick comments here:
1. localReferer is camelCase
2. Koha::Util sounds like a very bad idea (too generic)
Comment 8 Marcel de Rooy 2018-09-13 06:44:17 UTC
Created attachment 78618 [details] [review]
Bug 21299: (QA follow-up) Rename module and subroutine

See QA request at comment7.
Removing camelcase from subroutinename.
Renaming module to Koha::Util::CGI.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 9 Marcel de Rooy 2018-09-13 06:44:45 UTC
(In reply to Jonathan Druart from comment #7)
> Quick comments here:
> 1. localReferer is camelCase
> 2. Koha::Util sounds like a very bad idea (too generic)

Fixed
Comment 10 Jonathan Druart 2018-09-14 19:33:16 UTC
I'd like other QA point of views. I already had the need of a Koha::CGI, based on CGI - https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=66045
Comment 11 Marcel de Rooy 2018-09-17 07:37:05 UTC
(In reply to Jonathan Druart from comment #10)
> I'd like other QA point of views. I already had the need of a Koha::CGI,
> based on CGI -
> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=66045

This is not the same point however. Koha::Util::CGI includes one very specific CGI related utility function. Koha::CGI is a generic interface between Koha and CGI.pm.

Also note that Koha::Util::CGI is just a suggestion for a better name. But for me it could be something else too: Util::Navigation or Redirection or ? Do you have another idea/suggestion ?

Tomas/Martin: Any feedback on the name ?
Comment 12 Martin Renvoize 2018-10-12 14:29:28 UTC
Hmm.. could it perhaps sit within Koha::I18N or Koha::Template::Utils, Koha::Language::Utils.. not entirely opinionated about it to be honest.. :0
Comment 13 Marcel de Rooy 2018-10-15 07:07:45 UTC
@Nick: This is some sort of QA deadlock about the module name. Can we push this further now? If we find a better name, it will be trivial to do the rename operation..
Comment 14 Marcel de Rooy 2018-10-15 07:08:17 UTC
Nick: See previous comment
Comment 15 Jonathan Druart 2018-10-15 13:58:29 UTC
We are not stuck, we are discussing about a better name. The patches are not blocking another work and are not vital/a priority. Maybe we should ask other QAers?