Bug 18177 - Remove unused columns in aqbooksellers
Summary: Remove unused columns in aqbooksellers
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Database (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 15272
  Show dependency treegraph
 
Reported: 2017-02-27 12:39 UTC by Tomás Cohen Arazi
Modified: 2020-11-30 21:48 UTC (History)
7 users (show)

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


Attachments
Bug 18177: remove aqbooksellers.othersupplier (1.59 KB, patch)
2020-03-17 09:00 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18177: remove aqbooksellers.booksellerfax (2.75 KB, patch)
2020-03-17 09:00 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18177: remove aqbooksellers.booksellerurl (2.80 KB, patch)
2020-03-17 09:00 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18177: remove aqbooksellers.bookselleremail (1.66 KB, patch)
2020-03-17 09:00 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18177: remove aqbooksellers.currency (1.27 KB, patch)
2020-03-17 09:00 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18177: DB changes (1.40 KB, patch)
2020-03-17 09:00 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18177: DBIC changes (2.19 KB, patch)
2020-03-17 09:00 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18177: remove aqbooksellers.othersupplier (1.60 KB, patch)
2020-03-17 09:01 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18177: remove aqbooksellers.booksellerfax (2.75 KB, patch)
2020-03-17 09:01 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18177: remove aqbooksellers.booksellerurl (2.80 KB, patch)
2020-03-17 09:01 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18177: remove aqbooksellers.bookselleremail (1.66 KB, patch)
2020-03-17 09:02 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18177: remove aqbooksellers.currency (1.27 KB, patch)
2020-03-17 09:02 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18177: DB changes (2.31 KB, patch)
2020-03-17 09:02 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18177: DBIC changes (2.19 KB, patch)
2020-03-17 09:02 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18177: Do not remove columns if at least one value exists (1.46 KB, patch)
2020-03-24 15:29 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18177: remove aqbooksellers.othersupplier (1.67 KB, patch)
2020-03-25 11:40 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18177: remove aqbooksellers.booksellerfax (2.81 KB, patch)
2020-03-25 11:40 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18177: remove aqbooksellers.booksellerurl (2.87 KB, patch)
2020-03-25 11:40 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18177: remove aqbooksellers.bookselleremail (1.72 KB, patch)
2020-03-25 11:40 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18177: remove aqbooksellers.currency (1.34 KB, patch)
2020-03-25 11:40 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18177: DB changes (2.37 KB, patch)
2020-03-25 11:40 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18177: DBIC changes (2.26 KB, patch)
2020-03-25 11:40 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18177: Do not remove columns if at least one value exists (1.52 KB, patch)
2020-03-25 11:41 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18177: remove aqbooksellers.othersupplier (1.76 KB, patch)
2020-03-27 08:25 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18177: remove aqbooksellers.booksellerfax (2.91 KB, patch)
2020-03-27 08:25 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18177: remove aqbooksellers.booksellerurl (2.96 KB, patch)
2020-03-27 08:26 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18177: remove aqbooksellers.bookselleremail (1.82 KB, patch)
2020-03-27 08:26 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18177: remove aqbooksellers.currency (1.43 KB, patch)
2020-03-27 08:26 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18177: DB changes (2.47 KB, patch)
2020-03-27 08:26 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18177: DBIC changes (2.36 KB, patch)
2020-03-27 08:26 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18177: Do not remove columns if at least one value exists (1.62 KB, patch)
2020-03-27 08:26 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18177: (follow-up) remove aqbooksellers.currency (855 bytes, patch)
2020-03-27 15:46 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 Tomás Cohen Arazi 2017-02-27 12:39:42 UTC
I've found that the following columns are not actually used (in Koha's codebase):
- address2
- address3
- address4
- othersupplier
- currency
- booksellerfax
- booksellerurl
Comment 1 Tomás Cohen Arazi 2017-02-27 12:45:38 UTC
address2, address3 and address4 are used if they exist on the DB, by the template. They are concatenated using \n and splitted back on save...
Comment 2 Jonathan Druart 2020-03-17 09:00:22 UTC
Created attachment 100861 [details] [review]
Bug 18177: remove aqbooksellers.othersupplier

This patchset removes some columns from the aqbooksellers table that are
not used:
 * booksellerfax
 * booksellerurl
 * bookselleremail
 * currency

The first 3 ones are certainly leftover from bug 10402.

Test plan:
For each for these fields you will make sure that they cannot be edited
from the interface.
Comment 3 Jonathan Druart 2020-03-17 09:00:25 UTC
Created attachment 100862 [details] [review]
Bug 18177: remove aqbooksellers.booksellerfax
Comment 4 Jonathan Druart 2020-03-17 09:00:28 UTC
Created attachment 100863 [details] [review]
Bug 18177: remove aqbooksellers.booksellerurl
Comment 5 Jonathan Druart 2020-03-17 09:00:32 UTC
Created attachment 100864 [details] [review]
Bug 18177: remove aqbooksellers.bookselleremail
Comment 6 Jonathan Druart 2020-03-17 09:00:35 UTC
Created attachment 100865 [details] [review]
Bug 18177: remove aqbooksellers.currency
Comment 7 Jonathan Druart 2020-03-17 09:00:38 UTC
Created attachment 100866 [details] [review]
Bug 18177: DB changes
Comment 8 Jonathan Druart 2020-03-17 09:00:41 UTC
Created attachment 100867 [details] [review]
Bug 18177: DBIC changes
Comment 9 Jonathan Druart 2020-03-17 09:01:52 UTC
Created attachment 100868 [details] [review]
Bug 18177: remove aqbooksellers.othersupplier

