Bug 23445 - Loan period unit in circulation rules is untranslatable causing problems when editing rules
Summary: Loan period unit in circulation rules is untranslatable causing problems when...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: System Administration (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Katrin Fischer
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-08-10 19:22 UTC by Katrin Fischer
Modified: 2020-11-30 21:44 UTC (History)
4 users (show)

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


Attachments
Bug 23445: Make loan period unit translatable fixing editing issue (2.31 KB, patch)
2019-08-10 19:29 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 23445: Make loan period unit translatable fixing editing issue (2.36 KB, patch)
2019-08-15 14:18 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 23445: Make loan period unit translatable fixing editing issue (2.46 KB, patch)
2019-08-16 09:00 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 23445: (RM follow-up) Add warning for unexpected lengthunit (3.47 KB, patch)
2019-08-16 11:20 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Katrin Fischer 2019-08-10 19:22:13 UTC

    
Comment 1 Katrin Fischer 2019-08-10 19:29:29 UTC
Created attachment 92137 [details] [review]
Bug 23445: Make loan period unit translatable fixing editing issue

In the circulation rules, the loan period displayed in the table
of circulation rules is not translatable, always displaying in English.
This causes an issue when editing a circ rule with unit hours in any
other language than English.
When editing these rules, instead of hours, days will be preselected.

To test:
- Before applying the patch:
- Install a translation
- Activate and switch to the tranlation
- Create a circulation rule using 'hours' as the unit
- Edit the circulation rule
- Verify that instead of 'hours' 'days' will be selected in the pull-down
- Apply patch
- Update po files (kshell, cd misc/translator, perl translate update <langcode>)
- Edit po file <langcode-staff..., search for Hours and translate line,
  make sure to also remove fuzzy, and that your translation matches the
  pull down values for your language
- Reinstall translation
- Repeat circ rule edit test
- Verify the unit now shows up with your translation and editing works
Comment 2 Owen Leonard 2019-08-15 14:18:54 UTC
Created attachment 92240 [details] [review]
Bug 23445: Make loan period unit translatable fixing editing issue

In the circulation rules, the loan period displayed in the table
of circulation rules is not translatable, always displaying in English.
This causes an issue when editing a circ rule with unit hours in any
other language than English.
When editing these rules, instead of hours, days will be preselected.

To test:
- Before applying the patch:
- Install a translation
- Activate and switch to the tranlation
- Create a circulation rule using 'hours' as the unit
- Edit the circulation rule
- Verify that instead of 'hours' 'days' will be selected in the pull-down
- Apply patch
- Update po files (kshell, cd misc/translator, perl translate update <langcode>)
- Edit po file <langcode-staff..., search for Hours and translate line,
  make sure to also remove fuzzy, and that your translation matches the
  pull down values for your language
- Reinstall translation
- Repeat circ rule edit test
- Verify the unit now shows up with your translation and editing works

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 3 Katrin Fischer 2019-08-15 19:28:41 UTC
Thanks a lot!
Comment 4 Marcel de Rooy 2019-08-16 07:09:31 UTC
This might be just a theoretical thing: But I would rather not introduce Undefined here. Just as in the code of Circulation, I would go for if ==days then days else hours or even better if ==hours then hours else days (since days is the default).
Comment 5 Katrin Fischer 2019-08-16 08:48:49 UTC
(In reply to Marcel de Rooy from comment #4)
> This might be just a theoretical thing: But I would rather not introduce
> Undefined here. Just as in the code of Circulation, I would go for if ==days
> then days else hours or even better if ==hours then hours else days (since
> days is the default).

I think that would be hiding possible problems in the database from the GUI. I and probably other tend to edit the issuingrules in SQL, especially for loading them the first time. If you have a typo or other issue there, you'd hide it. It's only in the GUI. We can also imagine that at some point minutes will be introduced (there is already a bug report) and that makes the logic there obsolete.

Note: Undefined doesn't appear in the pull down!
Comment 6 Katrin Fischer 2019-08-16 08:49:22 UTC
Oh, and undefined or similar is used for the hard due dates - looked there for inspiration in the template.
Comment 7 Katrin Fischer 2019-08-16 08:53:06 UTC
Hm, not sure I phrased that well... I meant better check explicitly and at some point we will have more than 2 values. This just makes a 'nonexpected' value show as Undefined in the GUI which is think is a good clue for debugging.
Comment 8 Marcel de Rooy 2019-08-16 09:00:41 UTC
Created attachment 92261 [details] [review]
Bug 23445: Make loan period unit translatable fixing editing issue

In the circulation rules, the loan period displayed in the table
of circulation rules is not translatable, always displaying in English.
This causes an issue when editing a circ rule with unit hours in any
other language than English.
When editing these rules, instead of hours, days will be preselected.

To test:
- Before applying the patch:
- Install a translation
- Activate and switch to the tranlation
- Create a circulation rule using 'hours' as the unit
- Edit the circulation rule
- Verify that instead of 'hours' 'days' will be selected in the pull-down
- Apply patch
- Update po files (kshell, cd misc/translator, perl translate update <langcode>)
- Edit po file <langcode-staff..., search for Hours and translate line,
  make sure to also remove fuzzy, and that your translation matches the
  pull down values for your language
- Reinstall translation
- Repeat circ rule edit test
- Verify the unit now shows up with your translation and editing works

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 9 Martin Renvoize 2019-08-16 11:20:07 UTC
Created attachment 92270 [details] [review]
Bug 23445: (RM follow-up) Add warning for unexpected lengthunit

Discussions on this bug highlighted worries about behind the scenes
fallbacks to 'days' as a lengthunit should the lengthunit be set to an
unrecognised value. This patch adds a warning to the about page for any
such occurrences so librarians may be made aware of and resolve the issue.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 10 Martin Renvoize 2019-08-16 11:21:24 UTC
Nice work!

Pushed to master for 19.11.00
Comment 11 Fridolin Somers 2019-09-05 14:52:52 UTC
Pushed to 19.05.x for 19.05.04