https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20956 Josef Moravec <josef.moravec@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |josef.moravec@gmail.com -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20956 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=20813 Severity|major |enhancement CC| |katrin.fischer@bsz-bw.de --- Comment #2 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- I agree with both Christopher and Jonathan. We never logged permissions and this has never come up before, so I would also classify this as an enhancement. But at the same time I think it would be a very good and valuable addition. Currently there is some work done on permissions, we might want to wait for bug 20813 before implementing this. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20956 --- Comment #3 from Christopher Brannon <cbrannon@cdalibrary.org> --- I'm fine with waiting. Added dependency. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20956 --- Comment #4 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- It looks like bug 20813 is not happening, we might want to come back to this. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20956 Liz Rea <wizzyrea@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |wizzyrea@gmail.com --- Comment #5 from Liz Rea <wizzyrea@gmail.com> --- We definitely want to come back to this, there is a whole slew of legislation out right now that is going to require certifications that changes such as this can be audited (at least in the US, and I presume elsewhere). -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20956 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |tomascohen@gmail.com --- Comment #7 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Should it be in the action_logs? Maybe another table? -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20956 --- Comment #9 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- I think action_logs would be the logical place for it as it also has the other changes to the patron account for the librarians as it can be looked up easily then, but something extermal might be nice additionally. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20956 --- Comment #11 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- There are scripts that delete older entries from action_logs, something to think about if you don't want these to be deleted by accident. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20956 --- Comment #13 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- We just had a case where this kind of log would have been very useful. (In reply to Katrin Fischer from comment #11)

There are scripts that delete older entries from action_logs, something to think about if you don't want these to be deleted by accident.

We can limit deletions now to certain modules and even actions I think. So that should no longer be a problem. To be on the save side we could create a PermissionLog maybe? Keep it separate from other changes to the patron record and a separate module. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20956 Lisette Scheer <lisette.scheer@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |lisette.scheer@bywatersolut | |ions.com -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20956 --- Comment #15 from Kyle M Hall <kyle@bywatersolutions.com> --- (In reply to Magnus Enger from comment #14)

How verbose should we do the logging? As far as I can tell, the information about permissions is stored in a combination of borrowers.flags and the user_permissions table.

The information from the user_permissions table could probably be logged as is, and be pretty readable.

But the flags is just a number, that needs to be interpreted in some way to make sense. Should we log just the number, or the interpretation?

My initial concern was performance, but considering user permissions changes are not a frequent activity I think either solution is reasonable. Either decode it and store it as a list of key/value pairs or just log the binary representation with a link to a wiki page where we can explain how to decode it. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20956 --- Comment #17 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- I agree that we need a nice display in the log viewer at least, if we store it somewhat machine readable I would not mind that much as long as it display nicely in the GUI. Please also keep translatability in mind (also a pro for a template side solution). -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20956 --- Comment #18 from Fiona Borthwick <fiona.borthwick@ptfs-europe.com> --- Would like to resurrect this one please. Not storing changes to patron permissions is a security concern. We are also finding this requirement coming up in tenders. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20956 --- Comment #20 from David Cook <dcook@prosentient.com.au> --- (In reply to Lisette Scheer from comment #19)

Now that we store the diff in the json, maybe we can tie this into Bug 36698 to make a nice diff view of permission changes?

