From 2638ede80cff8aa5037f78533d365cef7168c9ab Mon Sep 17 00:00:00 2001
From: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Date: Wed, 25 Apr 2018 13:33:05 +0200
Subject: [PATCH] Bug 20100: (QA follow-up) Pref description and improve code
 in member-flags
Content-Type: text/plain; charset=utf-8

Two points as mentioned on Bugzilla comment29.
[1] Improve pref description. Feedback from comment30 incorporated.
[2] Improve code in member-flags. Check if we change librarian flag first.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Tested the die with "disable_superlibrarian_privs => 0" in member-flags.
---
 .../prog/en/modules/admin/preferences/patrons.pref      |  4 ++--
 members/member-flags.pl                                 | 17 ++++++-----------
 2 files changed, 8 insertions(+), 13 deletions(-)

diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/patrons.pref b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/patrons.pref
index ad50390..348e1dd 100644
--- a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/patrons.pref
+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/patrons.pref
@@ -203,8 +203,8 @@ Patrons:
          - pref: ProtectSuperlibrarianPrivileges
            choices:
                yes: Allow only superlibrarians
-               no: Do not block permitted non-superlibrarians
-         - to access/change superlibrarian privileges.
+               no: Allow all permitted users
+         - "to access/change superlibrarian privileges. Note: A permitted user needs to have the 'permissions' flag (if no superlibrarian)."
 
     "Norwegian patron database":
      -
diff --git a/members/member-flags.pl b/members/member-flags.pl
index 04582ea..c527ff0 100755
--- a/members/member-flags.pl
+++ b/members/member-flags.pl
@@ -84,18 +84,13 @@ if ($input->param('newflags')) {
     }
     
     $sth = $dbh->prepare("UPDATE borrowers SET flags=? WHERE borrowernumber=?");
-    if( !C4::Context->preference('ProtectSuperlibrarianPrivileges') || C4::Context->IsSuperLibrarian ) {
-        $sth->execute($module_flags, $member);
-    } else {
-        my $old_flags = $patron->flags // 0;
-        if( ( $old_flags == 1 || $module_flags == 1 ) &&
-              $old_flags != $module_flags ) {
-           die "Non-superlibrarian is changing superlibrarian privileges"; # Interface should not allow this, so we can just die here
-        } else {
-            $sth->execute($module_flags, $member);
-        }
+    my $old_flags = $patron->flags // 0;
+    if( ( $old_flags == 1 || $module_flags == 1 ) &&
+      $old_flags != $module_flags ) {
+        die "Non-superlibrarian is changing superlibrarian privileges" if !C4::Context->IsSuperLibrarian && C4::Context->preference('ProtectSuperlibrarianPrivileges'); # Interface should not allow this, so we can just die here
     }
-    
+    $sth->execute($module_flags, $member);
+
     # deal with subpermissions
     $sth = $dbh->prepare("DELETE FROM user_permissions WHERE borrowernumber = ?");
     $sth->execute($member); 
-- 
2.1.4