Bug 26534

Summary: Add a Font Awesome icon to help identify staff patrons
Product: Koha Reporter: Fridolin Somers <fridolin.somers>
Component: PatronsAssignee: Fridolin Somers <fridolin.somers>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: david, gmcharlt, jonathan.druart, kyle.m.hall, lucas, martin.renvoize, Niamh.WalkerHeadon
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
This adds a small shield icon to help identify staff members (having catalogue permission) in the staff interface. The icon displays next to the name on top of the brief patron information that is shown on all patron account pages.
Version(s) released in:
20.11.00
Bug Depends on:    
Bug Blocks: 26879, 26687    
Attachments: Screenshot
Bug 26534: Add Font Awesome icon only for staff patrons
Bug 26534: Add Font Awesome icon only for staff patrons
Bug 26534: Add Font Awesome icon only for staff patrons
Bug 26534: (QA follow-up) Fix CSS declaration
Bug 26534: (follow-up) Only center when using patronimages
Bug 26534: (follow-up) Only center when using patronimages
Bug 26534: (follow-up) Only center when using patronimages
Bug 26534: Replace 'grade' by 'is-patron' and 'patron-type'
Bug 26534: Use IF ELSE directly on h5 opening tag
Bug 26534: Use IF ELSE directly on h5 opening tag
Bug 26534: Add Font Awesome icon only for staff patrons
Bug 26534: (QA follow-up) Fix CSS declaration
Bug 26534: (follow-up) Only center when using patronimages
Bug 26534: Replace 'grade' by 'is-patron' and 'patron-type'
Bug 26534: Use IF ELSE directly on h5 opening tag
Bug 26534: Add Font Awesome icon only for staff patrons
Bug 26534: (QA follow-up) Fix CSS declaration
Bug 26534: (follow-up) Only center when using patronimages
Bug 26534: Replace 'grade' by 'is-patron' and 'patron-type'
Bug 26534: Use IF ELSE directly on h5 opening tag
Bug 26687: Add color to Font Awesome icon for superlibrarian patrons

Description Fridolin Somers 2020-09-25 07:41:51 UTC
In patron pages it would help to see an icon indicating that this is a staff patron (having permission to access staff interface).
This may avoid wrong patron deletion or permission mistakes.
I propose the shield Font Awesome icon.
Comment 1 Fridolin Somers 2020-09-25 07:49:16 UTC
Created attachment 110717 [details]
Screenshot

Screenshot of staff patron detail page
Comment 2 Fridolin Somers 2020-09-25 07:50:15 UTC
Created attachment 110718 [details] [review]
Bug 26534: Add Font Awesome icon only for staff patrons

In patron pages it would help to see an icon indicating that this is a staff patron (having permission to access staff interface).
This may avoid wrong patron deletion or permission mistakes.
I propose the shield Font Awesome icon and a grey background

Patch also shows patron name and cardnumber aligned center instead of right.
Patch adds to 'patroninfo' div a class 'staffgrade' or 'normalgrade' to
allow more styling depending on patron beeing staff or not.

Test plan:
Compare display between a patron having staff interface access and one without permissions.
Comment 3 David Nind 2020-09-25 09:49:09 UTC
Created attachment 110738 [details] [review]
Bug 26534: Add Font Awesome icon only for staff patrons

In patron pages it would help to see an icon indicating that this is a staff patron (having permission to access staff interface).
This may avoid wrong patron deletion or permission mistakes.
I propose the shield Font Awesome icon and a grey background

Patch also shows patron name and cardnumber aligned center instead of right.
Patch adds to 'patroninfo' div a class 'staffgrade' or 'normalgrade' to
allow more styling depending on patron beeing staff or not.

Test plan:
Compare display between a patron having staff interface access and one without permissions.

Signed-off-by: David Nind <david@davidnind.com>
Comment 4 Martin Renvoize 2020-09-25 10:05:56 UTC
Created attachment 110739 [details] [review]
Bug 26534: Add Font Awesome icon only for staff patrons

In patron pages it would help to see an icon indicating that this is a staff patron (having permission to access staff interface).
This may avoid wrong patron deletion or permission mistakes.
I propose the shield Font Awesome icon and a grey background

Patch also shows patron name and cardnumber aligned center instead of right.
Patch adds to 'patroninfo' div a class 'staffgrade' or 'normalgrade' to
allow more styling depending on patron beeing staff or not.

