Bug 21753

Summary: issuingrules.chargename is unused and should be removed
Product: Koha Reporter: Martin Renvoize <martin.renvoize>
Component: DatabaseAssignee: Martin Renvoize <martin.renvoize>
Status: CLOSED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: enhancement    
Priority: P5 - low CC: aleisha, jonathan.druart, m.de.rooy, martin.renvoize, nick, pierre-marc.thibault, tomascohen
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12365
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22356
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 21747, 22374    
Attachments: Bug 21753: Remove all reference of chargename from scripts
Bug 21753: DBRev to remove the field from the database
[SIGNED-OFF] Bug 21753: Remove all reference of chargename from scripts
[SIGNED-OFF] Bug 21753: DBRev to remove the field from the database
Bug 21753: Remove all reference of chargename from scripts
Bug 21753: DBRev to remove the field from the database
Bug 21753: (QA follow-up) Remove fourth %s in sprintf

Description Martin Renvoize 2018-11-01 20:09:25 UTC
There is no way to set the chargename field in the issuingrules table and there is at least one FIXME (in C4::Overdues::CalcFine) stating that it is never set.

A decade has passed since the FIXME was added to C4::Overdues, and nothing has changed. 

We should remove the field now ;)
Comment 1 Martin Renvoize 2018-11-02 10:42:06 UTC
Created attachment 81876 [details] [review]
Bug 21753: Remove all reference of chargename from scripts
Comment 2 Martin Renvoize 2018-11-02 10:42:09 UTC
Created attachment 81877 [details] [review]
Bug 21753: DBRev to remove the field from the database
Comment 3 Pierre-Marc Thibault 2018-11-02 20:36:52 UTC
Can you provide a test plan so it will be easy to test and sign it? Thank you.
Comment 4 Martin Renvoize 2018-11-03 08:17:11 UTC
Test Plan

Pre-patch:
1) Verify that there is no way to populate this field (short of manually entering data at the command line using SQL)
  - As far as I can tell, the issuingrules table is always set via the circulation map and there is not option to 'name' each circ rule.

Post-patch:
1) Verify that fines are still calculated correctly
2) Verify that all the circulation related tests still pass (or run the entire test suit to be especially sure there are no unseen wider implications)

Notes:
The field, should it ever have been available for input, would have displayed as part of the fines message in the action logs (if finelogs was turned on) and as part of the textual output if you ran the fines cronjob in verbose mode.  However, as it was never filled it obviously wasn't ever output.

Hope that helps.. it's hard to come up with a test plan for something that removes a field that was never visible to the end user (or used functionally internally either).
Comment 5 Aleisha Amohia 2018-11-28 23:12:56 UTC
Created attachment 82715 [details] [review]
[SIGNED-OFF] Bug 21753: Remove all reference of chargename from scripts

Signed-off-by: Aleisha Amohia <aleishaamohia@hotmail.com>
Comment 6 Aleisha Amohia 2018-11-28 23:13:05 UTC
Created attachment 82716 [details] [review]
[SIGNED-OFF] Bug 21753: DBRev to remove the field from the database

Signed-off-by: Aleisha Amohia <aleishaamohia@hotmail.com>
Comment 7 Marcel de Rooy 2018-11-29 12:01:55 UTC
Created attachment 82730 [details] [review]
Bug 21753: Remove all reference of chargename from scripts

Signed-off-by: Aleisha Amohia <aleishaamohia@hotmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 8 Marcel de Rooy 2018-11-29 12:02:03 UTC
Created attachment 82731 [details] [review]
Bug 21753: DBRev to remove the field from the database

Signed-off-by: Aleisha Amohia <aleishaamohia@hotmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 9 Marcel de Rooy 2018-11-29 12:02:09 UTC
Created attachment 82732 [details] [review]
Bug 21753: (QA follow-up) Remove fourth %s in sprintf

Trivial.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 10 Marcel de Rooy 2018-11-29 12:03:05 UTC
@RM: Dont forget DBIx
Comment 11 Nick Clemens 2019-01-09 01:17:29 UTC
Awesome work all!

Pushed to master for 19.05
Comment 12 Martin Renvoize 2019-01-14 14:01:55 UTC
Enhancement, will not be backported to 18.11.x series.
Comment 13 Jonathan Druart 2019-02-17 15:01:17 UTC
git bisect blames
  commit aa71519660a43cebc734763e4821818dfc636775
  Bug 21753: Remove all reference of chargename from scripts

for the following warning from t/db_dependent/Circulation.t:
Use of uninitialized value $actionname in concatenation (.) or string at /home/vagrant/kohaclone/C4/Log.pm line 85.
Comment 14 Martin Renvoize 2019-02-20 09:19:06 UTC
crap.. I missed one.. thanks Jonathan.. I'll add a followup in a new bug.
Comment 15 Martin Renvoize 2019-02-20 09:29:34 UTC
Bug 22374 created