Bug 35539 - Remove unused columns from categories table
Summary: Remove unused columns from categories table
Status: Signed Off
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Pedro Amorim
QA Contact: Jonathan Druart
URL:
Keywords:
Depends on: 26297 35681
Blocks:
  Show dependency treegraph
 
Reported: 2023-12-11 16:33 UTC by Pedro Amorim
Modified: 2024-04-29 09:11 UTC (History)
9 users (show)

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


Attachments
Bug 35539: [DONT PUSH] dbic (2.12 KB, patch)
2024-01-02 12:49 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35539: Remove 'bulk' column/field from categories table (7.09 KB, patch)
2024-01-02 12:49 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35539: Remove 'finetype' column/field from categories table (7.07 KB, patch)
2024-01-02 12:49 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35539: Remove 'issuelimit' column/field from categories table (7.21 KB, patch)
2024-01-02 12:49 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35539: Atomicupdate (3.35 KB, patch)
2024-01-02 12:49 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35539: [DONT PUSH] dbic (2.17 KB, patch)
2024-01-02 17:35 UTC, David Nind
Details | Diff | Splinter Review
Bug 35539: Remove 'bulk' column/field from categories table (7.13 KB, patch)
2024-01-02 17:35 UTC, David Nind
Details | Diff | Splinter Review
Bug 35539: Remove 'finetype' column/field from categories table (7.12 KB, patch)
2024-01-02 17:35 UTC, David Nind
Details | Diff | Splinter Review
Bug 35539: Remove 'issuelimit' column/field from categories table (7.25 KB, patch)
2024-01-02 17:35 UTC, David Nind
Details | Diff | Splinter Review
Bug 35539: Atomicupdate (3.40 KB, patch)
2024-01-02 17:35 UTC, David Nind
Details | Diff | Splinter Review
Bug 35539: Atomicupdate (3.53 KB, patch)
2024-01-03 10:25 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35539: Atomicupdate (3.59 KB, patch)
2024-04-29 09:11 UTC, Pedro Amorim
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Pedro Amorim 2023-12-11 16:33:42 UTC
Looking at the code (first commited 17 years ago) the "bulk" column in the 'categories' table appears to not be utilized anywhere in the code.

Is anyone able to confirm? I suggest we remove this if that is the case.
Comment 1 Pedro Amorim 2023-12-11 16:35:25 UTC
Same goes for the finetype column.
Comment 2 Pedro Amorim 2023-12-11 16:40:54 UTC
Same for issuelimit
Comment 3 Jonathan Druart 2023-12-12 07:29:17 UTC
Good catch.

Found this comment:
commit 3aa00f63131fdccba4bb57870bc86c93329d5050
Author: Nicole C. Engard <nicole@bywatersolutions.com>
Date:   Mon Oct 10 06:50:23 2011 -0400

    Bug 6716 : Categories table
    
    This patch documents the patron categories table in Koha.
    There are a couple fields marked unused and a couple that aren't
    documented because I wasn't sure:
    
    * dateofbirthrequired
    * bulk

Really, if Nicole didn't know about 'bulk' and marked the other 2 as unused, I am sure we can safely remove them!
Comment 4 Katrin Fischer 2023-12-12 09:42:36 UTC
I think it would be nice to check if the columns have 'content' (I imagine someone might have 'parked' data there from a migration) and not drop them in that case or force people to clean up before resuming the database update.

But yes +1 for clean-up.
Comment 5 Pedro Amorim 2024-01-02 12:49:30 UTC
Created attachment 160400 [details] [review]
Bug 35539: [DONT PUSH] dbic

Removal of bulk
Removal of finetype
Removal of issuelimit
Comment 6 Pedro Amorim 2024-01-02 12:49:32 UTC
Created attachment 160401 [details] [review]
Bug 35539: Remove 'bulk' column/field from categories table
Comment 7 Pedro Amorim 2024-01-02 12:49:35 UTC
Created attachment 160402 [details] [review]
Bug 35539: Remove 'finetype' column/field from categories table
Comment 8 Pedro Amorim 2024-01-02 12:49:38 UTC
Created attachment 160403 [details] [review]
Bug 35539: Remove 'issuelimit' column/field from categories table
Comment 9 Pedro Amorim 2024-01-02 12:49:40 UTC
Created attachment 160404 [details] [review]
Bug 35539: Atomicupdate

Removal of 'bulk' column
Removal of 'finetype' column
Removal of 'issuelimit' column

Test plan, k-t-d:
1) Add data to the relevant columns, run the following mysql:
update categories set bulk = 1;
update categories set finetype = 1;
update categories set issuelimit = 1;
2) Run updatedatabase, notice it shows a warning and does not update the database
3) Clean up the data, run the folllowing mysql:
update categories set bulk = null;
update categories set finetype = null;
update categories set issuelimit = null;
4) Run updatedatabase again, notice it updates the database as intended

Run:
t/db_dependent/Circulation/GetHardDueDate.t
Comment 10 Martin Renvoize 2024-01-02 13:46:45 UTC
Comment on attachment 160404 [details] [review]
Bug 35539: Atomicupdate

Review of attachment 160404 [details] [review]:
-----------------------------------------------------------------

