Bug 14187 - branchtransfer needs a primary key (id) for DBIx and common sense.
Summary: branchtransfer needs a primary key (id) for DBIx and common sense.
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Alex Sassmannshausen
QA Contact: Testopia
URL:
Keywords:
Depends on: 17234
Blocks: 9525 11897 20595
  Show dependency treegraph
 
Reported: 2015-05-12 07:16 UTC by Olli-Antti Kivilahti
Modified: 2018-04-17 14:25 UTC (History)
7 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 14187 - branchtransfer needs a primary key (id) for DBIx and common sense. (2.28 KB, patch)
2015-05-12 07:21 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
Bug 14187 - branchtransfer needs a primary key (id) for DBIx and common sense. (3.11 KB, patch)
2015-05-12 08:14 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
Bug 14187 - branchtransfer needs a primary key (id) for DBIx and common sense. (3.14 KB, patch)
2015-05-12 08:21 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
Bug 14187 - branchtransfer needs a primary key (id) for DBIx and common sense. (5.84 KB, patch)
2015-06-10 13:59 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
Bug 14187 - branchtransfer needs a primary key (id) for DBIx and common sense. (7.69 KB, patch)
2016-12-12 14:21 UTC, Alex Sassmannshausen
Details | Diff | Splinter Review
Bug 14187 - branchtransfer needs a primary key (id) for DBIx and common sense. (7.69 KB, patch)
2016-12-12 14:23 UTC, Alex Sassmannshausen
Details | Diff | Splinter Review
DO NOT PUSH: Updated Branchtransfer schema. (2.69 KB, patch)
2016-12-12 14:23 UTC, Alex Sassmannshausen
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 14187 - branchtransfer needs a primary key (id) for DBIx and common sense. (7.78 KB, patch)
2017-01-06 14:03 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 14187 - branchtransfer needs a primary key (id) for DBIx and common sense. (5.24 KB, patch)
2017-01-10 11:09 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14187: (QA followup) Properly check DB structure before altering it (1.68 KB, patch)
2017-01-12 15:43 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 14187: branchtransfer needs a primary key (id) for DBIx and common sense. (5.32 KB, patch)
2017-01-12 15:44 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 14187: (QA followup) Properly check DB structure before altering it (1.68 KB, patch)
2017-01-12 15:44 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Olli-Antti Kivilahti 2015-05-12 07:16:38 UTC

    
Comment 1 Olli-Antti Kivilahti 2015-05-12 07:21:05 UTC Comment hidden (obsolete)
Comment 2 Olli-Antti Kivilahti 2015-05-12 08:14:45 UTC Comment hidden (obsolete)
Comment 3 Olli-Antti Kivilahti 2015-05-12 08:21:04 UTC Comment hidden (obsolete)
Comment 4 Bernardo Gonzalez Kriegel 2015-05-17 13:50:59 UTC
Perhaps the key is needed, also a test plan.
And rewrite database update as atomicupdate.
Comment 5 Olli-Antti Kivilahti 2015-05-27 15:26:47 UTC
What do you mean with atomicupdate?

All existing unit tests pass and this doesn't include any new testable behaviour.
Comment 6 Olli-Antti Kivilahti 2015-05-27 15:27:15 UTC
The primary key is needed if you want to do anything with DBIx.
Comment 7 Katrin Fischer 2015-05-31 12:29:16 UTC
Olli, there is a new way to write database updates - atomicupdate - see here: 
http://wiki.koha-community.org/wiki/Database_updates

I think what Bernardo meant was not unit tests but a better description of your change in the commit message and hints on how to test this.
Comment 8 Olli-Antti Kivilahti 2015-06-10 13:59:08 UTC
Created attachment 40075 [details] [review]
Bug 14187 - branchtransfer needs a primary key (id) for DBIx and common sense.
Comment 9 Alex Sassmannshausen 2016-12-06 13:11:27 UTC
My intention is to add atomicupdate, and ensure this still applies nicely on master in order to submit it.

Alex
Comment 10 Alex Sassmannshausen 2016-12-12 14:21:54 UTC
Created attachment 58121 [details] [review]
Bug 14187 - branchtransfer needs a primary key (id) for DBIx and common sense.

* C4/Circulation.pm (GetTransfers, GetTransfersFromTo): Also return
  branchtransfer_id in return columns.
