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.
+1 This would be great if it could be applied to all notices using Template Toolkit (particularly ISSUE and HOLD slips).
+1
+1 for sure!
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.
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.
> 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?
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.
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.
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.
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 %]
Bug 20443 adds Koha::Patron::get_extended_attribute_value which works for non-repeatable attributes fwiw.
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>
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>
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).
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.
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?
I'm going to be away the next few days, but I'll take a look at this next week
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?
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
(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?
(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.
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.
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 %]
Created attachment 173819 [details] [review] Bug 30657: Move to AUTOLOAD
Created attachment 173901 [details] [review] Bug 30657: Move to AUTOLOAD
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.
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.
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.
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 %]
Created attachment 173912 [details] [review] Bug 30657: Move to AUTOLOAD
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.
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
Have you discussed adding a prefix to those accessors for safety? Something like `attr_$code`, for example.
(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.
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.