[Bug 17982] New: Working title
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17982 Bug ID: 17982 Summary: Working title Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: minor Priority: P5 - low Component: Templates Assignee: oleonard@myacpl.org Reporter: m.de.rooy@rijksmuseum.nl QA Contact: testopia@bugs.koha-community.org -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17982 --- Comment #1 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 59511 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=59511&action=edit Bug 17982: Fix the use of uniq in sub themelanguage Doing uniq( \@themes ) is useless. It will just return to you the only reference you gave it. List::MoreUtils::uniq requires a list instead of an arrayref. So it is a trivial fix that makes sub themelanguage return one theme instead of three themes like [ 'prog', 'prog', 'prog' ]. Note that Template->new inserts one or two include paths to TT for each of these three identical themes. Added a unit to be somewhat overcomplete :) Test plan: [1] Run t/db_dependent/Templates.t [2] Open a page on OPAC or intranet. (Did you restart Plack?) Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17982 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Summary|Working title |Fix the use of uniq in sub | |themelanguage Patch complexity|--- |Trivial patch Assignee|oleonard@myacpl.org |m.de.rooy@rijksmuseum.nl -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17982 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17982 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #59511|0 |1 is obsolete| | --- Comment #2 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 59537 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=59537&action=edit Bug 17982: Fix the use of uniq in sub themelanguage Doing uniq( \@themes ) is useless. It will just return to you the only reference you gave it. List::MoreUtils::uniq requires a list instead of an arrayref. So it is a trivial fix that makes sub themelanguage return one theme instead of three themes like [ 'prog', 'prog', 'prog' ]. Note that Template->new inserts one or two include paths to TT for each of these three identical themes. Added a unit test to be somewhat overcomplete :) Test plan: [1] Run t/db_dependent/Templates.t [2] Open a page on OPAC or intranet. (Did you restart Plack?) Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17982 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |17989 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17989 [Bug 17989] Working title -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17982 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #59537|0 |1 is obsolete| | --- Comment #3 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 59630 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=59630&action=edit Bug 17982: Fix the use of uniq in sub themelanguage Doing uniq( \@themes ) is useless. It will just return to you the only reference you gave it. List::MoreUtils::uniq requires a list instead of an arrayref. So it is a trivial fix that makes sub themelanguage return one theme instead of three themes like [ 'prog', 'prog', 'prog' ]. Note that Template->new inserts one or two include paths to TT for each of these three identical themes. Note: Removed a overcomplete unit test that basically tested uniq with uniq in order to remove a dependency for a more important bug report. Test plan: [1] Run t/db_dependent/Auth.t (triggering themelanguage) [2] Open a page on OPAC or intranet. (Did you restart Plack?) Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17982 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks|17989 | Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17989 [Bug 17989] Allow templates from plugin directories -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17982 --- Comment #4 from M. Tompsett <mtompset@hotmail.com> --- Comment on attachment 59630 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=59630 Bug 17982: Fix the use of uniq in sub themelanguage Review of attachment 59630: --> (https://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=17982&attachment=59630) ----------------------------------------------------------------- ::: C4/Templates.pm @@ +256,4 @@
my $where = $tmpl =~ /xsl$/ ? 'xslt' : 'modules'; for my $theme (@themes) { if ( -e "$htdocs/$theme/$lang/$where/$tmpl" ) { + return ( $theme, $lang, [ uniq(@themes) ] );
I'm not sure the parenthesis for uniq are necessary. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17982 --- Comment #5 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to M. Tompsett from comment #4)
Comment on attachment 59630 [details] [review] Bug 17982: Fix the use of uniq in sub themelanguage
Review of attachment 59630 [details] [review]: -----------------------------------------------------------------
::: C4/Templates.pm @@ +256,4 @@
my $where = $tmpl =~ /xsl$/ ? 'xslt' : 'modules'; for my $theme (@themes) { if ( -e "$htdocs/$theme/$lang/$where/$tmpl" ) { + return ( $theme, $lang, [ uniq(@themes) ] );
I'm not sure the parenthesis for uniq are necessary.
Seems to be a more a matter of taste. My personal preference is parentheses, only omitting them with builtins like open or print. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17982 --- Comment #6 from M. Tompsett <mtompset@hotmail.com> --- Created attachment 59747 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=59747&action=edit Bug 17982: Improve test coverage This test triggers the three cases that were modified. prove -v t/db_dependent/Templates.t -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17982 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #59747|0 |1 is obsolete| | --- Comment #7 from M. Tompsett <mtompset@hotmail.com> --- Created attachment 59748 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=59748&action=edit Bug 17982: Improve test coverage This test triggers the three cases that were modified. prove -v t/db_dependent/Templates.t -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17982 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #59630|0 |1 is obsolete| | --- Comment #8 from M. Tompsett <mtompset@hotmail.com> --- Created attachment 59749 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=59749&action=edit Bug 17982: Fix the use of uniq in sub themelanguage Doing uniq( \@themes ) is useless. It will just return to you the only reference you gave it. List::MoreUtils::uniq requires a list instead of an arrayref. So it is a trivial fix that makes sub themelanguage return one theme instead of three themes like [ 'prog', 'prog', 'prog' ]. Note that Template->new inserts one or two include paths to TT for each of these three identical themes. Note: Removed a overcomplete unit test that basically tested uniq with uniq in order to remove a dependency for a more important bug report. Test plan: [1] Run t/db_dependent/Auth.t (triggering themelanguage) [2] Open a page on OPAC or intranet. (Did you restart Plack?) Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Mark Tompsett <mtompset@hotmail.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17982 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #59748|0 |1 is obsolete| | --- Comment #9 from M. Tompsett <mtompset@hotmail.com> --- Created attachment 59750 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=59750&action=edit Bug 17982: Improve test coverage This test triggers the three cases that were modified. prove -v t/db_dependent/Templates.t -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17982 --- Comment #10 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Mark, Thx for signoff but please explain what the second test has to do with the first patch? You are testing language, but the change is about not duplicating themes by proper use of uniq. Marcel -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17982 --- Comment #11 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Apart from that, it fails too: # Subtest: Test C4::Templates::themelanguage 1..4 not ok 1 - Expected language: non-en # Failed test 'Expected language: non-en' # at t/db_dependent/Templates.t line 68. # got: 'en' # expected: 'fr-FR' ok 2 - Expected language: default en ok 3 - Expected language: unset ok 4 - Expected language: not coded for # Looks like you failed 1 test of 4. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17982 --- Comment #12 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Do not assume that fr-FR exists. I dont have it :) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17982 --- Comment #13 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Mock::FileSystem ? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17982 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |mtompset@hotmail.com --- Comment #14 from M. Tompsett <mtompset@hotmail.com> --- (In reply to Marcel de Rooy from comment #10)
Thx for signoff but please explain what the second test has to do with the first patch? You are testing language, but the change is about not duplicating themes by proper use of uniq.
You changed 3 lines of code, this test triggers those three lines of code. However, you rightly pointed out fr-FR might not exist. Shoot. :( If it does exist, it triggers the three cases you changed. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17982 --- Comment #15 from M. Tompsett <mtompset@hotmail.com> --- (In reply to Marcel de Rooy from comment #13)
Mock::FileSystem ?
That looks promising. I'll try something in about 2 hours. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17982 Josef Moravec <josef.moravec@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |josef.moravec@gmail.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17982 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #59750|0 |1 is obsolete| | --- Comment #16 from M. Tompsett <mtompset@hotmail.com> --- Comment on attachment 59750 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=59750 Bug 17982: Improve test coverage Because there is no easy way to mock the file system. I'll just forget this, and sign off. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17982 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17982 --- Comment #17 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 59765 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=59765&action=edit Bug 17982: Expose wrong use of uniq This test should fail without the patch fixing the uniq calls. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17982 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #59749|0 |1 is obsolete| | --- Comment #18 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 59766 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=59766&action=edit Bug 17982: Fix the use of uniq in sub themelanguage Doing uniq( \@themes ) is useless. It will just return to you the only reference you gave it. List::MoreUtils::uniq requires a list instead of an arrayref. So it is a trivial fix that makes sub themelanguage return one theme instead of three themes like [ 'prog', 'prog', 'prog' ]. Note that Template->new inserts one or two include paths to TT for each of these three identical themes. Test plan: [1] Run t/db_dependent/Templates.t (should no longer fail) [2] Run t/db_dependent/Auth.t (triggering themelanguage) [3] Open a page on OPAC or intranet. (Did you restart Plack?) Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Mark Tompsett <mtompset@hotmail.com> EDIT (Marcel): Amended test plan for additional unit test. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17982 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |18036 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18036 [Bug 18036] Improve test coverage for themelanguage -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17982 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |13169 CC| |jonathan.druart@bugs.koha-c | |ommunity.org Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13169 [Bug 13169] C4::Templates::themelanguage fails if the DB is not populated -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17982 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17982 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #59765|0 |1 is obsolete| | Attachment #59766|0 |1 is obsolete| | --- Comment #19 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 59790 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=59790&action=edit Bug 17982: Expose wrong use of uniq This test should fail without the patch fixing the uniq calls. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17982 --- Comment #20 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 59791 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=59791&action=edit Bug 17982: Fix the use of uniq in sub themelanguage Doing uniq( \@themes ) is useless. It will just return to you the only reference you gave it. List::MoreUtils::uniq requires a list instead of an arrayref. So it is a trivial fix that makes sub themelanguage return one theme instead of three themes like [ 'prog', 'prog', 'prog' ]. Note that Template->new inserts one or two include paths to TT for each of these three identical themes. Test plan: [1] Run t/db_dependent/Templates.t (should no longer fail) [2] Run t/db_dependent/Auth.t (triggering themelanguage) [3] Open a page on OPAC or intranet. (Did you restart Plack?) Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Mark Tompsett <mtompset@hotmail.com> EDIT (Marcel): Amended test plan for additional unit test. Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17982 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |kyle@bywatersolutions.com Status|Passed QA |Pushed to Master --- Comment #21 from Kyle M Hall <kyle@bywatersolutions.com> --- Pushed to master for 17.05, thanks Marcel! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17982 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |katrin.fischer@bsz-bw.de --- Comment #22 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Should this be in 16.11.x? Not sure if it fixes a bug atm. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17982 --- Comment #23 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to Katrin Fischer from comment #22)
Should this be in 16.11.x? Not sure if it fixes a bug atm.
You could call it a tiny bug, but no real need to backport it. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17982 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to Master |RESOLVED Resolution|--- |FIXED --- Comment #24 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Ok, will leave it out for now then. If it's needed for something later on, we can come back to this. -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org