Hi Debarred comment is copied in borrowers table but it's set to an empty string on a patron is modified in memberentry. In my understanding AddDebarrement and DelDebarment already properly update debarredcomment
Created attachment 113119 [details] [review] Bug 26940: don't reset debarredcomment on patron update Test plan: 1) Create a patron 2) In koha/members/moremember.pl add a manual restriction, with comment foobar 3) try to checkout, you have a message like Restricted: Patron's account is restricted with the explanation: foobar 4) Got to Edit patron, save 5) try to checkout, foobar is no more Restricted: Patron's account is restricted with the explanation: 6) Apply patch 7) Redo 1-4 8) try to checkout, foobar is there
Created attachment 113123 [details] [review] Bug 26940: don't reset debarredcomment on patron update Test plan: 1) Create a patron 2) In koha/members/moremember.pl add a manual restriction, with comment foobar 3) try to checkout, you have a message like Restricted: Patron's account is restricted with the explanation: foobar 4) Got to Edit patron, save 5) try to checkout, foobar is no more Restricted: Patron's account is restricted with the explanation: 6) Apply patch 7) Redo 1-4 8) try to checkout, foobar is there. Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Created attachment 113653 [details] [review] Bug 26940: don't reset debarredcomment on patron update Test plan: 1) Create a patron 2) In koha/members/moremember.pl add a manual restriction, with comment foobar 3) try to checkout, you have a message like Restricted: Patron's account is restricted with the explanation: foobar 4) Got to Edit patron, save 5) try to checkout, foobar is no more Restricted: Patron's account is restricted with the explanation: 6) Apply patch 7) Redo 1-4 8) try to checkout, foobar is there. Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Works as expected, no regressions found and passes QA scripts. Passing QA
Should not we have an update of the DB for the existing installations?
Hi, It'd need a perl script. I don't believe it's worth it, it' a minor bug. Regards Didier
Created attachment 113702 [details] [review] Bug 26940: Put in sync borrowers.debarredcomment with comments from borrower_debarment
I think we should, a script does not really make sense here. Please test this follow-up.
The update looks pretty solid to me.. pretty much exactly how I would have done it :)
(In reply to Jonathan Druart from comment #8) > I think we should, a script does not really make sense here. > > Please test this follow-up. I tested on today master and it doesn't work for me: at step 5) debarredcomment is not null, it's empty.
What do you have in borrower_debarments?
select * from borrower_debarments; +-----------------------+----------------+------------+--------+---------+------------+---------------------+---------+ | borrower_debarment_id | borrowernumber | expiration | type | comment | manager_id | created | updated | +-----------------------+----------------+------------+--------+---------+------------+---------------------+---------+ | 1 | 19 | NULL | MANUAL | foobar | 51 | 2020-11-17 14:25:20 | NULL | +-----------------------+----------------+------------+--------+---------+------------+---------------------+---------+ 1 row in set (0.00 sec) select debarredcomment from borrowers where borrowernumber = 19; +-----------------+ | debarredcomment | +-----------------+ | | +-----------------+ 1 row in set (0.00 sec)
The expiration date needs to be > NOW, otherwise the patron is not considered restricted.
(In reply to Jonathan Druart from comment #13) > The expiration date needs to be > NOW, otherwise the patron is not > considered restricted. It is, well it's null select debarredcomment, debarred from borrowers where borrowernumber = 19; +-----------------+------------+ | debarredcomment | debarred | +-----------------+------------+ | | 9999-12-31 | +-----------------+------------+ 1 row in set (0.00 sec) MariaDB [koha_kohadev]> select expiration, comment, NOW() from borrower_debarments where borrowernumber = 19; +------------+---------+---------------------+ | expiration | comment | NOW() | +------------+---------+---------------------+ | NULL | foobar | 2020-11-17 15:09:15 | +------------+---------+---------------------+ 1 row in set (0.00 sec) SQL below is working for me SELECT d.borrowernumber, GROUP_CONCAT(comment SEPARATOR '\n') AS comment FROM borrower_debarments d JOIN borrowers b ON b.borrowernumber=d.borrowernumber WHERE (b.debarredcomment IS NULL OR b.debarredcomment = '') AND ( expiration > CURRENT_DATE() OR expiration IS NULL ) GROUP BY d.borrowernumber;
My patch is only about updating the problematic entries in borrowers.debarredcomment. It should not modify the test plan of the first patch. Test plan for second patch is: Create some debarred (and several debarments for some patrons) > update borrowers set debarredcomment=null; Execute the DB entry => borrowers.debarredcomment is reset properly with current restrictions' descriptions.
Hi, (In reply to Jonathan Druart from comment #15) > My patch is only about updating the problematic entries in > borrowers.debarredcomment. But in my understanding it doesn't. After step 4) in the first test plan borrowers.debarredcomment is not wrongly set to a NULL value, it's set to an empty string. At least it's what I'm seeing here and in the library reporting the issue. Number of debarred borrowers with a NULL debarredcomment. SELECT COUNT(*) FROM (SELECT 1 FROM borrower_debarments d JOIN borrowers b ON b.borrowernumber=d.borrowernumber WHERE (b.debarredcomment IS NULL ) AND ( expiration > CURRENT_DATE() OR expiration IS NULL ) GROUP BY d.borrowernumber) as e; +----------+ | COUNT(*) | +----------+ | 14 | +----------+ 1 row in set (0.008 sec) Number of debarred borrowers with an empty debarredcomment but with at least one borrower_debarments comment. MariaDB [koha]> SELECT COUNT(*) FROM (SELECT 1 FROM borrower_debarments d JOIN borrowers b ON b.borrowernumber=d.borrowernumber WHERE (b.debarredcomment IS NULL OR b.debarredcomment = '' ) AND ( expiration > CURRENT_DATE() OR expiration IS NULL ) AND (comment IS NOT NULL OR comment <> '') GROUP BY d.borrowernumber) as e; +----------+ | COUNT(*) | +----------+ | 2539 | +----------+ 1 row in set (0.013 sec) > It should not modify the test plan of the first patch. > > Test plan for second patch is: > Create some debarred (and several debarments for some patrons) > > update borrowers set debarredcomment=null; > Execute the DB entry > > => borrowers.debarredcomment is reset properly with current restrictions' > descriptions.
Created attachment 113779 [details] [review] Bug 26940: Take empty strings into account
Created attachment 118500 [details] [review] Bug 26940: don't reset debarredcomment on patron update Test plan: 1) Create a patron 2) In koha/members/moremember.pl add a manual restriction, with comment foobar 3) try to checkout, you have a message like Restricted: Patron's account is restricted with the explanation: foobar 4) Got to Edit patron, save 5) try to checkout, foobar is no more Restricted: Patron's account is restricted with the explanation: 6) Apply patch 7) Redo 1-4 8) try to checkout, foobar is there. Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Katariina Hanhisalo <katariina.hanhisalo@xamk.fi>
Created attachment 118501 [details] [review] Bug 26940: Put in sync borrowers.debarredcomment with comments from borrower_debarment Signed-off-by: Katariina Hanhisalo <katariina.hanhisalo@xamk.fi>
Created attachment 118502 [details] [review] Bug 26940: Take empty strings into account Signed-off-by: Katariina Hanhisalo <katariina.hanhisalo@xamk.fi>
Setting this to Signed off seeing that there was a sign-off from the sandboxes and this doesn't happen automatically then.
(In reply to Katrin Fischer from comment #21) > Setting this to Signed off seeing that there was a sign-off from the > sandboxes and this doesn't happen automatically then. I am waiting for a signoff from Didier. The last 2 patches specifically, about the DB rev.
Jonathan and Katrin, I don't know if you're waiting for a specific sign off from Didier, but to me everything works fine. Here are the results of the different requests, wich seem consistent to me : mysql> SELECT d.borrowernumber, GROUP_CONCAT(comment SEPARATOR '\n') AS comment FROM borrower_debarments d JOIN borrowers b ON b.borrowernumber=d.borrowernumber WHERE (b.debarredcomment IS NULL OR b.debarredcomment = '') AND ( expiration > CURRENT_DATE() OR expiration IS NULL ) GROUP BY d.borrowernumber; +----------------+---------+ | borrowernumber | comment | +----------------+---------+ | 70 | NULL | +----------------+---------+ 1 row in set (0.00 sec) mysql> SELECT COUNT(*) FROM (SELECT 1 FROM borrower_debarments d JOIN borrowers b ON b.borrowernumber=d.borrowernumber WHERE (b.debarredcomment IS NULL ) AND ( expiration > CURRENT_DATE() OR expiration IS NULL ) GROUP BY d.borrowernumber) as e; +----------+ | COUNT(*) | +----------+ | 1 | +----------+ 1 row in set (0.01 sec) mysql> SELECT COUNT(*) FROM (SELECT 1 FROM borrower_debarments d JOIN borrowers b ON b.borrowernumber=d.borrowernumber WHERE (b.debarredcomment IS NULL OR b.debarredcomment = '' ) AND ( expiration > CURRENT_DATE() OR expiration IS NULL ) AND (comment IS NOT NULL OR comment <> '') GROUP BY d.borrowernumber) as e; +----------+ | COUNT(*) | +----------+ | 0 | +----------+ 1 row in set (0.00 sec) Let me know if you need I sign it !
Hi Jonathan, I don't think we should wait any longer with so many positive tests as we have now. This has already been waiting since November. This is bordering on a data loss bug (we can restore it, but still).
@Severine Never hesitate to sign something you tested!
Created attachment 119778 [details] [review] Bug 26940: don't reset debarredcomment on patron update Test plan: 1) Create a patron 2) In koha/members/moremember.pl add a manual restriction, with comment foobar 3) try to checkout, you have a message like Restricted: Patron's account is restricted with the explanation: foobar 4) Got to Edit patron, save 5) try to checkout, foobar is no more Restricted: Patron's account is restricted with the explanation: 6) Apply patch 7) Redo 1-4 8) try to checkout, foobar is there. Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Katariina Hanhisalo <katariina.hanhisalo@xamk.fi> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 119779 [details] [review] Bug 26940: Put in sync borrowers.debarredcomment with comments from borrower_debarment Signed-off-by: Katariina Hanhisalo <katariina.hanhisalo@xamk.fi> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 119780 [details] [review] Bug 26940: Take empty strings into account Signed-off-by: Katariina Hanhisalo <katariina.hanhisalo@xamk.fi> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Pushed to master for 21.05, thanks to everybody involved!
Pushed to 20.11.x for 20.11.06
Pushed to 20.05.x for 20.05.12
Not backported to oldoldstable (19.11.x). Feel free to ask if it's needed.