Attachment 149460 Details for Bug 31795 – Bug 31795: (QA follow-up) Use x-koha-override header

[patch] Bug 31795: (QA follow-up) Use x-koha-override header

Bug-31795-QA-follow-up-Use-x-koha-override-header.patch (text/plain), 10.20 KB, created by David Nind on 2023-04-11 18:11:40 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: David Nind

Created: 2023-04-11 18:11:40 UTC

Size: 10.20 KB

patch

obsolete

>From 9913a6c00d0abe2fd648da9246873dc7e1852a08 Mon Sep 17 00:00:00 2001
>From: Tomas Cohen Arazi <tomascohen@theke.io>
>Date: Mon, 10 Apr 2023 14:14:12 -0300
>Subject: [PATCH] Bug 31795: (QA follow-up) Use x-koha-override header
>
>Despite its title, this patch does a couple more things.
>
>1. The tests are rewritten to cover more things, and also to avoid
>   deleting all authorities inside the transaction. It is really not
>   required.
>2. It makes the endpoint rely on the already generically implemented
>   x-koha-override header, which is intended for the same use case as
>   x-confirm-not-duplicate is for.
>3. It changes the return codes to match the coding guidelines [1]
>4. Only checks for duplicates if no override passed.
>
>To test:
>1. Run:
>   $ ktd --shell
>  k$ prove t/db_dependent/api/v1/authorities.t
>=> SUCCESS: Tests pass!
>2. Apply this follow-up
>3. Repeat 1
>=> SUCCESS: Tests pass!
>4. Sign off :-D
>
>[1] https://wiki.koha-community.org/wiki/Coding_Guidelines_-_API#SWAGGER3.2.1_POST
>
>Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
>Signed-off-by: David Nind <david@davidnind.com>
>---
> Koha/REST/V1/Authorities.pm           | 67 +++++++++++++--------------
> api/v1/swagger/paths/authorities.yaml | 45 ++++++++++++------
> t/db_dependent/api/v1/authorities.t   | 50 +++++++++++++-------
> 3 files changed, 96 insertions(+), 66 deletions(-)
>
>diff --git a/Koha/REST/V1/Authorities.pm b/Koha/REST/V1/Authorities.pm
>index eac54c3df5..8a6c24b2c1 100644
>--- a/Koha/REST/V1/Authorities.pm
>+++ b/Koha/REST/V1/Authorities.pm
>@@ -138,7 +138,8 @@ sub add {
>     my $c = shift->openapi->valid_input or return;
> 
>     try {
>-        my $body = $c->validation->param('Body');
>+        my $headers   = $c->req->headers;
>+        my $overrides = $c->stash('koha.overrides');
> 
>         my $flavour =
>             C4::Context->preference('marcflavour') eq 'UNIMARC'
>@@ -146,49 +147,45 @@ sub add {
>             : 'MARC21';
> 
>         my $record;
>-        my $authtypecode;
> 
>-        if ( $c->req->headers->content_type =~ m/application\/json/ ) {
>-            $record = MARC::Record->new_from_xml( $body->{marcxml}, 'UTF-8', $flavour );
>-            $authtypecode = $body->{authtypecode};
>+        my $authtypecode = $headers->header('x-authority-type');
>+        my $content_type = $headers->content_type;
>+
>+        if ( $content_type =~ m/application\/marcxml\+xml/ ) {
>+            $record = MARC::Record->new_from_xml( $c->req->body, 'UTF-8', $flavour );
>+        } elsif ( $content_type =~ m/application\/marc-in-json/ ) {
>+            $record = MARC::Record->new_from_mij_structure( $c->req->json );
>+        } elsif ( $content_type =~ m/application\/marc/ ) {
>+            $record = MARC::Record->new_from_usmarc( $c->req->body );
>         } else {
>-            $authtypecode = $c->validation->param('x-authority-type');
>-            if ( $c->req->headers->content_type =~ m/application\/marcxml\+xml/ ) {
>-                $record = MARC::Record->new_from_xml( $body, 'UTF-8', $flavour );
>-            } elsif ( $c->req->headers->content_type =~ m/application\/marc-in-json/ ) {
>-                $record = MARC::Record->new_from_mij_structure( $body );
>-            } elsif ( $c->req->headers->content_type =~ m/application\/marc/ ) {
>-                $record = MARC::Record->new_from_usmarc( $body );
>-            } else {
>-                return $c->render(
>-                    status  => 406,
>-                    openapi => [
>-                        "application/json",
>-                        "application/marcxml+xml",
>-                        "application/marc-in-json",
>-                        "application/marc"
>-                    ]
>-                );
>-            }
>+            return $c->render(
>+                status  => 406,
>+                openapi => [
>+                    "application/marcxml+xml",
>+                    "application/marc-in-json",
>+                    "application/marc"
>+                ]
>+            );
>         }
> 
>-        my ($duplicateauthid,$duplicateauthvalue);
>-            ($duplicateauthid,$duplicateauthvalue) = FindDuplicateAuthority($record,$authtypecode);
>-
>-        my $confirm_not_duplicate = $c->validation->param('x-confirm-not-duplicate');
>+        unless ( $overrides->{any} || $overrides->{duplicate} ) {
>+            my ( $duplicateauthid, $duplicateauthvalue ) = FindDuplicateAuthority( $record, $authtypecode );
> 
>-        return $c->render(
>-            status  => 400,
>-            openapi => {
>-                error => "Duplicate authority $duplicateauthid"
>-            }
>-        ) unless !$duplicateauthid || $confirm_not_duplicate;
>+            return $c->render(
>+                status  => 409,
>+                openapi => {
>+                    error      => "Duplicate record ($duplicateauthid)",
>+                    error_code => 'duplicate',
>+                }
>+            ) unless !$duplicateauthid;
>+        }
> 
>         my $authid = AddAuthority( $record, undef, $authtypecode );
> 
>+        $c->res->headers->location($c->req->url->to_string . '/' . $authid);
>         $c->render(
>-            status  => 200,
>-            openapi => { id => $authid }
>+            status  => 201,
>+            openapi => q{},
>         );
>     }
>     catch {
>diff --git a/api/v1/swagger/paths/authorities.yaml b/api/v1/swagger/paths/authorities.yaml
>index a6575ce809..e733cf764e 100644
>--- a/api/v1/swagger/paths/authorities.yaml
>+++ b/api/v1/swagger/paths/authorities.yaml
>@@ -6,24 +6,36 @@
>     tags:
>       - authorities
>     summary: Add authority
>+    description: |
>+      Add an authority record to Koha. An optional `x-authority-type`
>+      may be passed to specify the cataloguing framework to be used (instead
>+      of the default).
>+
>+      The request body is expected to contain a MARC record in the format specified in
>+      the `Content-type` header you pass. Possible values for this header and the corresponding
>+      record formats expected are listed below:
>+
>+      * application/marcxml+xml: MARCXML
>+      * application/marc-in-json: MARC-in-JSON
>+      * application/marc: Raw USMARC binary data
>     parameters:
>-      - name: Body
>-        in: body
>-        description: A JSON object or the Marc string describing an authority
>-        required: true
>-        schema:
>-          type:
>-            - string
>-            - object
>       - $ref: "../swagger.yaml#/parameters/authority_type_header"
>-      - $ref: "../swagger.yaml#/parameters/confirm_not_duplicate_header"
>-    produces:
>-      - application/json
>+      - name: x-koha-override
>+        in: header
>+        required: false
>+        description: Overrides list sent as a request header
>+        type: array
>+        items:
>+          type: string
>+          enum:
>+            - any
>+            - duplicate
>+        collectionFormat: csv
>     responses:
>-      "200":
>+      "201":
>         description: An authority
>       "400":
>-        description: Bad request
>+        description: Bad request.
>         schema:
>           $ref: "../swagger.yaml#/definitions/error"
>       "401":
>@@ -41,6 +53,13 @@
>           description: Accepted content-types
>           items:
>             type: string
>+      "409":
>+        description: |
>+          Conflict creating the resource. Possible `error_code` attribute values:
>+
>+          * `duplicate`
>+        schema:
>+          $ref: "../swagger.yaml#/definitions/error"
>       "500":
>         description: |
>           Internal server error. Possible `error_code` attribute values:
>diff --git a/t/db_dependent/api/v1/authorities.t b/t/db_dependent/api/v1/authorities.t
>index 938fbfd214..1ead19fc8d 100755
>--- a/t/db_dependent/api/v1/authorities.t
>+++ b/t/db_dependent/api/v1/authorities.t
>@@ -155,12 +155,10 @@ subtest 'delete() tests' => sub {
> 
> subtest 'post() tests' => sub {
> 
>-    plan tests => 14;
>+    plan tests => 19;
> 
>     $schema->storage->txn_begin;
> 
>-    Koha::Authorities->delete;
>-
>     my $patron = $builder->build_object(
>         {
>             class => 'Koha::Patrons',
>@@ -202,21 +200,37 @@ subtest 'post() tests' => sub {
>         }
>     );
> 
>-    $t->post_ok("//$userid:$password@/api/v1/authorities" => json => $json)
>-      ->status_is(200)
>-      ->json_has('/id');
>-
>-    $t->post_ok("//$userid:$password@/api/v1/authorities" => {'Content-Type' => 'application/marcxml+xml', 'x-authority-type' => 'CORPO_NAME'} => $marcxml)
>-      ->status_is(200)
>-      ->json_has('/id');
>-
>-    $t->post_ok("//$userid:$password@/api/v1/authorities" => {'Content-Type' => 'application/marc-in-json', 'x-authority-type' => 'CORPO_NAME'} => $mij)
>-      ->status_is(200)
>-      ->json_has('/id');
>-
>-    $t->post_ok("//$userid:$password@/api/v1/authorities" => {'Content-Type' => 'application/marc', 'x-authority-type' => 'CORPO_NAME'} => $marc)
>-      ->status_is(200)
>-      ->json_has('/id');
>+    # x-koha-override passed to make sure it goes through
>+    $t->post_ok("//$userid:$password@/api/v1/authorities" => {'Content-Type' => 'application/marcxml+xml', 'x-authority-type' => 'CORPO_NAME', 'x-koha-override' => 'any' } => $marcxml)
>+      ->status_is(201)
>+      ->json_is(q{})
>+      ->header_like(
>+          Location => qr|^\/api\/v1\/authorities/\d*|,
>+          'SWAGGER3.4.1'
>+      );
>+
>+    # x-koha-override not passed to force block because duplicate
>+    $t->post_ok("//$userid:$password@/api/v1/authorities" => {'Content-Type' => 'application/marc-in-json', 'x-authority-type' => 'CORPO_NAME' } => $mij)
>+      ->status_is(409)
>+      ->header_exists_not( 'Location', 'Location header is only set when the new resource is created' )
>+      ->json_like( '/error' => qr/Duplicate record (\d*)/ )
>+      ->json_is( '/error_code' => q{duplicate} );
>+
>+    $t->post_ok("//$userid:$password@/api/v1/authorities" => {'Content-Type' => 'application/marc-in-json', 'x-authority-type' => 'CORPO_NAME', 'x-koha-override' => 'duplicate' } => $mij)
>+      ->status_is(201)
>+      ->json_is(q{})
>+      ->header_like(
>+          Location => qr|^\/api\/v1\/authorities/\d*|,
>+          'SWAGGER3.4.1'
>+      );
>+
>+    $t->post_ok("//$userid:$password@/api/v1/authorities" => {'Content-Type' => 'application/marc', 'x-authority-type' => 'CORPO_NAME', 'x-koha-override' => 'duplicate' } => $marc)
>+      ->status_is(201)
>+      ->json_is(q{})
>+      ->header_like(
>+          Location => qr|^\/api\/v1\/authorities/\d*|,
>+          'SWAGGER3.4.1'
>+      );
> 
>     $schema->storage->txn_rollback;
> };
>-- 
>2.30.2

Actions: View | Diff | Splinter Review

Attachments on bug 31795: 144555 | 144746 | 144747 | 144749 | 144750 | 149380 | 149407 | 149409 | 149451 | 149459 | 149460 | 149461 | 149462 | 150370 | 150371 | 150372 | 150373