::: installer/data/mysql/atomicupdate/bug_35539.pl
@@ +15,5 @@
> +                SELECT bulk FROM categories;
> +            |
> +            );
> +            if ($bulkdata) {
> +                say $out YELLOW,

We'll need to test how this looks in the UI.. it's a nice addition for the console output, but not everyone runs their updates this way.. many use the web UI
Comment 11 Pedro Amorim 2024-01-02 16:02:08 UTC
(In reply to Martin Renvoize from comment #10)
> We'll need to test how this looks in the UI.. it's a nice addition for the
> console output, but not everyone runs their updates this way.. many use the
> web UI

Fair point, after testing an upgrade through the browser, the ANSI codes are printed in the web UI and it doesn't look good. I gave a shot trying to grab the colors coming from the terminal and converting them to HTML (using HTML::FromANSI) but this seems to create more problems than it solves.

The idea here was to highlight - in a different color - that some action is required, before or following this upgrade.

I've posted my WIP at bug 35682
Comment 12 David Nind 2024-01-02 17:35:37 UTC
Created attachment 160413 [details] [review]
Bug 35539: [DONT PUSH] dbic

Removal of bulk
Removal of finetype
Removal of issuelimit

Signed-off-by: David Nind <david@davidnind.com>
Comment 13 David Nind 2024-01-02 17:35:40 UTC
Created attachment 160414 [details] [review]
Bug 35539: Remove 'bulk' column/field from categories table

Signed-off-by: David Nind <david@davidnind.com>
Comment 14 David Nind 2024-01-02 17:35:43 UTC
Created attachment 160415 [details] [review]
Bug 35539: Remove 'finetype' column/field from categories table

Signed-off-by: David Nind <david@davidnind.com>
Comment 15 David Nind 2024-01-02 17:35:46 UTC
Created attachment 160416 [details] [review]
Bug 35539: Remove 'issuelimit' column/field from categories table

Signed-off-by: David Nind <david@davidnind.com>
Comment 16 David Nind 2024-01-02 17:35:49 UTC
Created attachment 160417 [details] [review]
Bug 35539: Atomicupdate

Removal of 'bulk' column
Removal of 'finetype' column
Removal of 'issuelimit' column

Test plan, k-t-d:
1) Add data to the relevant columns, run the following mysql:
update categories set bulk = 1;
update categories set finetype = 1;
update categories set issuelimit = 1;
2) Run updatedatabase, notice it shows a warning and does not update the database
3) Clean up the data, run the folllowing mysql:
update categories set bulk = null;
update categories set finetype = null;
update categories set issuelimit = null;
4) Run updatedatabase again, notice it updates the database as intended

Run:
t/db_dependent/Circulation/GetHardDueDate.t

Signed-off-by: David Nind <david@davidnind.com>
Comment 17 Martin Renvoize 2024-01-02 23:29:47 UTC
Hmm, I'm not seeing the "reset" being called in the db update. As in, ANSIColor resets.. i'd suggest using the `colored()` method alternatively, as it automatically wrapped the strings with a reset at the end working well with `say`. Or you might use AUTORESET instead.
Comment 18 Pedro Amorim 2024-01-03 10:25:49 UTC
Created attachment 160446 [details] [review]
Bug 35539: Atomicupdate

Removal of 'bulk' column
Removal of 'finetype' column
Removal of 'issuelimit' column

Test plan, k-t-d:
1) Add data to the relevant columns, run the following mysql:
update categories set bulk = 1;
update categories set finetype = 1;
update categories set issuelimit = 1;
2) Run updatedatabase, notice it shows a warning and does not update the database
3) Clean up the data, run the folllowing mysql:
update categories set bulk = null;
update categories set finetype = null;
update categories set issuelimit = null;
4) Run updatedatabase again, notice it updates the database as intended

Run:
t/db_dependent/Circulation/GetHardDueDate.t

Signed-off-by: David Nind <david@davidnind.com>
Comment 19 Pedro Amorim 2024-01-03 10:27:32 UTC
I've updated the atomicupdate to use updatedatabase report colors work from bug 35681.

Colors should now be correctly applied only to the lines they're meant to apply, and be correctly shown in update reports in both CLI and browser UI.
Comment 20 Jonathan Druart 2024-01-25 11:28:36 UTC Comment hidden (obsolete)
Comment 21 Jonathan Druart 2024-01-25 11:29:03 UTC Comment hidden (obsolete)
Comment 22 Pedro Amorim 2024-04-29 09:11:11 UTC
Created attachment 165724 [details] [review]
Bug 35539: Atomicupdate

Removal of 'bulk' column
Removal of 'finetype' column
Removal of 'issuelimit' column

Test plan, k-t-d:
1) Add data to the relevant columns, run the following mysql:
update categories set bulk = 1;
update categories set finetype = 1;
update categories set issuelimit = 1;
2) Run updatedatabase, notice it shows a warning and does not update the database
3) Clean up the data, run the folllowing mysql:
update categories set bulk = null;
update categories set finetype = null;
update categories set issuelimit = null;
4) Run updatedatabase again, notice it updates the database as intended

Run:
t/db_dependent/Circulation/GetHardDueDate.t

Signed-off-by: David Nind <david@davidnind.com>