Bug 19841 - AddMember should raise an exception if categorycode is invalid
Summary: AddMember should raise an exception if categorycode is invalid
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Josef Moravec
URL:
Keywords:
Depends on:
Blocks: 16330
  Show dependency treegraph
 
Reported: 2017-12-19 19:42 UTC by Tomás Cohen Arazi
Modified: 2018-12-03 20:05 UTC (History)
4 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:


Attachments
Bug 19841: Unit tests (1.41 KB, patch)
2017-12-20 19:25 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 19841: Make AddMember raise an exception if categorycode is invalid (2.68 KB, patch)
2017-12-20 19:25 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 19841: Make AddMember raise an exception if categorycode is invalid (2.74 KB, patch)
2017-12-20 19:35 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 19841: Unit tests (1.68 KB, patch)
2017-12-21 13:11 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 19841: Make AddMember raise an exception if categorycode is invalid (2.74 KB, patch)
2017-12-21 13:11 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 19841: Make AddMember raise an exception if categorycode is invalid (2.79 KB, patch)
2017-12-22 21:25 UTC, Dominic Pichette
Details | Diff | Splinter Review
Bug 19841: Unit tests (1.72 KB, patch)
2017-12-23 08:01 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 19841: Make AddMember raise an exception if categorycode is invalid (2.84 KB, patch)
2017-12-23 08:01 UTC, Josef Moravec
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 2017-12-19 19:42:26 UTC
This should be dealt with in a move to Koha::Patron. But in the meantime, we need to handle this more gracefuly.

The thrown exception should be kept on moving to Koha::Patron
Comment 1 Tomás Cohen Arazi 2017-12-20 19:25:07 UTC Comment hidden (obsolete)
Comment 2 Tomás Cohen Arazi 2017-12-20 19:25:40 UTC Comment hidden (obsolete)
Comment 3 Tomás Cohen Arazi 2017-12-20 19:29:25 UTC
Note: 
- If the code is not catching problems, it is already failing, and it will now fail in a different way. But no behaviour change.
- If the code is catching problems (either Try::Tiny or just eval) the thing to test, is if the code handling the problem (in AddReturn) is expectig DBIC specific exceptions, die, etc. I grepped the code and didn't find places in which AddReturn is eval-ed and THEN the failure message is verified/inspected.

This means the behaviour is not changed by this patches. But another set of eyes is welcome.
Comment 4 Tomás Cohen Arazi 2017-12-20 19:35:35 UTC Comment hidden (obsolete)
Comment 5 Tomás Cohen Arazi 2017-12-21 13:11:15 UTC
Created attachment 70010 [details] [review]
Bug 19841: Unit tests

This patch introduces unit tests for the new behaviour of AddMember
(raising an exception if the passed categorycode is not valid.

 To test:
 - Apply this patch
 - Run:
   $ kshell
  k$ prove t/db_dependent/Members.t
=> FAIL: It should fail because the feature is still not implemented.
Comment 6 Tomás Cohen Arazi 2017-12-21 13:11:22 UTC
Created attachment 70011 [details] [review]
Bug 19841: Make AddMember raise an exception if categorycode is invalid

This patch makes C4::Members::AddMember raise a Koha::Exceptions::BadParameter
exception. The current behaviour is to just explode. Code calling
AddReturn should work as expected, unless they are looking for a
specific failure message.

It is just explodes in a way we know what happened, in the case the
categorycode is invalid.

To test:
- Apply the Unit tests patch
- Run:
  $ kshell
 k$ prove t/db_dependent/Members.t
=> FAIL: Tests fail because AddReturn doesn't raise an exception
- Apply this patch
- Run:
  $ kshell
 k$ prove t/db_dependent/Members.t
=> SUCCESS: Tests pass!
- Sign off :-D
Comment 7 Dominic Pichette 2017-12-22 21:25:09 UTC
Created attachment 70138 [details] [review]
Bug 19841: Make AddMember raise an exception if categorycode is invalid

This patch makes C4::Members::AddMember raise a Koha::Exceptions::BadParameter
exception. The current behaviour is to just explode. Code calling
AddReturn should work as expected, unless they are looking for a
specific failure message.

It is just explodes in a way we know what happened, in the case the
categorycode is invalid.

To test:
- Apply the Unit tests patch
- Run:
  $ kshell
 k$ prove t/db_dependent/Members.t
=> FAIL: Tests fail because AddReturn doesn't raise an exception
- Apply this patch
- Run:
  $ kshell
 k$ prove t/db_dependent/Members.t
=> SUCCESS: Tests pass!
- Sign off :-D

Signed-off-by: dom <dominic.pichette@inlibro.com>
Comment 8 Josef Moravec 2017-12-23 08:01:04 UTC
Created attachment 70147 [details] [review]
Bug 19841: Unit tests

This patch introduces unit tests for the new behaviour of AddMember
(raising an exception if the passed categorycode is not valid.

 To test:
 - Apply this patch
 - Run:
   $ kshell
  k$ prove t/db_dependent/Members.t
=> FAIL: It should fail because the feature is still not implemented.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 9 Josef Moravec 2017-12-23 08:01:11 UTC
Created attachment 70148 [details] [review]
Bug 19841: Make AddMember raise an exception if categorycode is invalid

This patch makes C4::Members::AddMember raise a Koha::Exceptions::BadParameter
exception. The current behaviour is to just explode. Code calling
AddReturn should work as expected, unless they are looking for a
specific failure message.

It is just explodes in a way we know what happened, in the case the
categorycode is invalid.

To test:
- Apply the Unit tests patch
- Run:
  $ kshell
 k$ prove t/db_dependent/Members.t
=> FAIL: Tests fail because AddReturn doesn't raise an exception
- Apply this patch
- Run:
  $ kshell
 k$ prove t/db_dependent/Members.t
=> SUCCESS: Tests pass!
- Sign off :-D

Signed-off-by: dom <dominic.pichette@inlibro.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 10 Jonathan Druart 2017-12-26 15:46:21 UTC
(In reply to Josef Moravec from comment #9)
> Signed-off-by: dom <dominic.pichette@inlibro.com>

Dominic, please fix you git alias!
Comment 11 Jonathan Druart 2017-12-26 15:53:57 UTC
Pushed to master for 18.05, thanks to everybody involved!
Comment 12 Nick Clemens 2018-01-16 12:18:04 UTC
Enhancement, skipping for 17.11.x.
Awesome work everybody!