Originally posted on bug 19799. See also 21267.
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>
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.
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.
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>
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>
(In reply to Tomás Cohen Arazi from comment #5) > Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Thanks!
Quick comments here: 1. localReferer is camelCase 2. Koha::Util sounds like a very bad idea (too generic)
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>
(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
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
(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 ?
Hmm.. could it perhaps sit within Koha::I18N or Koha::Template::Utils, Koha::Language::Utils.. not entirely opinionated about it to be honest.. :0
@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..
Nick: See previous comment
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?
(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.
Koha::Util::Navigation looks best for me, but no strong opinion here.
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>
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>
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>
(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..
Looks like you renamed in the package declaration but didn't rename the file?
Back in queue with the follow-up of course.
(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.
(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.
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
(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 ++++++------
OK, my mistake.. splinter review doesn't show file moves!
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>
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>
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>
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
Awesome work all! Pushed to master for 18.11
Enhancement, will not be backported to 18.05.x series.