Yeah, that could be nice. I think in the past I was concerned about using action_logs since they can be truncated periodically, but something would also be better than nothing, and this would be a nice way to do it. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20956 George Williams (NEKLS) <george@nekls.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |george@nekls.org -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20956 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- Severity|major |normal -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20956 Chris Rowlands <chris.rowlands6@nhs.net> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |chris.rowlands6@nhs.net -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20956 --- Comment #21 from Hammat wele <hammat.wele@inlibro.com> --- Created attachment 182955 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=182955&action=edit Bug 20956: BorrowersLog is not logging permission changes Changes to the flags field in the borrowers table are currently not logged in the Koha action_logs, making it impossible to trace changes to staff permissions. This represents a security issue. Test plan: 1. Go to a staff interface 2. Search for a patron and change their permissions (note their borrowernumber) 3. Go to tools/Log viewer 4. Filter by Module = Patrons, Object = borrowernumber. 5. Click on submit ==> No permission changes are logged. 6. Apply the patch 7. Repeat step 1, 2, 3, 4, 5 ==> Permission changes are now logged -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20956 Blou <philippe.blouin@inlibro.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on|20813 | Status|NEW |Needs Signoff Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20813 [Bug 20813] Revamp user permissions system -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20956 Lisette Scheer <lisette@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Failed QA --- Comment #23 from Lisette Scheer <lisette@bywatersolutions.com> --- Great start! I'm glad to see this is moving. A few issues: The action log should also follow the coding guideline for the diff column: https://wiki.koha-community.org/wiki/Coding_Guidelines#ACTN1:_New_additions_... Per Comment 10 it shouldn't be toggleable, so it should not follow the borrowers log sys pref. Additionally only the top level permissions are saved, not if there are changes to subpermissions. You'll get something like this: { "flags" : { "after" : 32, "before" : 32 } } Please see also the discussion in comments 14-17 about how those flags/subpermissions should ideally display as readable versions instead of just numbers that need to be decoded. I'm really excited about this getting some movement. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20956 Hammat wele <hammat.wele@inlibro.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #182955|0 |1 is obsolete| | --- Comment #24 from Hammat wele <hammat.wele@inlibro.com> --- Created attachment 185483 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=185483&action=edit Bug 20956: BorrowersLog is not logging permission changes Changes to permissions are not logged in the Koha action_logs, making it impossible to trace changes to staff permissions. This patch ensures that all changes to permissions are logged. Test plan: 1. Go to a staff interface 2. Search for a patron and change their permissions (note their borrowernumber) 3. Go to tools/Log viewer 4. Filter by Module = Patrons, Object = <borrowernumber>. 5. Click on submit ==> No permission changes are logged. 6. Apply the patch 7. Repeat step 1, 2, 3, 4, 5 ==> Permission changes are now logged 8. Check that the diff column contains the change: select diff from action_logs where object= <borrowernumber> order by action_id desc limit 1; -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20956 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20956 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |david@davidnind.com --- Comment #26 from David Nind <david@davidnind.com> --- Testing notes (using KTD): 1. For step 2 of the test plan, <borrowernumber> is the actual borrower number of the patron you changed. For me, I changed the permissions for Henry Acevedo - the borrow number to enter in the object field is 19. 2. For step 8 of the test plan, what shows in the log viewer and the SQL results: 2.1 If I add superlibrarian permissions to Henry: - The log viewer info column shows: {"superlibrarian":"Access to all librarian functions"} - The database query result: {"D":{"superlibrarian":{"A":"Access to all librarian functions"}}} 2.2 If I remove superlibrarian permissions from Henry: - The log viewer info column shows: {} - The database query result: {"D":{"superlibrarian":{"R":"Access to all librarian functions"}}} -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20956 Hammat wele <hammat.wele@inlibro.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #185491|0 |1 is obsolete| | --- Comment #28 from Hammat wele <hammat.wele@inlibro.com> --- Created attachment 186388 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=186388&action=edit Bug 20956: BorrowersLog is not logging permission changes Changes to permissions are not logged in the Koha action_logs, making it impossible to trace changes to staff permissions. This patch ensures that all changes to permissions are logged. Test plan: 1. Go to a staff interface 2. Search for a patron and change their permissions (note their borrowernumber) 3. Go to tools/Log viewer 4. Filter by Module = Patrons, Object = <borrowernumber>. 5. Click on submit ==> No permission changes are logged. 6. Apply the patch 7. Repeat step 1, 2, 3, 4, 5 ==> Permission changes are now logged 8. Check that the diff column contains the change: select diff from action_logs where object= <borrowernumber> order by action_id desc limit 1; Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20956 --- Comment #29 from Jonathan Druart <jonathan.druart@gmail.com> --- I have not followed the last changes in this area (logging), but I don't think you should take care of the changes in C4/Logs.pm Well, I do think we should centralize it, but not without discussion and advertising. For instance, here is the call from Koha::Patron->store to_json( $info, { utf8 => 1, pretty => 1, canonical => 1 } ) It's bad (to_json is slower), but it uses pretty and canonical, what you don't. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20956 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |In Discussion --- Comment #31 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- In view of former comments, changing status to In discussion. Please provide arguments to get this further while addressing raised questions. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20956 Kyle M Hall (khall) <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #186388|0 |1 is obsolete| | --- Comment #33 from Kyle M Hall (khall) <kyle@bywatersolutions.com> --- Created attachment 187592 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=187592&action=edit Bug 20956: BorrowersLog is not logging permission changes Changes to permissions are not logged in the Koha action_logs, making it impossible to trace changes to staff permissions. This patch ensures that all changes to permissions are logged. Test plan: 1. Go to a staff interface 2. Search for a patron and change their permissions (note their borrowernumber) 3. Go to tools/Log viewer 4. Filter by Module = Patrons, Object = <borrowernumber>. 5. Click on submit ==> No permission changes are logged. 6. Apply the patch 7. Repeat step 1, 2, 3, 4, 5 ==> Permission changes are now logged 8. Check that the diff column contains the change: select diff from action_logs where object= <borrowernumber> order by action_id desc limit 1; Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20956 Kyle M Hall (khall) <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|In Discussion |Passed QA --- Comment #35 from Kyle M Hall (khall) <kyle@bywatersolutions.com> --- (In reply to Jonathan Druart from comment #29)

