Bug 36641

Summary: Add an endpoint to list circulation rules
Product: Koha Reporter: Martin Renvoize (ashimema) <martin.renvoize>
Component: REST APIAssignee: Martin Renvoize (ashimema) <martin.renvoize>
Status: Pushed to main --- QA Contact: Kyle M Hall (khall) <kyle>
Severity: enhancement    
Priority: P5 - low CC: kyle, lucas, mspinney, tomascohen
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19037
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
24.11.00
Circulation function:
Bug Depends on:    
Bug Blocks: 10190, 19037, 34440, 37256, 37590    
Attachments: Bug 36641: Add endpoint for fetching circulation rules
Bug 36641: Add endpoint for fetching circulation rules
Bug 36641: (follow-up) Requested changes
Bug 36641: Add endpoint for fetching circulation rules
Bug 36641: (follow-up) Requested changes
Bug 36641: Introduce render_invalid_parameter_value helper
Bug 36641: Add endpoint for fetching circulation rules
Bug 36641: WIP - Add unit tests for /circulation_rules
Bug 36641: Introduce render_invalid_parameter_value helper
Bug 36641: Add unit tests for /circulation_rules
Bug 36641: Add endpoint for fetching circulation rules
Bug 36641: Introduce render_invalid_parameter_value helper
Bug 36641: Add unit tests for /circulation_rules
Bug 36641: Add endpoint for fetching circulation rules
Bug 36641: Harmonize parameter names
Bug 36641: Make tests cover all code paths
Bug 36641: Make tests cover all code paths
Bug 36641: (follow-up) Wrap in a try/catch block
Bug 36641: (follow-up) Wrap in a try/catch block
Bug 36641: Introduce render_invalid_parameter_value helper
Bug 36641: Add unit tests for /circulation_rules
Bug 36641: Add endpoint for fetching circulation rules
Bug 36641: Harmonize parameter names
Bug 36641: Make tests cover all code paths
Bug 36641: (follow-up) Wrap in a try/catch block
Bug 36641: (QA Follow-up) Fix variable name
Bug 36641: (follow-up) Update to allow returning non-effective rules
Bug 36641: Introduce render_invalid_parameter_value helper
Bug 36641: Add unit tests for /circulation_rules
Bug 36641: Add endpoint for fetching circulation rules
Bug 36641: Harmonize parameter names
Bug 36641: Make tests cover all code paths
Bug 36641: (follow-up) Wrap in a try/catch block
Bug 36641: (QA Follow-up) Fix variable name
Bug 36641: (follow-up) Update to allow returning non-effective rules
Bug 36641: Optionally filter to default rules using *
Bug 36641: Return '*' for 'default rule'
Bug 36641: Unit tests for effective=0
Bug 36641: Introduce render_invalid_parameter_value helper
Bug 36641: Add unit tests for /circulation_rules
Bug 36641: Add endpoint for fetching circulation rules
Bug 36641: Harmonize parameter names
Bug 36641: Make tests cover all code paths
Bug 36641: (follow-up) Wrap in a try/catch block
Bug 36641: (QA Follow-up) Fix variable name
Bug 36641: (follow-up) Update to allow returning non-effective rules
Bug 36641: Optionally filter to default rules using *
Bug 36641: Return '*' for 'default rule'
Bug 36641: Ensure we return all keys
Bug 36641: Unit tests for effective=0
Bug 36641: Introduce render_invalid_parameter_value helper
Bug 36641: Add unit tests for /circulation_rules
Bug 36641: Add endpoint for fetching circulation rules
Bug 36641: Harmonize parameter names
Bug 36641: Make tests cover all code paths
Bug 36641: (follow-up) Wrap in a try/catch block
Bug 36641: (QA Follow-up) Fix variable name
Bug 36641: (follow-up) Update to allow returning non-effective rules
Bug 36641: Optionally filter to default rules using *
Bug 36641: Return '*' for 'default rule'
Bug 36641: Ensure we return all keys
Bug 36641: Unit tests for effective=0
Bug 36641: (follow-up) Ensure limited return works
Bug 36641: (follow-up) Ensure limited return works
Bug 36641: Introduce render_invalid_parameter_value helper
Bug 36641: Add unit tests for /circulation_rules
Bug 36641: Add endpoint for fetching circulation rules
Bug 36641: Harmonize parameter names
Bug 36641: Make tests cover all code paths
Bug 36641: (follow-up) Wrap in a try/catch block
Bug 36641: (QA follow-up) Fix variable name
Bug 36641: (follow-up) Update to allow returning non-effective rules
Bug 36641: Optionally filter to default rules using *
Bug 36641: Return '*' for 'default rule'
Bug 36641: Ensure we return all keys
Bug 36641: Unit tests for effective=0
Bug 36641: (follow-up) Ensure limited return works
Bug 36641: Fix tests

