I consider this a bug (especially the one where TranslateNotices is off). If I'm the outlier, then I'd like to propose that we add a default language choice in the patron categories (like we do for messaging preferences) so that we don't have to change the preferred language for each and every new patron.

Since "default" is basically "English", unilingual non-english libraries absolutely need to change the patron's preferred language, they can't leave it at default.

Martin pointed me to the following lines that should be edited

https://git.koha-community.org/Koha-community/Koha/src/branch/master/C4/Letters.pm#L147

and 

https://git.koha-community.org/Koha-community/Koha/src/branch/master/C4/Letters.pm#L1597

Hm, I think the interface language is the best we could do in these cases and should work for our libraries as well. Our OPACs are usually at least English and German, but notices are often German only. The current behavior would therefore give us some trouble, but interface language should work ok.

Another alternative might be to use the "first" language in the language selection as default for these cases.

You can reorder them and the first in the list is, I think, also the fallback in other cases. So that might also work.

So if TranslateNotices is off, use interface language

if TranslateNotices is on, use first language as "default"

Would that be ok?

--> check patron's preferred language
--> if patron's preferred language is "default", check interface language
--> if no interface (for overdue_notices for example), check first language in "language" system preference

No need to check for TranslateNotices since borrowers.lang is always filled no matter what. If the pref is off, all the patrons' preferred languages will be "default".

Note that this bug is not for the notice templates. This already works well. It's only for the "includes" included in the templates. In my example, I used ACCOUNT_CREDIT template which uses accounts.inc to print the credit or debit account type. These are all translated in koha-tmpl/intranet-tmpl/prog/{lang}/includes/accounts.inc but only the english file is used in the slips, making all accounttypes (payment, lost item, manual fee, etc.) print in english no matter the language of the template. I added a screenshot of ACCOUNT_CREDIT with the parts that are currently not translated unless TranslateNotices is on and the patron has the correct preferred language in yellow.

Created attachment 148281 [details]
Screenshot of ACCOUNT_CREDIT with untranslated parts in yellow

Created attachment 148785 [details] [review]
Bug 33237: If TranslateNotices is off, use the interface language includes in slips

This patch set the language used in slips folloinw this logic:
   --> uses patron's preferred language
   --> if patron's preferred language is 'default', use the interface language
   --> if there is no interface (for overdue_notices for example), use the first language in 'language' system preference

To Test:
1. Install the other language (i used fr-CA here)
     1.1.

     gulp po:update fr-CA

     ./misc/translator/translate install fr-CA

     1.2. In Administration > Global system preferences, search for language and check the added language

2. Create a manual invoice in a patron's account and pay it

3. From the Transactions tab, click 'Print' next to the payment line
   --> On the printed slip, there's the word 'Payment' in English (OK)

4. Switch interface to other language

5. Redo step 3
   --> On the printed slip, the word 'Payment' is still in English (not ok)

6. In Administration > Global system preferences, enable TranslateNotices

7. Go to Tools > Notices and slips > ACCOUNT_CREDIT and copy the content of the letter into all the languages

8. Redo step 3 (you should still be in the other language interface)
   --> On the printed slip, the word 'Payment' is still in English (not ok)

9. Edit the patron's account and change the preferred language to the other language

10. Redo step 3
   --> On the printed slip, the word 'Payment' is in the other language (Paiement) (OK)

11. Apply the patch

12. Reset config
    12.1. Edit the patron's account and change back the preferred language to 'default'
    12.2. In Administration > Global system preferences, disable TranslateNotices

13. Redo step 3 (you should still be in the other language interface)
   --> On the printed slip, the word 'Payment' is in the other language (Paiement)

14. Switch interface to the english language

15. Redo step 3
   --> On the printed slip, the word 'Payment' is in English

16. In Administration > Global system preferences, enable TranslateNotices

17. Edit the patron's account and change the preferred language to the other language

