Bug 11672 - Untranslatable report areas in step 1 of Guided Reports
Summary: Untranslatable report areas in step 1 of Guided Reports
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: I18N/L10N (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-02-04 06:57 UTC by paxed
Modified: 2015-06-04 23:33 UTC (History)
3 users (show)

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


Attachments
Fix untranslatable report areas (2.08 KB, patch)
2014-05-06 06:46 UTC, paxed
Details | Diff | Splinter Review
Bug 11672: (regression test) get_report_areas gets tested (1.86 KB, patch)
2014-08-24 05:31 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 11672: Untranslatable dropdown on Guided Reports and dictionary (11.55 KB, patch)
2014-08-24 05:31 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 11672: (followup) warnings tested (1.32 KB, patch)
2014-08-27 13:12 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 11672: (regression test) get_report_areas gets tested (1.97 KB, patch)
2014-08-27 13:17 UTC, Owen Leonard
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 11672: Untranslatable dropdown on Guided Reports and dictionary (11.66 KB, patch)
2014-08-27 13:18 UTC, Owen Leonard
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 11672: (followup) warnings tested (1.42 KB, patch)
2014-08-27 13:21 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 11672: (regression test) get_report_areas gets tested (1.97 KB, patch)
2014-09-22 13:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11672: Untranslatable dropdown on Guided Reports and dictionary (11.66 KB, patch)
2014-09-22 13:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11672: (followup) warnings tested (1.43 KB, patch)
2014-09-22 13:21 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description paxed 2014-02-04 06:57:08 UTC
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)
Comment 1 paxed 2014-05-06 06:46:12 UTC Comment hidden (obsolete)
Comment 2 Owen Leonard 2014-05-06 19:58:18 UTC
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.
Comment 3 paxed 2014-05-07 05:43:09 UTC
(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.
Comment 4 Owen Leonard 2014-05-07 12:31:44 UTC
(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.
Comment 5 Tomás Cohen Arazi 2014-08-24 05:31:14 UTC Comment hidden (obsolete)
Comment 6 Tomás Cohen Arazi 2014-08-24 05:31:24 UTC Comment hidden (obsolete)
Comment 7 Tomás Cohen Arazi 2014-08-27 13:12:40 UTC Comment hidden (obsolete)
Comment 8 Owen Leonard 2014-08-27 13:17:54 UTC Comment hidden (obsolete)
Comment 9 Owen Leonard 2014-08-27 13:18:53 UTC Comment hidden (obsolete)
Comment 10 Owen Leonard 2014-08-27 13:21:33 UTC Comment hidden (obsolete)
Comment 11 Jonathan Druart 2014-09-22 13:21:00 UTC
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>
Comment 12 Jonathan Druart 2014-09-22 13:21:03 UTC
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>
Comment 13 Jonathan Druart 2014-09-22 13:21:06 UTC
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>
Comment 14 Tomás Cohen Arazi 2014-09-23 18:49:55 UTC
Patches pushed to master.