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: 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: Marcel de Rooy
QA Contact: Martin Renvoize
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-08-31 06:47 UTC by Marcel de Rooy
Modified: 2019-10-14 19:56 UTC (History)
6 users (show)

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


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
Bug 21299: Add localReferer function to Util module, add test (5.96 KB, patch)
2018-10-25 09:49 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21299: Introduce localReferer in [opac-]changelanguage.pl (3.21 KB, patch)
2018-10-25 09:49 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21299: (QA follow-up) Rename module and subroutine (6.89 KB, patch)
2018-10-25 09:49 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21299: Add localReferer function to Util module, add test (5.98 KB, patch)
2018-10-29 10:23 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21299: Introduce localReferer in [opac-]changelanguage.pl (3.25 KB, patch)
2018-10-29 10:23 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21299: (QA follow-up) Rename module and subroutine (6.93 KB, patch)
2018-10-29 10:23 UTC, Martin Renvoize
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?
Comment 16 Marcel de Rooy 2018-10-25 08:31:48 UTC
(In reply to Jonathan Druart from comment #15)
> 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?

There is no real discussion going on here. See the comments above. And I asked already feedback, and I did it again just now.
Comment 17 Josef Moravec 2018-10-25 09:29:24 UTC
Koha::Util::Navigation looks best for me, but no strong opinion here.
Comment 18 Marcel de Rooy 2018-10-25 09:49:12 UTC
Created attachment 81120 [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 19 Marcel de Rooy 2018-10-25 09:49:17 UTC
Created attachment 81121 [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 20 Marcel de Rooy 2018-10-25 09:49:22 UTC
Created attachment 81122 [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::Navigation (after cmty feedback).

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

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 21 Marcel de Rooy 2018-10-25 09:50:06 UTC
(In reply to Josef Moravec from comment #17)
> Koha::Util::Navigation looks best for me, but no strong opinion here.

Great. Good enough for me. Lets pull this out of discussion now..
Comment 22 Martin Renvoize 2018-10-28 12:33:03 UTC
Looks like you renamed in the package declaration but didn't rename the file?
Comment 23 Katrin Fischer 2018-10-28 12:37:24 UTC
Back in queue with the follow-up of course.
Comment 24 Marcel de Rooy 2018-10-29 09:14:51 UTC
(In reply to Martin Renvoize from comment #22)
> Looks like you renamed in the package declaration but didn't rename the file?

ll Koha/Util/
total 28
-rw-r--r-- 1 dev-koha dev-koha 1851 Dec 18  2017 FrameworkPlugin.pm
-rw-r--r-- 1 dev-koha dev-koha 5472 Dec 18  2017 MARC.pm
-rw-r--r-- 1 dev-koha dev-koha 2410 Oct 29 10:13 Navigation.pm
-rw-r--r-- 1 dev-koha dev-koha 2077 Dec 18  2017 Normalize.pm
-rw-r--r-- 1 dev-koha dev-koha 5049 Oct 10 14:34 StockRotation.pm

Please check, Martin.
Comment 25 Marcel de Rooy 2018-10-29 09:18:32 UTC
(In reply to Katrin Fischer from comment #23)
> Back in queue with the follow-up of course.

I see no reason to fail these patches. Please clarify.
Back to SO in the meantime.
Comment 26 Martin Renvoize 2018-10-29 09:23:11 UTC
In the QA followup patch.. we've renamed the package decalaration

package Koha::Util; => package Koha::Util::Navigation;

but the file Koha/Util.pm was not moved to Koha/Util/Navigation.pm
Comment 27 Marcel de Rooy 2018-10-29 09:44:12 UTC
(In reply to Martin Renvoize from comment #26)
> In the QA followup patch.. we've renamed the package decalaration
> 
> package Koha::Util; => package Koha::Util::Navigation;
> 
> but the file Koha/Util.pm was not moved to Koha/Util/Navigation.pm

 Koha/{Util.pm => Util/Navigation.pm}         | 12 ++++++------
Comment 28 Martin Renvoize 2018-10-29 09:52:25 UTC
OK, my mistake.. splinter review doesn't show file moves!
Comment 29 Martin Renvoize 2018-10-29 10:23:28 UTC
Created attachment 81470 [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>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 30 Martin Renvoize 2018-10-29 10:23:32 UTC
Created attachment 81471 [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>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 31 Martin Renvoize 2018-10-29 10:23:35 UTC
Created attachment 81472 [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::Navigation (after cmty feedback).

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

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 32 Martin Renvoize 2018-10-29 10:24:42 UTC
Passing QA.. all works as expected, I can't find any regressions and the requested module name change has been made.

Thanks for persevering Marcel
Comment 33 Nick Clemens 2018-11-07 23:16:37 UTC
Awesome work all!

Pushed to master for 18.11
Comment 34 Martin Renvoize 2018-11-16 12:52:05 UTC
Enhancement, will not be backported to 18.05.x series.