|
Description
Fridolin Somers
2025-05-26 15:12:30 UTC
Created attachment 183063 [details] [review] Bug 40000: Show that a patron is the anonymous in patrons search Show patron is the anonymous in patrons search. Display uses class 'circ-hlt' to show bold red text. Can be hidden using class 'patron_is_anonymous'. Test plan : 1) Create a new user "John Doe" 2) Add its borrowernumber in system preference 'AnonymousPatron' 3) Perform a search on letter 'd' 4) Check you see in table a text 'This is the anonymous patron' only on this patron Created attachment 183064 [details]
Screenshot with patch
Screenshot with patch on koha-testing-docker
> Display uses class 'circ-hlt' to show bold red text.
Maybe there is a better way for that.
Its is open to discussion ;)
Created attachment 183256 [details] [review] Bug 40000: Show that a patron is the anonymous in patrons search Show patron is the anonymous in patrons search. Display uses class 'circ-hlt' to show bold red text. Can be hidden using class 'patron_is_anonymous'. Test plan : 1) Create a new user "John Doe" 2) Add its borrowernumber in system preference 'AnonymousPatron' 3) Perform a search on letter 'd' 4) Check you see in table a text 'This is the anonymous patron' only on this patron Signed-off-by: David Nind <david@davidnind.com> (In reply to Fridolin Somers from comment #3) > > Display uses class 'circ-hlt' to show bold red text. > Maybe there is a better way for that. > Its is open to discussion ;) Maybe an option is a green badge like the one used to show the default SMTP server (Koha administration > Additional parameters > SMTP servers: add a new server and set as default, in the 'Is default' column there is a green badge with 'Default'). This was added in Bug 37311. So maybe 'Anonymous patron' as a green badge. Created attachment 183258 [details] [review] Bug 40000: (follow-up) Use badge to highlight anonymous patron I think a "warning" class is appropriate because we're warning the user that this patron should be treated differently. I trimmed to label to "Anonymous patron" because I don't think the "This is the..." is necessary for a label. (In reply to David Nind from comment #5) > So maybe 'Anonymous patron' as a green badge. I have attached an opinionated suggestion! (In reply to Owen Leonard from comment #7) > (In reply to David Nind from comment #5) > > So maybe 'Anonymous patron' as a green badge. > > I have attached an opinionated suggestion! Looks great to me Owen! Created attachment 183263 [details]
Screenshot - using a badge
Screenshot showing Owen's alternative patch that uses a yellow badge to highlight the anonymous patron (for the AnonymousPatron system preference).
Ohhh very nice with badge This feature is new to me : https://getbootstrap.com/docs/4.0/components/badge/ (In reply to Fridolin Somers from comment #10) > https://getbootstrap.com/docs/4.0/components/badge/ Not that version! This one: https://getbootstrap.com/docs/5.3/components/badge/ Maybe I should change to use "Patron->is_anonymous" from Bug 40401 (In reply to Fridolin Somers from comment #12) > Maybe I should change to use "Patron->is_anonymous" from Bug 40401 Do you want to make this change? Or do you like this as is? (In reply to Lucas Gass (lukeg) from comment #13) > (In reply to Fridolin Somers from comment #12) > > Maybe I should change to use "Patron->is_anonymous" from Bug 40401 > > Do you want to make this change? Or do you like this as is? Ahhh in fact this is not valid, in this JS code we dont have the Koha::Patron object, only the patron id. So lets go with those patches QA :
Ah maybe "[% Koha.Preference('AnonymousPatron') %]" needs html filter ?
I've looked at the code and it seems we never use Koha.Preference with an | html filter. There are also other occurrence in the code close to this one, where it doesn't seem to have complained.
[% IF ! open_on_row_click %]
r += "<a href=\"/cgi-bin/koha/members/moremember.pl?borrowernumber=" + patron_id + "\" class=\"patron_name\" data-borrowernumber=\"" + patron_id + "\" style=\"white-space:nowrap\">"
+ $patron_to_html(row, { invert_name: 1, showDiffFirstname: [% Koha.Preference('ShowPatronFirstnameIfDifferentThanPreferredname') ? 1 : 0 | html %] }) + "</a>";
[% ELSE %]
r += $patron_to_html(row, { invert_name: 1, showDiffFirstname: [% Koha.Preference('ShowPatronFirstnameIfDifferentThanPreferredname') ? 1 : 0 | html %] });
Patch works, nice little addition- sending this one a level higher up.
Created attachment 193044 [details] [review] Bug 40000: Show that a patron is the anonymous in patrons search Show patron is the anonymous in patrons search. Display uses class 'circ-hlt' to show bold red text. Can be hidden using class 'patron_is_anonymous'. Test plan : 1) Create a new user "John Doe" 2) Add its borrowernumber in system preference 'AnonymousPatron' 3) Perform a search on letter 'd' 4) Check you see in table a text 'This is the anonymous patron' only on this patron Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Created attachment 193045 [details] [review] Bug 40000: (follow-up) Use badge to highlight anonymous patron I think a "warning" class is appropriate because we're warning the user that this patron should be treated differently. I trimmed to label to "Anonymous patron" because I don't think the "This is the..." is necessary for a label. Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Sorry, back to FQA as a question in the Development channel led to some more information: https://chat.koha-community.org/koha-community/pl/wkyk1gcffbfgxysfshhb8khn3w TT vars must now be moved to a separate script tag for serialization see the tree under Bug 41324 There is also a "store" for syspref it's still not in the coding guideline, not everything is pushed yet, but this is the correct way to fix it see "addPrefs" > see "addPrefs"
Sorry I cant find it
(In reply to Fridolin Somers from comment #20) > > see "addPrefs" > Sorry I cant find it I was just curious on the discussion on mattermost. I guess that's what it should look like f.e.: addPrefs({'anonPatronId': [% Koha.Preference('AnonymousPatron') ? 1 : 0 %]}) then later compare it via prefs.anonPatronId Ah but is it not a boolean pref.
And needs HTML filter no ?
addPrefs({'anonPatronId': [% Koha.Preference('AnonymousPatron') | html %]})
(In reply to Fridolin Somers from comment #22) > Ah but is it not a boolean pref. > > And needs HTML filter no ? > > addPrefs({'anonPatronId': [% Koha.Preference('AnonymousPatron') | html %]}) @Joubu? (In reply to Fridolin Somers from comment #22) > Ah but is it not a boolean pref. > > And needs HTML filter no ? > > addPrefs({'anonPatronId': [% Koha.Preference('AnonymousPatron') | html %]}) Yes, have a look at `git grep addPrefs`, we already have occurrences pushed to main, or have a look at the remote branch bug_41324 in my Koha gitlab repo for more examples. OK thanks (In reply to Katrin Fischer from comment #19) > Sorry, back to FQA as a question in the Development channel led to some more > information: > > https://chat.koha-community.org/koha-community/pl/wkyk1gcffbfgxysfshhb8khn3w > > TT vars must now be moved to a separate script tag for serialization > see the tree under Bug 41324 > There is also a "store" for syspref > it's still not in the coding guideline, not everything is pushed yet, but > this is the correct way to fix it > see "addPrefs" I think that is bug 41562 I've suggested on Mattermost that we start using <script type="application/json"> for storing Template::Toolkit output for Javascript.
So above the Javascript <script> where you've added the logic, you could add
<script id="anonymous_patron_id" type="application/json">
[% To.json(Koha.Preference('AnonymousPatron')) | $raw %]
</script>
You don't have to use the To.json but then you definitely need a try/catch in your JS to make sure you can handle invalid data in there.
(In reply to David Cook from comment #27) > I've suggested on Mattermost that we start using <script > type="application/json"> for storing Template::Toolkit output for > Javascript. That said, I think the QA script might flag script elements with type attributes, but we should get rid of that QA check anyway because things like <script type="module"> would be awesome to start using... (In reply to David Cook from comment #28) > (In reply to David Cook from comment #27) > > I've suggested on Mattermost that we start using <script > > type="application/json"> for storing Template::Toolkit output for > > Javascript. > > That said, I think the QA script might flag script elements with type > attributes, but we should get rid of that QA check anyway because things > like <script type="module"> would be awesome to start using... Actually no... I think they just fail if the type is application/javascript or something, so it should be fine... |