Bug 25858

Summary: Borrower permissions are broken by update from bug 22868
Product: Koha Reporter: Nick Clemens <nick>
Component: PatronsAssignee: Nick Clemens <nick>
Status: RESOLVED FIXED QA Contact: Testopia <testopia>
Severity: blocker    
Priority: P1 - high CC: aleisha, didier.gautheron, gmcharlt, jonathan.druart+koha, julian.maurice, kyle.m.hall, lucas, victor
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Trivial patch
Text to go in the release notes:
Version(s) released in:
20.11.00, 20.05.02, 19.11.08, 19.05.13
Bug Depends on: 22868    
Bug Blocks:    
Attachments: Bug 25858: Don't attempt to set flag if already set
Bug 25858: Don't attempt to set flag if already set
Bug 25858: Don't attempt to set flag if already set
Bug 25858: Use bitwise OR for setting a bit in borrowers.flag

Description Nick Clemens 2020-06-23 19:12:16 UTC
The update in bug 22868 flips bit 12 in flags on, however, it was backported and there is no check that the flag is off.

A borrower who had bit 11 will now have 4096 added twice which will break their permissions
Comment 1 Nick Clemens 2020-06-24 10:07:20 UTC
Created attachment 106240 [details] [review]
Bug 25858: Don't attempt to set flag if already set
Comment 2 Didier Gautheron 2020-06-25 09:47:36 UTC
Hi

Why not use & | and ^ ?

UPDATE borrowers SET flags = flags | (1<<12) WHERE flags & (1 << 11)

Regards
Didier
Comment 3 Nick Clemens 2020-06-25 10:01:33 UTC
(In reply to didier from comment #2)
> Hi
> 
> Why not use & | and ^ ?
> 
> UPDATE borrowers SET flags = flags | (1<<12) WHERE flags & (1 << 11)
> 
> Regards
> Didier

That is much more concise, I am not sure if it is clearer, but I am happy to have either solution, we just need to get this fixed
Comment 4 Didier Gautheron 2020-06-25 10:09:52 UTC
Created attachment 106282 [details] [review]
Bug 25858: Don't attempt to set flag if already set

Signed-off-by: Didier Gautheron <didier.gautheron@biblibre.com>
Comment 5 Julian Maurice 2020-06-26 06:52:46 UTC
(In reply to Nick Clemens from comment #3)
> (In reply to didier from comment #2)
> > Hi
> > 
> > Why not use & | and ^ ?
> > 
> > UPDATE borrowers SET flags = flags | (1<<12) WHERE flags & (1 << 11)
> > 
> > Regards
> > Didier
> 
> That is much more concise, I am not sure if it is clearer, but I am happy to
> have either solution, we just need to get this fixed

I confirm both solutions work, but in my opinion Didier's solution is more clear about what the intention is. I'll provide a followup
Comment 6 Julian Maurice 2020-06-26 07:01:07 UTC
Created attachment 106314 [details] [review]
Bug 25858: Don't attempt to set flag if already set

Signed-off-by: Didier Gautheron <didier.gautheron@biblibre.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 7 Julian Maurice 2020-06-26 07:01:12 UTC
Created attachment 106315 [details] [review]
Bug 25858: Use bitwise OR for setting a bit in borrowers.flag

Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Thanks-to: Didier Gautheron <didier.gautheron@biblibre.com>
Comment 8 Jonathan Druart 2020-06-29 09:54:50 UTC
Looks like it is impossible to add a new update DB entry to fix this problem :-/
Comment 9 Jonathan Druart 2020-06-29 11:50:54 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 10 Lucas Gass 2020-07-13 15:58:01 UTC
backported to 20.05.x for 20.05.02
Comment 11 Aleisha Amohia 2020-07-16 04:25:51 UTC
backported to 19.11.x for 19.11.08
Comment 12 Victor Grousset/tuxayo 2020-07-22 22:10:19 UTC
Backported to 19.05.x branch for 19.05.13