Bug 28220

Summary: Exception not caught when importing patrons
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: ToolsAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Martin Renvoize <martin.renvoize>
Severity: major    
Priority: P5 - low CC: andrewfh, fridolin.somers, martin.renvoize, nick, tomascohen
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
21.05.00
Bug Depends on: 27833, 27858, 28293    
Bug Blocks: 28616    
Attachments: Bug 28220: Deal with merge
Bug 28220: Deal with merge
Bug 28220: Add tests to import
Bug 28220: prevent patron to be created if attributes not stored
Bug 28220: Handle InvalidType
Bug 28220: Handle NonRepeatable
Bug 28220: Add test for updating an existing patron
Bug 28220: Add more tests
Bug 28220: UI changes
Bug 28220: Deal with merge
Bug 28220: Add tests to import
Bug 28220: prevent patron to be created if attributes not stored
Bug 28220: Handle InvalidType
Bug 28220: Handle NonRepeatable
Bug 28220: Add test for updating an existing patron
Bug 28220: Add more tests
Bug 28220: UI changes
Bug 28220: Deal with merge
Bug 28220: Add tests to import
Bug 28220: prevent patron to be created if attributes not stored
Bug 28220: Handle InvalidType
Bug 28220: Handle NonRepeatable
Bug 28220: Add test for updating an existing patron
Bug 28220: Add more tests
Bug 28220: UI changes

