Bug 29245 - Fix the parameters of getTranslatedLanguages
Summary: Fix the parameters of getTranslatedLanguages
Status: CLOSED WONTFIX
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Bugs List
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-10-15 07:38 UTC by Marcel de Rooy
Modified: 2022-12-12 21:24 UTC (History)
0 users

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


Attachments
Bug 29245: Fix parameters of getTranslatedLanguages (3.12 KB, patch)
2021-10-15 10:06 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29245: Prepare Languages.t for changes (9.63 KB, patch)
2021-10-15 10:06 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29245: Extend tests for getTranslatedLanguages (4.44 KB, patch)
2021-10-15 10:06 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29245: Extend tests for getTranslatedLanguages (4.28 KB, patch)
2021-10-15 10:50 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29245: Extend tests for getTranslatedLanguages (4.29 KB, patch)
2021-10-15 12:00 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29245: Fix parameters of getTranslatedLanguages (3.12 KB, patch)
2021-10-15 14:42 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29245: Prepare Languages.t for changes (9.63 KB, patch)
2021-10-15 14:42 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29245: Extend tests for getTranslatedLanguages (4.29 KB, patch)
2021-10-15 14:42 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29245: Fix calls of getTranslatedLanguages (7.05 KB, patch)
2021-10-15 14:42 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marcel de Rooy 2021-10-15 07:38:51 UTC
Adjust POD
Parameter which
OPAC calls with preference (which is prog for staff)
Maybe more?
Comment 1 Marcel de Rooy 2021-10-15 10:06:01 UTC
Created attachment 126312 [details] [review]
Bug 29245: Fix parameters of getTranslatedLanguages

Removed which parameter. Unused.
Added fallbacks for interface and theme.
Adjusted POD.

No test plan. If qa tools doesnt complain, you are fine.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 2 Marcel de Rooy 2021-10-15 10:06:04 UTC
Created attachment 126313 [details] [review]
Bug 29245: Prepare Languages.t for changes

Add license.
Reorganize in subtests.
Cosmetic changes: whitespace.
Add rollback.

Test plan:
Run the test again. Confirm that it still passes.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 3 Marcel de Rooy 2021-10-15 10:06:07 UTC
Created attachment 126314 [details] [review]
Bug 29245: Extend tests for getTranslatedLanguages

Test plan:
Run test again.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 4 Marcel de Rooy 2021-10-15 10:07:56 UTC
WIP: Need to check calls in codebase now
Comment 5 Marcel de Rooy 2021-10-15 10:50:33 UTC
Created attachment 126315 [details] [review]
Bug 29245: Extend tests for getTranslatedLanguages

Test plan:
Run test again.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 6 Marcel de Rooy 2021-10-15 12:00:50 UTC
Created attachment 126322 [details] [review]
Bug 29245: Extend tests for getTranslatedLanguages

Test plan:
Run test again.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 7 Marcel de Rooy 2021-10-15 14:42:23 UTC
Created attachment 126345 [details] [review]
Bug 29245: Fix parameters of getTranslatedLanguages

Removed which parameter. Unused.
Added fallbacks for interface and theme.
Adjusted POD.

No test plan. If qa tools doesnt complain, you are fine.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 8 Marcel de Rooy 2021-10-15 14:42:27 UTC
Created attachment 126346 [details] [review]
Bug 29245: Prepare Languages.t for changes

Add license.
Reorganize in subtests.
Cosmetic changes: whitespace.
Add rollback.

Test plan:
Run the test again. Confirm that it still passes.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 9 Marcel de Rooy 2021-10-15 14:42:30 UTC
Created attachment 126347 [details] [review]
Bug 29245: Extend tests for getTranslatedLanguages

Test plan:
Run test again.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 10 Marcel de Rooy 2021-10-15 14:42:33 UTC
Created attachment 126348 [details] [review]
Bug 29245: Fix calls of getTranslatedLanguages

admin/preferences: $current_languages was ignored already.
admin/systempreferences: see preferences; note that bug
29251 should fix already broken behavior.
tools/letter: combining opac and prog was wrong, but caught
by the else branch in get_opac_languages.
members/memberentry: same as letter.
tools/additional-contents: actually no change, but better
to write in full.
opac/opac-messaging.pl: same as letter, caught by else.
admin/itemtypes: should be same as admin/localization.

Test plan:
Have a few opac and staff languages.
Check on some of the above forms the languages display.