Description Martin Renvoize (ashimema) 2024-04-18 12:45:08 UTC
It would be helpful to have an API endpoint that allowed fetching the circulation rules for a given itemtype, branchcode and patron category combination.
Comment 1 Martin Renvoize (ashimema) 2024-04-18 12:47:42 UTC
Created attachment 165086 [details] [review]
Bug 36641: Add endpoint for fetching circulation rules

This patch is a proof of concept for an API endpoint that returns a list
of circulation rules for the specified itemtype, branchcode and patron
category combination.  You can also add a list of `rules` as a query
parameter to limit the response to only the rules you are interested in
for this combination.
Comment 2 Martin Renvoize (ashimema) 2024-04-18 13:04:27 UTC
Setting to NSO to get eyes.. but right at this moment it would fail QA for lack of tests.

I want feedback specifically on the URL and the form of the response and if we're happy for it to be an open object or want to a spec for it with each of the available circ rule keys listed but optional.
Comment 3 Tomás Cohen Arazi (tcohen) 2024-04-26 12:58:05 UTC
A few remarks:

* The order of the path parameters is arbitrary, right? I assume you thought about it
* The parameter names should be fixed to our current uses
* The returned structure is under specified
* I never liked /circulation-rules, can we make it /circulation_rules so it matches all other routes?
Comment 4 Martin Renvoize (ashimema) 2024-06-20 14:40:38 UTC
I'm looking to come back to this now.

OK, so you'd like me to update the existing endpoint to `/circulation_rules/kinds` and add mine as `/circulation_rules`. I can do that.. though do we know if anyone out there is using the /kinds route already?

I did think about the order of parameters, though I not wed to it.  I had them as path params as I felt it made it clearer that the endpoint expects all of them to be filled.. however we could just have them as query params and then order is inherently meaningless and in fact we don't actually always needs all of them really.

Yes, I said the return structure is underspecified.. but it's also a bit variable.. I'll try to see what I can come up with.

Are you saying you want the 'kinds' param to be an enum of all rule kinds..  I was sort of trying to avoid having to list those again..  I wish it was simple to build this particular list from code to embed into the spec.. maybe I can.. I'll investigate.
Comment 5 Martin Renvoize (ashimema) 2024-06-20 15:02:13 UTC
(In reply to Martin Renvoize from comment #4)
> OK, so you'd like me to update the existing endpoint to
> `/circulation_rules/kinds` and add mine as `/circulation_rules`. I can do
> that.. though do we know if anyone out there is using the /kinds route
> already?

Consider in testing now, it's already broken I think we're safe to change it now.
Comment 6 Martin Renvoize (ashimema) 2024-06-20 16:26:25 UTC
Created attachment 167937 [details] [review]
Bug 36641: Add endpoint for fetching circulation rules

This patch is a proof of concept for an API endpoint that returns a list
of circulation rules for the specified itemtype, branchcode and patron
category combination.  You can also add a list of `rules` as a query
parameter to limit the response to only the rules you are interested in
for this combination.
Comment 7 Martin Renvoize (ashimema) 2024-06-20 16:26:28 UTC
Created attachment 167938 [details] [review]
Bug 36641: (follow-up) Requested changes

This patch does a few things:

1) Moves /circulation-rules to /circulation_rules
2) Adds a the missing permission to /circulation_rules/kinds
3) Updates the circ-rule-kind definition to allow for the new fields
   added since the route was first defined
4) Adds a 'render_invalid_parameter_value' helper to ::Responses
5) Uses the above helper to render a 400 when an invalid branchcode,
   itemtype or patron category code is used for filtering
6) Converts from required path based parameters to optional query based
   parameters.
7) Refined the definition for the 'rules' response so we now only allow
   for 'string' or 'integer' values for the keys in the response.