Description Jonathan Druart 2021-04-26 09:38:56 UTC
Tried to use an invalid attribute type. type=test1 at /usr/share/perl5/Exception/Class/Base.pm line 88
Comment 1 Jonathan Druart 2021-04-26 09:39:37 UTC
What would be the expected behaviour?
Comment 2 Tomás Cohen Arazi 2021-04-26 13:40:14 UTC
I think the exception should be caught, and the behavior the same as with any patron with invalid (non-extended attribute) data.
Comment 3 Jonathan Druart 2021-05-05 09:38:40 UTC
(In reply to Tomás Cohen Arazi from comment #2)
> I think the exception should be caught, and the behavior the same as with
> any patron with invalid (non-extended attribute) data.

Which means?

When a new patron is imported with an invalid patron attribute we explode. When the patron's attributes are overwritten during an import, we silently ignore them.

Should we, 1. explode (skip the whole patron) in both case or 2. do the job but ignore the invalid attributes?
Comment 4 Tomás Cohen Arazi 2021-05-05 11:22:53 UTC
(In reply to Jonathan Druart from comment #3)
> (In reply to Tomás Cohen Arazi from comment #2)
> > I think the exception should be caught, and the behavior the same as with
> > any patron with invalid (non-extended attribute) data.
> 
> Which means?
> 
> When a new patron is imported with an invalid patron attribute we explode.
> When the patron's attributes are overwritten during an import, we silently
> ignore them.
> 
> Should we, 1. explode (skip the whole patron) in both case or 2. do the job
> but ignore the invalid attributes?

My feeling is we've been too loose on how we handle this. I don't think we should go full 'blow it all' for bad attributes, but we should at least report back.

I have plans to work on enhancing patron import on the next cycle, and I will add more fine grained control on what's going on, there.
Comment 5 Tomás Cohen Arazi 2021-05-05 11:28:41 UTC
(In reply to Tomás Cohen Arazi from comment #4)
> My feeling is we've been too loose on how we handle this.

This sentence tried to say that I think attributes should be treated the same as  any other borrowers.* column. Specially those that have constraints (unique, not repeatable, mandatory). We are being more strict about this in the code, and it will start hurting people with bad data, so we shouldn't allow bad data in.

This needs some bigger changes in the import workflow, that I mentioned I plan to work on soon. So in the meantime, I would prefer to:
- Skip the patron with good error reporting, or
- At least skip the bad attributes BUT provide good reporting
Comment 6 Jonathan Druart 2021-05-05 12:20:13 UTC
What about:
1. unique not being unique?
2. non-rep
Comment 7 Tomás Cohen Arazi 2021-05-05 12:34:40 UTC
(In reply to Jonathan Druart from comment #6)
> What about:
> 1. unique not being unique?
> 2. non-rep

"I think attributes should be treated the same as any other borrowers.* column."

I think patron import should skip those patrons totally, and report back.
Comment 8 Jonathan Druart 2021-05-05 15:37:48 UTC
Created attachment 120549 [details] [review]
Bug 28220: Deal with merge
Comment 9 Tomás Cohen Arazi 2021-05-05 20:53:55 UTC
Your patch changes the merge method so it raises an exception. That would be a good start.

The caller doesn't wrap things in a try/catch block yet, but I'm wondering:

shouldn't we do it all inside a transaction for each patron?
Comment 10 Jonathan Druart 2021-05-06 10:48:56 UTC
Created attachment 120607 [details] [review]
Bug 28220: Deal with merge
Comment 11 Jonathan Druart 2021-05-06 10:49:00 UTC
Created attachment 120608 [details] [review]
Bug 28220: Add tests to import

failure expected, we need to add a transaction
Comment 12 Jonathan Druart 2021-05-06 10:49:04 UTC
Created attachment 120609 [details] [review]
Bug 28220: prevent patron to be created if attributes not stored

Using a transaction
Comment 13 Jonathan Druart 2021-05-06 10:49:08 UTC
Created attachment 120610 [details] [review]
Bug 28220: Handle InvalidType
Comment 14 Jonathan Druart 2021-05-06 10:49:12 UTC
Created attachment 120611 [details] [review]
Bug 28220: Handle NonRepeatable
Comment 15 Jonathan Druart 2021-05-06 10:49:16 UTC
Created attachment 120612 [details] [review]
Bug 28220: Add test for updating an existing patron
Comment 16 Jonathan Druart 2021-05-06 10:49:21 UTC
Created attachment 120613 [details] [review]
Bug 28220: Add more tests

* Add the transaction when a patron is created
* The changes in merge_and_replace_with prevent the creation of the
patron and so the attributes if several non-repeatable attributes are
passed
Comment 17 Jonathan Druart 2021-05-06 10:49:25 UTC
Created attachment 120614 [details] [review]
Bug 28220: UI changes
Comment 18 Jonathan Druart 2021-05-06 10:53:38 UTC
Please try this patch, I think it covers a lot of cases.

Test plan (but be creative!):
Use the import patron tool to
1. Create a new patron with:
  * 1 new normal (non-repeatable, not unique) attribute
  * several repeatable attribute
  * unique with unique value
=> all good
2. Create a new patron with:
  * several new-repeatable attribute
=> boom
3. Create a new patron with:
  * unique with non-unique value
=> boom
4. Create a new patron with:
  * an invalid patron attribute type code (does not exist)

And do the same for an existing patron (with and without the "preserve patron attributes" option).

If an error happened the patron will not be created/modified (attributes and other infos)
Comment 19 Jonathan Druart 2021-05-06 10:55:17 UTC
And, I should add, don't be afraid by the number of patches or their sizes, most of the changes is adding new unit tests and indentation (for transaction and exception handling).
Comment 20 Nick Clemens 2021-05-10 13:27:57 UTC
Created attachment 120783 [details] [review]
Bug 28220: Deal with merge

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 21 Nick Clemens 2021-05-10 13:28:01 UTC
Created attachment 120784 [details] [review]
Bug 28220: Add tests to import

failure expected, we need to add a transaction

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 22 Nick Clemens 2021-05-10 13:28:05 UTC
Created attachment 120785 [details] [review]
Bug 28220: prevent patron to be created if attributes not stored

Using a transaction

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 23 Nick Clemens 2021-05-10 13:28:09 UTC
Created attachment 120786 [details] [review]
Bug 28220: Handle InvalidType

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 24 Nick Clemens 2021-05-10 13:28:13 UTC
Created attachment 120787 [details] [review]
Bug 28220: Handle NonRepeatable

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 25 Nick Clemens 2021-05-10 13:28:17 UTC
Created attachment 120788 [details] [review]
Bug 28220: Add test for updating an existing patron

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 26 Nick Clemens 2021-05-10 13:28:21 UTC
Created attachment 120789 [details] [review]
Bug 28220: Add more tests

* Add the transaction when a patron is created
* The changes in merge_and_replace_with prevent the creation of the
patron and so the attributes if several non-repeatable attributes are
passed

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 27 Nick Clemens 2021-05-10 13:28:25 UTC
Created attachment 120790 [details] [review]
Bug 28220: UI changes

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 28 Katrin Fischer 2021-05-10 21:37:58 UTC
Nick, as you signed this one off, could you please also take a look at signing off the dependency bug as well?
Comment 29 Martin Renvoize 2021-05-12 15:57:30 UTC
Created attachment 120885 [details] [review]
Bug 28220: Deal with merge

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 30 Martin Renvoize 2021-05-12 15:57:35 UTC
Created attachment 120886 [details] [review]
Bug 28220: Add tests to import

failure expected, we need to add a transaction

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 31 Martin Renvoize 2021-05-12 15:57:39 UTC
Created attachment 120887 [details] [review]
Bug 28220: prevent patron to be created if attributes not stored

Using a transaction

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 32 Martin Renvoize 2021-05-12 15:57:43 UTC
Created attachment 120888 [details] [review]
Bug 28220: Handle InvalidType

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 33 Martin Renvoize 2021-05-12 15:57:47 UTC
Created attachment 120889 [details] [review]
Bug 28220: Handle NonRepeatable

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 34 Martin Renvoize 2021-05-12 15:57:52 UTC
Created attachment 120890 [details] [review]
Bug 28220: Add test for updating an existing patron

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 35 Martin Renvoize 2021-05-12 15:57:56 UTC
Created attachment 120891 [details] [review]
Bug 28220: Add more tests

* Add the transaction when a patron is created
* The changes in merge_and_replace_with prevent the creation of the
patron and so the attributes if several non-repeatable attributes are
passed

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 36 Martin Renvoize 2021-05-12 15:58:01 UTC
Created attachment 120892 [details] [review]
Bug 28220: UI changes

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 37 Martin Renvoize 2021-05-12 15:59:47 UTC
Wow, that took a while to work through.. really great test coverage here helps with confidence.

I've tested it with a few different sets of data and it's all working as expected.  Great job Jonathan.

Passing QA
Comment 38 Jonathan Druart 2021-05-17 14:19:54 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 39 Fridolin Somers 2021-05-18 10:12:05 UTC
Does not apply on 20.11.x.
Fails because t/db_dependent/Koha/Patron/Attribute.t does not exists.
This file as been added by Bug 27858. Is it a dependance ?
Comment 40 Jonathan Druart 2021-08-03 07:46:37 UTC
(In reply to Fridolin Somers from comment #39)
> Does not apply on 20.11.x.
> Fails because t/db_dependent/Koha/Patron/Attribute.t does not exists.
> This file as been added by Bug 27858. Is it a dependance ?

Yes, you certainly need bug 27858 and bug 27833 as well.