Bug 30657 - Make patron attributes available via Template Toolkit in notices
Summary: Make patron attributes available via Template Toolkit in notices
Status: Signed Off
Alias: None
Product: Koha
Classification: Unclassified
Component: Notices (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement
Assignee: Martin Renvoize (ashimema)
QA Contact: Testopia
URL:
Keywords: Hackfest, roadmap_24_11
Depends on:
Blocks: 15277 15278 36270 36588
  Show dependency treegraph
 
Reported: 2022-04-29 15:08 UTC by Andrew Fuerste-Henry
Modified: 2025-01-16 10:39 UTC (History)
16 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
Circulation function:


Attachments
Bug 30657: Unit tests (1.98 KB, patch)
2024-07-16 15:49 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 30657: Dynamically add attribute accessors (3.42 KB, patch)
2024-07-16 15:49 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 30657: Unit tests (2.03 KB, patch)
2024-07-16 18:15 UTC, Brendan Lawlor
Details | Diff | Splinter Review
Bug 30657: Dynamically add attribute accessors (3.47 KB, patch)
2024-07-16 18:15 UTC, Brendan Lawlor
Details | Diff | Splinter Review
Bug 30657: Unit tests (1.98 KB, patch)
2024-10-31 16:40 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 30657: Dynamically add attribute accessors (3.42 KB, patch)
2024-10-31 16:40 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 30657: Move to AUTOLOAD (2.72 KB, patch)
2024-10-31 16:40 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 30657: Move to AUTOLOAD (2.76 KB, patch)
2024-11-04 07:48 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 30657: Try parent method first (4.07 KB, patch)
2024-11-04 09:25 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 30657: Unit tests (1.98 KB, patch)
2024-11-04 12:00 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 30657: Dynamically add attribute accessors (3.42 KB, patch)
2024-11-04 12:00 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 30657: Move to AUTOLOAD (2.76 KB, patch)
2024-11-04 12:00 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 30657: Try parent method first (4.10 KB, patch)
2024-11-04 12:00 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 30657: Further unit tests (4.29 KB, patch)
2024-11-04 12:00 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Fuerste-Henry 2022-04-29 15:08:11 UTC
When using Template Toolkit to fetch patron values, one should be able to fetch patron attribute values. This would be useful both for printing those values into the notice content and for using those values in logic to vary the notices per patron.
Comment 1 Michael Adamyk 2022-05-03 14:56:34 UTC
+1
This would be great if it could be applied to all notices using Template Toolkit (particularly ISSUE and HOLD slips).
Comment 2 Katrin Fischer 2022-06-17 23:06:09 UTC
+1
Comment 3 George Williams (NEKLS) 2023-03-16 14:36:49 UTC
+1
Comment 4 Christopher Brannon 2023-03-16 17:18:29 UTC
+1 for sure!
Comment 5 Rebecca Coert 2023-10-10 16:59:54 UTC
+1
Comment 6 Barbara Johnson 2023-11-16 19:02:19 UTC
+1
Comment 7 Martin Renvoize (ashimema) 2024-03-08 10:20:53 UTC
There are already a couple of options here I believe.. but I think we should perhaps make it simpler/clearer.

You can use all accessors available from the Koha::Patron object assuming you've been passed said object into the template.

We have a few different attribute accessors:

* 'extended_attributes' which returns a resultset of attached extended attributes
* 'get_extended_attribute(code)' which returns the attribute associated with the code passed (but doesn't work properly for repeatable attributes)

To use those in TT:

*  [% FOREACH extendedattribute IN patron.extended_attributes %]
        <li class="patronattribute">
            <span class="patronattributelabel">[% extendedattribute.type.description | html %]</span>: [% extendedattribute.description | html %]
        </li>
    [% END %]

* [% SET attribute = patron.get_extended_attribute('code') %]
  [% attribute.description | html %]

The second one is clearly simpler to use, but only works for non-repeatable attributes so you need to know a little about the underlying patron data setup.  I also think it's a get wordy 'get_' and would prefer, I think, to have a simple 'extended_attribute($code)' accessor.
Comment 8 Martin Renvoize (ashimema) 2024-03-08 10:33:11 UTC
Annoyingly the extended_attributes accessor is also set to be a setter which means it won't pass through any filters you want to pass.. we should really rectify that.

In my opinion we should have two simple accessors that both accept standard dbic query filters.

* extended_attributes($where, $attr);
* extended_attribute($where,$attr);

We wrap our DBIx::Clas objects and thus remove the nice easy scalar handling of _rs on accessors which means we also have to deal with that in 'clever' ways at the TT level.. I can't remember the details for that off the top of my head though.

We really need to document all this better somewhere in short.
Comment 9 Katrin Fischer 2024-03-08 16:34:50 UTC
> In my opinion we should have two simple accessors that both accept standard
> dbic query filters.
> 
> * extended_attributes($where, $attr);
> * extended_attribute($where,$attr);

I like the look of this and I think you have made a very good point to make it easier to use. This will help people creating notices, but also be generally useful.
In your example code: $attr would be the attribute code. What is $where here?
Comment 10 Martin Renvoize (ashimema) 2024-03-08 16:43:59 UTC
Sorry, I wasn't clear with what those parameters meant.   They're a fairly standard pair of names for dbix query parameters.. i.e where is the SQL abstract where "field => value", then attr is other conditions like joins.

But that give perhaps more complication than end users in notices want..  it's a balance between simplicity and functionality.
Comment 11 Lucas Gass (lukeg) 2024-04-24 15:09:54 UTC
A real world example I had to cook up for a HOLD_SLIP:

[% SET alt_id = '' %]
[% FOREACH ba IN borrower.extended_attributes %]
     [% IF ba.code == 'UNCLE' %]
              [% alt_id = ba.attribute %]
     [% END %]
[% END %]

[% IF alt_id == 'BOB' %]
    Warning: Bob is your uncle.
[% ELSE %]
    Warning: Bob is NOT your uncle.
[% END %]


Would be nice to make it a bit easier.
Comment 12 Martin Renvoize (ashimema) 2024-07-16 15:49:52 UTC
Created attachment 169049 [details] [review]
Bug 30657: Unit tests

This patch adds some basic unit tests for dynamically added accessors
based on extended patron attributes.
Comment 13 Martin Renvoize (ashimema) 2024-07-16 15:49:55 UTC
Created attachment 169050 [details] [review]
Bug 30657: Dynamically add attribute accessors

This patch adds dynamic creation of accessors for extended patron
attributes based on attribute code.

It should work for Koha::Patron objects resulting from calls to
Koha::Patron->new(), Koha::Patrons->search, Koha::Patrons->find and any
relations that return a Koha::Patron object.

We return a single value for non-repeatable attributes and an arrayref
of values for repeatable ones.

This should make it simple to access such attributes in Koha Notice
Templates.

Simply use the 'code' in your variables.. example:

You've added a 'smartcard' extended attribute to your system, you would
refer to it in your notice as [% patron.smartcard | html %]
Comment 14 Kyle M Hall (khall) 2024-07-16 18:10:34 UTC
Bug 20443 adds Koha::Patron::get_extended_attribute_value which works for non-repeatable attributes fwiw.
Comment 15 Brendan Lawlor 2024-07-16 18:15:47 UTC
Created attachment 169056 [details] [review]
Bug 30657: Unit tests

This patch adds some basic unit tests for dynamically added accessors
based on extended patron attributes.

Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org>
Comment 16 Brendan Lawlor 2024-07-16 18:15:50 UTC
Created attachment 169057 [details] [review]
Bug 30657: Dynamically add attribute accessors

This patch adds dynamic creation of accessors for extended patron
attributes based on attribute code.

It should work for Koha::Patron objects resulting from calls to
Koha::Patron->new(), Koha::Patrons->search, Koha::Patrons->find and any
relations that return a Koha::Patron object.

We return a single value for non-repeatable attributes and an arrayref
of values for repeatable ones.

This should make it simple to access such attributes in Koha Notice
Templates.

Simply use the 'code' in your variables.. example:

You've added a 'smartcard' extended attribute to your system, you would
refer to it in your notice as [% patron.smartcard | html %]

Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org>
Comment 17 Brendan Lawlor 2024-07-16 18:19:46 UTC
Test notes:

This worked but I found the name of the object was borrower instead of patron:
[% borrower.party_mode | html %]

The test Patron.t passed everything, but the qa script returned one failure:
 FAIL   Koha/Patron.pm
   FAIL   critic
                # TestingAndDebugging::ProhibitNoStrict: Got 1 violation(s).
Comment 18 Martin Renvoize (ashimema) 2024-07-17 06:09:50 UTC
We're really inconsistent in our notice varaibles unfortunately.. sometimes it's borrower, sometimes it's patron and other times it's more descriptive like 'owner' or 'librarian'.

I'd really like to try and use the more prescriptive variable names long term and in my preference, I'd rather see fewer top-level variables available and instead encourage end users to understand the relationships between things to be able to lookup the information they need via object traversal.  In reality, we need to really improve the editor to help with that.

Thanks for testing.
Comment 19 Martin Renvoize (ashimema) 2024-07-17 06:31:59 UTC
What are people's general opinion on the accessors proposed here?

With this patch we add accessors per attribute code.

For non-repatable it's as simple as:
[% borrower.attribute_code %]

Whilst for repeatable we have:
[% FOR attribute IN borrower.attribute_code %]
    [% attribute %]
[% END %]

This adds a bit of code complexity and possibly has some effect on performance whilst we do already offer some alternatives.

For non-repeatable we already support:
[% attribute = borrower.get_extended_attribute(attribute_code) %]
[% attribute.description %]

For repeatable we have
[% FOREACH ba IN borrower.extended_attributes %]
     [% IF ba.code == 'UNCLE' %]
              [% alt_id = ba.attribute %]
     [% END %]
[% END %]

Is the extra code I introduce here to simplify for the end user worth it's weight?
Comment 20 David Cook 2024-07-17 07:24:05 UTC
I'm going to be away the next few days, but I'll take a look at this next week
Comment 21 Emily Lamancusa (emlam) 2024-07-23 20:53:50 UTC
I really like the new accessors added in this patch! I do think there's a lot of value in making it more intuitive to access extended attributes in notice templates, since that can be a big usability improvement.

The performance question is worth considering... If I'm correct in thinking that Koha::Patron::Attribute::Types->search(); will add an extra database call each time a patron object is created, I could see how that may add up when a script creates a large number of patron objects. Can we work the same basic logic into an AUTOLOAD instead, so that it won't run unless it's actually needed?
Comment 22 Martin Renvoize (ashimema) 2024-10-08 14:30:02 UTC
Well.. I did actually have some code that does it in an autoload.. we stripped it from another submission recently.. I'll dig it out
Comment 23 Ray Delahunty 2024-10-13 15:02:09 UTC
(In reply to Martin Renvoize (ashimema) from comment #18)
> We're really inconsistent in our notice variables unfortunately.. sometimes
> it's borrower, sometimes it's patron 
> 
> [...] encourage end users to understand the relationships
> between things to be able to lookup the information they need via object
> traversal.

Am I off on a tangent here? This may be one of the reasons we librarians trying to use template toolkit to tidy up our notices struggle with TT. The table is borrowers in Koha but in template toolkit it is borrower. I'm gradually knowing what to use (not that I know why). Is there something like a schema for TT (probably a silly question), or can the TT variables (accessors??) gradually replace the alligator code on the left hand side of the notice design interface?
Comment 24 Martin Renvoize (ashimema) 2024-10-31 16:22:14 UTC
(In reply to Ray Delahunty from comment #23)
> (In reply to Martin Renvoize (ashimema) from comment #18)
> > We're really inconsistent in our notice variables unfortunately.. sometimes
> > it's borrower, sometimes it's patron 
> > 
> > [...] encourage end users to understand the relationships
> > between things to be able to lookup the information they need via object
> > traversal.
> 
> Am I off on a tangent here? This may be one of the reasons we librarians
> trying to use template toolkit to tidy up our notices struggle with TT. The
> table is borrowers in Koha but in template toolkit it is borrower. I'm
> gradually knowing what to use (not that I know why). Is there something like
> a schema for TT (probably a silly question), or can the TT variables
> (accessors??) gradually replace the alligator code on the left hand side of
> the notice design interface?

Unfortunately Ray it's not quite as simple as that :(.

There's lots of history here.. and some technical challenges too.

Notices are all generated by called to a method called 'GetPreparedLetter'.. that method is passed the template (which you define in your notices) and a "bunch of variables" the template can use.  The challenge comes in identifying which variables are likely to get passed into the call to GetPreparedLetter so you can use them in your template.. it's a bit of a 'chicken and the egg' problem.. before we call the method we can't know what's in there.. but we can't call the method until we have a template to pass it.. but how does the person building the template know what variables that can use to build their template?

I've been wanting for a long time to work on cleaning up our notice generation and write such a schema so the notice editor can definitely know what variables it's getting passed for each notice type and you can therefore build notices with more knowledge. (right now the <<>> helpers are a 'best guess' and often get it wrong).

Back to this bug.. the aim here is to make extended patron attributes more accessible for any notice that pass passed a patron object.. said patron objects might be exposed with various variable names.. most often 'borrower'.. but I've also seen 'librarian', 'patron', 'user', 'author' etc etc in use... we need that schema writing and maintaining in the future.
Comment 25 Martin Renvoize (ashimema) 2024-10-31 16:40:18 UTC
Created attachment 173817 [details] [review]
Bug 30657: Unit tests

This patch adds some basic unit tests for dynamically added accessors
based on extended patron attributes.
Comment 26 Martin Renvoize (ashimema) 2024-10-31 16:40:22 UTC
Created attachment 173818 [details] [review]
Bug 30657: Dynamically add attribute accessors

This patch adds dynamic creation of accessors for extended patron
attributes based on attribute code.

It should work for Koha::Patron objects resulting from calls to
Koha::Patron->new(), Koha::Patrons->search, Koha::Patrons->find and any
relations that return a Koha::Patron object.

We return a single value for non-repeatable attributes and an arrayref
of values for repeatable ones.

This should make it simple to access such attributes in Koha Notice
Templates.

Simply use the 'code' in your variables.. example:

You've added a 'smartcard' extended attribute to your system, you would
refer to it in your notice as [% patron.smartcard | html %]
Comment 27 Martin Renvoize (ashimema) 2024-10-31 16:40:25 UTC
Created attachment 173819 [details] [review]
Bug 30657: Move to AUTOLOAD
Comment 28 Martin Renvoize (ashimema) 2024-11-04 07:48:43 UTC
Created attachment 173901 [details] [review]
Bug 30657: Move to AUTOLOAD
Comment 29 Martin Renvoize (ashimema) 2024-11-04 07:50:50 UTC
So, whilst this still works as an AUTOLOAD I think the code is very slightly less clear and because we actually autoload so much from the parent (include ALL the object field accessors) we don't actually save the DB call we were aiming to save at all..  We could possibly try running the parent AUTOLOAD prior to the child AUTOLOAD from within the child to negate this.. but I couldn't get that to pass the tests consistently yet.
Comment 30 Martin Renvoize (ashimema) 2024-11-04 09:25:34 UTC
Created attachment 173902 [details] [review]
Bug 30657: Try parent method first

This patch adds a try/catch inside the Koha::Patron AUTOLOAD to try and
find the method on the parent, including it's AUTOLOAD, prior to running
the local AUTOLOAD code to generate attribute accessors and then use the
called method.
Comment 31 Martin Renvoize (ashimema) 2024-11-04 12:00:04 UTC
Created attachment 173910 [details] [review]
Bug 30657: Unit tests

This patch adds some basic unit tests for dynamically added accessors
based on extended patron attributes.
Comment 32 Martin Renvoize (ashimema) 2024-11-04 12:00:08 UTC
Created attachment 173911 [details] [review]
Bug 30657: Dynamically add attribute accessors

This patch adds dynamic creation of accessors for extended patron
attributes based on attribute code.

It should work for Koha::Patron objects resulting from calls to
Koha::Patron->new(), Koha::Patrons->search, Koha::Patrons->find and any
relations that return a Koha::Patron object.

We return a single value for non-repeatable attributes and an arrayref
of values for repeatable ones.

This should make it simple to access such attributes in Koha Notice
Templates.

Simply use the 'code' in your variables.. example:

You've added a 'smartcard' extended attribute to your system, you would
refer to it in your notice as [% patron.smartcard | html %]
Comment 33 Martin Renvoize (ashimema) 2024-11-04 12:00:11 UTC
Created attachment 173912 [details] [review]
Bug 30657: Move to AUTOLOAD
Comment 34 Martin Renvoize (ashimema) 2024-11-04 12:00:14 UTC
Created attachment 173913 [details] [review]
Bug 30657: Try parent method first

This patch adds a try/catch inside the Koha::Patron AUTOLOAD to try and
find the method on the parent, including it's AUTOLOAD, prior to running
the local AUTOLOAD code to generate attribute accessors and then use the
called method.
Comment 35 Martin Renvoize (ashimema) 2024-11-04 12:00:17 UTC
Created attachment 173914 [details] [review]
Bug 30657: Further unit tests

Test the repeatable handling and error rethrow.  There's also an attempt
at counting calls to the database for subsequent attribute accessors,
but it fails in an odd way I'm struggling to understand, so that test is
commented for now
Comment 36 Tomás Cohen Arazi (tcohen) 2024-11-06 14:17:47 UTC
Have you discussed adding a prefix to those accessors for safety? Something like `attr_$code`, for example.
Comment 37 Martin Renvoize (ashimema) 2024-11-08 12:38:10 UTC
(In reply to Tomás Cohen Arazi (tcohen) from comment #36)
> Have you discussed adding a prefix to those accessors for safety? Something
> like `attr_$code`, for example.

For ILL that's exactly what we did.. but you also won't hit the attribute accessor here if you clash with an existing patron field name.  Honestly, I wasn't sure which way to jump, but the basic code only felt nicer looking to me for end users.
Comment 38 Jonathan Druart 2025-01-16 10:39:42 UTC
This is nice but could be confusing. I would prefer an explicit way to retrieve the extended attributes.

patron.get_extended_attribute("attribute")
patron.extended_attributes.get("attribute")
patron.extended_attributes["attribute"]

I don't think adding (black) magic for people writing template notices is a good idea.