This patchset removes some columns from the aqbooksellers table that are
not used:
 * booksellerfax
 * booksellerurl
 * bookselleremail
 * othersupplier
 * currency

The first 3 ones are certainly leftover from bug 10402.

Test plan:
For each for these fields you will make sure that they cannot be edited
from the interface.
Comment 10 Jonathan Druart 2020-03-17 09:01:56 UTC
Created attachment 100869 [details] [review]
Bug 18177: remove aqbooksellers.booksellerfax
Comment 11 Jonathan Druart 2020-03-17 09:01:59 UTC
Created attachment 100870 [details] [review]
Bug 18177: remove aqbooksellers.booksellerurl
Comment 12 Jonathan Druart 2020-03-17 09:02:02 UTC
Created attachment 100871 [details] [review]
Bug 18177: remove aqbooksellers.bookselleremail
Comment 13 Jonathan Druart 2020-03-17 09:02:05 UTC
Created attachment 100872 [details] [review]
Bug 18177: remove aqbooksellers.currency
Comment 14 Jonathan Druart 2020-03-17 09:02:08 UTC
Created attachment 100873 [details] [review]
Bug 18177: DB changes
Comment 15 Jonathan Druart 2020-03-17 09:02:12 UTC
Created attachment 100874 [details] [review]
Bug 18177: DBIC changes
Comment 16 Jonathan Druart 2020-03-17 09:03:30 UTC
The 4 fields address[1-4] are built from the "Physical address" input.
However, if the address entered has more than 4 lines, the following ones are dropped.
That is for a separate bug report.
Comment 17 Frank Hansen 2020-03-24 12:23:50 UTC
When we migrated from your previous ILS (Virtua), I kept some vendor contact information in booksellerfax, bookselleremail and currency. This means that we are will lose this information when you drop them from DB.
Comment 18 Jonathan Druart 2020-03-24 15:29:20 UTC
Created attachment 101612 [details] [review]
Bug 18177: Do not remove columns if at least one value exists
Comment 19 Jonathan Druart 2020-03-24 15:30:13 UTC Comment hidden (obsolete)
Comment 20 Jonathan Druart 2020-03-24 15:30:45 UTC
(In reply to Frank Hansen from comment #17)
> When we migrated from your previous ILS (Virtua), I kept some vendor contact
> information in booksellerfax, bookselleremail and currency. This means that
> we are will lose this information when you drop them from DB.

Thanks Frank for this feedback!

What about this last patch? We are going to remove the column only if there is no data, otherwise a warning will be displayed.
Comment 21 Tomás Cohen Arazi 2020-03-24 17:02:35 UTC
(In reply to Jonathan Druart from comment #20)
> (In reply to Frank Hansen from comment #17)
> > When we migrated from your previous ILS (Virtua), I kept some vendor contact
> > information in booksellerfax, bookselleremail and currency. This means that
> > we are will lose this information when you drop them from DB.
> 
> Thanks Frank for this feedback!
> 
> What about this last patch? We are going to remove the column only if there
> is no data, otherwise a warning will be displayed.

Maybe we can give some advise for migrating that data?
Comment 22 Frank Hansen 2020-03-25 08:33:43 UTC
(In reply to Jonathan Druart from comment #20)
> (In reply to Frank Hansen from comment #17)
> > When we migrated from your previous ILS (Virtua), I kept some vendor contact
> > information in booksellerfax, bookselleremail and currency. This means that
> > we are will lose this information when you drop them from DB.
> 
> Thanks Frank for this feedback!
> 
> What about this last patch? We are going to remove the column only if there
> is no data, otherwise a warning will be displayed.

Yes, this will at least keep the data in those columns so they're not getting lost. But is the plan to remove those fields completely from UI as well? If that is the case, we have no use for them. We can probably live without those columns anyway.
Comment 23 Katrin Fischer 2020-03-25 08:54:56 UTC
Unused should not be in the GUI - if they are, we might need to keep some!
Comment 24 Jonathan Druart 2020-03-25 09:22:53 UTC
They are not displayed (view or edit) anywhere.
Comment 25 Frank Hansen 2020-03-25 10:00:19 UTC
(In reply to Jonathan Druart from comment #24)
> They are not displayed (view or edit) anywhere.

When I dig a little deeper into how we migrated, it turned out that we copied the same value to multiple columns. For example, currency is the same as invoiceprice; booksellerfax is the same as fax; and bookselleremail was also inserted in aqcontacts.email. So this means that we will not lose any data at all. The columns othersupplier and booksellerurl is not used here at Lund University.
Comment 26 Martin Renvoize 2020-03-25 11:40:38 UTC
Created attachment 101687 [details] [review]
Bug 18177: remove aqbooksellers.othersupplier

This patchset removes some columns from the aqbooksellers table that are
not used:
 * booksellerfax
 * booksellerurl
 * bookselleremail
 * othersupplier
 * currency

The first 3 ones are certainly leftover from bug 10402.

Test plan:
For each for these fields you will make sure that they cannot be edited
from the interface.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 27 Martin Renvoize 2020-03-25 11:40:42 UTC
Created attachment 101688 [details] [review]
Bug 18177: remove aqbooksellers.booksellerfax

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 28 Martin Renvoize 2020-03-25 11:40:45 UTC
Created attachment 101689 [details] [review]
Bug 18177: remove aqbooksellers.booksellerurl

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 29 Martin Renvoize 2020-03-25 11:40:48 UTC
Created attachment 101690 [details] [review]
Bug 18177: remove aqbooksellers.bookselleremail

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 30 Martin Renvoize 2020-03-25 11:40:51 UTC
Created attachment 101691 [details] [review]
Bug 18177: remove aqbooksellers.currency

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 31 Martin Renvoize 2020-03-25 11:40:54 UTC
Created attachment 101692 [details] [review]
Bug 18177: DB changes

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 32 Martin Renvoize 2020-03-25 11:40:57 UTC
Created attachment 101693 [details] [review]
Bug 18177: DBIC changes

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 33 Martin Renvoize 2020-03-25 11:41:00 UTC
Created attachment 101694 [details] [review]
Bug 18177: Do not remove columns if at least one value exists

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 34 Martin Renvoize 2020-03-25 11:43:48 UTC
Looks like I overlapped with incoming comments.. really pleased to see this cleanup being done.

I've signed off as the patches all do as intended and I'm happy to agree that these fields have not been used in the codebase for some time.  The final patch is nice to have in cases where people have migrated data as a 'just in case'.. 

Signing off.
Comment 35 Marcel de Rooy 2020-03-27 08:12:15 UTC
QAing
Comment 36 Marcel de Rooy 2020-03-27 08:25:52 UTC
Created attachment 101958 [details] [review]
Bug 18177: remove aqbooksellers.othersupplier

This patchset removes some columns from the aqbooksellers table that are
not used:
 * booksellerfax
 * booksellerurl
 * bookselleremail
 * othersupplier
 * currency

The first 3 ones are certainly leftover from bug 10402.

Test plan:
For each for these fields you will make sure that they cannot be edited
from the interface.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 37 Marcel de Rooy 2020-03-27 08:25:56 UTC
Created attachment 101959 [details] [review]
Bug 18177: remove aqbooksellers.booksellerfax

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 38 Marcel de Rooy 2020-03-27 08:26:01 UTC
Created attachment 101960 [details] [review]
Bug 18177: remove aqbooksellers.booksellerurl

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 39 Marcel de Rooy 2020-03-27 08:26:05 UTC
Created attachment 101961 [details] [review]
Bug 18177: remove aqbooksellers.bookselleremail

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 40 Marcel de Rooy 2020-03-27 08:26:10 UTC
Created attachment 101962 [details] [review]
Bug 18177: remove aqbooksellers.currency

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 41 Marcel de Rooy 2020-03-27 08:26:15 UTC
Created attachment 101963 [details] [review]
Bug 18177: DB changes

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 42 Marcel de Rooy 2020-03-27 08:26:19 UTC
Created attachment 101964 [details] [review]
Bug 18177: DBIC changes

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 43 Marcel de Rooy 2020-03-27 08:26:24 UTC
Created attachment 101965 [details] [review]
Bug 18177: Do not remove columns if at least one value exists

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 44 Marcel de Rooy 2020-03-27 08:27:12 UTC
+                warn "Warning - Cannot remove column aqbooksellers.$column. At least one value exists";


Does this warn find its way to the interface. Probably not. Print it ?
I will pass QA. Feel free to add follow-up.

@RM: Watch for DBIx changes
Comment 45 Jonathan Druart 2020-03-27 09:28:12 UTC
(In reply to Marcel de Rooy from comment #44)
> +                warn "Warning - Cannot remove column aqbooksellers.$column.
> At least one value exists";
> 
> 
> Does this warn find its way to the interface. Probably not. Print it ?
> I will pass QA. Feel free to add follow-up.
> 
> @RM: Watch for DBIx changes

warnings should be seen from the interface, maybe at the bottom in the red part?
Comment 46 Martin Renvoize 2020-03-27 09:44:08 UTC
They will indeed be displayed at the bottom of the page in red when I tested :)
Comment 47 Martin Renvoize 2020-03-27 12:23:46 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 48 Jonathan Druart 2020-03-27 15:46:58 UTC
Created attachment 102011 [details] [review]
Bug 18177: (follow-up) remove aqbooksellers.currency

There was an occurrence in tests
Comment 49 Joy Nelson 2020-04-03 22:54:10 UTC
enhancement not backported to 19.11.x