Test plan:
Compare display between a patron having staff interface access and one without permissions.

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 5 Martin Renvoize 2020-09-25 10:06:37 UTC
Nice enhancement, Passing QA
Comment 6 Owen Leonard 2020-09-25 10:39:14 UTC
    + .staffgrade {
        h5 {
            background-color: #EEE;
        }
    }

This rule doesn't work as I think it is intended. If the staff name heading is to have a background color it should be written as:

    &.staffgrade {
        h5 {
            background-color: #EEE;
        }
    }
Comment 7 Martin Renvoize 2020-09-25 11:50:18 UTC
Created attachment 110749 [details] [review]
Bug 26534: (QA follow-up) Fix CSS declaration
Comment 8 Martin Renvoize 2020-09-25 11:50:24 UTC
Created attachment 110750 [details] [review]
Bug 26534: (follow-up) Only center when using patronimages

I felt the change to always center the user title worked well when
patronimages was enabled, but not so well when it was not (it looked
strange without a background for non-staff users).
Comment 9 Martin Renvoize 2020-09-25 11:52:05 UTC
Setting back to Signed off to get a second QA opinion as I missed the CSS issue first time around :(.

I've corrected the CSS issue with a QA followup, but it also brought my attention to how the centered names looked when patronimages were not in use.. I felt they didn't look right in that case so have added a followup which should be looked at distinctly.
Comment 10 Martin Renvoize 2020-09-25 12:39:34 UTC
Created attachment 110753 [details] [review]
Bug 26534: (follow-up) Only center when using patronimages

I felt the change to always center the user title worked well when
patronimages was enabled, but not so well when it was not (it looked
strange without a background for non-staff users).
Comment 11 Martin Renvoize 2020-09-25 12:41:02 UTC
Created attachment 110754 [details] [review]
Bug 26534: (follow-up) Only center when using patronimages

I felt the change to always center the user title worked well when
patronimages was enabled, but not so well when it was not (it looked
strange without a background for non-staff users).
Comment 12 Martin Renvoize 2020-09-25 12:56:13 UTC
Discussed in IRC and followup cleaned as per discussion.. Setting back to PQA as we seemed happy as a group :)
Comment 13 Fridolin Somers 2020-09-25 13:07:22 UTC
Maybe avoid duplicated code this :

[% IF ( patronimages ) %]
    <h5 class="text-center">
[% ELSE %]
    <h5>
