Bug 27204 - SIP patron information request with fee line items returns incorrect data
Summary: SIP patron information request with fee line items returns incorrect data
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: SIP2 (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Kyle M Hall
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-12-11 12:08 UTC by Kyle M Hall
Modified: 2021-12-13 21:08 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
21.05.00,20.11.03,20.05.09


Attachments
Bug 27204: Add unit tests for fine_items (3.42 KB, patch)
2020-12-11 12:16 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 27204: Fix end boundary index, never return results for non-existant accountlines (1.84 KB, patch)
2020-12-11 12:16 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 27204: Add unit tests for fine_items (3.47 KB, patch)
2021-01-26 16:26 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27204: Fix end boundary index, never return results for non-existant accountlines (1.89 KB, patch)
2021-01-26 16:26 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27204: Add unit tests for fine_items (3.53 KB, patch)
2021-01-31 14:40 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 27204: Fix end boundary index, never return results for non-existant accountlines (1.95 KB, patch)
2021-01-31 14:40 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2020-12-11 12:08:26 UTC
There are two primary issues I've identified with requesting line items for fees via SIP:
1) The end boundary is incorrect. For example, if send a request with a BP ( starting item) of 1, and a BQ (end item) of 1, I should get just the first item. Instead I will get two items

2) Our SIP server does not check bounds. For example, if I have 3 fines, but I send a BP of 1 and a BQ of 5, I will get back 5 AVs, two of them being "empty" because the patron only has 3 accountlines!
Comment 1 Kyle M Hall 2020-12-11 12:16:42 UTC
Created attachment 114338 [details] [review]
Bug 27204: Add unit tests for fine_items
Comment 2 Kyle M Hall 2020-12-11 12:16:51 UTC
Created attachment 114339 [details] [review]
Bug 27204: Fix end boundary index, never return results for non-existant accountlines

There are two primary issues I've identified with requesting line items
for fees via SIP:

1) The end boundary is incorrect. For example, if send a request with a
BP ( starting item) of 1, and a BQ (end item) of 1, I should get just
the first item. Instead I will get two items

2) Our SIP server does not check bounds. For example, if I have 3 fines,
but I send a BP of 1 and a BQ of 5, I will get back 5 AVs, two of
them being "empty" because the patron only has 3 accountlines!

Test Plan:
1) Apply the unit test patch
2) prove t/db_dependent/SIP/Patron.t
3) Note the failures
4) Apply the second patch
5) prove t/db_dependent/SIP/Patron.t
6) All tests should pass!
Comment 3 Martin Renvoize 2021-01-26 16:26:44 UTC
Created attachment 115824 [details] [review]
Bug 27204: Add unit tests for fine_items

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 4 Martin Renvoize 2021-01-26 16:26:47 UTC
Created attachment 115825 [details] [review]
Bug 27204: Fix end boundary index, never return results for non-existant accountlines

There are two primary issues I've identified with requesting line items
for fees via SIP:

1) The end boundary is incorrect. For example, if send a request with a
BP ( starting item) of 1, and a BQ (end item) of 1, I should get just
the first item. Instead I will get two items

2) Our SIP server does not check bounds. For example, if I have 3 fines,
but I send a BP of 1 and a BQ of 5, I will get back 5 AVs, two of
them being "empty" because the patron only has 3 accountlines!

Test Plan:
1) Apply the unit test patch
2) prove t/db_dependent/SIP/Patron.t
3) Note the failures
4) Apply the second patch
5) prove t/db_dependent/SIP/Patron.t
6) All tests should pass!

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 5 Martin Renvoize 2021-01-26 16:27:34 UTC
Nice catch, 

Signing off
Comment 6 Katrin Fischer 2021-01-31 14:40:55 UTC
Created attachment 116119 [details] [review]
Bug 27204: Add unit tests for fine_items

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 7 Katrin Fischer 2021-01-31 14:40:59 UTC
Created attachment 116120 [details] [review]
Bug 27204: Fix end boundary index, never return results for non-existant accountlines

There are two primary issues I've identified with requesting line items
for fees via SIP:

1) The end boundary is incorrect. For example, if send a request with a
BP ( starting item) of 1, and a BQ (end item) of 1, I should get just
the first item. Instead I will get two items

2) Our SIP server does not check bounds. For example, if I have 3 fines,
but I send a BP of 1 and a BQ of 5, I will get back 5 AVs, two of
them being "empty" because the patron only has 3 accountlines!

Test Plan:
1) Apply the unit test patch
2) prove t/db_dependent/SIP/Patron.t
3) Note the failures
4) Apply the second patch
5) prove t/db_dependent/SIP/Patron.t
6) All tests should pass!

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 8 Katrin Fischer 2021-01-31 14:41:39 UTC
Test makes sense and passes tests. For functionality testing I trust Kyle and Martin here, they are the SIP experts.
Comment 9 Jonathan Druart 2021-02-01 16:26:32 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 10 Fridolin Somers 2021-02-05 15:42:07 UTC
Pushed to 20.11.x for 20.11.03
Comment 11 Andrew Fuerste-Henry 2021-02-09 12:44:54 UTC
Pushed to 20.05.x for 20.05.09
Comment 12 Victor Grousset/tuxayo 2021-02-11 01:47:44 UTC
Not backported to oldoldstable (19.11.x). Feel free to ask if it's needed.