Comment 8 Martin Renvoize (ashimema) 2024-06-20 16:27:49 UTC
I'm still not especially happy with the /kinds endpoint but at least with this patch it responds something.. I think we should handle cleaning it up properly in another bug.

I am fairly happy with the end result of the follow-up however. Please let me know what you think Tomas.
Comment 9 Martin Renvoize (ashimema) 2024-06-21 11:24:45 UTC
Created attachment 167946 [details] [review]
Bug 36641: Add endpoint for fetching circulation rules

This patch is a proof of concept for an API endpoint that returns a list
of circulation rules for the specified itemtype, branchcode and patron
category combination.  You can also add a list of `rules` as a query
parameter to limit the response to only the rules you are interested in
for this combination.
Comment 10 Martin Renvoize (ashimema) 2024-06-21 11:24:48 UTC
Created attachment 167947 [details] [review]
Bug 36641: (follow-up) Requested changes

This patch does a few things:

1) Moves /circulation-rules to /circulation_rules
2) Adds a the missing permission to /circulation_rules/kinds
3) Updates the circ-rule-kind definition to allow for the new fields
   added since the route was first defined
4) Adds a 'render_invalid_parameter_value' helper to ::Responses
5) Uses the above helper to render a 400 when an invalid branchcode,
   itemtype or patron category code is used for filtering
6) Converts from required path based parameters to optional query based
   parameters.
7) Refined the definition for the 'rules' response so we now only allow
   for 'string' or 'integer' values for the keys in the response.

NOTE: The multi for the 'rules' query parameter fails to work at the
moment.
Comment 11 Martin Renvoize (ashimema) 2024-06-24 15:29:05 UTC
Created attachment 168018 [details] [review]
Bug 36641: Introduce render_invalid_parameter_value helper

This patch introduces a new 'render_invalid_parameter_value' helper
method that accepts 'path' and 'values' parameters to denote which field
has failed validation and where the end user can get valid options.
Comment 12 Martin Renvoize (ashimema) 2024-06-24 15:29:08 UTC
Created attachment 168019 [details] [review]
Bug 36641: Add endpoint for fetching circulation rules

This patch adds an endpoint for fetching ciruclations rules given the
constraints of the passed parameters.

We optionally expect item_type, library and patron_category as query
parameters and we return a list of relevant circulation rules pertaining
to that combination of requirements.

You can also add a list of `rules` as a query parameter to limit the
response to only the rules you are interested in for this combination.
Comment 13 Martin Renvoize (ashimema) 2024-06-24 15:30:29 UTC
Created attachment 168020 [details] [review]
Bug 36641: WIP - Add unit tests for /circulation_rules
Comment 14 Martin Renvoize (ashimema) 2024-06-24 16:05:37 UTC
Created attachment 168023 [details] [review]
Bug 36641: Introduce render_invalid_parameter_value helper

This patch introduces a new 'render_invalid_parameter_value' helper
method that accepts 'path' and 'values' parameters to denote which field
has failed validation and where the end user can get valid options.
Comment 15 Martin Renvoize (ashimema) 2024-06-24 16:05:40 UTC
Created attachment 168024 [details] [review]
Bug 36641: Add unit tests for /circulation_rules
Comment 16 Martin Renvoize (ashimema) 2024-06-24 16:05:43 UTC
Created attachment 168025 [details] [review]
Bug 36641: Add endpoint for fetching circulation rules

This patch adds an endpoint for fetching ciruclations rules given the
constraints of the passed parameters.

We optionally expect item_type, library and patron_category as query
parameters and we return a list of relevant circulation rules pertaining
to that combination of requirements.

You can also add a list of `rules` as a query parameter to limit the
response to only the rules you are interested in for this combination.
Comment 17 Tomás Cohen Arazi (tcohen) 2024-07-02 13:17:42 UTC
Created attachment 168365 [details] [review]
Bug 36641: Introduce render_invalid_parameter_value helper

This patch introduces a new 'render_invalid_parameter_value' helper
method that accepts 'path' and 'values' parameters to denote which field
has failed validation and where the end user can get valid options.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 18 Tomás Cohen Arazi (tcohen) 2024-07-02 13:17:45 UTC
Created attachment 168366 [details] [review]
Bug 36641: Add unit tests for /circulation_rules

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 19 Tomás Cohen Arazi (tcohen) 2024-07-02 13:17:48 UTC
Created attachment 168367 [details] [review]
Bug 36641: Add endpoint for fetching circulation rules

