Bug 17982 - Fix the use of uniq in sub themelanguage
Summary: Fix the use of uniq in sub themelanguage
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Templates (show other bugs)
Version: master
Hardware: All All
: P5 - low minor (vote)
Assignee: Marcel de Rooy
QA Contact: Testopia
URL:
Keywords:
Depends on: 13169
Blocks: 18036
  Show dependency treegraph
 
Reported: 2017-01-24 15:34 UTC by Marcel de Rooy
Modified: 2017-12-07 22:16 UTC (History)
5 users (show)

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


Attachments
Bug 17982: Fix the use of uniq in sub themelanguage (3.02 KB, patch)
2017-01-24 16:03 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17982: Fix the use of uniq in sub themelanguage (3.02 KB, patch)
2017-01-25 08:24 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17982: Fix the use of uniq in sub themelanguage (1.98 KB, patch)
2017-01-29 14:07 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17982: Improve test coverage (2.93 KB, patch)
2017-02-01 18:34 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 17982: Improve test coverage (2.92 KB, patch)
2017-02-01 18:39 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 17982: Fix the use of uniq in sub themelanguage (1.99 KB, patch)
2017-02-01 18:40 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 17982: Improve test coverage (2.86 KB, patch)
2017-02-01 18:48 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 17982: Expose wrong use of uniq (1.64 KB, patch)
2017-02-02 07:53 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17982: Fix the use of uniq in sub themelanguage (2.01 KB, patch)
2017-02-02 07:53 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17982: Expose wrong use of uniq (1.67 KB, patch)
2017-02-02 11:15 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17982: Fix the use of uniq in sub themelanguage (2.04 KB, patch)
2017-02-02 11:15 UTC, Jonathan Druart
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 2017-01-24 15:34:31 UTC

    
Comment 1 Marcel de Rooy 2017-01-24 16:03:53 UTC Comment hidden (obsolete)
Comment 2 Marcel de Rooy 2017-01-25 08:24:56 UTC Comment hidden (obsolete)
Comment 3 Marcel de Rooy 2017-01-29 14:07:48 UTC
Created attachment 59630 [details] [review]
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>
Comment 4 Mark Tompsett 2017-02-01 02:17:14 UTC
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.
Comment 5 Marcel de Rooy 2017-02-01 12:07:01 UTC
(In reply to M. Tompsett from comment #4)
> Comment on attachment 59630 [details] [review] [review]
> Bug 17982: Fix the use of uniq in sub themelanguage
> 
> Review of attachment 59630 [details] [review] [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.
Comment 6 Mark Tompsett 2017-02-01 18:34:49 UTC
Created attachment 59747 [details] [review]
Bug 17982: Improve test coverage

This test triggers the three cases that were modified.
prove -v t/db_dependent/Templates.t
Comment 7 Mark Tompsett 2017-02-01 18:39:39 UTC
Created attachment 59748 [details] [review]
Bug 17982: Improve test coverage

This test triggers the three cases that were modified.
prove -v t/db_dependent/Templates.t
Comment 8 Mark Tompsett 2017-02-01 18:40:57 UTC
Created attachment 59749 [details] [review]
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>
Comment 9 Mark Tompsett 2017-02-01 18:48:53 UTC
Created attachment 59750 [details] [review]
Bug 17982: Improve test coverage

This test triggers the three cases that were modified.
prove -v t/db_dependent/Templates.t
Comment 10 Marcel de Rooy 2017-02-01 20:23:14 UTC
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
Comment 11 Marcel de Rooy 2017-02-01 20:25:34 UTC
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.
Comment 12 Marcel de Rooy 2017-02-01 20:26:05 UTC
Do not assume that fr-FR exists. I dont have it :)
Comment 13 Marcel de Rooy 2017-02-01 20:27:18 UTC
Mock::FileSystem ?
Comment 14 Mark Tompsett 2017-02-01 20:37:23 UTC
(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.
Comment 15 Mark Tompsett 2017-02-01 20:38:53 UTC
(In reply to Marcel de Rooy from comment #13)
> Mock::FileSystem ?

That looks promising. I'll try something in about 2 hours.
Comment 16 Mark Tompsett 2017-02-01 22:56:17 UTC
Comment on attachment 59750 [details] [review]
Bug 17982: Improve test coverage

Because there is no easy way to mock the file system. I'll just forget this, and sign off.
Comment 17 Marcel de Rooy 2017-02-02 07:53:17 UTC
Created attachment 59765 [details] [review]
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>
Comment 18 Marcel de Rooy 2017-02-02 07:53:21 UTC
Created attachment 59766 [details] [review]
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.
Comment 19 Jonathan Druart 2017-02-02 11:15:09 UTC
Created attachment 59790 [details] [review]
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>
Comment 20 Jonathan Druart 2017-02-02 11:15:16 UTC
Created attachment 59791 [details] [review]
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>
Comment 21 Kyle M Hall 2017-02-14 14:10:29 UTC
Pushed to master for 17.05, thanks Marcel!
Comment 22 Katrin Fischer 2017-02-14 20:47:44 UTC
Should this be in 16.11.x? Not sure if it fixes a bug atm.
Comment 23 Marcel de Rooy 2017-02-15 11:59:30 UTC
(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.
Comment 24 Katrin Fischer 2017-02-15 12:35:54 UTC
Ok, will leave it out for now then. If it's needed for something later on, we can come back to this.