I'm not sure when this crept in. At some point we switched from storing material type in illrequests.medium to an arbitrary attribute in illrequestattributes. However, the display end of things wasn't updated accordingly. As such, material type doesn't display in any of the UIs, it should!
Created attachment 76052 [details] [review] Bug 20941: Add Illrequest->getType method This patch adds a method to the Illrequest object enabling it to return its type, which is stored as an Illrequestattribute object To observe broken state: - Do not apply patch - Ensure you have at least one Interlibrary loan created - In OPAC view, go to "your interlibrary loan requests" - => TEST: Observe that the "Request type" column is not populated - Click on "View" for one of the requests - => TEST: Observer that the "Request type" row is not populated - In Staff view, go to "ILL requests" - Click on "Manage request" on a request - => TEST: Observer that the "Request type" row is not populated To Test: - Apply patch - In OPAC view, go to "your interlibrary loan requests" - => TEST: Observe that the "Request type" column IS populated - Click on "View" for one of the requests - => TEST: Observer that the "Request type" row IS populated - In Staff view, go to "ILL requests" - Click on "Manage request" on a request - => TEST: Observer that the "Request type" row IS populated
Created attachment 79577 [details] [review] Bug 20941: Add Illrequest->getType method This patch adds a method to the Illrequest object enabling it to return its type, which is stored as an Illrequestattribute object To observe broken state: - Do not apply patch - Ensure you have at least one Interlibrary loan created - In OPAC view, go to "your interlibrary loan requests" - => TEST: Observe that the "Request type" column is not populated - Click on "View" for one of the requests - => TEST: Observer that the "Request type" row is not populated - In Staff view, go to "ILL requests" - Click on "Manage request" on a request - => TEST: Observer that the "Request type" row is not populated To Test: - Apply patch - In OPAC view, go to "your interlibrary loan requests" - => TEST: Observe that the "Request type" column IS populated - Click on "View" for one of the requests - => TEST: Observer that the "Request type" row IS populated - In Staff view, go to "ILL requests" - Click on "Manage request" on a request - => TEST: Observer that the "Request type" row IS populated Signed-off-by: Niamh.Walker-Headon@it-tallaght.ie
Created attachment 79825 [details] [review] Bug 20941: Add unit tests
Comment on attachment 79577 [details] [review] Bug 20941: Add Illrequest->getType method Review of attachment 79577 [details] [review]: ----------------------------------------------------------------- ::: Koha/Illrequest.pm @@ +724,4 @@ > || ""; # "the empty prefix" > } > > +=head3 getType should be get_type, or maybe better get_material_type, please rename the method @@ +734,5 @@ > + > +sub getType { > + my ($self) = @_; > + my $attr = $self->illrequestattributes->find({ type => 'type'}); > + return $attr ? $attr->value : '<span>N/A</span>'; We should return implicit undef if no type is set and then handle it in templates.
Created attachment 79865 [details] [review] Bug 20941: (follow-up) Carry out comment #4 advice - Switch getType -> get_type - get_type now returns a type or undef - Ternary now performed in template
Thanks for the review Joseph, all great points, much appreciated.
s/ph$/f/ (sorry!)
Created attachment 79918 [details] [review] Bug 20941: Add Illrequest->getType method This patch adds a method to the Illrequest object enabling it to return its type, which is stored as an Illrequestattribute object To observe broken state: - Do not apply patch - Ensure you have at least one Interlibrary loan created - In OPAC view, go to "your interlibrary loan requests" - => TEST: Observe that the "Request type" column is not populated - Click on "View" for one of the requests - => TEST: Observer that the "Request type" row is not populated - In Staff view, go to "ILL requests" - Click on "Manage request" on a request - => TEST: Observer that the "Request type" row is not populated To Test: - Apply patch - In OPAC view, go to "your interlibrary loan requests" - => TEST: Observe that the "Request type" column IS populated - Click on "View" for one of the requests - => TEST: Observer that the "Request type" row IS populated - In Staff view, go to "ILL requests" - Click on "Manage request" on a request - => TEST: Observer that the "Request type" row IS populated Signed-off-by: Niamh.Walker-Headon@it-tallaght.ie Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 79919 [details] [review] Bug 20941: Add unit tests Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 79920 [details] [review] Bug 20941: (follow-up) Carry out comment #4 advice - Switch getType -> get_type - get_type now returns a type or undef - Ternary now performed in template Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 79921 [details] [review] Bug 20941: (QA follow-up) Fix return value of get_type, make templates more consistant Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
I added a follow-up: - in get_type the return could be explicit undef, which is wrong - the test needed to adjust to test for undef when no type is set - in templates I changed ?: to || for consistancy - in opac templates I removed span tag I've also added filters during rebase
Thanks for that Josef, much better! The inclusion of the span was based on advice I've been given in the past that wrapping strings with a span aids with translation, though I'm not clear how. Anyway, many thanks for your work on this, it's much appreciated!
(In reply to Andrew Isherwood from comment #13) > Thanks for that Josef, much better! The inclusion of the span was based on > advice I've been given in the past that wrapping strings with a span aids > with translation, though I'm not clear how. Anyway, many thanks for your > work on this, it's much appreciated! Yes it is right, I saw you done that on other patches... so maybe we should do it here right now...
Created attachment 79945 [details] [review] Bug 20941: (follow-up) Wrap N/A strings in <span> All N/A strings in ILL templates wrapped in <span> to aid translation
Hi Josef, I've now wrapped all N/A in <span> tags, so this should be correct and consistent now. Many thanks.
It would be good to test that this construct actually gets picked up. I am not sure if our script look for HTML inside the TT constructs... I think it would be safer not to take shortcuts here: [% request.cost || '<span>N/A</span>' | html %] Could also be: [% IF ( request.cost ) %][%request.cost%][% ELSE %]<span>N/A</span>[% END %] To test, in koha-shell you can do: cd misc/translator perl translate update de-DE (or any other language) Then check the updated po files for N/A.
... also... should the html filter not escape that?
[% request.cost | html %] of course (update your QA script!)
(In reply to Katrin Fischer from comment #17) > It would be good to test that this construct actually gets picked up. I am > not sure if our script look for HTML inside the TT constructs... I think it > would be safer not to take shortcuts here: > > [% request.cost || '<span>N/A</span>' | html %] True, this also has problem, that html tags are escaped an showed to user... > > Could also be: > > [% IF ( request.cost ) %][%request.cost%][% ELSE %]<span>N/A</span>[% END %] Thanks Katrin, that's the way it should be done... > To test, in koha-shell you can do: > > cd misc/translator > perl translate update de-DE (or any other language) > > Then check the updated po files for N/A.
Thanks very much for that Katrin. I'll create a follow up for these today.
Created attachment 79999 [details] [review] Bug 20941: (follow-up) Update N/A handling Modify the conditional display of N/A in accordance with comment #17
Created attachment 80112 [details] [review] Bug 20941: Add Illrequest->getType method This patch adds a method to the Illrequest object enabling it to return its type, which is stored as an Illrequestattribute object To observe broken state: - Do not apply patch - Ensure you have at least one Interlibrary loan created - In OPAC view, go to "your interlibrary loan requests" - => TEST: Observe that the "Request type" column is not populated - Click on "View" for one of the requests - => TEST: Observer that the "Request type" row is not populated - In Staff view, go to "ILL requests" - Click on "Manage request" on a request - => TEST: Observer that the "Request type" row is not populated To Test: - Apply patch - In OPAC view, go to "your interlibrary loan requests" - => TEST: Observe that the "Request type" column IS populated - Click on "View" for one of the requests - => TEST: Observer that the "Request type" row IS populated - In Staff view, go to "ILL requests" - Click on "Manage request" on a request - => TEST: Observer that the "Request type" row IS populated Signed-off-by: Niamh.Walker-Headon@it-tallaght.ie Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 80113 [details] [review] Bug 20941: Add unit tests Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 80114 [details] [review] Bug 20941: (follow-up) Carry out comment #4 advice - Switch getType -> get_type - get_type now returns a type or undef - Ternary now performed in template Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 80115 [details] [review] Bug 20941: (QA follow-up) Fix return value of get_type, make templates more consistant Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 80116 [details] [review] Bug 20941: (follow-up) Wrap N/A strings in <span> All N/A strings in ILL templates wrapped in <span> to aid translation
Created attachment 80117 [details] [review] Bug 20941: (follow-up) Update N/A handling Modify the conditional display of N/A in accordance with comment #17
Created attachment 80127 [details] [review] Bug 20941: Add Illrequest->getType method This patch adds a method to the Illrequest object enabling it to return its type, which is stored as an Illrequestattribute object To observe broken state: - Do not apply patch - Ensure you have at least one Interlibrary loan created - In OPAC view, go to "your interlibrary loan requests" - => TEST: Observe that the "Request type" column is not populated - Click on "View" for one of the requests - => TEST: Observer that the "Request type" row is not populated - In Staff view, go to "ILL requests" - Click on "Manage request" on a request - => TEST: Observer that the "Request type" row is not populated To Test: - Apply patch - In OPAC view, go to "your interlibrary loan requests" - => TEST: Observe that the "Request type" column IS populated - Click on "View" for one of the requests - => TEST: Observer that the "Request type" row IS populated - In Staff view, go to "ILL requests" - Click on "Manage request" on a request - => TEST: Observer that the "Request type" row IS populated Signed-off-by: Niamh.Walker-Headon@it-tallaght.ie Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 80128 [details] [review] Bug 20941: Add unit tests Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 80129 [details] [review] Bug 20941: (follow-up) Carry out comment #4 advice - Switch getType -> get_type - get_type now returns a type or undef - Ternary now performed in template Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 80130 [details] [review] Bug 20941: (QA follow-up) Fix return value of get_type, make templates more consistant Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 80131 [details] [review] Bug 20941: (follow-up) Wrap N/A strings in <span> All N/A strings in ILL templates wrapped in <span> to aid translation
Created attachment 80132 [details] [review] Bug 20941: (follow-up) Update N/A handling Modify the conditional display of N/A in accordance with comment #17
There are some weird things going on: - Add a new requst with free form - Edit item metadata - The Type pull down is empty This seems to be a spelling mismatch. The type for new requests saves with the first letter being lower case (article) while the the pull downs look for (Article) I am also not clear on the switch from medium to type, there are still lines like these in the code: media => [ "Book", "Article", "Journal" ], (ill-requests.pl) Can we clear this up? It's also a little odd that the rquest type shows in 2 places, in the "Request details" and the "Details from supplier" sections.
Created attachment 80212 [details] [review] Bug 20941: (follow-up) Switch 'media' -> 'types' Clearing up the inconsistency mentioned in comment #35. There is one place where we use the term 'media' for a template variable, everywhere we refer to material types as 'types'. NOTE: This is a breaking change for existing backends that still use 'media'. Of the PTFS Europe backends, only the Koha backend uses it, this will be modified as necessary. Generally backends will supply this variable themselves, so the breaking-ness of this change should be minimal. No test plan as it's backend dependent.
Responses to comment #35: >This seems to be a spelling mismatch. The type for new requests saves with the first letter being lower case (article) while the the pull downs look for (Article) This was a bug in the FreeForm backend and has now been fixed. Details: https://github.com/PTFS-Europe/koha-ill-freeform/commit/ddff82bc9adc4bd7205192543b6e18c4ad844bb0 >I am also not clear on the switch from medium to type, there are still lines like these in the code: media => [ "Book", "Article", "Journal" ], (ill-requests.pl) This has been fixed, see comment #36 >It's also a little odd that the rquest type shows in 2 places, in the "Request details" and the "Details from supplier" sections. The details listed here are all metadata relating to the request and will be dependent on the backend from which the request was made, some backends (BLDSS for example) won't even include "type". Whether this display is even necessary is debatable, since bug 20561 added a modal that displays all this information, it just formats it a bit less nicely. I think there's room for improvement in this area, but would suggest it as a future enhancement rather than addressing it here.
Hi Andrew, switching back to signed-off with your follow-up
Created attachment 81688 [details] [review] Bug 20941: Add Illrequest->getType method This patch adds a method to the Illrequest object enabling it to return its type, which is stored as an Illrequestattribute object To observe broken state: - Do not apply patch - Ensure you have at least one Interlibrary loan created - In OPAC view, go to "your interlibrary loan requests" - => TEST: Observe that the "Request type" column is not populated - Click on "View" for one of the requests - => TEST: Observer that the "Request type" row is not populated - In Staff view, go to "ILL requests" - Click on "Manage request" on a request - => TEST: Observer that the "Request type" row is not populated To Test: - Apply patch - In OPAC view, go to "your interlibrary loan requests" - => TEST: Observe that the "Request type" column IS populated - Click on "View" for one of the requests - => TEST: Observer that the "Request type" row IS populated - In Staff view, go to "ILL requests" - Click on "Manage request" on a request - => TEST: Observer that the "Request type" row IS populated Signed-off-by: Niamh.Walker-Headon@it-tallaght.ie Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 81689 [details] [review] Bug 20941: Add unit tests Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 81690 [details] [review] Bug 20941: (follow-up) Carry out comment #4 advice - Switch getType -> get_type - get_type now returns a type or undef - Ternary now performed in template Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 81691 [details] [review] Bug 20941: (QA follow-up) Fix return value of get_type, make templates more consistant Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 81692 [details] [review] Bug 20941: (follow-up) Wrap N/A strings in <span> All N/A strings in ILL templates wrapped in <span> to aid translation Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 81693 [details] [review] Bug 20941: (follow-up) Update N/A handling Modify the conditional display of N/A in accordance with comment #17 Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 81694 [details] [review] Bug 20941: (follow-up) Switch 'media' -> 'types' Clearing up the inconsistency mentioned in comment #35. There is one place where we use the term 'media' for a template variable, everywhere we refer to material types as 'types'. NOTE: This is a breaking change for existing backends that still use 'media'. Of the PTFS Europe backends, only the Koha backend uses it, this will be modified as necessary. Generally backends will supply this variable themselves, so the breaking-ness of this change should be minimal. No test plan as it's backend dependent. Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Awesome work all! Pushed to master for 18.11