Bug 27660

Summary: Should getletter not use the fallback to default?
Product: Koha Reporter: Marcel de Rooy <m.de.rooy>
Component: NoticesAssignee: Martin Renvoize <martin.renvoize>
Status: CLOSED INVALID QA Contact: Testopia <testopia>
Severity: normal    
Priority: P5 - low CC: jonathan.druart, nick, tomascohen
Version: master   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26734
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28487
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:

Description Marcel de Rooy 2021-02-09 12:23:32 UTC
Looking at the code:

sub getletter {
    my ( $module, $code, $branchcode, $message_transport_type, $lang) = @_;
..
    $lang = 'default' unless( $lang && C4::Context->preference('TranslateNotices') );
..
    my $sth = $dbh->prepare(q{
        SELECT *
        FROM letter
        WHERE module=? AND code=? AND (branchcode = ? OR branchcode = '')
        AND message_transport_type LIKE ?
        AND lang =?
        ORDER BY branchcode DESC LIMIT 1
    });
    $sth->execute( $module, $code, $branchcode, $message_transport_type, $lang );
    my $line = $sth->fetchrow_hashref
      or return;
  ..

I am just wondering if I miss something. If the language is passed (which is often just a $patron->lang or so), then we are fetching with that language! If that notice has not been translated yet, we will not fallback to en ?!
Comment 1 Katrin Fischer 2021-02-09 12:26:24 UTC
We should not fall back to en, but to whatever is the "Default" tab in that installation. I thought that was the case tbh :(
Comment 2 Marcel de Rooy 2021-02-09 12:54:31 UTC
(In reply to Katrin Fischer from comment #1)
> We should not fall back to en, but to whatever is the "Default" tab in that
> installation. I thought that was the case tbh :(

Well look at the code carefully.
It depends on a few conditions: 1) Do you use TranslateNotices pref 2) Did the patron fill in the language for notices?
If you do and patron lang is e.g. 'en' but there is no notice for en, there is no fallback in the above.
As long as most people not have filled the language, it does not hurt too much..
Comment 3 Katrin Fischer 2021-02-09 13:02:30 UTC
So I think you are right, but it should fallback to default :)
Comment 4 Jonathan Druart 2021-02-09 15:30:25 UTC
It's handled in GetPreparredLetter

 638         $letter = getletter( $module, $letter_code, $branchcode, $mtt, $lang );
 639 
 640         unless ( $letter ) {
 641             $letter = getletter( $module, $letter_code, $branchcode, $mtt, 'default' )
 642                 or warn( "No $module $letter_code letter transported by " . $mtt ),
 643                     return;
 644         }
Comment 5 Marcel de Rooy 2021-02-09 15:57:25 UTC
(In reply to Jonathan Druart from comment #4)
> It's handled in GetPreparredLetter
> 
>  638         $letter = getletter( $module, $letter_code, $branchcode, $mtt,
> $lang );
>  639 
>  640         unless ( $letter ) {
>  641             $letter = getletter( $module, $letter_code, $branchcode,
> $mtt, 'default' )
>  642                 or warn( "No $module $letter_code letter transported by
> " . $mtt ),
>  643                     return;
>  644         }

Ah. Thats good. Didnt test it on master ;) Thx.
Will have to check if there are other callers to getletter that dont do the same thing?
And if this change in GetPreparedLetter came together in older versions..
Comment 6 Jonathan Druart 2021-02-09 16:29:19 UTC
getletter is the "raw" subroutine to get what you asked in parameter (for tools/letter.pl)

It's correctly handled in GetPreparedLetter since the beginning, see
  commit c49bdc3a8d8e5a9389a27a8dca90544ff5c42f95
  Bug 17762: Add the lang parameter to C4::Letters::getletter
Comment 7 Jonathan Druart 2021-02-09 16:30:52 UTC
Looks like those occurrences need to be double-checked:

members/printfeercpt.pl:my $letter = C4::Letters::getletter( 'circulation', 'ACCOUNT_CREDIT', C4::Context::mybranch, 'print', $patron->lang );                                                                    

members/printinvoice.pl:my $letter = C4::Letters::getletter( 'circulation', 'ACCOUNT_DEBIT', C4::Context::mybranch, 'print', $patron->lang );                                                                     

misc/cronjobs/overdue_notices.pl:                my $letter = C4::Letters::getletter( 'circulation', $overdue_rules->{"letter$i"}, $branchcode, undef, $patron->lang );                                           
misc/cronjobs/overdue_notices.pl:                    my $letter_exists = C4::Letters::getletter( 'circulation', $overdue_rules->{"letter$i"}, $branchcode, $effective_mtt, $patron->lang ) ? 1 : 0;               

pos/printreceipt.pl:  C4::Letters::getletter( 'pos', 'RECEIPT', C4::Context::mybranch, 'print' );
Comment 8 Martin Renvoize 2021-03-11 08:27:56 UTC
I caught most of these occurrences in bug 26734.. looks like overdue slips need the same handling.

It re-opens a question I raised in the roadmap sheet though: https://annuel.framapad.org/p/koha_21.05_roadmap

Namely, "Should we converge back to /printslip.pl + circ/print-slip.tt or continue down the route of a controller + template for each slip type?"
Comment 9 Jonathan Druart 2021-06-04 10:14:34 UTC
This is invalid, getletter should not have a fallback.
Comment 10 Jonathan Druart 2021-06-04 10:27:20 UTC
*** Bug 26787 has been marked as a duplicate of this bug. ***