Bug 27660 - Should getletter not use the fallback to default?
Summary: Should getletter not use the fallback to default?
Status: RESOLVED INVALID
Alias: None
Product: Koha
Classification: Unclassified
Component: Notices (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Martin Renvoize
QA Contact: Testopia
URL:
Keywords:
: 26787 (view as bug list)
Depends on:
Blocks:
 
Reported: 2021-02-09 12:23 UTC by Marcel de Rooy
Modified: 2021-06-04 10:27 UTC (History)
3 users (show)

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


Attachments

Note You need to log in before you can comment on or make changes to this bug.
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. ***