18. Redo step 3
   --> On the printed slip, the word 'Payment' is in the other language (Paiement)

Sorry, can we get some unit tests here.

Created attachment 163952 [details] [review]
Bug 33237: If TranslateNotices is off, use the interface language includes in slips

This patch set the language used in slips folloinw this logic:
   --> uses patron's preferred language
   --> if patron's preferred language is 'default', use the interface language
   --> if there is no interface (for overdue_notices for example), use the first language in 'language' system preference

To Test:
1. Install the other language (i used fr-CA here)
     1.1.

     gulp po:update fr-CA

     ./misc/translator/translate install fr-CA

     1.2. In Administration > Global system preferences, search for language and check the added language

2. Create a manual invoice in a patron's account and pay it

3. From the Transactions tab, click 'Print' next to the payment line
   --> On the printed slip, there's the word 'Payment' in English (OK)

4. Switch interface to other language

5. Redo step 3
   --> On the printed slip, the word 'Payment' is still in English (not ok)

6. In Administration > Global system preferences, enable TranslateNotices

7. Go to Tools > Notices and slips > ACCOUNT_CREDIT and copy the content of the letter into all the languages

8. Redo step 3 (you should still be in the other language interface)
   --> On the printed slip, the word 'Payment' is still in English (not ok)

9. Edit the patron's account and change the preferred language to the other language

10. Redo step 3
   --> On the printed slip, the word 'Payment' is in the other language (Paiement) (OK)

11. Apply the patch

12. Reset config
    12.1. Edit the patron's account and change back the preferred language to 'default'
    12.2. In Administration > Global system preferences, disable TranslateNotices

13. Redo step 3 (you should still be in the other language interface)
   --> On the printed slip, the word 'Payment' is in the other language (Paiement)

14. Switch interface to the english language

15. Redo step 3
   --> On the printed slip, the word 'Payment' is in English

16. In Administration > Global system preferences, enable TranslateNotices

17. Edit the patron's account and change the preferred language to the other language

18. Redo step 3
   --> On the printed slip, the word 'Payment' is in the other language (Paiement)

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Created attachment 163953 [details] [review]
Bug 33237: (follow-up) Tidy

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