I have not followed the last changes in this area (logging), but I don't think you should take care of the changes in C4/Logs.pm

Well, I do think we should centralize it, but not without discussion and advertising.

For instance, here is the call from Koha::Patron->store to_json( $info, { utf8 => 1, pretty => 1, canonical => 1 } )

It's bad (to_json is slower), but it uses pretty and canonical, what you don't.

As lisette referenced, this topic was discussed and approved via bug 25159 and the corresponding ACTN1 coding guideline which was approved at a developers meeting. A good next step would be to centralize permissions changes within Koha::Patron but I think that is out of scope for this bug ( but would be a great next step ). -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20956 Kyle M Hall (khall) <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #187593|0 |1 is obsolete| | --- Comment #36 from Kyle M Hall (khall) <kyle@bywatersolutions.com> --- Created attachment 187594 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=187594&action=edit Bug 20956: [QA Follow-up] Move subs to Koha::Patron, add unit tests Subroutines in members/member-flags.pl are combined and moved to Koha::Patron Removed the html-based english description of each permission from the logging Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20956 Hammat wele <hammat.wele@inlibro.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #187592|0 |1 is obsolete| | --- Comment #38 from Hammat wele <hammat.wele@inlibro.com> --- Created attachment 188260 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=188260&action=edit Bug 20956: BorrowersLog is not logging permission changes Changes to permissions are not logged in the Koha action_logs, making it impossible to trace changes to staff permissions. This patch ensures that all changes to permissions are logged. Test plan: 1. Go to a staff interface 2. Search for a patron and change their permissions (note their borrowernumber) 3. Go to tools/Log viewer 4. Filter by Module = Patrons, Object = <borrowernumber>. 5. Click on submit ==> No permission changes are logged. 6. Apply the patch 7. Repeat step 1, 2, 3, 4, 5 ==> Permission changes are now logged 8. Check that the diff column contains the change: select diff from action_logs where object= <borrowernumber> order by action_id desc limit 1; Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20956 --- Comment #40 from Hammat wele <hammat.wele@inlibro.com> --- Created attachment 188262 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=188262&action=edit Bug 20956: (follow-up) Tidyness and fix no pod coverage in Koha/Patron.pm -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20956 Hammat wele <hammat.wele@inlibro.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #188262|0 |1 is obsolete| | --- Comment #41 from Hammat wele <hammat.wele@inlibro.com> --- Created attachment 188263 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=188263&action=edit Bug 20956: (follow-up) Tidyness and fix no pod coverage in Koha/Patron.pm Signed-off-by: Hammat Wele <hammat.wele@inlibro.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20956 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |BLOCKED --- Comment #42 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Looking here -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20956 --- Comment #43 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Tests are failing: * Proving /usr/share/koha/t/db_dependent/Koha/Patron.t FAIL # Failed test 'Patron has catalogue permission' # at /usr/share/koha/t/db_dependent/Koha/Patron.t line 826. # got: undef # expected: '1' # Looks like you failed 1 test of 2. # Failed test 'is_superlibrarian() tests' # at /usr/share/koha/t/db_dependent/Koha/Patron.t line 829. # Failed test 'no warnings' # at /usr/share/perl/5.36/Test/Builder.pm line 193. # There were 1 warning(s) # Previous test 0 '' # Reference found where even-sized list expected at /usr/share/koha/t/db_dependent/Koha/Patron.t line 823. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20956 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|BLOCKED |Failed QA -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20956 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff Patch complexity|--- |Small patch -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20956 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #188261|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20956 --- Comment #46 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 190686 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=190686&action=edit Bug 20956: BorrowersLog is not logging permission changes Changes to permissions are not logged in the Koha action_logs, making it impossible to trace changes to staff permissions. This patch ensures that all changes to permissions are logged. Test plan: 1. Go to a staff interface 2. Search for a patron and change their permissions (note their borrowernumber) 3. Go to tools/Log viewer 4. Filter by Module = Patrons, Object = <borrowernumber>. 5. Click on submit ==> No permission changes are logged. 6. Apply the patch 7. Repeat step 1, 2, 3, 4, 5 ==> Permission changes are now logged 8. Check that the diff column contains the change: select diff from action_logs where object= <borrowernumber> order by action_id desc limit 1; Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20956 --- Comment #48 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 190688 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=190688&action=edit Bug 20956: (follow-up) Tidyness and fix no pod coverage in Koha/Patron.pm Signed-off-by: Hammat Wele <hammat.wele@inlibro.com> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20956 --- Comment #50 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 190690 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=190690&action=edit Bug 20956: (QA follow-up) Match getuserflags() return structure This patch changes Koha::Patron->permissions() to return the same structure as C4::Auth::getuserflags() for architectural consistency. Previously, permissions() only returned granted permissions: { catalogue => 1, circulate => { override_renewals => 1 } } Now it returns ALL flags (matching getuserflags()): { superlibrarian => 0, catalogue => 1, circulate => { override_renewals => 1 }, borrowers => 0, ... } This provides several benefits: 1. Consistency - Matches established C4::Auth::getuserflags() pattern 2. Simpler checks - Can use "if ($flags->{module})" instead of "if (exists $flags->{module})" 3. Future compatibility - Enables eventual deprecation of getuserflags() in favor of the ORM-based $patron->permissions() 4. Helper compatibility - Works with existing _dispatch() logic The logging functionality still works correctly - the diff will now show explicit changes from 0 to 1 rather than absence to presence. Test plan: 1. prove t/db_dependent/Koha/Patron.t 2. prove t/db_dependent/Log.t 3. Verify all tests pass 4. Confirm permissions() now returns all flags with 0/1 values Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20956 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |andrew@bywatersolutions.com | |, | |aude.charillon@openfifth.co | |.uk, | |lucy.vaux-harvey@openfifth. | |co.uk -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20956 --- Comment #52 from David Nind <david@davidnind.com> --- Still sitting as "Signed Off", should this be "Passed QA"? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20956 --- Comment #54 from Ayoub Glizi-Vicioso <ayoub.glizi-vicioso@inLibro.com> --- Created attachment 191472 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=191472&action=edit Bug 20956: fixup syntax error -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20956 Ayoub Glizi-Vicioso <ayoub.glizi-vicioso@inLibro.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #190687|0 |1 is obsolete| | --- Comment #56 from Ayoub Glizi-Vicioso <ayoub.glizi-vicioso@inLibro.com> --- Created attachment 191474 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=191474&action=edit Bug 20956: (QA follow-up) Move subs to Koha::Patron, add unit tests Subroutines in members/member-flags.pl are combined and moved to Koha::Patron Removed the html-based english description of each permission from the logging Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20956 Ayoub Glizi-Vicioso <ayoub.glizi-vicioso@inLibro.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #190689|0 |1 is obsolete| | --- Comment #58 from Ayoub Glizi-Vicioso <ayoub.glizi-vicioso@inLibro.com> --- Created attachment 191476 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=191476&action=edit Bug 20956: (QA follow-up) Fix tests and refactor to use ORM This patch addresses several QA concerns: 1. Fixed duplicate subtest name - renamed second "is_superlibrarian() tests" to "permissions() tests" which accurately reflects what it tests 2. Fixed incorrect hash assignment - permissions() returns a hashref, not a hash. Changed `my %permissions` to `my $permissions` 3. Improved test coverage - expanded from 2 to 8 assertions covering: - Hashref return type verification - Single flag permission - No permissions - Multiple flags - Granular subpermissions from user_permissions table 4. Refactored permissions() method to use DBIx::Class ORM instead of raw SQL queries, following Koha::Object architecture principles All tests now pass without warnings. Test plan: 1. prove t/db_dependent/Koha/Patron.t 2. prove t/db_dependent/Log.t 3. Verify all tests pass 4. Confirm the test now properly validates the intended behavior Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20956 Ayoub Glizi-Vicioso <ayoub.glizi-vicioso@inLibro.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #191472|0 |1 is obsolete| | --- Comment #60 from Ayoub Glizi-Vicioso <ayoub.glizi-vicioso@inLibro.com> --- Created attachment 191478 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=191478&action=edit Bug 20956: fixup syntax error -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20956 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Failed QA --- Comment #61 from David Nind <david@davidnind.com> --- The tests for prove t/db_dependent/Koha/Patron.t fail for me (latest main): prove t/db_dependent/Koha/Patron.t t/db_dependent/Koha/Patron.t .. 6/46 # Failed test 'Patron has catalogue permission' # at t/db_dependent/Koha/Patron.t line 826. # got: undef # expected: '1' # Looks like you failed 1 test of 2. # Failed test 'is_superlibrarian() tests' # at t/db_dependent/Koha/Patron.t line 829. t/db_dependent/Koha/Patron.t .. 46/46 # Failed test 'no warnings' # at /usr/share/perl/5.36/Test/Builder.pm line 193. # There were 1 warning(s) # Previous test 0 '' # Reference found where even-sized list expected at t/db_dependent/Koha/Patron.t line 823. # at t/db_dependent/Koha/Patron.t line 823. # main::__ANON__() called at /usr/share/perl/5.36/Test/Builder.pm line 374 # eval {...} called at /usr/share/perl/5.36/Test/Builder.pm line 374 # Test::Builder::subtest(Test::Builder=HASH(0x5fc68e942028), "is_superlibrarian() tests", CODE(0x5fc68e7698f0)) called at /usr/share/perl/5.36/Test/More.pm line 809 # Test::More::subtest("is_superlibrarian() tests", CODE(0x5fc68e7698f0)) called at t/db_dependent/Koha/Patron.t line 829 # # Looks like you planned 46 tests but ran 47. # Looks like you failed 2 tests of 47 run. t/db_dependent/Koha/Patron.t .. Dubious, test returned 2 (wstat 512, 0x200) Failed 1/46 subtests Test Summary Report ------------------- t/db_dependent/Koha/Patron.t (Wstat: 512 (exited 2) Tests: 47 Failed: 2) Failed tests: 11, 47 Non-zero exit status: 2 Parse errors: Bad plan. You planned 46 tests but ran 47. Files=1, Tests=47, 44 wallclock secs ( 0.09 usr 0.02 sys + 28.55 cusr 13.86 csys = 42.52 CPU) Result: FAIL -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20956 Ayoub Glizi-Vicioso <ayoub.glizi-vicioso@inLibro.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Signed Off -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20956 Ayoub Glizi-Vicioso <ayoub.glizi-vicioso@inLibro.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #191474|0 |1 is obsolete| | --- Comment #64 from Ayoub Glizi-Vicioso <ayoub.glizi-vicioso@inLibro.com> --- Created attachment 192507 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=192507&action=edit Bug 20956: (QA follow-up) Move subs to Koha::Patron, add unit tests Subroutines in members/member-flags.pl are combined and moved to Koha::Patron Removed the html-based english description of each permission from the logging Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20956 Ayoub Glizi-Vicioso <ayoub.glizi-vicioso@inLibro.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #191476|0 |1 is obsolete| | --- Comment #66 from Ayoub Glizi-Vicioso <ayoub.glizi-vicioso@inLibro.com> --- Created attachment 192509 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=192509&action=edit Bug 20956: (QA follow-up) Fix tests and refactor to use ORM This patch addresses several QA concerns: 1. Fixed duplicate subtest name - renamed second "is_superlibrarian() tests" to "permissions() tests" which accurately reflects what it tests 2. Fixed incorrect hash assignment - permissions() returns a hashref, not a hash. Changed `my %permissions` to `my $permissions` 3. Improved test coverage - expanded from 2 to 8 assertions covering: - Hashref return type verification - Single flag permission - No permissions - Multiple flags - Granular subpermissions from user_permissions table 4. Refactored permissions() method to use DBIx::Class ORM instead of raw SQL queries, following Koha::Object architecture principles All tests now pass without warnings. Test plan: 1. prove t/db_dependent/Koha/Patron.t 2. prove t/db_dependent/Log.t 3. Verify all tests pass 4. Confirm the test now properly validates the intended behavior Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20956 Ayoub Glizi-Vicioso <ayoub.glizi-vicioso@inLibro.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #191478|0 |1 is obsolete| | --- Comment #68 from Ayoub Glizi-Vicioso <ayoub.glizi-vicioso@inLibro.com> --- Created attachment 192511 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=192511&action=edit Bug 20956: fixup syntax error -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20956 Ayoub Glizi-Vicioso <ayoub.glizi-vicioso@inLibro.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20956 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #192506|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20956 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #192508|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20956 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #192510|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20956 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #192512|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20956 --- Comment #71 from David Nind <david@davidnind.com> --- Created attachment 192605 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=192605&action=edit Bug 20956: (QA follow-up) Move subs to Koha::Patron, add unit tests Subroutines in members/member-flags.pl are combined and moved to Koha::Patron Removed the html-based english description of each permission from the logging Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20956 --- Comment #73 from David Nind <david@davidnind.com> --- Created attachment 192607 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=192607&action=edit Bug 20956: (QA follow-up) Fix tests and refactor to use ORM This patch addresses several QA concerns: 1. Fixed duplicate subtest name - renamed second "is_superlibrarian() tests" to "permissions() tests" which accurately reflects what it tests 2. Fixed incorrect hash assignment - permissions() returns a hashref, not a hash. Changed `my %permissions` to `my $permissions` 3. Improved test coverage - expanded from 2 to 8 assertions covering: - Hashref return type verification - Single flag permission - No permissions - Multiple flags - Granular subpermissions from user_permissions table 4. Refactored permissions() method to use DBIx::Class ORM instead of raw SQL queries, following Koha::Object architecture principles All tests now pass without warnings. Test plan: 1. prove t/db_dependent/Koha/Patron.t 2. prove t/db_dependent/Log.t 3. Verify all tests pass 4. Confirm the test now properly validates the intended behavior Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20956 --- Comment #75 from David Nind <david@davidnind.com> --- Created attachment 192609 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=192609&action=edit Bug 20956: fixup syntax error Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20956 --- Comment #77 from David Nind <david@davidnind.com> --- I've signed off, but one thing I don't really understand is what should be shown in the log viewer - versus what is recorded in the database. For example: 1. I added and removed superlibrarian permissions for Henry. 2. What is shown in the log viewer vs what is in the database: - Added: . Log viewer: {"superlibrarian":1} . Database diff column: {"D":{"superlibrarian":{"A":1}}} - Removed: . Log viewer: {} . Database diff column: {"D":{"superlibrarian":{"R":1}}} 3. What is shown in the info column in the log viewer is almost meaningless/useless for a "normal" staff patrons - unless you know what it means, or can access the database/run a report to show the diff. Which, in my view, defeats the purpose of logging permission changes. While you can show the diff column, the info column should at least provide reasonably understandable information on what was changed, for example: XYZ permission added or removed. Also, as I am not a developer, I have no idea for the test patches how to: - Confirm the test now properly validates the intended behavour - Confirm permissions() now returns all flags with 0/1 values Feel free to change back to needs sign off if this is something that you require a tester to do. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20956 Michelle Spinney <mspinney@clamsnet.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |mspinney@clamsnet.org -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20956 Johanna Räisä <johanna.raisa@koha-suomi.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |johanna.raisa@koha-suomi.fi -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20956 Ayoub Glizi-Vicioso <ayoub.glizi-vicioso@inLibro.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20956 Ayoub Glizi-Vicioso <ayoub.glizi-vicioso@inLibro.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #192605|0 |1 is obsolete| | --- Comment #81 from Ayoub Glizi-Vicioso <ayoub.glizi-vicioso@inLibro.com> --- Created attachment 194528 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=194528&action=edit Bug 20956: (QA follow-up) Move subs to Koha::Patron, add unit tests Subroutines in members/member-flags.pl are combined and moved to Koha::Patron Removed the html-based english description of each permission from the logging Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20956 Ayoub Glizi-Vicioso <ayoub.glizi-vicioso@inLibro.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #192607|0 |1 is obsolete| | --- Comment #83 from Ayoub Glizi-Vicioso <ayoub.glizi-vicioso@inLibro.com> --- Created attachment 194530 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=194530&action=edit Bug 20956: (QA follow-up) Fix tests and refactor to use ORM This patch addresses several QA concerns: 1. Fixed duplicate subtest name - renamed second "is_superlibrarian() tests" to "permissions() tests" which accurately reflects what it tests 2. Fixed incorrect hash assignment - permissions() returns a hashref, not a hash. Changed `my %permissions` to `my $permissions` 3. Improved test coverage - expanded from 2 to 8 assertions covering: - Hashref return type verification - Single flag permission - No permissions - Multiple flags - Granular subpermissions from user_permissions table 4. Refactored permissions() method to use DBIx::Class ORM instead of raw SQL queries, following Koha::Object architecture principles All tests now pass without warnings. Test plan: 1. prove t/db_dependent/Koha/Patron.t 2. prove t/db_dependent/Log.t 3. Verify all tests pass 4. Confirm the test now properly validates the intended behavior Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20956 Ayoub Glizi-Vicioso <ayoub.glizi-vicioso@inLibro.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #192609|0 |1 is obsolete| | --- Comment #85 from Ayoub Glizi-Vicioso <ayoub.glizi-vicioso@inLibro.com> --- Created attachment 194532 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=194532&action=edit Bug 20956: fixup syntax error Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20956 --- Comment #87 from Ayoub Glizi-Vicioso <ayoub.glizi-vicioso@inLibro.com> --- Created attachment 194534 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=194534&action=edit Bug 20956: (QA follow-up) Fix display of permission logs -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20956 --- Comment #89 from Lisette Scheer <lisette@bywatersolutions.com> --- (In reply to Ayoub Glizi-Vicioso from comment #88)