This patch adds an endpoint for fetching ciruclations rules given the
constraints of the passed parameters.

We optionally expect item_type, library and patron_category as query
parameters and we return a list of relevant circulation rules pertaining
to that combination of requirements.

You can also add a list of `rules` as a query parameter to limit the
response to only the rules you are interested in for this combination.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 20 Tomás Cohen Arazi (tcohen) 2024-07-02 13:17:51 UTC
Created attachment 168368 [details] [review]
Bug 36641: Harmonize parameter names

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 21 Tomás Cohen Arazi (tcohen) 2024-07-02 13:17:54 UTC Comment hidden (obsolete)
Comment 22 Tomás Cohen Arazi (tcohen) 2024-07-02 13:21:11 UTC
Created attachment 168370 [details] [review]
Bug 36641: Make tests cover all code paths

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 23 Tomás Cohen Arazi (tcohen) 2024-07-02 13:22:58 UTC
Hi, this all looks great. I would go with a straight QA, but I want your opinion, Martin on wrapping it in a try/catch block, having the usual `$c->unhandled_exception($_);`.
Comment 24 Martin Renvoize (ashimema) 2024-07-02 13:45:25 UTC
(In reply to Tomás Cohen Arazi from comment #23)
> Hi, this all looks great. I would go with a straight QA, but I want your
> opinion, Martin on wrapping it in a try/catch block, having the usual
> `$c->unhandled_exception($_);`.

That's not a bad idea.. I did wonder it briefly myself.. I forget now why I didn't add it.... hmm

Thanks for all the work here btw, really appreciate it.
Comment 25 Martin Renvoize (ashimema) 2024-07-02 15:20:27 UTC
Created attachment 168378 [details] [review]
Bug 36641: (follow-up) Wrap in a try/catch block

This patch adds the missing try/catch and unhandled exception error
handler to the new list_effective_rules endpoint.
Comment 26 Martin Renvoize (ashimema) 2024-07-02 15:27:27 UTC
Created attachment 168379 [details] [review]
Bug 36641: (follow-up) Wrap in a try/catch block

This patch adds the missing try/catch and unhandled exception error
handler to the new list_effective_rules endpoint.
Comment 27 Kyle M Hall (khall) 2024-07-02 17:02:53 UTC
Created attachment 168395 [details] [review]
Bug 36641: Introduce render_invalid_parameter_value helper

This patch introduces a new 'render_invalid_parameter_value' helper
method that accepts 'path' and 'values' parameters to denote which field
has failed validation and where the end user can get valid options.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 28 Kyle M Hall (khall) 2024-07-02 17:03:00 UTC
Created attachment 168396 [details] [review]
Bug 36641: Add unit tests for /circulation_rules

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 29 Kyle M Hall (khall) 2024-07-02 17:03:03 UTC
Created attachment 168397 [details] [review]
Bug 36641: Add endpoint for fetching circulation rules

This patch adds an endpoint for fetching ciruclations rules given the
constraints of the passed parameters.

We optionally expect item_type, library and patron_category as query
parameters and we return a list of relevant circulation rules pertaining
to that combination of requirements.

You can also add a list of `rules` as a query parameter to limit the
response to only the rules you are interested in for this combination.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 30 Kyle M Hall (khall) 2024-07-02 17:03:06 UTC
Created attachment 168398 [details] [review]
Bug 36641: Harmonize parameter names

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 31 Kyle M Hall (khall) 2024-07-02 17:03:09 UTC
Created attachment 168399 [details] [review]
Bug 36641: Make tests cover all code paths

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 32 Kyle M Hall (khall) 2024-07-02 17:03:12 UTC
Created attachment 168400 [details] [review]
Bug 36641: (follow-up) Wrap in a try/catch block

This patch adds the missing try/catch and unhandled exception error
handler to the new list_effective_rules endpoint.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 33 Kyle M Hall (khall) 2024-07-02 17:47:12 UTC
Created attachment 168401 [details] [review]
Bug 36641: (QA Follow-up) Fix variable name
Comment 34 Martin Renvoize (ashimema) 2024-07-05 13:03:56 UTC
I'm re-considering this slightly
Comment 35 Martin Renvoize (ashimema) 2024-07-11 16:16:58 UTC
Created attachment 168841 [details] [review]
Bug 36641: (follow-up) Update to allow returning non-effective rules

This patch updates the existing /circulation_rules endpoint introduced
in this patchset to allow return of all rule sets rather than only the
effective set.

We continue to default to the effective set for the parameters passed
which will mean by default you will get an arrayref containing one
hashref entry with each rule kind as a key in that hashref.

However, if you add 'effective=false' as a query parameter, you will now
have an arrayref of all rule sets that match your passed criteria
including rules that may have fallen back to defaults in the effective
case.

WIP: Need to add support for * in branch, itemtype, patron category
query parameters to allow explicit return of default rules vs no filter
passed. (i.e. for the effective=false case where all rules for should be
returned when no filters are passed vs * passed to filter to just default
rules vs specifics passed for branch/item/patron filtering.
Comment 36 Martin Renvoize (ashimema) 2024-07-12 10:27:16 UTC
Created attachment 168874 [details] [review]
Bug 36641: Introduce render_invalid_parameter_value helper

This patch introduces a new 'render_invalid_parameter_value' helper
method that accepts 'path' and 'values' parameters to denote which field
has failed validation and where the end user can get valid options.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 37 Martin Renvoize (ashimema) 2024-07-12 10:27:19 UTC
Created attachment 168875 [details] [review]
Bug 36641: Add unit tests for /circulation_rules

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 38 Martin Renvoize (ashimema) 2024-07-12 10:27:21 UTC
Created attachment 168876 [details] [review]
Bug 36641: Add endpoint for fetching circulation rules

This patch adds an endpoint for fetching ciruclations rules given the
constraints of the passed parameters.

We optionally expect item_type, library and patron_category as query
parameters and we return a list of relevant circulation rules pertaining
to that combination of requirements.

You can also add a list of `rules` as a query parameter to limit the
response to only the rules you are interested in for this combination.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 39 Martin Renvoize (ashimema) 2024-07-12 10:27:24 UTC
Created attachment 168877 [details] [review]
Bug 36641: Harmonize parameter names

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 40 Martin Renvoize (ashimema) 2024-07-12 10:27:27 UTC
Created attachment 168878 [details] [review]
Bug 36641: Make tests cover all code paths

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 41 Martin Renvoize (ashimema) 2024-07-12 10:27:30 UTC
Created attachment 168879 [details] [review]
Bug 36641: (follow-up) Wrap in a try/catch block

This patch adds the missing try/catch and unhandled exception error
handler to the new list_effective_rules endpoint.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 42 Martin Renvoize (ashimema) 2024-07-12 10:27:32 UTC
Created attachment 168880 [details] [review]
Bug 36641: (QA Follow-up) Fix variable name
Comment 43 Martin Renvoize (ashimema) 2024-07-12 10:27:35 UTC
Created attachment 168881 [details] [review]
Bug 36641: (follow-up) Update to allow returning non-effective rules

This patch updates the existing /circulation_rules endpoint introduced
in this patchset to allow return of all rule sets rather than only the
effective set.

We continue to default to the effective set for the parameters passed
which will mean by default you will get an arrayref containing one
hashref entry with each rule kind as a key in that hashref.

However, if you add 'effective=false' as a query parameter, you will now
have an arrayref of all rule sets that match your passed criteria
including rules that may have fallen back to defaults in the effective
case.

WIP: Need to add support for * in branch, itemtype, patron category
query parameters to allow explicit return of default rules vs no filter
passed. (i.e. for the effective=false case where all rules for should be
returned when no filters are passed vs * passed to filter to just default
rules vs specifics passed for branch/item/patron filtering.
Comment 44 Martin Renvoize (ashimema) 2024-07-12 10:27:38 UTC
Created attachment 168882 [details] [review]
Bug 36641: Optionally filter to default rules using *
Comment 45 Martin Renvoize (ashimema) 2024-07-12 10:27:41 UTC
Created attachment 168883 [details] [review]
Bug 36641: Return '*' for 'default rule'
Comment 46 Martin Renvoize (ashimema) 2024-07-12 10:27:43 UTC
Created attachment 168884 [details] [review]
Bug 36641: Unit tests for effective=0
Comment 47 Martin Renvoize (ashimema) 2024-07-12 10:31:38 UTC
The scope here grew a bit as I tried to think about the future and how we might implement the Create, Update and Delete parts of the API.

We continue to default to returning 'Effective' rule sets, however, I've now introduced an 'effective' query parameter which can be set to '0' to get back all branchcode, category, itemtype combination rule sets.

The response differs slightly between effective and all responses.. both are now an arrayref of hashrefs but the effective rules only returns defined rules in the hash whereas the all response hashes will always contain all keys, defined or otherwise.
Comment 48 Martin Renvoize (ashimema) 2024-07-12 14:16:45 UTC
Created attachment 168905 [details] [review]
Bug 36641: Introduce render_invalid_parameter_value helper

This patch introduces a new 'render_invalid_parameter_value' helper
method that accepts 'path' and 'values' parameters to denote which field
has failed validation and where the end user can get valid options.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 49 Martin Renvoize (ashimema) 2024-07-12 14:16:48 UTC
Created attachment 168906 [details] [review]
Bug 36641: Add unit tests for /circulation_rules

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 50 Martin Renvoize (ashimema) 2024-07-12 14:16:50 UTC
Created attachment 168907 [details] [review]
Bug 36641: Add endpoint for fetching circulation rules

This patch adds an endpoint for fetching ciruclations rules given the
constraints of the passed parameters.

We optionally expect item_type, library and patron_category as query
parameters and we return a list of relevant circulation rules pertaining
to that combination of requirements.

You can also add a list of `rules` as a query parameter to limit the
response to only the rules you are interested in for this combination.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 51 Martin Renvoize (ashimema) 2024-07-12 14:16:53 UTC
Created attachment 168908 [details] [review]
Bug 36641: Harmonize parameter names

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 52 Martin Renvoize (ashimema) 2024-07-12 14:16:56 UTC
Created attachment 168909 [details] [review]
Bug 36641: Make tests cover all code paths

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 53 Martin Renvoize (ashimema) 2024-07-12 14:16:59 UTC
Created attachment 168910 [details] [review]
Bug 36641: (follow-up) Wrap in a try/catch block

This patch adds the missing try/catch and unhandled exception error
handler to the new list_effective_rules endpoint.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 54 Martin Renvoize (ashimema) 2024-07-12 14:17:01 UTC
Created attachment 168911 [details] [review]
Bug 36641: (QA Follow-up) Fix variable name
Comment 55 Martin Renvoize (ashimema) 2024-07-12 14:17:04 UTC
Created attachment 168912 [details] [review]
Bug 36641: (follow-up) Update to allow returning non-effective rules

This patch updates the existing /circulation_rules endpoint introduced
in this patchset to allow return of all rule sets rather than only the
effective set.

We continue to default to the effective set for the parameters passed
which will mean by default you will get an arrayref containing one
hashref entry with each rule kind as a key in that hashref.

However, if you add 'effective=false' as a query parameter, you will now
have an arrayref of all rule sets that match your passed criteria
including rules that may have fallen back to defaults in the effective
case.

WIP: Need to add support for * in branch, itemtype, patron category
query parameters to allow explicit return of default rules vs no filter
passed. (i.e. for the effective=false case where all rules for should be
returned when no filters are passed vs * passed to filter to just default
rules vs specifics passed for branch/item/patron filtering.
Comment 56 Martin Renvoize (ashimema) 2024-07-12 14:17:07 UTC
Created attachment 168913 [details] [review]
Bug 36641: Optionally filter to default rules using *
Comment 57 Martin Renvoize (ashimema) 2024-07-12 14:17:10 UTC
Created attachment 168914 [details] [review]
Bug 36641: Return '*' for 'default rule'
Comment 58 Martin Renvoize (ashimema) 2024-07-12 14:17:12 UTC
Created attachment 168915 [details] [review]
Bug 36641: Ensure we return all keys

When using the effective route, we were missing some rule kinds if a
rule hadn't yet been defined for them. We should instead return the rule
as undefined.
Comment 59 Martin Renvoize (ashimema) 2024-07-12 14:17:15 UTC
Created attachment 168916 [details] [review]
Bug 36641: Unit tests for effective=0
Comment 60 Martin Renvoize (ashimema) 2024-07-13 12:14:16 UTC
Created attachment 168926 [details] [review]
Bug 36641: Introduce render_invalid_parameter_value helper

This patch introduces a new 'render_invalid_parameter_value' helper
method that accepts 'path' and 'values' parameters to denote which field
has failed validation and where the end user can get valid options.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 61 Martin Renvoize (ashimema) 2024-07-13 12:14:20 UTC
Created attachment 168927 [details] [review]
Bug 36641: Add unit tests for /circulation_rules

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 62 Martin Renvoize (ashimema) 2024-07-13 12:14:23 UTC
Created attachment 168928 [details] [review]
Bug 36641: Add endpoint for fetching circulation rules

This patch adds an endpoint for fetching ciruclations rules given the
constraints of the passed parameters.

We optionally expect item_type, library and patron_category as query
parameters and we return a list of relevant circulation rules pertaining
to that combination of requirements.

You can also add a list of `rules` as a query parameter to limit the
response to only the rules you are interested in for this combination.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 63 Martin Renvoize (ashimema) 2024-07-13 12:14:26 UTC
Created attachment 168929 [details] [review]
Bug 36641: Harmonize parameter names

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 64 Martin Renvoize (ashimema) 2024-07-13 12:14:29 UTC
Created attachment 168930 [details] [review]
Bug 36641: Make tests cover all code paths

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 65 Martin Renvoize (ashimema) 2024-07-13 12:14:32 UTC
Created attachment 168931 [details] [review]
Bug 36641: (follow-up) Wrap in a try/catch block

This patch adds the missing try/catch and unhandled exception error
handler to the new list_effective_rules endpoint.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 66 Martin Renvoize (ashimema) 2024-07-13 12:14:35 UTC
Created attachment 168932 [details] [review]
Bug 36641: (QA Follow-up) Fix variable name
Comment 67 Martin Renvoize (ashimema) 2024-07-13 12:14:38 UTC
Created attachment 168933 [details] [review]
Bug 36641: (follow-up) Update to allow returning non-effective rules

This patch updates the existing /circulation_rules endpoint introduced
in this patchset to allow return of all rule sets rather than only the
effective set.

We continue to default to the effective set for the parameters passed
which will mean by default you will get an arrayref containing one
hashref entry with each rule kind as a key in that hashref.

However, if you add 'effective=false' as a query parameter, you will now
have an arrayref of all rule sets that match your passed criteria
including rules that may have fallen back to defaults in the effective
case.

WIP: Need to add support for * in branch, itemtype, patron category
query parameters to allow explicit return of default rules vs no filter
passed. (i.e. for the effective=false case where all rules for should be
returned when no filters are passed vs * passed to filter to just default
rules vs specifics passed for branch/item/patron filtering.
Comment 68 Martin Renvoize (ashimema) 2024-07-13 12:14:41 UTC
Created attachment 168934 [details] [review]
Bug 36641: Optionally filter to default rules using *
Comment 69 Martin Renvoize (ashimema) 2024-07-13 12:14:44 UTC
Created attachment 168935 [details] [review]
Bug 36641: Return '*' for 'default rule'
Comment 70 Martin Renvoize (ashimema) 2024-07-13 12:14:47 UTC
Created attachment 168936 [details] [review]
Bug 36641: Ensure we return all keys

When using the effective route, we were missing some rule kinds if a
rule hadn't yet been defined for them. We should instead return the rule
as undefined.
Comment 71 Martin Renvoize (ashimema) 2024-07-13 12:14:49 UTC
Created attachment 168937 [details] [review]
Bug 36641: Unit tests for effective=0
Comment 72 Martin Renvoize (ashimema) 2024-07-13 12:14:52 UTC
Created attachment 168938 [details] [review]
Bug 36641: (follow-up) Ensure limited return works

We have a ?rules=a,b parameter available for specifying a subset of
rules in the set to return. This patch adds a test to confirm that works
(and fixes the controller and specification as the test proved it wasn't
working)
Comment 73 Martin Renvoize (ashimema) 2024-07-14 05:44:31 UTC
Created attachment 168940 [details] [review]
Bug 36641: (follow-up) Ensure limited return works

We have a ?rules=a,b parameter available for specifying a subset of
rules in the set to return. This patch adds a test to confirm that works
(and fixes the controller and specification as the test proved it wasn't
working)
Comment 74 Tomás Cohen Arazi (tcohen) 2024-07-15 19:00:37 UTC
Created attachment 169013 [details] [review]
Bug 36641: Introduce render_invalid_parameter_value helper

This patch introduces a new 'render_invalid_parameter_value' helper
method that accepts 'path' and 'values' parameters to denote which field
has failed validation and where the end user can get valid options.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 75 Tomás Cohen Arazi (tcohen) 2024-07-15 19:00:40 UTC
Created attachment 169014 [details] [review]
Bug 36641: Add unit tests for /circulation_rules

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 76 Tomás Cohen Arazi (tcohen) 2024-07-15 19:00:44 UTC
Created attachment 169015 [details] [review]
Bug 36641: Add endpoint for fetching circulation rules

This patch adds an endpoint for fetching ciruclations rules given the
constraints of the passed parameters.

We optionally expect item_type, library and patron_category as query
parameters and we return a list of relevant circulation rules pertaining
to that combination of requirements.

You can also add a list of `rules` as a query parameter to limit the
response to only the rules you are interested in for this combination.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 77 Tomás Cohen Arazi (tcohen) 2024-07-15 19:00:47 UTC
Created attachment 169016 [details] [review]
Bug 36641: Harmonize parameter names

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 78 Tomás Cohen Arazi (tcohen) 2024-07-15 19:00:49 UTC
Created attachment 169017 [details] [review]
Bug 36641: Make tests cover all code paths

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 79 Tomás Cohen Arazi (tcohen) 2024-07-15 19:00:52 UTC
Created attachment 169018 [details] [review]
Bug 36641: (follow-up) Wrap in a try/catch block

This patch adds the missing try/catch and unhandled exception error
handler to the new list_effective_rules endpoint.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 80 Tomás Cohen Arazi (tcohen) 2024-07-15 19:00:55 UTC
Created attachment 169019 [details] [review]
Bug 36641: (QA follow-up) Fix variable name

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 81 Tomás Cohen Arazi (tcohen) 2024-07-15 19:00:59 UTC
Created attachment 169020 [details] [review]
Bug 36641: (follow-up) Update to allow returning non-effective rules

This patch updates the existing /circulation_rules endpoint introduced
in this patchset to allow return of all rule sets rather than only the
effective set.

We continue to default to the effective set for the parameters passed
which will mean by default you will get an arrayref containing one
hashref entry with each rule kind as a key in that hashref.

However, if you add 'effective=false' as a query parameter, you will now
have an arrayref of all rule sets that match your passed criteria
including rules that may have fallen back to defaults in the effective
case.

WIP: Need to add support for * in branch, itemtype, patron category
query parameters to allow explicit return of default rules vs no filter
passed. (i.e. for the effective=false case where all rules for should be
returned when no filters are passed vs * passed to filter to just default
rules vs specifics passed for branch/item/patron filtering.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 82 Tomás Cohen Arazi (tcohen) 2024-07-15 19:01:02 UTC
Created attachment 169021 [details] [review]
Bug 36641: Optionally filter to default rules using *

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 83 Tomás Cohen Arazi (tcohen) 2024-07-15 19:01:05 UTC
Created attachment 169022 [details] [review]
Bug 36641: Return '*' for 'default rule'

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 84 Tomás Cohen Arazi (tcohen) 2024-07-15 19:01:08 UTC
Created attachment 169023 [details] [review]
Bug 36641: Ensure we return all keys

When using the effective route, we were missing some rule kinds if a
rule hadn't yet been defined for them. We should instead return the rule
as undefined.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 85 Tomás Cohen Arazi (tcohen) 2024-07-15 19:01:11 UTC
Created attachment 169024 [details] [review]
Bug 36641: Unit tests for effective=0

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 86 Tomás Cohen Arazi (tcohen) 2024-07-15 19:01:15 UTC
Created attachment 169025 [details] [review]
Bug 36641: (follow-up) Ensure limited return works

We have a ?rules=a,b parameter available for specifying a subset of
rules in the set to return. This patch adds a test to confirm that works
(and fixes the controller and specification as the test proved it wasn't
working)

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 87 Tomás Cohen Arazi (tcohen) 2024-07-15 19:01:18 UTC
Created attachment 169026 [details] [review]
Bug 36641: Fix tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 88 Martin Renvoize (ashimema) 2024-07-16 05:46:48 UTC
﷐[U+1F389]﷑ Thanks Tomas!
Comment 89 Katrin Fischer 2024-07-18 16:27:30 UTC
Pushed for 24.11!

Well done everyone, thank you!