|
Description
Marcel de Rooy
2021-10-15 07:38:51 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> 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> 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> WIP: Need to check calls in codebase now 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> 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> 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> 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> 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> 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> Trivial small fix, waiting for 2 months ? 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' );
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. (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 ;) (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 :) 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. (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. This just takes too much time. Without the prospect of getting someone to sign off. Closing the report. (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'. (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 ;) |