Bug 36481 - Add GET /libraries/:library_id/cash_registers
Summary: Add GET /libraries/:library_id/cash_registers
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: REST API (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Tomás Cohen Arazi
QA Contact:
URL:
Keywords:
Depends on: 36480
Blocks: 36482
  Show dependency treegraph
 
Reported: 2024-04-01 20:47 UTC by Tomás Cohen Arazi
Modified: 2024-05-01 12:52 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
This enhancement adds an API endpoint for requesting a list of cash registers for a library. For example: http://127.0.0.1:8080/api/v1/libraries/cpl/cash_registers
Version(s) released in:
24.05.00


Attachments
Bug 36481: Add GET /libraries/:library_id/cash_registers (8.63 KB, patch)
2024-04-01 20:56 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 36481: Add GET /libraries/:library_id/cash_registers (8.67 KB, patch)
2024-04-02 07:58 UTC, David Nind
Details | Diff | Splinter Review
Bug 36481: Add GET /libraries/:library_id/cash_registers (8.73 KB, patch)
2024-04-04 10:55 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 36481: (QA follow-up) Rename branch_default to library_default (1.50 KB, patch)
2024-05-01 12:50 UTC, Martin Renvoize
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 2024-04-01 20:47:34 UTC
I propose to add an endpoint for fetching a library's list of cash registers. Context is bug 36237.
Comment 1 Tomás Cohen Arazi 2024-04-01 20:56:27 UTC
Created attachment 164228 [details] [review]
Bug 36481: Add GET /libraries/:library_id/cash_registers

This patch adds the mentioned route. For the task it:

* Adds Koha::Cash::Register->to_api_mapping with trivial mappings
* Adds a cash_register object definition on the API spec
* Adds a controller to handle requests
* Adds tests for the new endpoint

To test:
1. Apply this patch
2. Run:
   $ ktd --shell
  k$ qa
=> SUCCESS: All green! Tests pass!
3. Play with Postman!
4. Sign off :-D
Comment 2 David Nind 2024-04-02 07:58:48 UTC
Created attachment 164237 [details] [review]
Bug 36481: Add GET /libraries/:library_id/cash_registers

This patch adds the mentioned route. For the task it:

* Adds Koha::Cash::Register->to_api_mapping with trivial mappings
* Adds a cash_register object definition on the API spec
* Adds a controller to handle requests
* Adds tests for the new endpoint

To test:
1. Apply this patch
2. Run:
   $ ktd --shell
  k$ qa
=> SUCCESS: All green! Tests pass!
3. Play with Postman!
4. Sign off :-D

Signed-off-by: David Nind <david@davidnind.com>
Comment 3 David Nind 2024-04-02 08:02:27 UTC
Not able to the patch on current master without applying 36480 first. Assuming bug 36480 needs to be added as a dependency.

Testing notes (using KTD)
=========================

1. Apply bug 36480
2. Apply the patches for this bug (and dependencies).
3. qa and tests should pass without any errors
4. Enable these system preferences:
   - UseCashRegisters
   - RESTBasicAuth 
5. Test the new API endpoint using Postman (or browser):
   - In the staff interface, add some cash registers for different libraries, some with multiple registers
   - Add a get request to request the cash registers for a library, for example: add two cash registers to Centerville and one to Fairfield:
     . Set Basic Auth in Postman to use koha/koha
     . http://127.0.0.1:8080/api/v1/libraries/cpl/cash_registers
     . http://127.0.0.1:8080/api/v1/libraries/ffl/cash_registers
   - Results should return the cash register information for the library, for example:
[
    {
        "archived": false,
        "branch_default": false,
        "cash_register_id": 3,
        "description": "Test cash register for Centerville",
        "library_id": "CPL",
        "name": "TEST1",
        "starting_float": 0.0
    },
    {
        "archived": false,
        "branch_default": false,
        "cash_register_id": 4,
        "description": "Another test cash register for Centerville",
        "library_id": "CPL",
        "name": "TEST2",
        "starting_float": 0.0
    }
]
   - Put an invalid library code in the request, for example xxx:
     . http://127.0.0.1:8080/api/v1/libraries/xxx/cash_registers
     . Should get "Library not found" error
   - Disable cash registers and send above request. Should get:
      {
          "error": "Feature disabled"
      }

Note: If cash registers are enabled, but a library has none set up, then the response is blank. Should there be an error/response saying this library has no cash registers setup?
. Response received where library doesn't have a cash register:
  []

Path doesn't apply messages
===========================

git bz apply 36481

Bug 36481 Depends on bug 36237 (Signed Off)
Follow? [(y)es, (n)o] y

Bug 36237 - Improve set-library UI after 34478

163013 - Bug 36237: UI improvments for set-library page

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 36237: UI improvments for set-library page

Bug 36481 - Add GET /libraries/:library_id/cash_registers

164228 - Bug 36481: Add GET /libraries/:library_id/cash_registers

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 36481: Add GET /libraries/:library_id/cash_registers
Using index info to reconstruct a base tree...
M	Koha/REST/V1/Libraries.pm
M	api/v1/swagger/paths/libraries.yaml
M	api/v1/swagger/swagger.yaml
M	t/db_dependent/api/v1/libraries.t
Falling back to patching base and 3-way merge...
Auto-merging t/db_dependent/api/v1/libraries.t
CONFLICT (content): Merge conflict in t/db_dependent/api/v1/libraries.t
Auto-merging api/v1/swagger/swagger.yaml
CONFLICT (content): Merge conflict in api/v1/swagger/swagger.yaml
Auto-merging api/v1/swagger/paths/libraries.yaml
CONFLICT (content): Merge conflict in api/v1/swagger/paths/libraries.yaml
Auto-merging Koha/REST/V1/Libraries.pm
CONFLICT (content): Merge conflict in Koha/REST/V1/Libraries.pm
error: Failed to merge in the changes.
Patch failed at 0001 Bug 36481: Add GET /libraries/:library_id/cash_registers
Comment 4 Tomás Cohen Arazi 2024-04-02 11:52:00 UTC
(In reply to David Nind from comment #3)
> Not able to the patch on current master without applying 36480 first.
> Assuming bug 36480 needs to be added as a dependency.

That's correct. I forgot to add it. Thanks!
Comment 5 Pedro Amorim 2024-04-04 10:55:33 UTC
Created attachment 164415 [details] [review]
Bug 36481: Add GET /libraries/:library_id/cash_registers

This patch adds the mentioned route. For the task it:

* Adds Koha::Cash::Register->to_api_mapping with trivial mappings
* Adds a cash_register object definition on the API spec
* Adds a controller to handle requests
* Adds tests for the new endpoint

To test:
1. Apply this patch
2. Run:
   $ ktd --shell
  k$ qa
=> SUCCESS: All green! Tests pass!
3. Play with Postman!
4. Sign off :-D

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Comment 6 Katrin Fischer 2024-04-26 15:30:23 UTC
Being a little nit-picky, but shouldn't this be "library default"?

+  branch_default:
+    type: boolean
+    description: If this till is the branch default

Adding "additional_work_needed", meaning "Asking for feedback". 

Not a blocker.
Comment 7 Katrin Fischer 2024-04-26 16:07:25 UTC
Pushed for 24.05!

Well done everyone, thank you!
Comment 8 Martin Renvoize 2024-05-01 12:50:29 UTC
Created attachment 165965 [details] [review]
Bug 36481: (QA follow-up) Rename branch_default to library_default

Use agreed terminology

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 9 Martin Renvoize 2024-05-01 12:52:13 UTC
Follow-up created and pushed.. removing additional_work_needed