From a71140fe607ce73211c9a1f1bbecdd845743544a Mon Sep 17 00:00:00 2001 From: Tomas Cohen Arazi Date: Tue, 10 Oct 2023 16:02:38 -0300 Subject: [PATCH] Bug 30719: (QA follow-up) Pick better column names and cleanup This patch takes on normalizing the attribute names, embeds, and also makes the whole API more kosher, in terms of using accessors for related objects, using the standard structure for strings_map, etc. Signed-off-by: Tomas Cohen Arazi --- Koha/Illbatch.pm | 87 ++++++------ Koha/REST/V1/Illbatches.pm | 19 ++- Koha/Schema/Result/Borrower.pm | 6 +- Koha/Schema/Result/Branch.pm | 6 +- Koha/Schema/Result/Illbatch.pm | 85 ++++++------ Koha/Schema/Result/IllbatchStatus.pm | 6 +- Koha/Schema/Result/Illrequest.pm | 6 +- api/v1/swagger/definitions/ill_batch.yaml | 37 +++-- api/v1/swagger/paths/ill_batches.yaml | 9 +- .../atomicupdate/bug_30719_add_ill_batches.pl | 55 ++------ installer/data/mysql/kohastructure.sql | 19 +-- t/db_dependent/IllbatchStatuses.t | 10 +- t/db_dependent/Illbatches.t | 25 ++-- t/db_dependent/api/v1/ill_batches.t | 129 +++++++++--------- 14 files changed, 250 insertions(+), 249 deletions(-) diff --git a/Koha/Illbatch.pm b/Koha/Illbatch.pm index f4163a8ed0..a44c344731 100644 --- a/Koha/Illbatch.pm +++ b/Koha/Illbatch.pm @@ -18,9 +18,15 @@ package Koha::Illbatch; # along with Koha; if not, see . use Modern::Perl; + use Koha::Database; + +use Koha::Illrequests; use Koha::Illrequest::Logger; -use Koha::IllbatchStatus; +use Koha::IllbatchStatuses; +use Koha::Libraries; +use Koha::Patrons; + use JSON qw( to_json ); use base qw(Koha::Object); @@ -40,35 +46,37 @@ Return the status object associated with this batch sub status { my ($self) = @_; - return Koha::IllbatchStatus->_new_from_dbic( scalar $self->_result->statuscode ); + return Koha::IllbatchStatus->_new_from_dbic( scalar $self->_result->status_code ); } =head3 patron my $patron = Koha::Illbatch->patron; -Return the patron object associated with this batch +Return the I object associated with this batch =cut sub patron { my ($self) = @_; - my $patron = return Koha::Patrons->find( { borrowernumber => $self->borrowernumber } ); + my $patron = $self->_result->patron; return unless $patron; + return Koha::Patron->_new_from_dbic($patron); } -=head3 branch +=head3 library - my $branch = Koha::Illbatch->branch; + my $library = Koha::Illbatch->library; -Return the branch object associated with this batch +Return the I object associated with this batch =cut -sub branch { +sub library { my ($self) = @_; - my $library = return Koha::Libraries->find( { branchcode => $self->branchcode } ); + my $library = $self->_result->library; return unless $library; + return Koha::Library->_new_from_dbic($library); } =head3 requests_count @@ -86,13 +94,13 @@ sub requests_count { =head3 requests -Return the requests for this batch +Return the I for this batch =cut sub requests { my ($self) = @_; - my $requests = $self->_result->illrequests; + my $requests = $self->_result->requests; return Koha::Illrequests->_new_from_dbic($requests); } @@ -134,7 +142,7 @@ sub update_and_log { my $before = { name => $self->name, - branchcode => $self->branchcode + library_id => $self->library_id, }; $self->set($params); @@ -142,7 +150,7 @@ sub update_and_log { my $after = { name => $self->name, - branchcode => $self->branchcode + library_id => $self->library_id, }; my $logger = Koha::Illrequest::Logger->new; @@ -192,6 +200,17 @@ sub delete_and_log { =head3 strings_map +Returns a map of column name to string representations including the string, +the mapping type and the mapping category where appropriate. + +Currently handles library and ILL batch status expansions. +expansions. + +Accepts a param hashref where the I key denotes whether we want the public +or staff client strings. + +Note: the I parameter is not currently used. + =cut sub strings_map { @@ -199,41 +218,31 @@ sub strings_map { my $strings = {}; - if ( defined $self->statuscode ) { - my $ill_batch_status = Koha::IllbatchStatuses->search( { code => $self->statuscode } ); - my $ill_batch_status_str = - $ill_batch_status->count - ? $ill_batch_status->next->name - : $self->statuscode; + if ( defined $self->status_code ) { + my $status = $self->status; - $strings->{status} = { - name => $ill_batch_status_str, - }; + if ($status) { + $strings->{status_code} = { + str => $status->name, + type => 'ill_batch_status', + }; + } } - if ( defined $self->branchcode ) { - my $ill_batch_branch = Koha::Libraries->find( $self->branchcode ); - my $ill_batch_branchname_str = $ill_batch_branch ? $ill_batch_branch->branchname : $self->branchcode; + if ( defined $self->library_id ) { + my $library = $self->library; - $strings->{branchname} = $ill_batch_branchname_str; + if ($library) { + $strings->{library_id} = { + str => $library->branchname, + type => 'library', + }; + } } return $strings; } -=head3 to_api_mapping - -=cut - -sub to_api_mapping { - return { - id => 'batch_id', - branchcode => 'library_id', - borrowernumber => 'patron_id', - status => 'batch_status', - }; -} - =head3 _type my $type = Koha::Illbatch->_type; diff --git a/Koha/REST/V1/Illbatches.pm b/Koha/REST/V1/Illbatches.pm index 1b25c7956c..b4e7d6aa02 100644 --- a/Koha/REST/V1/Illbatches.pm +++ b/Koha/REST/V1/Illbatches.pm @@ -41,12 +41,14 @@ sub list { my $c = shift->openapi->valid_input or return; return try { - my $illbatches = $c->objects->search( Koha::Illbatches->new ); - return $c->render( status => 200, openapi => $illbatches ); + return $c->render( + status => 200, + openapi => $c->objects->search( Koha::Illbatches->new ) + ); } catch { + warn "$_"; $c->unhandled_exception($_); }; - } =head3 get @@ -59,7 +61,7 @@ sub get { my $c = shift->openapi->valid_input or return; return try { - my $ill_batch = $c->objects->find( Koha::Illbatches->search, $c->param('ill_batch_id') ); + my $ill_batch = $c->objects->find( Koha::Illbatches->new, $c->param('ill_batch_id') ); unless ($ill_batch) { return $c->render( @@ -98,8 +100,7 @@ sub add { } delete $body->{cardnumber}; - $body->{borrowernumber} = $patron->borrowernumber; - $body->{branchcode} = delete $body->{library_id}; + $body->{patron_id} = $patron->id; return try { my $batch = Koha::Illbatch->new_from_api($body); @@ -107,7 +108,7 @@ sub add { $c->res->headers->location( $c->req->url->to_string . '/' . $batch->id ); - my $ill_batch = $c->objects->find( Koha::Illbatches->search, $batch->id ); + my $ill_batch = $c->objects->find( Koha::Illbatches->new, $batch->id ); return $c->render( status => 201, @@ -146,15 +147,13 @@ sub update { my $params = $c->req->json; delete $params->{cardnumber}; - $params->{borrowernumber} = delete $params->{patron_id} if $params->{patron_id}; - $params->{branchcode} = delete $params->{library_id} if $params->{library_id}; return try { $batch->update_and_log($params); return $c->render( status => 200, - openapi => $batch->to_api + openapi => $c->objects->to_api($batch), ); } catch { $c->unhandled_exception($_); diff --git a/Koha/Schema/Result/Borrower.pm b/Koha/Schema/Result/Borrower.pm index 075cef0c7f..ff0c1ca9b1 100644 --- a/Koha/Schema/Result/Borrower.pm +++ b/Koha/Schema/Result/Borrower.pm @@ -1413,7 +1413,7 @@ Related object: L __PACKAGE__->has_many( "illbatches", "Koha::Schema::Result::Illbatch", - { "foreign.borrowernumber" => "self.borrowernumber" }, + { "foreign.patron_id" => "self.borrowernumber" }, { cascade_copy => 0, cascade_delete => 0 }, ); @@ -2118,8 +2118,8 @@ Composing rels: L -> permission __PACKAGE__->many_to_many("permissions", "user_permissions", "permission"); -# Created by DBIx::Class::Schema::Loader v0.07049 @ 2023-04-28 11:24:21 -# DO NOT MODIFY THIS OR ANYTHING ABOVE! md5sum:RUWvcq9kgQvACo14H/u9jQ +# Created by DBIx::Class::Schema::Loader v0.07049 @ 2023-10-10 14:16:46 +# DO NOT MODIFY THIS OR ANYTHING ABOVE! md5sum:nIMnqyBVBam7NvIx4aDfHw __PACKAGE__->has_many( "restrictions", diff --git a/Koha/Schema/Result/Branch.pm b/Koha/Schema/Result/Branch.pm index 7d0cbaffaf..31039bda60 100644 --- a/Koha/Schema/Result/Branch.pm +++ b/Koha/Schema/Result/Branch.pm @@ -703,7 +703,7 @@ Related object: L __PACKAGE__->has_many( "illbatches", "Koha::Schema::Result::Illbatch", - { "foreign.branchcode" => "self.branchcode" }, + { "foreign.library_id" => "self.branchcode" }, { cascade_copy => 0, cascade_delete => 0 }, ); @@ -933,8 +933,8 @@ __PACKAGE__->has_many( ); -# Created by DBIx::Class::Schema::Loader v0.07049 @ 2023-04-28 11:24:21 -# DO NOT MODIFY THIS OR ANYTHING ABOVE! md5sum:GGYhqyTVMmaFo9hAY4Jy5w +# Created by DBIx::Class::Schema::Loader v0.07049 @ 2023-10-10 14:16:46 +# DO NOT MODIFY THIS OR ANYTHING ABOVE! md5sum:LjdauIP6C1I1/1liCIddJQ __PACKAGE__->add_columns( '+pickup_location' => { is_boolean => 1 }, diff --git a/Koha/Schema/Result/Illbatch.pm b/Koha/Schema/Result/Illbatch.pm index 22c402b254..d7e758cae7 100644 --- a/Koha/Schema/Result/Illbatch.pm +++ b/Koha/Schema/Result/Illbatch.pm @@ -23,7 +23,7 @@ __PACKAGE__->table("illbatches"); =head1 ACCESSORS -=head2 id +=head2 ill_batch_id data_type: 'integer' is_auto_increment: 1 @@ -47,7 +47,7 @@ Unique name of batch Name of batch backend -=head2 borrowernumber +=head2 patron_id data_type: 'integer' is_foreign_key: 1 @@ -55,7 +55,7 @@ Name of batch backend Patron associated with batch -=head2 branchcode +=head2 library_id data_type: 'varchar' is_foreign_key: 1 @@ -64,7 +64,7 @@ Patron associated with batch Branch associated with batch -=head2 statuscode +=head2 status_code data_type: 'varchar' is_foreign_key: 1 @@ -76,17 +76,17 @@ Status of batch =cut __PACKAGE__->add_columns( - "id", + "ill_batch_id", { data_type => "integer", is_auto_increment => 1, is_nullable => 0 }, "name", { data_type => "varchar", is_nullable => 0, size => 100 }, "backend", { data_type => "varchar", is_nullable => 0, size => 20 }, - "borrowernumber", + "patron_id", { data_type => "integer", is_foreign_key => 1, is_nullable => 1 }, - "branchcode", + "library_id", { data_type => "varchar", is_foreign_key => 1, is_nullable => 1, size => 50 }, - "statuscode", + "status_code", { data_type => "varchar", is_foreign_key => 1, is_nullable => 1, size => 20 }, ); @@ -94,13 +94,13 @@ __PACKAGE__->add_columns( =over 4 -=item * L +=item * L =back =cut -__PACKAGE__->set_primary_key("id"); +__PACKAGE__->set_primary_key("ill_batch_id"); =head1 UNIQUE CONSTRAINTS @@ -118,27 +118,22 @@ __PACKAGE__->add_unique_constraint("u_illbatches__name", ["name"]); =head1 RELATIONS -=head2 borrowernumber +=head2 illrequests -Type: belongs_to +Type: has_many -Related object: L +Related object: L =cut -__PACKAGE__->belongs_to( - "borrowernumber", - "Koha::Schema::Result::Borrower", - { borrowernumber => "borrowernumber" }, - { - is_deferrable => 1, - join_type => "LEFT", - on_delete => "SET NULL", - on_update => "CASCADE", - }, +__PACKAGE__->has_many( + "illrequests", + "Koha::Schema::Result::Illrequest", + { "foreign.batch_id" => "self.ill_batch_id" }, + { cascade_copy => 0, cascade_delete => 0 }, ); -=head2 branchcode +=head2 library Type: belongs_to @@ -147,9 +142,9 @@ Related object: L =cut __PACKAGE__->belongs_to( - "branchcode", + "library", "Koha::Schema::Result::Branch", - { branchcode => "branchcode" }, + { branchcode => "library_id" }, { is_deferrable => 1, join_type => "LEFT", @@ -158,22 +153,27 @@ __PACKAGE__->belongs_to( }, ); -=head2 illrequests +=head2 patron -Type: has_many +Type: belongs_to -Related object: L +Related object: L =cut -__PACKAGE__->has_many( - "illrequests", - "Koha::Schema::Result::Illrequest", - { "foreign.batch_id" => "self.id" }, - { cascade_copy => 0, cascade_delete => 0 }, +__PACKAGE__->belongs_to( + "patron", + "Koha::Schema::Result::Borrower", + { borrowernumber => "patron_id" }, + { + is_deferrable => 1, + join_type => "LEFT", + on_delete => "SET NULL", + on_update => "CASCADE", + }, ); -=head2 statuscode +=head2 status_code Type: belongs_to @@ -182,9 +182,9 @@ Related object: L =cut __PACKAGE__->belongs_to( - "statuscode", + "status_code", "Koha::Schema::Result::IllbatchStatus", - { code => "statuscode" }, + { code => "status_code" }, { is_deferrable => 1, join_type => "LEFT", @@ -194,8 +194,15 @@ __PACKAGE__->belongs_to( ); -# Created by DBIx::Class::Schema::Loader v0.07049 @ 2023-09-08 13:49:29 -# DO NOT MODIFY THIS OR ANYTHING ABOVE! md5sum:YKxQxJMKxdBP9X4+i0Rfzw +# Created by DBIx::Class::Schema::Loader v0.07049 @ 2023-10-10 18:12:30 +# DO NOT MODIFY THIS OR ANYTHING ABOVE! md5sum:81afdstzxyW2hhIx6mu4KA + +__PACKAGE__->has_many( + "requests", + "Koha::Schema::Result::Illrequest", + { "foreign.batch_id" => "self.ill_batch_id" }, + { cascade_copy => 0, cascade_delete => 0 }, +); sub koha_object_class { 'Koha::Illbatch'; diff --git a/Koha/Schema/Result/IllbatchStatus.pm b/Koha/Schema/Result/IllbatchStatus.pm index 64b6fd867e..f0b9e983ca 100644 --- a/Koha/Schema/Result/IllbatchStatus.pm +++ b/Koha/Schema/Result/IllbatchStatus.pm @@ -106,13 +106,13 @@ Related object: L __PACKAGE__->has_many( "illbatches", "Koha::Schema::Result::Illbatch", - { "foreign.statuscode" => "self.code" }, + { "foreign.status_code" => "self.code" }, { cascade_copy => 0, cascade_delete => 0 }, ); -# Created by DBIx::Class::Schema::Loader v0.07049 @ 2023-09-08 13:49:29 -# DO NOT MODIFY THIS OR ANYTHING ABOVE! md5sum:yo60FJ+kyRj8QuEMac8CFA +# Created by DBIx::Class::Schema::Loader v0.07049 @ 2023-10-10 18:12:30 +# DO NOT MODIFY THIS OR ANYTHING ABOVE! md5sum:sRgblQWtTH/cdtdMT3KP+w __PACKAGE__->add_columns( '+is_system' => { is_boolean => 1 }, diff --git a/Koha/Schema/Result/Illrequest.pm b/Koha/Schema/Result/Illrequest.pm index d7c646b975..e1c29012bc 100644 --- a/Koha/Schema/Result/Illrequest.pm +++ b/Koha/Schema/Result/Illrequest.pm @@ -276,7 +276,7 @@ Related object: L __PACKAGE__->belongs_to( "batch", "Koha::Schema::Result::Illbatch", - { id => "batch_id" }, + { ill_batch_id => "batch_id" }, { is_deferrable => 1, join_type => "LEFT", @@ -391,8 +391,8 @@ __PACKAGE__->belongs_to( ); -# Created by DBIx::Class::Schema::Loader v0.07049 @ 2023-09-08 13:51:09 -# DO NOT MODIFY THIS OR ANYTHING ABOVE! md5sum:QS3E1jO/d797B0ADcjT0yQ +# Created by DBIx::Class::Schema::Loader v0.07049 @ 2023-10-10 14:49:40 +# DO NOT MODIFY THIS OR ANYTHING ABOVE! md5sum:DQl+7uwLCz5GeqU2hBFmMA __PACKAGE__->has_many( "comments", diff --git a/api/v1/swagger/definitions/ill_batch.yaml b/api/v1/swagger/definitions/ill_batch.yaml index eadbe244c5..51249f8634 100644 --- a/api/v1/swagger/definitions/ill_batch.yaml +++ b/api/v1/swagger/definitions/ill_batch.yaml @@ -1,8 +1,8 @@ --- type: object properties: - batch_id: - type: string + ill_batch_id: + type: integer description: Internal ILL batch identifier name: type: string @@ -12,34 +12,41 @@ properties: description: Backend name cardnumber: type: string - description: Card number of the patron of the ILL batch + description: Library assigned user identifier of the ILL batch patron_id: type: string - description: Borrower number of the patron of the ILL batch + description: Internal identifier the patron of the ILL batch library_id: type: string - description: Branch code of the branch of the ILL batch + description: Internal identifier for the ILL batch's library + status_code: + type: string + description: Code of the status of the ILL batch patron: type: - object - "null" description: The patron associated with the batch - branch: + library: type: - object - "null" - description: The branch associated with the batch - statuscode: - type: string - description: Code of the status of the ILL batch + description: The library associated with the batch + requests: + type: + - array + - "null" + description: The requests in this batch (x-koha-embed) + requests_count: + type: + - integer + - "null" + description: The number of requests in this batch (x-koha-embed) status: type: - object - "null" - description: The status associated with the batch - requests_count: - type: string - description: The number of requests in this batch + description: The status associated with the batch (x-koha-embed) _strings: type: - object @@ -50,4 +57,4 @@ required: - name - backend - library_id - - statuscode \ No newline at end of file + - status_code diff --git a/api/v1/swagger/paths/ill_batches.yaml b/api/v1/swagger/paths/ill_batches.yaml index dd3173cdd6..3d747be2a5 100644 --- a/api/v1/swagger/paths/ill_batches.yaml +++ b/api/v1/swagger/paths/ill_batches.yaml @@ -23,9 +23,10 @@ type: string enum: - +strings + - requests - requests+count - patron - - branch + - library collectionFormat: csv produces: - application/json @@ -82,9 +83,10 @@ type: string enum: - +strings + - requests - requests+count - patron - - branch + - library collectionFormat: csv produces: - application/json @@ -156,9 +158,10 @@ type: string enum: - +strings + - requests - requests+count - patron - - branch + - library collectionFormat: csv produces: - application/json diff --git a/installer/data/mysql/atomicupdate/bug_30719_add_ill_batches.pl b/installer/data/mysql/atomicupdate/bug_30719_add_ill_batches.pl index 76329b35dd..e49efc369b 100755 --- a/installer/data/mysql/atomicupdate/bug_30719_add_ill_batches.pl +++ b/installer/data/mysql/atomicupdate/bug_30719_add_ill_batches.pl @@ -10,14 +10,14 @@ return { unless ( TableExists('illbatch_statuses') ) { $dbh->do( q{ - CREATE TABLE`illbatch_statuses` ( + CREATE TABLE `illbatch_statuses` ( `id` int(11) NOT NULL auto_increment COMMENT "Status ID", `name` varchar(100) NOT NULL COMMENT "Name of status", `code` varchar(20) NOT NULL COMMENT "Unique, immutable code for status", `is_system` tinyint(1) COMMENT "Is this status required for system operation", - PRIMARY KEY (`id`), + PRIMARY KEY(`id`), UNIQUE KEY `u_illbatchstatuses__code` (`code`) - ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_unicode_ci + ) ENGINE = InnoDB DEFAULT CHARSET = utf8mb4 COLLATE = utf8mb4_unicode_ci; } ); @@ -28,18 +28,18 @@ return { $dbh->do( q{ CREATE TABLE `illbatches` ( - `id` int(11) NOT NULL auto_increment COMMENT "Batch ID", + `ill_batch_id` int(11) NOT NULL auto_increment COMMENT "Batch ID", `name` varchar(100) NOT NULL COMMENT "Unique name of batch", `backend` varchar(20) NOT NULL COMMENT "Name of batch backend", - `borrowernumber` int(11) COMMENT "Patron associated with batch", - `branchcode` varchar(50) COMMENT "Branch associated with batch", - `statuscode` varchar(20) COMMENT "Status of batch", - PRIMARY KEY (`id`), + `patron_id` int(11) NULL DEFAULT NULL COMMENT "Patron associated with batch", + `library_id` varchar(50) NULL DEFAULT NULL COMMENT "Branch associated with batch", + `status_code` varchar(20) NULL DEFAULT NULL COMMENT "Status of batch", + PRIMARY KEY (`ill_batch_id`), UNIQUE KEY `u_illbatches__name` (`name`), - CONSTRAINT `illbatches_bnfk` FOREIGN KEY (`borrowernumber`) REFERENCES `borrowers` (`borrowernumber`) ON DELETE SET NULL ON UPDATE CASCADE, - CONSTRAINT `illbatches_bcfk` FOREIGN KEY (`branchcode`) REFERENCES `branches` (`branchcode`) ON DELETE SET NULL ON UPDATE CASCADE, - CONSTRAINT `illbatches_sfk` FOREIGN KEY (`statuscode`) REFERENCES `illbatch_statuses` (`code`) ON DELETE SET NULL ON UPDATE CASCADE - ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_unicode_ci + CONSTRAINT `illbatches_bnfk` FOREIGN KEY (`patron_id`) REFERENCES `borrowers` (`borrowernumber`) ON DELETE SET NULL ON UPDATE CASCADE, + CONSTRAINT `illbatches_bcfk` FOREIGN KEY (`library_id`) REFERENCES `branches` (`branchcode`) ON DELETE SET NULL ON UPDATE CASCADE, + CONSTRAINT `illbatches_sfk` FOREIGN KEY (`status_code`) REFERENCES `illbatch_statuses` (`code`) ON DELETE SET NULL ON UPDATE CASCADE + ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_unicode_ci; } ); @@ -49,7 +49,7 @@ return { $dbh->do( q{ ALTER TABLE `illrequests` - ADD COLUMN `batch_id` int(11) AFTER backend -- Optional ID of batch that this request belongs to + ADD COLUMN `batch_id` int(11) COMMENT 'Optional ID of batch that this request belongs to' AFTER backend } ); @@ -60,34 +60,7 @@ return { $dbh->do( q{ ALTER TABLE `illrequests` - ADD CONSTRAINT `illrequests_ibfk` FOREIGN KEY (`batch_id`) REFERENCES `illbatches` (`id`) ON DELETE SET NULL ON UPDATE CASCADE - } - ); - } - - unless ( foreign_key_exists( 'illbatches', 'illbatches_bnfk' ) ) { - $dbh->do( - q{ - ALTER TABLE `illbatches` - ADD CONSTRAINT `illbatches_bnfk` FOREIGN KEY (`borrowernumber`) REFERENCES `borrowers` (`borrowernumber`) ON DELETE SET NULL ON UPDATE CASCADE - } - ); - } - - unless ( foreign_key_exists( 'illbatches', 'illbatches_bcfk' ) ) { - $dbh->do( - q{ - ALTER TABLE `illbatches` - ADD CONSTRAINT `illbatches_bcfk` FOREIGN KEY (`branchcode`) REFERENCES `branches` (`branchcode`) ON DELETE SET NULL ON UPDATE CASCADE - } - ); - } - - unless ( foreign_key_exists( 'illbatches', 'illbatches_sfk' ) ) { - $dbh->do( - q{ - ALTER TABLE `illbatches` - ADD CONSTRAINT `illbatches_sfk` FOREIGN KEY (`statuscode`) REFERENCES `illbatch_statuses` (`code`) ON DELETE SET NULL ON UPDATE CASCADE + ADD CONSTRAINT `illrequests_ibfk` FOREIGN KEY (`batch_id`) REFERENCES `illbatches` (`ill_batch_id`) ON DELETE SET NULL ON UPDATE CASCADE } ); } diff --git a/installer/data/mysql/kohastructure.sql b/installer/data/mysql/kohastructure.sql index a3709bdd21..822b222cfc 100644 --- a/installer/data/mysql/kohastructure.sql +++ b/installer/data/mysql/kohastructure.sql @@ -3319,17 +3319,17 @@ CREATE TABLE `illbatch_statuses` ( -- DROP TABLE IF EXISTS `illbatches`; CREATE TABLE `illbatches` ( - `id` int(11) NOT NULL auto_increment COMMENT "Batch ID", + `ill_batch_id` int(11) NOT NULL auto_increment COMMENT "Batch ID", `name` varchar(100) NOT NULL COMMENT "Unique name of batch", `backend` varchar(20) NOT NULL COMMENT "Name of batch backend", - `borrowernumber` int(11) COMMENT "Patron associated with batch", - `branchcode` varchar(50) COMMENT "Branch associated with batch", - `statuscode` varchar(20) COMMENT "Status of batch", - PRIMARY KEY (`id`), + `patron_id` int(11) NULL DEFAULT NULL COMMENT "Patron associated with batch", + `library_id` varchar(50) NULL DEFAULT NULL COMMENT "Branch associated with batch", + `status_code` varchar(20) NULL DEFAULT NULL COMMENT "Status of batch", + PRIMARY KEY (`ill_batch_id`), UNIQUE KEY `u_illbatches__name` (`name`), - CONSTRAINT `illbatches_bnfk` FOREIGN KEY (`borrowernumber`) REFERENCES `borrowers` (`borrowernumber`) ON DELETE SET NULL ON UPDATE CASCADE, - CONSTRAINT `illbatches_bcfk` FOREIGN KEY (`branchcode`) REFERENCES `branches` (`branchcode`) ON DELETE SET NULL ON UPDATE CASCADE, - CONSTRAINT `illbatches_sfk` FOREIGN KEY (`statuscode`) REFERENCES `illbatch_statuses` (`code`) ON DELETE SET NULL ON UPDATE CASCADE + CONSTRAINT `illbatches_bnfk` FOREIGN KEY (`patron_id`) REFERENCES `borrowers` (`borrowernumber`) ON DELETE SET NULL ON UPDATE CASCADE, + CONSTRAINT `illbatches_bcfk` FOREIGN KEY (`library_id`) REFERENCES `branches` (`branchcode`) ON DELETE SET NULL ON UPDATE CASCADE, + CONSTRAINT `illbatches_sfk` FOREIGN KEY (`status_code`) REFERENCES `illbatch_statuses` (`code`) ON DELETE SET NULL ON UPDATE CASCADE ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_unicode_ci; -- @@ -3366,11 +3366,12 @@ CREATE TABLE `illrequests` ( KEY `illrequests_bcfk_2` (`branchcode`), KEY `illrequests_safk` (`status_alias`), KEY `illrequests_bibfk` (`biblio_id`), + KEY `illrequests_ibfk` (`batch_id`), CONSTRAINT `illrequests_bcfk_2` FOREIGN KEY (`branchcode`) REFERENCES `branches` (`branchcode`) ON DELETE CASCADE ON UPDATE CASCADE, CONSTRAINT `illrequests_bibfk` FOREIGN KEY (`biblio_id`) REFERENCES `biblio` (`biblionumber`) ON DELETE SET NULL ON UPDATE CASCADE, CONSTRAINT `illrequests_bnfk` FOREIGN KEY (`borrowernumber`) REFERENCES `borrowers` (`borrowernumber`) ON DELETE CASCADE ON UPDATE CASCADE, CONSTRAINT `illrequests_safk` FOREIGN KEY (`status_alias`) REFERENCES `authorised_values` (`authorised_value`) ON DELETE SET NULL ON UPDATE CASCADE, - CONSTRAINT `illrequests_ibfk` FOREIGN KEY (`batch_id`) REFERENCES `illbatches` (`id`) ON DELETE SET NULL ON UPDATE CASCADE + CONSTRAINT `illrequests_ibfk` FOREIGN KEY (`batch_id`) REFERENCES `illbatches` (`ill_batch_id`) ON DELETE SET NULL ON UPDATE CASCADE ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_unicode_ci; /*!40101 SET character_set_client = @saved_cs_client */; diff --git a/t/db_dependent/IllbatchStatuses.t b/t/db_dependent/IllbatchStatuses.t index 45fd7b1a03..c03e81a426 100755 --- a/t/db_dependent/IllbatchStatuses.t +++ b/t/db_dependent/IllbatchStatuses.t @@ -193,11 +193,11 @@ my $status5 = Koha::IllbatchStatus->new( $status5->create_and_log; my $batch = Koha::Illbatch->new( { - name => "My test batch", - borrowernumber => $patron->borrowernumber, - branchcode => $library->branchcode, - backend => "TEST", - statuscode => $status5->code + name => "My test batch", + patron_id => $patron->borrowernumber, + library_id => $library->branchcode, + backend => "TEST", + statuscode => $status5->code } ); $batch->create_and_log; diff --git a/t/db_dependent/Illbatches.t b/t/db_dependent/Illbatches.t index 4f2b215bca..18f79bab9b 100755 --- a/t/db_dependent/Illbatches.t +++ b/t/db_dependent/Illbatches.t @@ -17,7 +17,6 @@ use Modern::Perl; -use File::Basename qw/basename/; use Koha::Database; use Koha::Illbatch; use Koha::Illbatches; @@ -28,7 +27,7 @@ use t::lib::TestBuilder; use Test::MockObject; use Test::MockModule; -use Test::More tests => 8; +use Test::More tests => 7; my $schema = Koha::Database->new->schema; my $builder = t::lib::TestBuilder->new; @@ -54,19 +53,17 @@ my $librarian = $builder->build( my $branch = $builder->build( { source => 'Branch' } ); # Create a batch -my $illbatch = $builder->build( +my $illbatch = $builder->build_object( { - source => 'Illbatch', + class => 'Koha::Illbatches', value => { - name => "My test batch", - backend => "Mock", - borrowernumber => $librarian->{borrowernumber}, - branchcode => $branch->{branchcode} + name => "My test batch", + backend => "Mock", + patron_id => $librarian->{borrowernumber}, + library_id => $branch->{branchcode} } } ); -my $batch_obj = Koha::Illbatches->find( $illbatch->{id} ); -isa_ok( $batch_obj, 'Koha::Illbatch' ); # Create an ILL request in the batch my $illrq = $builder->build( @@ -74,23 +71,23 @@ my $illrq = $builder->build( source => 'Illrequest', value => { borrowernumber => $patron->{borrowernumber}, - batch_id => $illbatch->{id} + batch_id => $illbatch->id } } ); my $illrq_obj = Koha::Illrequests->find( $illrq->{illrequest_id} ); # Check requests_count -my $requests_count = $batch_obj->requests_count; +my $requests_count = $illbatch->requests_count; is( $requests_count, 1, 'requests_count returns correctly' ); # Check patron -my $batch_patron = $batch_obj->patron; +my $batch_patron = $illbatch->patron; isa_ok( $batch_patron, 'Koha::Patron' ); is( $batch_patron->firstname, "Grogu", "patron returns correctly" ); # Check branch -my $batch_branch = $batch_obj->branch; +my $batch_branch = $illbatch->library; isa_ok( $batch_branch, 'Koha::Library' ); is( $batch_branch->branchcode, $branch->{branchcode}, "branch returns correctly" ); diff --git a/t/db_dependent/api/v1/ill_batches.t b/t/db_dependent/api/v1/ill_batches.t index 3b2b5c57fa..aae636d829 100755 --- a/t/db_dependent/api/v1/ill_batches.t +++ b/t/db_dependent/api/v1/ill_batches.t @@ -23,6 +23,8 @@ use Test::Mojo; use t::lib::TestBuilder; use t::lib::Mocks; +use JSON qw(encode_json); + use Koha::Illbatch; use Koha::Illbatches; use Koha::Illrequests; @@ -37,12 +39,10 @@ t::lib::Mocks::mock_preference( 'RESTBasicAuth', 1 ); subtest 'list() tests' => sub { - plan tests => 19; + plan tests => 21; $schema->storage->txn_begin; - Koha::Illbatches->search->delete; - my $librarian = $builder->build_object( { class => 'Koha::Patrons', @@ -52,55 +52,62 @@ subtest 'list() tests' => sub { } ); - my $branch = $builder->build_object( { class => 'Koha::Libraries' } ); + my $library = $builder->build_object( { class => 'Koha::Libraries' } ); my $password = 'sheev_is_da_boss!'; $librarian->set_password( { password => $password, skip_validation => 1 } ); my $userid = $librarian->userid; + my $batch_to_delete = $builder->build_object( { class => 'Koha::Illbatches' } ); + my $deleted_batch_id = $batch_to_delete->id; + $batch_to_delete->delete; + + my $query = { ill_batch_id => [$deleted_batch_id] }; + ## Authorized user tests # No batches, so empty array should be returned - $t->get_ok("//$userid:$password@/api/v1/ill/batches")->status_is(200)->json_is( [] ); + $t->get_ok( "//$userid:$password@/api/v1/ill/batches?q=" . encode_json($query) )->status_is(200)->json_is( [] ); - my $batch = $builder->build_object( + my $batch_1 = $builder->build_object( { class => 'Koha::Illbatches', value => { - name => "PapaPalpatine", - backend => "Mock", - borrowernumber => $librarian->borrowernumber, - branchcode => $branch->branchcode + backend => "Mock", + patron_id => $librarian->id, + library_id => $library->id, } } ); - my $illrq = $builder->build( + my $illrq = $builder->build_object( { - source => 'Illrequest', - value => { - borrowernumber => $librarian->borrowernumber, - batch_id => $batch->id + class => 'Koha::Illrequests', + value => { + batch_id => $batch_1->id, + borrowernumber => $librarian->id, } } ); - # One batch created, should get returned - $t->get_ok( - "//$userid:$password@/api/v1/ill/batches" => { 'x-koha-embed' => '+strings,requests+count,patron,branch' } ) - ->status_is(200)->json_has( '/0/batch_id', 'Batch ID' )->json_has( '/0/name', 'Batch name' ) - ->json_has( '/0/backend', 'Backend name' )->json_has( '/0/patron_id', 'Borrowernumber' ) - ->json_has( '/0/library_id', 'Branchcode' )->json_has( '/0/patron', 'patron embedded' ) - ->json_has( '/0/branch', 'branch embedded' )->json_has( '/0/requests_count', 'request count' ); + $query = { ill_batch_id => [ $batch_1->id ] }; - # Try to create a second batch with the same name, this should fail - my $another_batch = $builder->build_object( { class => 'Koha::Illbatches', value => { name => $batch->name } } ); + # One batch created, should get returned + $t->get_ok( "//$userid:$password@/api/v1/ill/batches?q=" + . encode_json($query) => { 'x-koha-embed' => '+strings,requests+count,patron,library' } )->status_is(200) + ->json_has( '/0/ill_batch_id', 'Batch ID' )->json_has( '/0/name', 'Batch name' ) + ->json_has( '/0/backend', 'Backend name' )->json_has( '/0/patron_id', 'Borrowernumber' ) + ->json_has( '/0/library_id', 'Branchcode' )->json_has( '/0/patron', 'patron embedded' ) + ->json_has( '/0/library', 'branch embedded' )->json_has( '/0/requests_count', 'request count' ); # Create a second batch with a different name - my $batch_with_another_name = $builder->build_object( { class => 'Koha::Illbatches' } ); + my $batch_2 = $builder->build_object( { class => 'Koha::Illbatches' } ); + + $query = { ill_batch_id => [ $batch_1->id, $batch_2->id ] }; # Two batches created, they should both be returned - $t->get_ok("//$userid:$password@/api/v1/ill/batches")->status_is(200)->json_has( '/0', 'has first batch' ) - ->json_has( '/1', 'has second batch' ); + $t->get_ok( "//$userid:$password@/api/v1/ill/batches?q=" . encode_json($query) )->status_is(200) + ->json_has( '/0', 'has first batch' )->json_is( '/0/ill_batch_id', $batch_1->id ) + ->json_has( '/1', 'has second batch' )->json_is( '/1/ill_batch_id', $batch_2->id ); my $patron = $builder->build_object( { @@ -144,16 +151,15 @@ subtest 'get() tests' => sub { } ); - my $branch = $builder->build_object( { class => 'Koha::Libraries' } ); + my $library = $builder->build_object( { class => 'Koha::Libraries' } ); my $batch = $builder->build_object( { class => 'Koha::Illbatches', value => { - name => "LeiaOrgana", - backend => "Mock", - borrowernumber => $librarian->borrowernumber, - branchcode => $branch->branchcode + backend => "Mock", + patron_id => $librarian->id, + library_id => $library->id, } } ); @@ -162,11 +168,11 @@ subtest 'get() tests' => sub { my $unauth_userid = $patron->userid; $t->get_ok( "//$userid:$password@/api/v1/ill/batches/" - . $batch->id => { 'x-koha-embed' => '+strings,requests+count,patron,branch' } )->status_is(200) - ->json_has( '/batch_id', 'Batch ID' )->json_has( '/name', 'Batch name' ) - ->json_has( '/backend', 'Backend name' )->json_has( '/patron_id', 'Borrowernumber' ) - ->json_has( '/library_id', 'Branchcode' )->json_has( '/patron', 'patron embedded' ) - ->json_has( '/branch', 'branch embedded' )->json_has( '/requests_count', 'request count' ); + . $batch->id => { 'x-koha-embed' => '+strings,requests+count,patron,library' } )->status_is(200) + ->json_has( '/ill_batch_id', 'Batch ID' )->json_has( '/name', 'Batch name' ) + ->json_has( '/backend', 'Backend name' )->json_has( '/patron_id', 'Borrowernumber' ) + ->json_has( '/library_id', 'Branchcode' )->json_has( '/patron', 'patron embedded' ) + ->json_has( '/library', 'library embedded' )->json_has( '/requests_count', 'request count' ); $t->get_ok( "//$unauth_userid:$password@/api/v1/ill/batches/" . $batch->id )->status_is(403); @@ -182,7 +188,7 @@ subtest 'get() tests' => sub { subtest 'add() tests' => sub { - plan tests => 19; + plan tests => 20; $schema->storage->txn_begin; @@ -206,16 +212,16 @@ subtest 'add() tests' => sub { $patron->set_password( { password => $password, skip_validation => 1 } ); my $unauth_userid = $patron->userid; - my $branch = $builder->build_object( { class => 'Koha::Libraries' } ); + my $library = $builder->build_object( { class => 'Koha::Libraries' } ); my $batch_status = $builder->build_object( { class => 'Koha::IllbatchStatuses' } ); my $batch_metadata = { - name => "Anakin's requests", - backend => "Mock", - cardnumber => $librarian->cardnumber, - library_id => $branch->branchcode, - statuscode => $batch_status->code + name => "Anakin's requests", + backend => "Mock", + cardnumber => $librarian->cardnumber, + library_id => $library->branchcode, + status_code => $batch_status->code }; # Unauthorized attempt to write @@ -239,13 +245,13 @@ subtest 'add() tests' => sub { # Authorized attempt to write my $batch_id = - $t->post_ok( "//$userid:$password@/api/v1/ill/batches" => - { 'x-koha-embed' => '+strings,requests+count,patron,branch' } => json => $batch_metadata ) - ->status_is(201) - ->json_is( '/name' => $batch_metadata->{name} )->json_is( '/backend' => $batch_metadata->{backend} ) - ->json_is( '/patron_id' => $librarian->borrowernumber ) - ->json_is( '/library_id' => $batch_metadata->{library_id} )->json_is( '/statuscode' => $batch_status->code ) - ->json_has('/patron')->json_has('/_strings/status')->json_has('/requests_count')->json_has('/branch'); + $t->post_ok( + "//$userid:$password@/api/v1/ill/batches" => { 'x-koha-embed' => '+strings,requests+count,patron,library' } => + json => $batch_metadata )->status_is(201)->json_is( '/name' => $batch_metadata->{name} ) + ->json_is( '/backend' => $batch_metadata->{backend} )->json_is( '/patron_id' => $librarian->borrowernumber ) + ->json_is( '/library_id' => $batch_metadata->{library_id} )->json_is( '/status_code' => $batch_status->code ) + ->json_has('/patron')->json_has('/_strings/status_code')->json_has('/_strings/library_id') + ->json_has('/requests_count')->json_has('/library'); # Authorized attempt to create with null id $batch_metadata->{id} = undef; @@ -281,8 +287,7 @@ subtest 'update() tests' => sub { $patron->set_password( { password => $password, skip_validation => 1 } ); my $unauth_userid = $patron->userid; - my $branch = $builder->build_object( { class => 'Koha::Libraries' } ); - + my $library = $builder->build_object( { class => 'Koha::Libraries' } ); my $batch_id = $builder->build_object( { class => 'Koha::Illbatches' } )->id; # Unauthorized attempt to update @@ -293,10 +298,10 @@ subtest 'update() tests' => sub { # Attempt partial update on a PUT my $batch_with_missing_field = { - backend => "Mock", - patron_id => $librarian->borrowernumber, - library_id => $branch->branchcode, - statuscode => $batch_status->code + backend => "Mock", + patron_id => $librarian->borrowernumber, + library_id => $library->branchcode, + status_code => $batch_status->code }; $t->put_ok( "//$userid:$password@/api/v1/ill/batches/$batch_id" => json => $batch_with_missing_field ) @@ -304,11 +309,11 @@ subtest 'update() tests' => sub { # Full object update on PUT my $batch_with_updated_field = { - name => "Master Ploo Koon", - backend => "Mock", - patron_id => $librarian->borrowernumber, - library_id => $branch->branchcode, - statuscode => $batch_status->code + name => "Master Ploo Koon", + backend => "Mock", + patron_id => $librarian->borrowernumber, + library_id => $library->branchcode, + status_code => $batch_status->code }; $t->put_ok( "//$userid:$password@/api/v1/ill/batches/$batch_id" => json => $batch_with_updated_field ) -- 2.39.5