(In reply to Martin Renvoize from comment #8)
> Sorry, can we get some unit tests here.

Signed off as test plan works, but Martin is correct, unit tests would be useful

Created attachment 164521 [details] [review]
Bug 33237: (follow-up) Unit tests

Thanks for adding the unit test Hammut... but I'm afraid I can't seem to get it to pass on ktd.. I'll investigate a little but I have a feeling it's got something to do with installed languages in that environment.. we don't install any by default but this would require we do.. or at least we install the translated notice for those particular test cases.

Yes, I would move this logic in a dedicated subroutine. It will isolate it and make it easier to test ("sub effective_language_for_notices"?)

my $lang       = 'en'; 

This is also not needed and confusing as you are actually removing the fallback to "en".

I would re-add it, after the if/else block, add `$lang //= 'en';`

OK, now that I've dug through the test I'm a lot less sure this bug makes sense.

You are setting the language to low down in my opinion.. we should be setting it from GetPreparedLetter.. otherwise you're going to end up with mixed language notices aren't you?

Surely it's about picking the right notice template in the first place rather than just the processing step... in the current iteration you could end up with an English notice with includes that come out in french.. that feels strange to me.

Right... I see.. it's about what 'default' means.. right?

Either way, we need a clear and correct test and that's not the case here as it stands.

Created attachment 165382 [details] [review]
Bug 33237: TMP Fix for Budget test failing in main

Created attachment 165383 [details] [review]
Bug 33237: (follow-up) Restore final fallback

This restores the final fallback to 'en' as mentioned by Jonathan

Created attachment 165384 [details] [review]
Bug 33237: (QA follow-up) Clarify tests

This patch tries to clarify the tests a little and expand their
coverage.

I've added notes where I think we still need a little work, especially
in regards to having them pass in the jenkins runs as we don't have
translations installed there and thus at the moment the tests will fail
without manual intervention to install the translated includes.

My conclusion here is that we shouldn't be using interface language at all.. we can't reliably tell that 'interface' and 'default' are written in matching languages.

I think we have to assume that 'default' matches 'first in list of translated languages'.

I think we should be doing the language code manipulation inside GetPreparredLetter and passing that along to the tt processor for includes.

Created attachment 165404 [details] [review]
Bug 33237: TMP Fix for Budget test failing in main

Created attachment 165405 [details] [review]
Bug 33237: (follow-up) Restore final fallback

This restores the final fallback to 'en' as mentioned by Jonathan

Created attachment 165406 [details] [review]
Bug 33237: (QA follow-up) Clarify tests and Mock

This patch tries to clarify the tests a little and expand their
coverage whilst also adding some defualt translated includes to the test
area and mocking the config to allow the tests to run without installing
translations.

Right.. we can drop the TMP fix patch once those have been corrected upstream in main.


I also think the Restore final fallback isn't really required but I did it whilst I was in here as Jonathan mentioned it.

The final patch is the important one as it brings us a step closer to the end goal by getting the tests working without having to manually install a translation.

However, I'm now strongly of the opinion we should not be doing the interface level check and instead use the highest priority language in the system preferences as the 'default'.


My understanding of how 'default' is used is that the 'default' notice is generally written in the primary language of the system and so it would match the top pick for interface language selection and possible not match the selected interface language of the moment.

Use cases:
1) US English only.  Default == English + Includes should be English
2) Single translation only (French Canadian). Default == French Canadian + Includes should also be French Canadian.
3) Multiple languages. Default template written in French Canadian, the Include should also use French Canadian (even if the staff user clicking to print has switched to Italian) otherwise we have the main notice in French Candian with the Include parts in Itelian.

Right now. case 3 fails.

I also think we could re-arrange the tests a bit to clean up.. I think we could/should be testing Template selection _+ include selection in the same places.. i.e each time we check the 'title' we would also check the 'content' to ensure they match consistently.. rather than the current check of template selection followed by a section for include selection checking.

Can I get some real-life users to comment on my thoughts regarding point three in the above comment

I CC'd a couple of people who I know use/support Koha in other languages. I will also send an email to the French mailing list to get their opinions.

For bonus points I'm tempted to display a 'Expected language' field for the default notices in the editor to show the output of the highest priority language when more than one is installed.  This clarifying to staff what language we're expecting

From my experience with German public libraries, the user would assume the 'Default' contents of a notice will be written in German.
TBH our libraries generally haven't been using TranslateNotices, maybe my feedback is not so important. :)
But if they were to activate the syspref, I would expect the 'default' template (for instance for overdues) to be in German and the includes to be in German as well. Martin, for your case 3 I agree with you. The used language for includes and for the 'default' notice content should match. But a display of 'effective language' would really be the best thing for the user as you said.
Or: add a checkbox to syspref "Languages" for "Default language for notices". :)

Created attachment 165455 [details] [review]
Bug 33237: If TranslateNotices is off, use the interface language includes in slips

This patch set the language used in slips folloinw this logic:
   --> uses patron's preferred language
   --> if patron's preferred language is 'default', use the interface language
   --> if there is no interface (for overdue_notices for example), use the first language in 'language' system preference

To Test:
1. Install the other language (i used fr-CA here)
     1.1.

     gulp po:update fr-CA

     ./misc/translator/translate install fr-CA

     1.2. In Administration > Global system preferences, search for language and check the added language

2. Create a manual invoice in a patron's account and pay it

3. From the Transactions tab, click 'Print' next to the payment line
   --> On the printed slip, there's the word 'Payment' in English (OK)

4. Switch interface to other language

5. Redo step 3
   --> On the printed slip, the word 'Payment' is still in English (not ok)

