Bug 19841

Summary: AddMember should raise an exception if categorycode is invalid
Product: Koha Reporter: Tomás Cohen Arazi <tomascohen>
Component: Architecture, internals, and plumbingAssignee: Tomás Cohen Arazi <tomascohen>
Status: CLOSED FIXED QA Contact: Josef Moravec <josef.moravec>
Severity: enhancement    
Priority: P5 - low CC: dominic.pichette, jonathan.druart, josef.moravec, nick
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Small patch
Who signed the patch off: Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 16330    
Attachments: Bug 19841: Unit tests
Bug 19841: Make AddMember raise an exception if categorycode is invalid
Bug 19841: Make AddMember raise an exception if categorycode is invalid
Bug 19841: Unit tests
Bug 19841: Make AddMember raise an exception if categorycode is invalid
Bug 19841: Make AddMember raise an exception if categorycode is invalid
Bug 19841: Unit tests
Bug 19841: Make AddMember raise an exception if categorycode is invalid

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!