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!

(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 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.

(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?

(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 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 )

> 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.

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.

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.

Patch pushed to master.

Thanks Frédéric!