|
Description
paxed
2014-02-04 06:57:08 UTC
Created attachment 27955 [details] [review] Fix untranslatable report areas The guided reports UI shows a drop-down box with "Circulation", "Catalogue", etc. These are hard-coded in C4/Reports/Guided.pm (the @REPORT_AREA hash) To test: 1) cd misc/translator 2) perl translate update xx-YY 3) there should be no translations msgid for the report area names in the po/xx-YY-i-staff-t-prog-v-3006000.po 4) apply patch 5) perl translate update xx-YY 6) check po/xx-YY-i-staff-t-prog-v-3006000.po that it contains the msgid - search for "translate_report_areas" This looks good to me as far as functionality goes, but Koha's templates use the American "Catalog" instead of "Catalogue." Not really worth failing the patch, but I disagree with this non-standard indentation in the template: [% BLOCK translate_report_areas %] [% SWITCH area %] [% CASE 'CIRC' %]Circulation [% CASE 'CAT' %]Catalogue [% CASE 'PAT' %]Patrons [% CASE 'ACQ' %]Acquisition [% CASE 'ACC' %]Accounts [% END %] [% END %] I would prefer to see standard indentation on a line-by-line basis. (In reply to Owen Leonard from comment #2) > This looks good to me as far as functionality goes, but Koha's templates use > the American "Catalog" instead of "Catalogue." > C4/Reports/Guided.pm, where those values came from: [CAT => "Catalogue"], > Not really worth failing the patch, but I disagree with this non-standard > indentation in the template: > > [% BLOCK translate_report_areas %] > [% SWITCH area %] > [% CASE 'CIRC' %]Circulation > [% CASE 'CAT' %]Catalogue > [% CASE 'PAT' %]Patrons > [% CASE 'ACQ' %]Acquisition > [% CASE 'ACC' %]Accounts > [% END %] > [% END %] > > I would prefer to see standard indentation on a line-by-line basis. Define standard indentation. I see no mention of indentation for the templates in the coding guidelines. (In reply to paxed from comment #3) > (In reply to Owen Leonard from comment #2) > > This looks good to me as far as functionality goes, but Koha's templates use > > the American "Catalog" instead of "Catalogue." > > > > C4/Reports/Guided.pm, where those values came from: > [CAT => "Catalogue"], Yes of course it's logical to have used that spelling since you were referencing the text in the script. However you will find that in the default en templates the standard spelling is "Catalog." > > [% BLOCK translate_report_areas %] > > [% SWITCH area %] > > [% CASE 'CIRC' %]Circulation > > [% CASE 'CAT' %]Catalogue > > [% CASE 'PAT' %]Patrons > > [% CASE 'ACQ' %]Acquisition > > [% CASE 'ACC' %]Accounts > > [% END %] > > [% END %] > > Define standard indentation. I see no mention of indentation for the > templates in the coding guidelines. Again, sometimes the standard is simply, "what you see consistently in other templates." Indentation isn't always consistent, but generally speaking I would consider standard indentation to be: [% BLOCK translate_report_areas %] [% SWITCH area %] [% CASE 'CIRC' %]Circulation [% CASE 'CAT' %]Catalogue [% CASE 'PAT' %]Patrons [% CASE 'ACQ' %]Acquisition [% CASE 'ACC' %]Accounts [% END %] [% END %] I think this kind of indentation helps others more easily understand the logical structure of the template tags. It also works better with a text editor's indentation tools. That's just my opinion of course and a discussion on the developer's list could help sort it out. Created attachment 31125 [details] [review] Bug 11672: (regression test) get_report_areas gets tested This patch introduces a unit test for the new get_report_areas functionality. To try it, run $ prove t/db_dependent/Reports_Guided.t It is expected to have 16 tests defined, and to fail the introduced one Created attachment 31126 [details] [review] Bug 11672: Untranslatable dropdown on Guided Reports and dictionary This patch removes hardcoded descriptions and sets them in the templates using the variable content as id. To test, create a new guided report and verify the 'module to report on' dropdown shows as usual [1]. Functionality shouldn't get changed. The patch also changes the dictionary pages where 'area' should be displayed/selectable with the same strings as the guided reports. Try all the possible disctionary pages. The last page when creating a dictionary now shows the 'area description' instead of the code. The same happens to the dictionary list once you have dictionaries saved. [1] The following texts get changed: Catalogue -> Catalog Acquisition -> Acquisitions Created attachment 31187 [details] [review] Bug 11672: (followup) warnings tested Created attachment 31188 [details] [review] [SIGNED-OFF] Bug 11672: (regression test) get_report_areas gets tested This patch introduces a unit test for the new get_report_areas functionality. To try it, run $ prove t/db_dependent/Reports_Guided.t It is expected to have 16 tests defined, and to fail the introduced one Signed-off-by: Owen Leonard <oleonard@myacpl.org> Created attachment 31189 [details] [review] [SIGNED-OFF] Bug 11672: Untranslatable dropdown on Guided Reports and dictionary This patch removes hardcoded descriptions and sets them in the templates using the variable content as id. To test, create a new guided report and verify the 'module to report on' dropdown shows as usual [1]. Functionality shouldn't get changed. The patch also changes the dictionary pages where 'area' should be displayed/selectable with the same strings as the guided reports. Try all the possible disctionary pages. The last page when creating a dictionary now shows the 'area description' instead of the code. The same happens to the dictionary list once you have dictionaries saved. [1] The following texts get changed: Catalogue -> Catalog Acquisition -> Acquisitions Signed-off-by: Owen Leonard <oleonard@myacpl.org> Created attachment 31190 [details] [review] [SIGNED-OFF] Bug 11672: (followup) warnings tested Signed-off-by: Owen Leonard <oleonard@myacpl.org> Created attachment 31808 [details] [review] Bug 11672: (regression test) get_report_areas gets tested This patch introduces a unit test for the new get_report_areas functionality. To try it, run $ prove t/db_dependent/Reports_Guided.t It is expected to have 16 tests defined, and to fail the introduced one Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Created attachment 31809 [details] [review] Bug 11672: Untranslatable dropdown on Guided Reports and dictionary This patch removes hardcoded descriptions and sets them in the templates using the variable content as id. To test, create a new guided report and verify the 'module to report on' dropdown shows as usual [1]. Functionality shouldn't get changed. The patch also changes the dictionary pages where 'area' should be displayed/selectable with the same strings as the guided reports. Try all the possible disctionary pages. The last page when creating a dictionary now shows the 'area description' instead of the code. The same happens to the dictionary list once you have dictionaries saved. [1] The following texts get changed: Catalogue -> Catalog Acquisition -> Acquisitions Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Created attachment 31810 [details] [review] Bug 11672: (followup) warnings tested Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Patches pushed to master. |