Bug 21753 - issuingrules.chargename is unused and should be removed
Summary: issuingrules.chargename is unused and should be removed
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Database (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Martin Renvoize
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks: 21747 22374
  Show dependency treegraph
 
Reported: 2018-11-01 20:09 UTC by Martin Renvoize
Modified: 2019-06-27 09:24 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 21753: Remove all reference of chargename from scripts (11.58 KB, patch)
2018-11-02 10:42 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21753: DBRev to remove the field from the database (2.00 KB, patch)
2018-11-02 10:42 UTC, Martin Renvoize
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 21753: Remove all reference of chargename from scripts (11.63 KB, patch)
2018-11-28 23:12 UTC, Aleisha Amohia
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 21753: DBRev to remove the field from the database (2.06 KB, patch)
2018-11-28 23:13 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 21753: Remove all reference of chargename from scripts (11.71 KB, patch)
2018-11-29 12:01 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21753: DBRev to remove the field from the database (2.14 KB, patch)
2018-11-29 12:02 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21753: (QA follow-up) Remove fourth %s in sprintf (1.01 KB, patch)
2018-11-29 12:02 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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