Keep in mind that some staff scripts select translated languages
for the OPAC.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 11 Marcel de Rooy 2021-12-13 13:56:03 UTC
Trivial small fix, waiting for 2 months ?
Comment 12 Katrin Fischer 2021-12-13 23:05:58 UTC
Tbh this doesn't read like a trivial fix from the number of code changes. I also think we shouldn't hardcode templates, as this breaks the templating system:

     if ( C4::Context->preference('TranslateNotices') ) {
         my $translated_languages =
-          C4::Languages::getTranslatedLanguages( 'opac',
-            C4::Context->preference('template') );
+            C4::Languages::getTranslatedLanguages( 'opac', 'bootstrap' );
Comment 13 Katrin Fischer 2021-12-13 23:12:54 UTC
I think one problem is the lack of a test plan apart from running the tests, which limits the people who can test, and that there is no easy to understand description of the issue this is trying to fix. - This makes it harder to test, especially for our non-programmer testers.
Comment 14 Marcel de Rooy 2021-12-15 13:11:30 UTC
(In reply to Katrin Fischer from comment #12)
> Tbh this doesn't read like a trivial fix from the number of code changes. I
> also think we shouldn't hardcode templates, as this breaks the templating
> system:
> 
>      if ( C4::Context->preference('TranslateNotices') ) {
>          my $translated_languages =
> -          C4::Languages::getTranslatedLanguages( 'opac',
> -            C4::Context->preference('template') );
> +            C4::Languages::getTranslatedLanguages( 'opac', 'bootstrap' );

Try a git grep ;)
Comment 15 Marcel de Rooy 2021-12-15 13:11:57 UTC
(In reply to Katrin Fischer from comment #13)
> I think one problem is the lack of a test plan apart from running the tests,
> which limits the people who can test, and that there is no easy to
> understand description of the issue this is trying to fix. - This makes it
> harder to test, especially for our non-programmer testers.

You're probably right :)
Comment 16 Katrin Fischer 2021-12-15 13:34:07 UTC
I count myself as a non-progammer tester as well - I think you mean that we already break the rule for not hardcoding the template, but I think those are bugs we should fix then. Hea shows that having your own OPAC template is not as rare as one would think.
Comment 17 Marcel de Rooy 2021-12-15 13:39:16 UTC
(In reply to Katrin Fischer from comment #16)
> I count myself as a non-progammer tester as well - I think you mean that we
> already break the rule for not hardcoding the template, but I think those
> are bugs we should fix then. Hea shows that having your own OPAC template is
> not as rare as one would think.

This is an architectural patch, not the popular kind, yes. And insisting that it should also fix such occurrences makes it even harder. Since the patch is about fixing parameters, I would be tempted to say that it is outside the scope too ;)

Having a look now how to get this on track again. But if it takes too much time, I will abandon it.
Comment 18 Marcel de Rooy 2021-12-17 08:50:14 UTC
This just takes too much time. Without the prospect of getting someone to sign off. Closing the report.
Comment 19 Katrin Fischer 2021-12-17 23:54:38 UTC
(In reply to Marcel de Rooy from comment #17)
> (In reply to Katrin Fischer from comment #16)
> > I count myself as a non-progammer tester as well - I think you mean that we
> > already break the rule for not hardcoding the template, but I think those
> > are bugs we should fix then. Hea shows that having your own OPAC template is
> > not as rare as one would think.
> 
> This is an architectural patch, not the popular kind, yes. And insisting
> that it should also fix such occurrences makes it even harder. Since the
> patch is about fixing parameters, I would be tempted to say that it is
> outside the scope too ;)
> 
> Having a look now how to get this on track again. But if it takes too much
> time, I will abandon it.

I didn't mean fixing them on this bug - just that I would consider it a bug and that we should not add more hardcoded 'bootstraps'.
Comment 20 Marcel de Rooy 2021-12-21 12:11:33 UTC
(In reply to Katrin Fischer from comment #19)
> (In reply to Marcel de Rooy from comment #17)
> > (In reply to Katrin Fischer from comment #16)
> > > I count myself as a non-progammer tester as well - I think you mean that we
> > > already break the rule for not hardcoding the template, but I think those
> > > are bugs we should fix then. Hea shows that having your own OPAC template is
> > > not as rare as one would think.
> > 
> > This is an architectural patch, not the popular kind, yes. And insisting
> > that it should also fix such occurrences makes it even harder. Since the
> > patch is about fixing parameters, I would be tempted to say that it is
> > outside the scope too ;)
> > 
> > Having a look now how to get this on track again. But if it takes too much
> > time, I will abandon it.
> 
> I didn't mean fixing them on this bug - just that I would consider it a bug
> and that we should not add more hardcoded 'bootstraps'.

No problem. I understand your point and was willing to revert a few occurrences too. But working further, I decided to refactor a little bit more. Add more tests and at some point my time was up ;)