Bug 26940 - debarred comment in borrowers table is lost on patron modifications in memberentry.pl page
Summary: debarred comment in borrowers table is lost on patron modifications in member...
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Didier Gautheron
QA Contact: Martin Renvoize
URL:
Keywords:
Depends on: 20287
Blocks:
  Show dependency treegraph
 
Reported: 2020-11-05 14:01 UTC by Didier Gautheron
Modified: 2021-05-24 16:57 UTC (History)
9 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Text to go in the release notes:
Version(s) released in:
21.05.00,20.11.06,20.05.12


Attachments
Bug 26940: don't reset debarredcomment on patron update (1.76 KB, patch)
2020-11-05 14:39 UTC, Didier Gautheron
Details | Diff | Splinter Review
Bug 26940: don't reset debarredcomment on patron update (1.83 KB, patch)
2020-11-05 15:04 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 26940: don't reset debarredcomment on patron update (1.89 KB, patch)
2020-11-16 09:44 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26940: Put in sync borrowers.debarredcomment with comments from borrower_debarment (1.53 KB, patch)
2020-11-17 11:19 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 26940: Take empty strings into account (1.11 KB, patch)
2020-11-18 11:24 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 26940: don't reset debarredcomment on patron update (1.96 KB, patch)
2021-03-19 10:06 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 26940: Put in sync borrowers.debarredcomment with comments from borrower_debarment (1.59 KB, patch)
2021-03-19 10:06 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 26940: Take empty strings into account (1.18 KB, patch)
2021-03-19 10:06 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 26940: don't reset debarredcomment on patron update (2.03 KB, patch)
2021-04-16 18:13 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 26940: Put in sync borrowers.debarredcomment with comments from borrower_debarment (1.66 KB, patch)
2021-04-16 18:13 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 26940: Take empty strings into account (1.25 KB, patch)
2021-04-16 18:13 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Didier Gautheron 2020-11-05 14:01:08 UTC
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
Comment 1 Didier Gautheron 2020-11-05 14:39:12 UTC
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
Comment 2 Andrew Fuerste-Henry 2020-11-05 15:04:38 UTC
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>
Comment 3 Martin Renvoize 2020-11-16 09:44:44 UTC
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>
Comment 4 Martin Renvoize 2020-11-16 09:45:12 UTC
Works as expected, no regressions found and passes QA scripts.

Passing QA
Comment 5 Jonathan Druart 2020-11-16 13:18:47 UTC
Should not we have an update of the DB for the existing installations?
Comment 6 Didier Gautheron 2020-11-17 10:26:29 UTC
Hi,

It'd need a perl script. I don't believe it's worth it, it' a minor bug.

Regards
Didier
Comment 7 Jonathan Druart 2020-11-17 11:19:26 UTC
Created attachment 113702 [details] [review]
Bug 26940: Put in sync borrowers.debarredcomment with comments from borrower_debarment
Comment 8 Jonathan Druart 2020-11-17 11:19:56 UTC
I think we should, a script does not really make sense here.

Please test this follow-up.
Comment 9 Martin Renvoize 2020-11-17 12:42:59 UTC
The update looks pretty solid to me.. pretty much exactly how I would have done it :)
Comment 10 Didier Gautheron 2020-11-17 14:37:32 UTC
(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.
Comment 11 Jonathan Druart 2020-11-17 14:47:20 UTC
What do you have in borrower_debarments?
Comment 12 Didier Gautheron 2020-11-17 14:52:11 UTC
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)
Comment 13 Jonathan Druart 2020-11-17 14:59:48 UTC
The expiration date needs to be > NOW, otherwise the patron is not considered restricted.
Comment 14 Didier Gautheron 2020-11-17 15:13:01 UTC
(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;
Comment 15 Jonathan Druart 2020-11-17 15:33:53 UTC
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.
Comment 16 Didier Gautheron 2020-11-17 19:41:01 UTC
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.
Comment 17 Jonathan Druart 2020-11-18 11:24:09 UTC
Created attachment 113779 [details] [review]
Bug 26940: Take empty strings into account
Comment 18 ByWater Sandboxes 2021-03-19 10:06:43 UTC
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>
Comment 19 ByWater Sandboxes 2021-03-19 10:06:45 UTC
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>
Comment 20 ByWater Sandboxes 2021-03-19 10:06:48 UTC
Created attachment 118502 [details] [review]
Bug 26940: Take empty strings into account

Signed-off-by: Katariina Hanhisalo <katariina.hanhisalo@xamk.fi>
Comment 21 Katrin Fischer 2021-03-21 12:25:49 UTC
Setting this to Signed off seeing that there was a sign-off from the sandboxes and this doesn't happen automatically then.
Comment 22 Jonathan Druart 2021-03-23 11:08:22 UTC
(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.
Comment 23 Séverine Queune 2021-04-01 13:24:25 UTC
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 !
Comment 24 Katrin Fischer 2021-04-03 22:31:28 UTC
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).
Comment 25 Katrin Fischer 2021-04-03 22:32:53 UTC
@Severine Never hesitate to sign something you tested!
Comment 26 Kyle M Hall 2021-04-16 18:13:41 UTC
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>
Comment 27 Kyle M Hall 2021-04-16 18:13:54 UTC
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>
Comment 28 Kyle M Hall 2021-04-16 18:13:57 UTC
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>
Comment 29 Jonathan Druart 2021-04-21 08:46:35 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 30 Fridolin Somers 2021-04-29 09:52:46 UTC
Pushed to 20.11.x for 20.11.06
Comment 31 Andrew Fuerste-Henry 2021-05-24 15:39:55 UTC
Pushed to 20.05.x for 20.05.12
Comment 32 Victor Grousset/tuxayo 2021-05-24 16:57:55 UTC
Not backported to oldoldstable (19.11.x). Feel free to ask if it's needed.