6. In Administration > Global system preferences, enable TranslateNotices

7. Go to Tools > Notices and slips > ACCOUNT_CREDIT and copy the content of the letter into all the languages

8. Redo step 3 (you should still be in the other language interface)
   --> On the printed slip, the word 'Payment' is still in English (not ok)

9. Edit the patron's account and change the preferred language to the other language

10. Redo step 3
   --> On the printed slip, the word 'Payment' is in the other language (Paiement) (OK)

11. Apply the patch

12. Reset config
    12.1. Edit the patron's account and change back the preferred language to 'default'
    12.2. In Administration > Global system preferences, disable TranslateNotices

13. Redo step 3 (you should still be in the other language interface)
   --> On the printed slip, the word 'Payment' is in the other language (Paiement)

14. Switch interface to the english language

15. Redo step 3
   --> On the printed slip, the word 'Payment' is in English

16. In Administration > Global system preferences, enable TranslateNotices

17. Edit the patron's account and change the preferred language to the other language

18. Redo step 3
   --> On the printed slip, the word 'Payment' is in the other language (Paiement)

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 165456 [details] [review]
Bug 33237: (follow-up) Tidy

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 165457 [details] [review]
Bug 33237: (follow-up) Unit tests

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 165458 [details] [review]
Bug 33237: (follow-up) Restore final fallback

This restores the final fallback to 'en' as mentioned by Jonathan

Created attachment 165459 [details] [review]
Bug 33237: (QA follow-up) Clarify tests and Mock

This patch tries to clarify the tests a little and expand their
coverage whilst also adding some defualt translated includes to the test
area and mocking the config to allow the tests to run without installing
translations.

Created attachment 165460 [details] [review]
Bug 33237: Add display of which language default language is expected

This patch adds the display of 'Default language' to the 'Default'
language in the notices editor tool.

This is so that librarians know which language they are expected to be
writing the notice in so we can remain consistent in both template and
include language used.

I've added the display of which language the system is expecting the default to be written in to allow consistency between template and include language.

I still think point three is valid and would like to tidy up the tests and remove the interface clause.

However, Magnus raised that perhaps these 'Default' templates may actually be deliberately bilingual.. in which case these patches will cause even more confusion and harm when using INCLUDES.

Created attachment 165473 [details] [review]
Bug 33237: If TranslateNotices is off, use the interface language includes in slips

This patch set the language used in slips folloinw this logic:
   --> uses patron's preferred language
   --> if patron's preferred language is 'default', use the interface language
   --> if there is no interface (for overdue_notices for example), use the first language in 'language' system preference

To Test:
1. Install the other language (i used fr-CA here)
     1.1.

     gulp po:update fr-CA

     ./misc/translator/translate install fr-CA

     1.2. In Administration > Global system preferences, search for language and check the added language

2. Create a manual invoice in a patron's account and pay it

3. From the Transactions tab, click 'Print' next to the payment line
   --> On the printed slip, there's the word 'Payment' in English (OK)

4. Switch interface to other language

5. Redo step 3
   --> On the printed slip, the word 'Payment' is still in English (not ok)

6. In Administration > Global system preferences, enable TranslateNotices

7. Go to Tools > Notices and slips > ACCOUNT_CREDIT and copy the content of the letter into all the languages

8. Redo step 3 (you should still be in the other language interface)
   --> On the printed slip, the word 'Payment' is still in English (not ok)

9. Edit the patron's account and change the preferred language to the other language

10. Redo step 3
   --> On the printed slip, the word 'Payment' is in the other language (Paiement) (OK)

11. Apply the patch

12. Reset config
    12.1. Edit the patron's account and change back the preferred language to 'default'
    12.2. In Administration > Global system preferences, disable TranslateNotices

13. Redo step 3 (you should still be in the other language interface)
   --> On the printed slip, the word 'Payment' is in the other language (Paiement)

