Bug 13170 - Remove of prog theme broke the OPAC's "view plain" option for MARC details
Summary: Remove of prog theme broke the OPAC's "view plain" option for MARC details
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Templates (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Frédéric Demians
QA Contact: Testopia
URL: /cgi-bin/koha/opac-showmarc.pl?id=XXX...
Keywords:
Depends on:
Blocks:
 
Reported: 2014-10-30 18:59 UTC by Owen Leonard
Modified: 2015-06-04 23:33 UTC (History)
4 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 13170 Remove of prog theme broke the OPAC's "view plain" option for MARC details (1.89 KB, patch)
2014-11-02 07:50 UTC, Frédéric Demians
Details | Diff | Splinter Review
Bug 13170 Remove of prog theme broke the OPAC's "view plain" option for MARC details (2.70 KB, patch)
2014-11-02 13:56 UTC, Frédéric Demians
Details | Diff | Splinter Review
Bug 13170 Remove of prog theme broke the OPAC's "view plain" option for MARC details (4.00 KB, patch)
2014-11-03 22:53 UTC, Frédéric Demians
Details | Diff | Splinter Review
Bug 13170 Remove of prog theme broke the OPAC's "view plain" option for MARC details (3.72 KB, patch)
2014-11-03 23:29 UTC, Frédéric Demians
Details | Diff | Splinter Review
Bug 13170 Remove of prog theme broke the OPAC's "view plain" option for MARC details (3.81 KB, patch)
2014-11-04 05:46 UTC, Chris Cormack
Details | Diff | Splinter Review
[PASSED QA] Bug 13170 Remove of prog theme broke the OPAC's "view plain" option for MARC details (3.92 KB, patch)
2014-11-04 06:50 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Owen Leonard 2014-10-30 18:59:36 UTC
In the OPAC if you view the MARC details for a title (and have  OPACXSLTDetailsDisplay enabled) there is a "view plain" link which displays the output of opac-showmarc.pl. This is broken in master:

opac-showmarc.pl: XSLT file not found. at kohaclone/Koha/XSLT_Handler.pm

git bisect blames Bug 12233: Remove the prog and ccsr themes files.
Comment 1 Frédéric Demians 2014-11-02 07:50:34 UTC Comment hidden (obsolete)
Comment 2 Tomás Cohen Arazi 2014-11-02 12:42:12 UTC
Frederic, I'm on the phone right now, so cannot test. But could you make the patch use C4::Templates::themelanguage instead so we use the fallback logic from only one place? It does exactly what you want. Thanks!
Comment 3 Frédéric Demians 2014-11-02 13:29:43 UTC
(In reply to Tomás Cohen Arazi from comment #2)
> Frederic, I'm on the phone right now, so cannot test. But could you make the
> patch use C4::Templates::themelanguage instead so we use the fallback logic
> from only one place? It does exactly what you want. Thanks!

Yes, of course. Thanks for pointing to the right direction!
Comment 4 Frédéric Demians 2014-11-02 13:56:27 UTC Comment hidden (obsolete)
Comment 5 Chris Cormack 2014-11-03 22:08:25 UTC
Comment on attachment 33129 [details] [review]
Bug 13170 Remove of prog theme broke the OPAC's "view plain" option for MARC details

Review of attachment 33129 [details] [review]:
-----------------------------------------------------------------

::: opac/opac-showmarc.pl
@@ +57,5 @@
> +        C4::Context->config('opachtdocs'),
> +        C4::Context->preference('opacthemes'),
> +        C4::Languages::getlanguage($input),
> +        $view eq 'card' ? 'compact.xsl' : 'plainMARC.xsl'
> +    );

Why are we doing this? Instead of using C4::Template::themelanguage ?

Now we still have different behaviour for xslt and for normal templates

themelanguage will never fallback to prog for the opac. however _get_best_defautl_xslt_filename will.

So if have a new theme chrisc theme. And I make all the stuff, but no new xslt file. It will now fall back to prog, instead of bootstrap.

This is a problem as prog no longer exists in the OPAC.
Comment 6 Frédéric Demians 2014-11-03 22:39:41 UTC
(In reply to Chris Cormack from comment #5)

> Why are we doing this? Instead of using C4::Template::themelanguage ?
> 
> Now we still have different behaviour for xslt and for normal templates
> 
> themelanguage will never fallback to prog for the opac. however
> _get_best_defautl_xslt_filename will.
> 
> So if have a new theme chrisc theme. And I make all the stuff, but no new
> xslt file. It will now fall back to prog, instead of bootstrap.
> 
> This is a problem as prog no longer exists in the OPAC.

Ok. I see. I forgot to modify _get_best_default_xslt_filename to fall back to 'bootstrap' theme rather than 'prog' for OPAC interface. 

There is a difference between C4::Template::themelanguage & C4::XSLT::_get_best_default_xslt_filename. 'themelanguage' search tempalates in subdirectories of directory named 'modules'. '_get_best_default_xslt_filename' search XSL file in a subdirectory named 'xslt'.

Would you suggest to factorize both functions in one?
Comment 7 Chris Cormack 2014-11-03 22:49:58 UTC
(In reply to Frédéric Demians from comment #6)
> (In reply to Chris Cormack from comment #5)
> 
> > Why are we doing this? Instead of using C4::Template::themelanguage ?
> > 
> > Now we still have different behaviour for xslt and for normal templates
> > 
> > themelanguage will never fallback to prog for the opac. however
> > _get_best_defautl_xslt_filename will.
> > 
> > So if have a new theme chrisc theme. And I make all the stuff, but no new
> > xslt file. It will now fall back to prog, instead of bootstrap.
> > 
> > This is a problem as prog no longer exists in the OPAC.
> 
> Ok. I see. I forgot to modify _get_best_default_xslt_filename to fall back
> to 'bootstrap' theme rather than 'prog' for OPAC interface. 

It should fall back to whateverr OPACFallback is set to. Currently thats bootstrap, but we dont want to be in this same position when we change default theme again sometime in the future of having hardcoded theme names.
> 
> There is a difference between C4::Template::themelanguage &
> C4::XSLT::_get_best_default_xslt_filename. 'themelanguage' search tempalates
> in subdirectories of directory named 'modules'.
> '_get_best_default_xslt_filename' search XSL file in a subdirectory named
> 'xslt'.
> 
> Would you suggest to factorize both functions in one?

Something like that, a single place where we pick the theme and language seems to be a lot less error prone and easier to maintain.
Comment 8 Frédéric Demians 2014-11-03 22:53:08 UTC Comment hidden (obsolete)
Comment 9 Chris Cormack 2014-11-03 22:56:33 UTC
Comment on attachment 33179 [details] [review]
Bug 13170 Remove of prog theme broke the OPAC's "view plain" option for MARC details

Review of attachment 33179 [details] [review]:
-----------------------------------------------------------------

::: C4/XSLT.pm
@@ +145,4 @@
>      my @candidates = (
>          "$htdocs/$theme/$lang/xslt/${base_xslfile}", # exact match
>          "$htdocs/$theme/en/xslt/${base_xslfile}",    # if not, preferred theme in English
> +        "$htdocs/bootstrap/$lang/xslt/${base_xslfile}",   # if not, 'bootstrap' theme in preferred language

Hmm nope, still not the same behaviour

in themelanguage we do

my $fallback_syspref = ($interface eq 'intranet') ? 'template' : 'OPACFallback';
C4::Context->preference( $fallback_syspref )
Comment 10 Frédéric Demians 2014-11-03 22:58:51 UTC
> It should fall back to whateverr OPACFallback is set to. Currently thats
> bootstrap, but we dont want to be in this same position when we change
> default theme again sometime in the future of having hardcoded theme names.

Sorry. I wasn't even aware of this syspref.

> Something like that, a single place where we pick the theme and language
> seems to be a lot less error prone and easier to maintain.

Will try. Thanks.

> Hmm nope, still not the same behaviour
> 
> in themelanguage we do
> 
> my $fallback_syspref = ($interface eq 'intranet') ? 'template' :
> 'OPACFallback';
> C4::Context->preference( $fallback_syspref )

I couldn't agree with you more.
Comment 11 Frédéric Demians 2014-11-03 23:29:41 UTC Comment hidden (obsolete)
Comment 12 Chris Cormack 2014-11-03 23:39:25 UTC
Excellent this looks good, Ill do a proper test tomorrow (i might edit the commit message to remove mention of the sub we aren't using now).

But from a code review it looks good. Just to do some regression testing now.
Comment 13 Chris Cormack 2014-11-04 05:46:10 UTC Comment hidden (obsolete)
Comment 14 Katrin Fischer 2014-11-04 06:50:34 UTC
Created attachment 33182 [details] [review]
[PASSED QA] Bug 13170 Remove of prog theme broke the OPAC's "view plain" option for MARC details

In the OPAC if you view the MARC details for a title (and have
OPACXSLTDetailsDisplay enabled) there is a "view plain" link which displays the
output of opac-showmarc.pl. This is broken in master: fixed by this patch.

Test plan:

(1) Set OPACXSLTDetailsDisplay to default
(2) Do a search on OPAC, then display a specific biblio record
(3) Click on MARC view tab. Then click on 'view plain' link. Nothing is
    displayed.
(4) Apply the patch. And refresh the MARC detail page.
(5) Click on 'view plain' link. Check that a plain text MARC record is
    displayed.

Signed-off-by: Chris <chris@bigballofwax.co.nz>

Note: This makes a small change to C4::Templates::themelanguage so that
it works with .xsl files too (They live in the xslt dir)

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>

Works as described, passes tests and QA script.
Comment 15 Tomás Cohen Arazi 2014-11-05 11:35:06 UTC
Patch pushed to master.

Thanks Frédéric!