* installer/data/mysql/atomicupdate/14187.perl: New file.
* installer/data/mysql/kohastructure.sql: Modify branchtransfers structure.
* t/db_dependent/Circulation/transfers.t: Update tests to expect
  branchtransfer_id.
Comment 11 Alex Sassmannshausen 2016-12-12 14:23:07 UTC
Created attachment 58123 [details] [review]
Bug 14187 - branchtransfer needs a primary key (id) for DBIx and common sense.

* C4/Circulation.pm (GetTransfers, GetTransfersFromTo): Also return
  branchtransfer_id in return columns.
* installer/data/mysql/atomicupdate/14187.perl: New file.
* installer/data/mysql/kohastructure.sql: Modify branchtransfers structure.
* t/db_dependent/Circulation/transfers.t: Update tests to expect
  branchtransfer_id.
Comment 12 Alex Sassmannshausen 2016-12-12 14:23:12 UTC
Created attachment 58124 [details] [review]
DO NOT PUSH: Updated Branchtransfer schema.

* Koha/Schema/Result/Branchtransfer.pm: Newly generated.
Comment 13 Alex Sassmannshausen 2016-12-12 14:25:17 UTC
Hello,

I've now been able to:

- [X] Re-checked code paths to see if all required changes were made
- [X] Updated test files
- [X] Added atomic update script
- [X] Tested database update & new database creation
- [X] Separated out schema changes
- [X] Ran QA scripts
- [X] Upload new patches.

This should now be ready for sign-off/QA.

Alex
Comment 14 Alex Sassmannshausen 2017-01-05 10:31:25 UTC
Test Plan

- Ensure the patch applies cleanly
- Run unit tests, and ensure no regression (especially t/db_dependent/Circulation/transfers.t)
- Set in motion a branch transfer (e.g. check in an item at a branch that is not it's home branch when floating stock is switched off)
- Receive a branch transfer (check in an item previously in branch transfer)

Alex
Comment 15 Owen Leonard 2017-01-06 14:03:34 UTC
Created attachment 58646 [details] [review]
[SIGNED-OFF] Bug 14187 - branchtransfer needs a primary key (id) for DBIx and common sense.

* C4/Circulation.pm (GetTransfers, GetTransfersFromTo): Also return
  branchtransfer_id in return columns.
* installer/data/mysql/atomicupdate/14187.perl: New file.
* installer/data/mysql/kohastructure.sql: Modify branchtransfers structure.
* t/db_dependent/Circulation/transfers.t: Update tests to expect
  branchtransfer_id.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Test plan successful on all steps.
Comment 16 Jonathan Druart 2017-01-10 11:09:31 UTC
Created attachment 58730 [details] [review]
Bug 14187 - branchtransfer needs a primary key (id) for DBIx and common sense.

* C4/Circulation.pm (GetTransfers, GetTransfersFromTo): Also return
  branchtransfer_id in return columns.
* installer/data/mysql/atomicupdate/14187.perl: New file.
* installer/data/mysql/kohastructure.sql: Modify branchtransfers structure.
* t/db_dependent/Circulation/transfers.t: Update tests to expect
  branchtransfer_id.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Test plan successful on all steps.

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Amended patch: Remove Schema changes from this patch
Comment 17 Tomás Cohen Arazi 2017-01-12 15:43:28 UTC
Created attachment 58880 [details] [review]
Bug 14187: (QA followup) Properly check DB structure before altering it

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 18 Tomás Cohen Arazi 2017-01-12 15:44:29 UTC
Created attachment 58881 [details] [review]
Bug 14187: branchtransfer needs a primary key (id) for DBIx and common sense.

* C4/Circulation.pm (GetTransfers, GetTransfersFromTo): Also return
  branchtransfer_id in return columns.
* installer/data/mysql/atomicupdate/14187.perl: New file.
* installer/data/mysql/kohastructure.sql: Modify branchtransfers structure.
* t/db_dependent/Circulation/transfers.t: Update tests to expect
  branchtransfer_id.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Test plan successful on all steps.

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Amended patch: Remove Schema changes from this patch
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 19 Tomás Cohen Arazi 2017-01-12 15:44:37 UTC
Created attachment 58882 [details] [review]
Bug 14187: (QA followup) Properly check DB structure before altering it

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 20 Kyle M Hall 2017-01-13 12:24:19 UTC
Pushed to master for 17.11, thanks Olli, Tomas!
Comment 21 Katrin Fischer 2017-01-15 18:12:52 UTC
This won't get ported back to 16.11.x as it is an enhancement.