|
Description
Jonathan Druart
2021-04-26 09:38:56 UTC
What would be the expected behaviour? I think the exception should be caught, and the behavior the same as with any patron with invalid (non-extended attribute) data. (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? (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. (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 What about: 1. unique not being unique? 2. non-rep (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. Created attachment 120549 [details] [review] Bug 28220: Deal with merge 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? Created attachment 120607 [details] [review] Bug 28220: Deal with merge Created attachment 120608 [details] [review] Bug 28220: Add tests to import failure expected, we need to add a transaction Created attachment 120609 [details] [review] Bug 28220: prevent patron to be created if attributes not stored Using a transaction Created attachment 120610 [details] [review] Bug 28220: Handle InvalidType Created attachment 120611 [details] [review] Bug 28220: Handle NonRepeatable Created attachment 120612 [details] [review] Bug 28220: Add test for updating an existing patron 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 Created attachment 120614 [details] [review] Bug 28220: UI changes 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) 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). Created attachment 120783 [details] [review] Bug 28220: Deal with merge Signed-off-by: Nick Clemens <nick@bywatersolutions.com> 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> 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> Created attachment 120786 [details] [review] Bug 28220: Handle InvalidType Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 120787 [details] [review] Bug 28220: Handle NonRepeatable Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 120788 [details] [review] Bug 28220: Add test for updating an existing patron Signed-off-by: Nick Clemens <nick@bywatersolutions.com> 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> Created attachment 120790 [details] [review] Bug 28220: UI changes Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Nick, as you signed this one off, could you please also take a look at signing off the dependency bug as well? 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> 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> 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> 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> 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> 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> 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> 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> 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 Pushed to master for 21.05, thanks to everybody involved! 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 ? (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. |