14. Switch interface to the english language

15. Redo step 3
   --> On the printed slip, the word 'Payment' is in English

16. In Administration > Global system preferences, enable TranslateNotices

17. Edit the patron's account and change the preferred language to the other language

18. Redo step 3
   --> On the printed slip, the word 'Payment' is in the other language (Paiement)

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 165474 [details] [review]
Bug 33237: (follow-up) Tidy

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 165475 [details] [review]
Bug 33237: (follow-up) Unit tests

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 165476 [details] [review]
Bug 33237: (follow-up) Restore final fallback

This restores the final fallback to 'en' as mentioned by Jonathan

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 165477 [details] [review]
Bug 33237: (QA follow-up) Clarify tests and Mock

This patch tries to clarify the tests a little and expand their
coverage whilst also adding some defualt translated includes to the test
area and mocking the config to allow the tests to run without installing
translations.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 165478 [details] [review]
Bug 33237: Add display of which language default language is expected

This patch adds the display of 'Default language' to the 'Default'
language in the notices editor tool.

This is so that librarians know which language they are expected to be
writing the notice in so we can remain consistent in both template and
include language used.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 165479 [details] [review]
Bug 33237: Rearrange unit tests and ensure consistency

This patch re-arranges the unit tests and sets them to ensure we
consistently return the notice and notice includes in the same language.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 165480 [details] [review]
Bug 33237: Move and simplify langauge selection logic

This patch moves the language selection into GetPreparedLetter, drops
the interface check step and ensures the default template always uses
the default include language as per the OPACLanguages settings.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

I'm comfortable with this all now.. we have:

* Strong test coverage and don't require installation of translations
* Consistent behaviour across use cases
  - 'Default' = 'Top priority OPACLanguage' regardless of the interface used to trigger the notice or whether it's a fallback to the default notice because a localised version doesn't exist or TranslateNotices is disabled.
* Display of which language the default notice is expected to be written in.

Unit tests pass, QA scripts are happy and discussions have all reached conclusion on Mattermost and here.

Passing QA

I just wanted to ponder the change a bit since we had also had some trouble here. I will be back for this soon!

This patch made quite a u-turn from "use the interface language" in the second patch to dropping it totally out of the equation.
As we don't have an adjusted test plan - could you please explain/summarize the behavior in the release notes?

Some thoughts on this for (much?) later:

* We now rely on the language sequence being changed. This sequence defined, if I am not mistaken, which is the fallback language for missing templates, the display order, ... and? It might be worth adding a note/documentation about this to the system preference description at least. We never changed these settings, we will have to start doing so now.

* With the move from "hardcoded" notice templates to the nice configurable notices we had a slight regression for some libraries: Until then it was possible to print the slip in any installed language by switching the staff interface language. Now this no longer works. We rely on the settings in the patron record or the default sequence. It makes sense, but it's a change and there is no way to "simulate" the old behavior. Maybe it would be nice to have another way of selecting the language for printing something in from the GUI.

Pushed for 24.05!

Well done everyone, thank you!

Follow-up bugs reported and linked to this one.

(In reply to Martin Renvoize from comment #49)
> Follow-up bugs reported and linked to this one.

Thank you!

+    t::lib::Mocks::mock_config( 'intrahtdocs', '/kohadevbox/koha/t/mock_templates/intranet-tmpl' );

This should not have passed QA.
Hardcoded kohadevbox paths.
Will submit follow-up.

Created attachment 166860 [details] [review]
Bug 33237: (QA follow-up) Remove hardcoded kohadevbox path

Test plan:
Run Letters.t (outside kohadevbox :)

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Picked QA follow-up for main.

Pushed for 24.05!

Well done everyone, thank you!

Not backported to 23.11.x

Can we have a backport for this please? 
I know that 22.11 is affected.

OK I have a look

Pushed to 23.11.x for 23.11.07

not backporting to 23.05.x unless requested