[% END %]
Comment 14 Jonathan Druart 2020-09-29 13:45:14 UTC
Fridolin, "grade" does not sound appropriate, could you use is_superlibrarian for instance?
Comment 15 Jonathan Druart 2020-09-29 13:46:09 UTC
(In reply to Jonathan Druart from comment #14)
> Fridolin, "grade" does not sound appropriate, could you use
> is_superlibrarian for instance?

would actually be "is_staff" in this context :)
Comment 16 Fridolin Somers 2020-09-29 15:27:59 UTC
(In reply to Jonathan Druart from comment #15)
> (In reply to Jonathan Druart from comment #14)
> > Fridolin, "grade" does not sound appropriate, could you use
> > is_superlibrarian for instance?
> 
> would actually be "is_staff" in this context :)

Yep ok no problem.
Comment 17 Fridolin Somers 2020-09-30 08:14:17 UTC
(In reply to Fridolin SOMERS from comment #13)
> Maybe avoid duplicated code this :
> 
> [% IF ( patronimages ) %]
>     <h5 class="text-center">
> [% ELSE %]
>     <h5>
> [% END %]

Bah forget it.
Comment 18 Fridolin Somers 2020-09-30 08:17:46 UTC
Created attachment 110960 [details] [review]
Bug 26534: Replace 'grade' by 'is-patron' and 'patron-type'
Comment 19 Fridolin Somers 2020-09-30 08:20:38 UTC
Looks great now, needs a final signoff ?
Comment 20 Martin Renvoize 2020-09-30 08:24:06 UTC
(In reply to Fridolin SOMERS from comment #17)
> (In reply to Fridolin SOMERS from comment #13)
> > Maybe avoid duplicated code this :
> > 
> > [% IF ( patronimages ) %]
> >     <h5 class="text-center">
> > [% ELSE %]
> >     <h5>
> > [% END %]
> 
> Bah forget it.

Sorry, it wasn't a bad idea.. I just hadn't gotten around to it..I'm happy with it either way really myself.
Comment 21 Fridolin Somers 2020-09-30 08:56:35 UTC
(In reply to Martin Renvoize from comment #20)
> (In reply to Fridolin SOMERS from comment #17)
> > (In reply to Fridolin SOMERS from comment #13)
> > > Maybe avoid duplicated code this :
> > > 
> > > [% IF ( patronimages ) %]
> > >     <h5 class="text-center">
> > > [% ELSE %]
> > >     <h5>
> > > [% END %]
> > 
> > Bah forget it.
> 
> Sorry, it wasn't a bad idea.. I just hadn't gotten around to it..I'm happy
> with it either way really myself.

Ah OK super, i patch :D
Comment 22 Fridolin Somers 2020-09-30 09:02:22 UTC
Created attachment 110972 [details] [review]
Bug 26534: Use IF ELSE directly on h5 opening tag
Comment 23 Owen Leonard 2020-09-30 13:32:30 UTC
Something has gone wrong with these patches... Looks like maybe a merge error? They apply but I get an error. "Template process failed: file error - parse error - circ-menu.inc line 181: unexpected end of input"
Comment 24 Fridolin Somers 2020-09-30 14:56:48 UTC
Created attachment 110995 [details] [review]
Bug 26534: Use IF ELSE directly on h5 opening tag
Comment 25 Fridolin Somers 2020-09-30 14:58:03 UTC
(In reply to Owen Leonard from comment #23)
> Something has gone wrong with these patches... Looks like maybe a merge
> error? They apply but I get an error. "Template process failed: file error -
> parse error - circ-menu.inc line 181: unexpected end of input"

Ah of course : IF, ELSE, was missing END ;)
Comment 26 David Nind 2020-10-03 00:28:03 UTC
Created attachment 111185 [details] [review]
Bug 26534: Add Font Awesome icon only for staff patrons

In patron pages it would help to see an icon indicating that this is a staff patron (having permission to access staff interface).
This may avoid wrong patron deletion or permission mistakes.
I propose the shield Font Awesome icon and a grey background

Patch also shows patron name and cardnumber aligned center instead of right.
Patch adds to 'patroninfo' div a class 'staffgrade' or 'normalgrade' to
allow more styling depending on patron beeing staff or not.

Test plan:
Compare display between a patron having staff interface access and one without permissions.

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: David Nind <david@davidnind.com>
Comment 27 David Nind 2020-10-03 00:28:07 UTC
Created attachment 111186 [details] [review]
Bug 26534: (QA follow-up) Fix CSS declaration

Signed-off-by: David Nind <david@davidnind.com>
Comment 28 David Nind 2020-10-03 00:28:12 UTC
Created attachment 111187 [details] [review]
Bug 26534: (follow-up) Only center when using patronimages

I felt the change to always center the user title worked well when
patronimages was enabled, but not so well when it was not (it looked
strange without a background for non-staff users).

Signed-off-by: David Nind <david@davidnind.com>
Comment 29 David Nind 2020-10-03 00:28:17 UTC
Created attachment 111188 [details] [review]
Bug 26534: Replace 'grade' by 'is-patron' and 'patron-type'

Signed-off-by: David Nind <david@davidnind.com>
Comment 30 David Nind 2020-10-03 00:28:24 UTC
Created attachment 111189 [details] [review]
Bug 26534: Use IF ELSE directly on h5 opening tag

Signed-off-by: David Nind <david@davidnind.com>
Comment 31 David Nind 2020-10-03 00:55:36 UTC
This is the test plan I used:

1. In a tab open a patron record for a staff member who has at least 'Staff access..." permissions.
2. In another tab enable the patronimages system preference:
   . Go to Home > Koha administration
   . Search for the patronimages system preference
   . Change to 'Allow'
3. In the same tab load the same patron as used in step 1.
4. Apply the patch.
5. Update the staff interface CSS: see https://wiki.koha-community.org/wiki/Working_with_SCSS_in_the_OPAC_and_staff_client
6. In yet another tab open the same patron record as in step 1.
7. It should look similar to the screenshot attached to the bug, with the shield icon, patron name and card number centred above the image with a light grey background.
8. Experiment with different browser widths to make sure things display nicely.
9. Turn off the patronimages system preference (Home > Koha administration > search for patronimages > change to 'Don't allow').
10. Refresh the patron page.
11. It should display the shield icon, patron name, and card number left aligned with a light grey background.
12. Experiment with different browser widths to make sure things display nicely.
13. Sign off!
Comment 32 Katrin Fischer 2020-10-14 22:17:47 UTC
Created attachment 111648 [details] [review]
Bug 26534: Add Font Awesome icon only for staff patrons

In patron pages it would help to see an icon indicating that this is a staff patron (having permission to access staff interface).
This may avoid wrong patron deletion or permission mistakes.
I propose the shield Font Awesome icon and a grey background

Patch also shows patron name and cardnumber aligned center instead of right.
Patch adds to 'patroninfo' div a class 'staffgrade' or 'normalgrade' to
allow more styling depending on patron beeing staff or not.

Test plan:
Compare display between a patron having staff interface access and one without permissions.

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 33 Katrin Fischer 2020-10-14 22:17:52 UTC
Created attachment 111649 [details] [review]
Bug 26534: (QA follow-up) Fix CSS declaration

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 34 Katrin Fischer 2020-10-14 22:18:00 UTC
Created attachment 111650 [details] [review]
Bug 26534: (follow-up) Only center when using patronimages

I felt the change to always center the user title worked well when
patronimages was enabled, but not so well when it was not (it looked
strange without a background for non-staff users).

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 35 Katrin Fischer 2020-10-14 22:18:06 UTC
Created attachment 111651 [details] [review]
Bug 26534: Replace 'grade' by 'is-patron' and 'patron-type'

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 36 Katrin Fischer 2020-10-14 22:18:16 UTC
Created attachment 111652 [details] [review]
Bug 26534: Use IF ELSE directly on h5 opening tag

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 37 Katrin Fischer 2020-10-14 22:19:16 UTC
I really like this and think it would be nice in other spots like the patron search as well. But this is a solid start.
Comment 38 Fridolin Somers 2020-10-15 07:28:07 UTC
Oh now I see that my code :
[% SET patron_is_staff = patron.has_permission({ 'catalogue' => 1 }) %]

Could have used existing var [% IF ( CAN_user_catalogue ) %].

Do I change ?
Comment 39 Martin Renvoize 2020-10-15 08:10:20 UTC
(In reply to Fridolin SOMERS from comment #38)
> Oh now I see that my code :
> [% SET patron_is_staff = patron.has_permission({ 'catalogue' => 1 }) %]
> 
> Could have used existing var [% IF ( CAN_user_catalogue ) %].
> 
> Do I change ?

tl:dr; Keep is as is.

In this case, I actually like the addition of the patron_is_staff variable as it makes it clear we're not so much looking for 'cataglogue' as we're trying to just distinguish if the user is staff or not (In the future, we might eventually add role based access control).. also, CAN_user_catalogue is at the currently logged in user level, rather than on the patron we're interested in.. isn't it.
Comment 40 Fridolin Somers 2020-10-15 08:32:51 UTC
> CAN_user_catalogue is at the currently logged in user
Ooooo crap sure, forget my comment ;)
Comment 41 Fridolin Somers 2020-10-15 08:53:49 UTC Comment hidden (obsolete)
Comment 42 Fridolin Somers 2020-10-15 08:54:35 UTC
Oups wrong bug in git-bz
Comment 43 Fridolin Somers 2020-10-15 09:06:51 UTC
(In reply to Katrin Fischer from comment #37)
> I really like this and think it would be nice in other spots like the patron
> search as well. But this is a solid start.
I'll try to work on that.

I've created Bug 26687 to color icon for superlibrarians B-)
Comment 44 Jonathan Druart 2020-10-15 09:35:24 UTC
Isn't it weird to have a background colour only for staff users? Isn't the icon enough?
Comment 45 Katrin Fischer 2020-10-15 09:38:54 UTC
(In reply to Jonathan Druart from comment #44)
> Isn't it weird to have a background colour only for staff users? Isn't the
> icon enough?

I think it makes it stand out a little more than only the small icon - I liked it, but opinions may vary.
Comment 46 Jonathan Druart 2020-10-15 10:55:23 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 47 Fridolin Somers 2020-10-15 12:13:53 UTC
(In reply to Jonathan Druart from comment #44)
> Isn't it weird to have a background colour only for staff users? Isn't the
> icon enough?

Class 'is-not-staff' on 'patroninfo' div allows an easy CSS tunning depending on staff/non-staff
Comment 48 Fridolin Somers 2020-10-15 14:37:58 UTC
Thanks a lot everyone. This code is my little baby, will feel proud each time I see the little shield :D
Comment 49 Lucas Gass 2020-11-13 15:39:42 UTC
enhancement will not be backported to 20.05.x