Updated patch to address QA and usability feedback:

Readability: Enabled pretty JSON formatting in both the "Info" and "Diff" columns to resolve table width issues.

Clarity: Enhanced the "Diff" display by replacing raw 0/1 values with explicit Added ✅ and Removed ❌ labels.

Standards: Ran tidy script and fixed spelling errors to meet Koha coding standards.

This is looking great! I haven't finished testing, but I'd like another sign off since it's a big visual change. I think a lot of the pretty work could be adapted on 36698 and I've posted a screenshot over there to get some feedback for a wider adoption down the road. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20956 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=36698 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20956 --- Comment #92 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- This introduced another special case.. I'd rather we solved the diff view generically to all diffs display 'nicer'.. I've added a patch and screenshot to bug 36698 that works for all logs containing a diff -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20956 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |40136 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40136 [Bug 40136] Record diff in action logs when modifying a patron -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20956 --- Comment #95 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- I'm going to move that final patch to the other bug.. and I have some further QA considerations here. I think we need to keep the action distinct.. i.e have a 'MODIFY_PERMISSIONS' as apposed to reusing the 'MODIFY'.. else were going to end up with two quite different forms of data mingled in the borrowers MODIFY logs. (This matters more now I'm working on bug 40136) -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20956 --- Comment #97 from Lisette Scheer <lisette@bywatersolutions.com> --- (In reply to Martin Renvoize (ashimema) from comment #95)

