Bug 16007 - Correction of 'Remove columns branchcode, categorytype from table overduerules_transport_types'
Summary: Correction of 'Remove columns branchcode, categorytype from table overduerule...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low critical (vote)
Assignee: Charles Farmer
QA Contact: Testopia
URL:
Keywords:
Depends on: 13624
Blocks: 12772
  Show dependency treegraph
 
Reported: 2016-03-08 20:27 UTC by Charles Farmer
Modified: 2019-06-27 09:24 UTC (History)
9 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 16007: Adding back column 'letternumber' to table 'overduerules_transport_types' (2.61 KB, patch)
2016-03-21 16:40 UTC, Charles Farmer
Details | Diff | Splinter Review
Bug 16007: Adding back column 'letternumber' to table 'overduerules_transport_types' (3.00 KB, patch)
2016-03-21 18:18 UTC, Charles Farmer
Details | Diff | Splinter Review
Bug 16007: Adding back column 'letternumber' to table 'overduerules_transport_types' (2.84 KB, patch)
2016-03-21 19:03 UTC, Charles Farmer
Details | Diff | Splinter Review
Bug 16007: Adding back column 'letternumber' to table 'overduerules_transport_types' (2.89 KB, patch)
2016-03-21 19:28 UTC, M. Tompsett
Details | Diff | Splinter Review
Schema update (1.61 KB, patch)
2016-03-21 19:28 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 16007: Simplify the DB entry (2.32 KB, patch)
2016-03-23 17:00 UTC, Jonathan Druart
Details | Diff | Splinter Review
[PASSED QA] Bug 16007: Adding back column 'letternumber' to table 'overduerules_transport_types' (2.96 KB, patch)
2016-03-24 06:45 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 16007: Simplify the DB entry (2.39 KB, patch)
2016-03-24 06:46 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Charles Farmer 2016-03-08 20:27:11 UTC
Bug 13624 definitely broke the master by removing the column `overduerules_transport_type`.`letternumber` from kohastructure.sql. This seems to be a mistake, since we only dropped columns `branchcode` and `categorycode` in the atomicupdate file.

This new bug is to keep track of everything made to address this issue.

I don't know if we'll start with a patch correcting the master, or if we'll rebase to old patch and move from there.
Comment 1 M. Tompsett 2016-03-09 15:43:29 UTC
(In reply to Charles Farmer from comment #0)
> I don't know if we'll start with a patch correcting the master, or if we'll
> rebase to old patch and move from there.

Start from master. It's easier for people to test from that point. :)
Comment 2 Katrin Fischer 2016-03-21 07:18:01 UTC
Any progress on this?
Comment 3 Charles Farmer 2016-03-21 12:33:00 UTC
(In reply to Katrin Fischer from comment #2)
> Any progress on this?

Not yet. I'll try to make a patch in-between two tasks soon enough
Comment 4 Charles Farmer 2016-03-21 16:40:09 UTC Comment hidden (obsolete)
Comment 5 M. Tompsett 2016-03-21 16:57:13 UTC
Comment on attachment 49398 [details] [review]
Bug 16007: Adding back column 'letternumber' to table 'overduerules_transport_types'

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

Take a look at C4/Members.pm sub columns. I think that is a better way to determine column names. :)
Comment 6 Charles Farmer 2016-03-21 18:18:32 UTC Comment hidden (obsolete)
Comment 7 Charles Farmer 2016-03-21 18:21:08 UTC
(In reply to M. Tompsett from comment #5)
> Take a look at C4/Members.pm sub columns. I think that is a better way to
> determine column names. :)

That's a much cleaner, less db-dependent way of doing it.

I obsoleted my last patch and made a new one. Reviews are welcomed, as always.

If I ever fix the patch in 13624, should I post it back there again, somewhere here on this bug, or should we open a new tracker?
Comment 8 M. Tompsett 2016-03-21 18:36:27 UTC
Comment on attachment 49401 [details] [review]
Bug 16007: Adding back column 'letternumber' to table 'overduerules_transport_types'

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

::: installer/data/mysql/atomicupdate/Bug_16007_adding_column_letternumber_back_into_table_overduerules_transport_types.perl
@@ +23,5 @@
> +        {
> +            $letternumberExists = 1;
> +            last;
> +        }
> +    }

While this would work. Why not use grep?
my @letternumber = grep { $_ eq 'letternumber' } @data;
if (@letternumber) { print "EXISTS!\n"; } else { print "DIDN'T FIND!\n"; }
http://perldoc.perl.org/functions/grep.html
Comment 9 Charles Farmer 2016-03-21 18:47:04 UTC
(In reply to M. Tompsett from comment #8)
> While this would work. Why not use grep?

Silly java coding weekend, why do you make me forget about grep :(

Do you want another patch?
Comment 10 M. Tompsett 2016-03-21 18:53:50 UTC
(In reply to Charles Farmer from comment #9)
> Do you want another patch?

If you don't mind, that would be nicer. :)
Comment 11 Charles Farmer 2016-03-21 19:03:53 UTC Comment hidden (obsolete)
Comment 12 M. Tompsett 2016-03-21 19:28:06 UTC Comment hidden (obsolete)
Comment 13 M. Tompsett 2016-03-21 19:28:56 UTC Comment hidden (obsolete)
Comment 14 Charles Farmer 2016-03-21 19:35:04 UTC
The previous patch, from 13624, has yet to be corrected.

How do I go about this? New tracker, or new patche in #13624 ?
Comment 15 M. Tompsett 2016-03-21 22:21:28 UTC
(In reply to Charles Farmer from comment #14)
> The previous patch, from 13624, has yet to be corrected.
> 
> How do I go about this? New tracker, or new patche in #13624 ?

13624 is already pushed to master, just wait for this to pass QA and then get pushed. I don't think there is a need to touch 13624 at all. I'm sure a QA person can comment more authoritatively. :)
Comment 16 Jonathan Druart 2016-03-23 17:00:23 UTC Comment hidden (obsolete)
Comment 17 Jonathan Druart 2016-03-23 17:01:05 UTC
The DB entry is very complicated. I have attached a patch to simplify it.

Letting another QAer to have a look.
Comment 18 Jonathan Druart 2016-03-23 17:01:29 UTC
Upgrading the severity, we need it asap!
Comment 19 Jonathan Druart 2016-03-23 17:01:49 UTC
Comment on attachment 49404 [details] [review]
Schema update

No needed, it's the RM job.
Comment 20 Katrin Fischer 2016-03-24 06:45:40 UTC
Created attachment 49514 [details] [review]
[PASSED QA] Bug 16007: Adding back column 'letternumber' to table 'overduerules_transport_types'

Bug 13624 definitely broke the master by removing the column `overduerules_transport_type`.`letternumber` from kohastructure.sql.
This patch aims to fix the problem by adding the column back on systems which had their 'letternumber' removed.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 21 Katrin Fischer 2016-03-24 06:46:38 UTC
Tested with and without the column in the database - database update didn't throw an error and table was corrected. Thx!
Comment 22 Katrin Fischer 2016-03-24 06:46:49 UTC
Created attachment 49515 [details] [review]
[PASSED QA] Bug 16007: Simplify the DB entry

- No need to check if the table exists
- No need to check if the column exists, just remove the warning

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 23 Brendan Gallagher 2016-03-31 15:57:35 UTC
Pushed to Master - Should be in the May 2016 release.  Thanks
Comment 24 Julian Maurice 2016-04-01 12:55:04 UTC
Not needed in stable versions, status changed to RESOLVED