Bug 17813 - Table borrower_attributes needs a primary key
Summary: Table borrower_attributes needs a primary key
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Target Milestone: ---
Assignee: Tomás Cohen Arazi
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on: 17234
Blocks: 17792
  Show dependency treegraph
 
Reported: 2016-12-23 18:46 UTC by Tomás Cohen Arazi
Modified: 2017-12-07 22:17 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 17813: Add a primary key to borrower_attributes (2.46 KB, patch)
2016-12-23 18:58 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 17813: DBIC update (1.71 KB, patch)
2016-12-23 18:58 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 17813: Add a primary key to borrower_attributes (2.50 KB, patch)
2016-12-29 14:44 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 17813: DBIC update (1.75 KB, patch)
2016-12-29 14:45 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 17813: (QA followup) Properly check DB structure before altering it (2.46 KB, patch)
2017-01-12 15:11 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 17813: Add a primary key to borrower_attributes (2.57 KB, patch)
2017-01-17 08:27 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17813: DBIC update (1.83 KB, patch)
2017-01-17 08:27 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17813: (QA followup) Properly check DB structure before altering it (2.52 KB, patch)
2017-01-17 08:27 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 2016-12-23 18:46:39 UTC
In order to be used correctly on DBIC the table needs a primary key. Otherwise calls to ->delete would fail.
Comment 1 Tomás Cohen Arazi 2016-12-23 18:58:46 UTC
Created attachment 58422 [details] [review]
Bug 17813: Add a primary key to borrower_attributes

This patch adds 'borrower_attributes' a field (if) which
will act as a primary key.

This is needed for DBIC to be used to handle rows, and also will help
when faced with the implementation of the REST api for this resource.

To test:
- Run all patron modification / attributes and verify nothing breaks
- Sign off :-D
Comment 2 Tomás Cohen Arazi 2016-12-23 18:58:51 UTC
Created attachment 58423 [details] [review]
Bug 17813: DBIC update
Comment 3 Kyle M Hall 2016-12-29 14:44:51 UTC
Created attachment 58493 [details] [review]
Bug 17813: Add a primary key to borrower_attributes

This patch adds 'borrower_attributes' a field (if) which
will act as a primary key.

This is needed for DBIC to be used to handle rows, and also will help
when faced with the implementation of the REST api for this resource.

To test:
- Run all patron modification / attributes and verify nothing breaks
- Sign off :-D

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 4 Kyle M Hall 2016-12-29 14:45:08 UTC
Created attachment 58494 [details] [review]
Bug 17813: DBIC update

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 5 Jonathan Druart 2017-01-11 08:38:05 UTC
I think we need to provide a version using column_exists, from bug 17234.
Comment 6 Tomás Cohen Arazi 2017-01-12 15:11:41 UTC
Created attachment 58879 [details] [review]
Bug 17813: (QA followup) Properly check DB structure before altering it

This patch makes the atomic update use the new functions introduced by
bug 17234 for checking the structure before attempting to call ALTER
TABLE.

It checks for the column existence, and also if it is a primary key.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 7 Jonathan Druart 2017-01-17 08:27:34 UTC
Created attachment 59070 [details] [review]
Bug 17813: Add a primary key to borrower_attributes

This patch adds 'borrower_attributes' a field (if) which
will act as a primary key.

This is needed for DBIC to be used to handle rows, and also will help
when faced with the implementation of the REST api for this resource.

To test:
- Run all patron modification / attributes and verify nothing breaks
- Sign off :-D

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 8 Jonathan Druart 2017-01-17 08:27:39 UTC
Created attachment 59071 [details] [review]
Bug 17813: DBIC update

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 9 Jonathan Druart 2017-01-17 08:27:43 UTC
Created attachment 59072 [details] [review]
Bug 17813: (QA followup) Properly check DB structure before altering it

This patch makes the atomic update use the new functions introduced by
bug 17234 for checking the structure before attempting to call ALTER
TABLE.

It checks for the column existence, and also if it is a primary key.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 10 Kyle M Hall 2017-01-20 14:02:48 UTC
Pushed to master for 17.05, thanks Tomas!
Comment 11 Katrin Fischer 2017-01-22 20:12:49 UTC
I have changed this to 'enh' as I think while it's needed to move forward it's not fixing a bug at the moment (false testable behaviour). 

Not including this in 16.11.2.
Comment 12 Katrin Fischer 2017-02-08 21:26:22 UTC
Just wondering - why don't we replace the old for the new variable in the notices with SQL? I think we have done that once when we added the new syntax for <<items.fine>> already.
Comment 13 Jonathan Druart 2017-02-10 07:40:52 UTC
(In reply to Katrin Fischer from comment #12)
> Just wondering - why don't we replace the old for the new variable in the
> notices with SQL? I think we have done that once when we added the new
> syntax for <<items.fine>> already.

There is no old column, only a new one.