I'm going to move that final patch to the other bug.. and I have some further QA considerations here.

I think we need to keep the action distinct.. i.e have a 'MODIFY_PERMISSIONS' as apposed to reusing the 'MODIFY'.. else were going to end up with two quite different forms of data mingled in the borrowers MODIFY logs. (This matters more now I'm working on bug 40136)

I was going to split the patch because part of it is fixing no line breaks in the info, which makes the table to wide. Your nesting/new action makes sense. New action makes it easier to filter on in the table. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20956 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #194528|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20956 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #194530|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20956 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #194532|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20956 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #194534|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20956 --- Comment #99 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 194866 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=194866&action=edit Bug 20956: (QA follow-up) Move subs to Koha::Patron, add unit tests Subroutines in members/member-flags.pl are combined and moved to Koha::Patron Removed the html-based english description of each permission from the logging Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20956 --- Comment #101 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 194868 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=194868&action=edit Bug 20956: (QA follow-up) Fix tests and refactor to use ORM This patch addresses several QA concerns: 1. Fixed duplicate subtest name - renamed second "is_superlibrarian() tests" to "permissions() tests" which accurately reflects what it tests 2. Fixed incorrect hash assignment - permissions() returns a hashref, not a hash. Changed `my %permissions` to `my $permissions` 3. Improved test coverage - expanded from 2 to 8 assertions covering: - Hashref return type verification - Single flag permission - No permissions - Multiple flags - Granular subpermissions from user_permissions table 4. Refactored permissions() method to use DBIx::Class ORM instead of raw SQL queries, following Koha::Object architecture principles All tests now pass without warnings. Test plan: 1. prove t/db_dependent/Koha/Patron.t 2. prove t/db_dependent/Log.t 3. Verify all tests pass 4. Confirm the test now properly validates the intended behavior Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20956 --- Comment #103 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 194870 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=194870&action=edit Bug 20956: Fix test failures Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20956 --- Comment #105 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- (In reply to Lisette Scheer from comment #97)

(In reply to Martin Renvoize (ashimema) from comment #95)

...

I'm going to move that final patch to the other bug.. and I have some further QA considerations here.

I think we need to keep the action distinct.. i.e have a 'MODIFY_PERMISSIONS' as apposed to reusing the 'MODIFY'.. else were going to end up with two quite different forms of data mingled in the borrowers MODIFY logs. (This matters more now I'm working on bug 40136)

I was going to split the patch because part of it is fixing no line breaks in the info, which makes the table to wide.

Your nesting/new action makes sense. New action makes it easier to filter on in the table.

I went with Nesting... and I think in another bug we should quickly add the 'diff' to the search options.. then you you do 'MODIFY' + 'permission' in the diff as your search to filter easily -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20956 --- Comment #107 from Lisette Scheer <lisette@bywatersolutions.com> --- Created attachment 194882 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=194882&action=edit Bug 20956: (QA follow-up) Restore info pretty I restored the pretty code for the info file because the page needs to be able to wrap, like it does for other logs. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20956 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #194865|0 |1 is obsolete| | --- Comment #108 from David Nind <david@davidnind.com> --- Created attachment 194883 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=194883&action=edit Bug 20956: BorrowersLog is not logging permission changes Changes to permissions are not logged in the Koha action_logs, making it impossible to trace changes to staff permissions. This patch ensures that all changes to permissions are logged. Test plan: 1. Go to a staff interface 2. Search for a patron and change their permissions (note their borrowernumber) 3. Go to tools/Log viewer 4. Filter by Module = Patrons, Object = <borrowernumber>. 5. Click on submit ==> No permission changes are logged. 6. Apply the patch 7. Repeat step 1, 2, 3, 4, 5 ==> Permission changes are now logged 8. Check that the diff column contains the change: select diff from action_logs where object= <borrowernumber> order by action_id desc limit 1; Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20956 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #194867|0 |1 is obsolete| | --- Comment #110 from David Nind <david@davidnind.com> --- Created attachment 194885 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=194885&action=edit Bug 20956: (follow-up) Tidyness and fix no pod coverage in Koha/Patron.pm Signed-off-by: Hammat Wele <hammat.wele@inlibro.com> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20956 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #194869|0 |1 is obsolete| | --- Comment #112 from David Nind <david@davidnind.com> --- Created attachment 194887 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=194887&action=edit Bug 20956: (QA follow-up) Match getuserflags() return structure This patch changes Koha::Patron->permissions() to return the same structure as C4::Auth::getuserflags() for architectural consistency. Previously, permissions() only returned granted permissions: { catalogue => 1, circulate => { override_renewals => 1 } } Now it returns ALL flags (matching getuserflags()): { superlibrarian => 0, catalogue => 1, circulate => { override_renewals => 1 }, borrowers => 0, ... } This provides several benefits: 1. Consistency - Matches established C4::Auth::getuserflags() pattern 2. Simpler checks - Can use "if ($flags->{module})" instead of "if (exists $flags->{module})" 3. Future compatibility - Enables eventual deprecation of getuserflags() in favor of the ORM-based $patron->permissions() 4. Helper compatibility - Works with existing _dispatch() logic The logging functionality still works correctly - the diff will now show explicit changes from 0 to 1 rather than absence to presence. Test plan: 1. prove t/db_dependent/Koha/Patron.t 2. prove t/db_dependent/Log.t 3. Verify all tests pass 4. Confirm permissions() now returns all flags with 0/1 values Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20956 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #194871|0 |1 is obsolete| | --- Comment #114 from David Nind <david@davidnind.com> --- Created attachment 194889 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=194889&action=edit Bug 20956: (follow-up) Nest permissions data under 'permissions' key in MEMBERS/MODIFY log Changes to patron permissions via member-flags.pl are logged as MEMBERS/MODIFY, indistinguishable from regular patron field edits. This patch wraps the before/after permission hashes under a 'permissions' key so that the diff renderer displays changes as 'permissions.cataloguing' etc., making it clear the diff relates to permissions rather than first-level patron fields. Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20956 Lisette Scheer <lisette@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20956 Lisette Scheer <lisette@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #194884|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20956 Lisette Scheer <lisette@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #194886|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20956 Lisette Scheer <lisette@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #194888|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20956 Lisette Scheer <lisette@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #194890|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20956 --- Comment #117 from Lisette Scheer <lisette@bywatersolutions.com> --- Created attachment 194893 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=194893&action=edit Bug 20956: (QA follow-up) Move subs to Koha::Patron, add unit tests Subroutines in members/member-flags.pl are combined and moved to Koha::Patron Removed the html-based english description of each permission from the logging Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20956 --- Comment #119 from Lisette Scheer <lisette@bywatersolutions.com> --- Created attachment 194895 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=194895&action=edit Bug 20956: (QA follow-up) Fix tests and refactor to use ORM This patch addresses several QA concerns: 1. Fixed duplicate subtest name - renamed second "is_superlibrarian() tests" to "permissions() tests" which accurately reflects what it tests 2. Fixed incorrect hash assignment - permissions() returns a hashref, not a hash. Changed `my %permissions` to `my $permissions` 3. Improved test coverage - expanded from 2 to 8 assertions covering: - Hashref return type verification - Single flag permission - No permissions - Multiple flags - Granular subpermissions from user_permissions table 4. Refactored permissions() method to use DBIx::Class ORM instead of raw SQL queries, following Koha::Object architecture principles All tests now pass without warnings. Test plan: 1. prove t/db_dependent/Koha/Patron.t 2. prove t/db_dependent/Log.t 3. Verify all tests pass 4. Confirm the test now properly validates the intended behavior Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20956 --- Comment #121 from Lisette Scheer <lisette@bywatersolutions.com> --- Created attachment 194897 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=194897&action=edit Bug 20956: Fix test failures Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20956 --- Comment #123 from Lisette Scheer <lisette@bywatersolutions.com> --- Created attachment 194899 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=194899&action=edit Bug 20956: (QA follow-up) Restore info pretty I restored the pretty code for the info file because the page needs to be able to wrap, like it does for other logs. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20956 Lisette Scheer <lisette@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |42085 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42085 [Bug 42085] Permissions should be logged when a patron is deleted -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20956 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to main Version(s)| |26.05.00 released in| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20956 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Text to go in the|This patch adds permission |This enhancement adds release notes|changes to the members log. |logging of patron |Permission changes are |permission changes when |always logged, similar to |BorrowersLog is enabled. |the System Administration |Detailed information about |logging. |the changes is now included | |in the log viewer tool info | |and diff columns. Before | |this change, permission | |changes were not logged. -- You are receiving this mail because: You are watching all bug changes.