To recreate: - create a course in course reserves, add an item to it. - confirm your item shows Available for its status on course-details.pl - edit your item to be withdrawn, lost, damaged, notforloan, and restricted use - reload course-details.pl, confirm it still shows available filed bug 28761 for this on the OPAC. On the OPAC, withdrawn, lost, and damaged *do* update the status.
Created attachment 123233 [details] [review] Bug 28762: Handle different item statuses in course details To test: 1. create a course in course reserves, add an item to it. 2. confirm your item shows Available for its status on course-details.pl 3. edit your item to be withdrawn, lost, damaged, notforloan, and restricted use 4. reload course-details.pl, confirm it still shows available 5. apply patch 6. repeat step 3 with each of the statuses and make sure it correctly shows on course-details.pl https://bugs.koha-community.org/show_bug.cgi?id=28761
Created attachment 123234 [details] [review] Bug 28762: add staff item-status.inc and better handle statues on course-details.tt This patch adds an item-status.inc to the staff side much like what is already in place on the OPAC. To test: 1. create a course in course reserves, add an item to it. 2. confirm your item shows Available for its status on course-details.pl 3. edit your item to be withdrawn, lost, damaged, notforloan, and restricted use 4. reload course-details.pl, confirm it still shows available 5. apply patch 6. repeat step 3 with each of the statuses and make sure it correctly shows on course-details.pl
Created attachment 123236 [details] [review] Bug 28762: handle notforloan better
Created attachment 123237 [details] [review] Bug 28762: handle notforloan better
Hi Lucas, I can confirm that the patch fixes the Status of the course reserve item to show whether it is withdrawn, damaged, etc. However, if it is none of those, the status column no longer shows Available, but instead is blank. I am assuming it should say available if none of those values you listed are true? Cheers, Hayley
Created attachment 124661 [details] [review] Bug 28762: (follow-up) fix template logic with Available
(In reply to Hayley Pelham from comment #5) > Hi Lucas, > > I can confirm that the patch fixes the Status of the course reserve item to > show whether it is withdrawn, damaged, etc. > > However, if it is none of those, the status column no longer shows > Available, but instead is blank. > > I am assuming it should say available if none of those values you listed are > true? > > Cheers, > Hayley Thanks Haley! I am have made a correction and I am reseting the status of this one to 'Needs sign-off'
Created attachment 124666 [details] [review] Bug 28762: add staff item-status.inc and better handle statues on course-details.tt This patch adds an item-status.inc to the staff side much like what is already in place on the OPAC. To test: 1. create a course in course reserves, add an item to it. 2. confirm your item shows Available for its status on course-details.pl 3. edit your item to be withdrawn, lost, damaged, notforloan, and restricted use 4. reload course-details.pl, confirm it still shows available 5. apply patch 6. repeat step 3 with each of the statuses and make sure it correctly shows on course-details.pl Signed-off-by: Hayley Pelham <hayleypelham@catalyst.net.nz>
Created attachment 124667 [details] [review] Bug 28762: handle notforloan better Signed-off-by: Hayley Pelham <hayleypelham@catalyst.net.nz>
Created attachment 124668 [details] [review] Bug 28762: (follow-up) fix template logic with Available Signed-off-by: Hayley Pelham <hayleypelham@catalyst.net.nz>
QA: Looking here
My concern here is introducing a item-status.inc that will be deviating from the opac version quite a bit already. Thinking about how we could keep them together. Maybe a few more lines here or there, but if they could be the same?
Created attachment 124744 [details] [review] Bug 28762: (QA follow-up) A first attempt to sync the includes This is just a start. Should we resolve all differences?
Jonathan: What do you think? Get the includes correct here and enforce synced changes or just push them while different, and hope to keep them closer? ;)
Obviously you would need to pass staff => 1 now too in the course reserves page in staff client.. If we go this direction?
(In reply to Marcel de Rooy from comment #14) > Jonathan: What do you think? Get the includes correct here and enforce > synced changes or just push them while different, and hope to keep them > closer? ;) Hard call, I don't think we should have them all in sync. Maybe we could have a 'common' directory with some include files that would be identically both sides? We even could have a test to ensure it.
+ [% IF ( OPACShowCheckoutName ) %] this is wrong for staff, there is no test on "opac".
(In reply to Jonathan Druart from comment #17) > + [% IF ( OPACShowCheckoutName ) %] > > this is wrong for staff, there is no test on "opac". Yeah. I did not finish this sync operation. But wanted to get some discussion first. It should be possible to add some opac/staff checks and have them exactly identical. But it might be to hard to require that here.
Comment on attachment 124744 [details] [review] Bug 28762: (QA follow-up) A first attempt to sync the includes Coming here again, I think we are asking too much when insisting on a synced item status include. So I am obsoleting this follow-up.
So from this perspective looking at a new include, I see things like: [% IF item.isa('Koha::Item') %] .. [% IF NOT ( item.isa('Koha::Item') ) AND item.transfertwhen %] Which is awful. Yes, I know that it comes from the opac include, but we should really solve its twisted usage. When we are starting at staff here, please use one consistent approach. Either always pass a Koha object, or do something else. But keep it consistent.
Changing status for comment17 and comment20
*** Bug 29095 has been marked as a duplicate of this bug. ***
OK, this certainly needs some work.. if we're going to introduce an 'item-status' include here I feel it should really be re-usable and simple/clear. For that to be the case we should really be expecting a simple 'item' object to be passed and not deal with special cases for weird structures. I propose updating the minimal controller to use Koha:: Objects instead of C4:: classes and methods for these item details.
Created attachment 125220 [details] [review] Bug 28762: Use Koha::Course in course-details controller This patch updates Koha::Course to include the 'instructors' relation accessor and then update the course-details controller to use the Koha::Course object and pass it to the template instead of building a hash using GetCourse.
My follow-up should allow us to switch out the `isa` in the template include now I believe.
Created attachment 125228 [details] [review] Bug 28762: Update item-status include This patch updates the item-status include so that it expects just an item object making if simpler and more widely re-usable.
Created attachment 125229 [details] [review] Bug 28762: Update item-status include This patch updates the item-status include so that it expects just an item object making if simpler and more widely re-usable.
The include here was heavily inspired by the opac equivalent, the status cell of the holdings table in catalogue/detail.tt and the equivalent include included in bug 28854/bug 24023. What I'm least familiar with here is holds and independent branches, so please test that area carefully. The challenge is holds in the holds table vs holds in the tmp_holdsqueue.. they have different cases but I think I've handled both. As for IndependantBranches.. there's some really funky code in catalogue/details and GetItemInfo that deals with that.. we should be moving to Koha::Item objects in catalogue/details as soon as possible with this patch in my opinion.. but I feel that area may need some work.
Created attachment 125233 [details] [review] Bug 28762: Move notforloan fallback into object
Upon testing this, I found that the Not for Loan status was not showing accurately in the Course Reserve table on the OPAC. Testing for the status availability is correct on Staff Interface. A not-for-loan status shows available regardless of what is chosen (Ordered, Staff Collection, Not for Loan) in the Course Reserve table on the OPAC. Looking at the specific item within the OPAC, the not-for-loan status is showing correctly- specifically, it is using what is on the Authorized Value OPAC description.
It would be nice if the course reserves status would show in course reserves. When items are long long over due this keeps staff from hunting down a book that may look available in the staff interface. They can toggle to the opac to check, but they shouldn't have to.
Created attachment 161563 [details] [review] Bug 28762: add staff item-status.inc and better handle statues on course-details.tt This patch adds an item-status.inc to the staff side much like what is already in place on the OPAC. To test: 1. create a course in course reserves, add an item to it. 2. confirm your item shows Available for its status on course-details.pl 3. edit your item to be withdrawn, lost, damaged, notforloan, and restricted use 4. reload course-details.pl, confirm it still shows available 5. apply patch 6. repeat step 3 with each of the statuses and make sure it correctly shows on course-details.pl Signed-off-by: Hayley Pelham <hayleypelham@catalyst.net.nz>
Created attachment 161564 [details] [review] Bug 28762: handle notforloan better Signed-off-by: Hayley Pelham <hayleypelham@catalyst.net.nz>
Created attachment 161565 [details] [review] Bug 28762: (follow-up) fix template logic with Available Signed-off-by: Hayley Pelham <hayleypelham@catalyst.net.nz>
Created attachment 161566 [details] [review] Bug 28762: Use Koha::Course in course-details controller This patch updates Koha::Course to include the 'instructors' relation accessor and then update the course-details controller to use the Koha::Course object and pass it to the template instead of building a hash using GetCourse.
Created attachment 161567 [details] [review] Bug 28762: Update item-status include This patch updates the item-status include so that it expects just an item object making if simpler and more widely re-usable.
Created attachment 161568 [details] [review] Bug 28762: Move notforloan fallback into object
Created attachment 161569 [details] [review] Bug 28762: add staff item-status.inc and better handle statues on course-details.tt This patch adds an item-status.inc to the staff side much like what is already in place on the OPAC. To test: 1. create a course in course reserves, add an item to it. 2. confirm your item shows Available for its status on course-details.pl 3. edit your item to be withdrawn, lost, damaged, notforloan, and restricted use 4. reload course-details.pl, confirm it still shows available 5. apply patch 6. repeat step 3 with each of the statuses and make sure it correctly shows on course-details.pl Signed-off-by: Hayley Pelham <hayleypelham@catalyst.net.nz> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 161570 [details] [review] Bug 28762: handle notforloan better Signed-off-by: Hayley Pelham <hayleypelham@catalyst.net.nz> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 161571 [details] [review] Bug 28762: (follow-up) fix template logic with Available Signed-off-by: Hayley Pelham <hayleypelham@catalyst.net.nz> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 161572 [details] [review] Bug 28762: Use Koha::Course in course-details controller This patch updates Koha::Course to include the 'instructors' relation accessor and then update the course-details controller to use the Koha::Course object and pass it to the template instead of building a hash using GetCourse. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 161573 [details] [review] Bug 28762: Update item-status include This patch updates the item-status include so that it expects just an item object making if simpler and more widely re-usable. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 161574 [details] [review] Bug 28762: Move notforloan fallback into object Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 161584 [details] [review] Bug 28762: add staff item-status.inc and better handle statues on course-details.tt This patch adds an item-status.inc to the staff side much like what is already in place on the OPAC. To test: 1. create a course in course reserves, add an item to it. 2. confirm your item shows Available for its status on course-details.pl 3. edit your item to be withdrawn, lost, damaged, notforloan, and restricted use 4. reload course-details.pl, confirm it still shows available 5. apply patch 6. repeat step 3 with each of the statuses and make sure it correctly shows on course-details.pl Signed-off-by: Hayley Pelham <hayleypelham@catalyst.net.nz> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 161585 [details] [review] Bug 28762: handle notforloan better Signed-off-by: Hayley Pelham <hayleypelham@catalyst.net.nz> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 161586 [details] [review] Bug 28762: (follow-up) fix template logic with Available Signed-off-by: Hayley Pelham <hayleypelham@catalyst.net.nz> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 161587 [details] [review] Bug 28762: Use Koha::Course in course-details controller This patch updates Koha::Course to include the 'instructors' relation accessor and then update the course-details controller to use the Koha::Course object and pass it to the template instead of building a hash using GetCourse. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 161588 [details] [review] Bug 28762: Update item-status include This patch updates the item-status include so that it expects just an item object making if simpler and more widely re-usable. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 161589 [details] [review] Bug 28762: Move notforloan fallback into object Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 161590 [details] [review] Bug 28762: Add test for new 'instructors' accessor
Created attachment 161591 [details] [review] Bug 28762: Unit tests for not_for_loan accessor
Created attachment 163310 [details] [review] Bug 28762: add staff item-status.inc and better handle statues on course-details.tt This patch adds an item-status.inc to the staff side much like what is already in place on the OPAC. To test: 1. create a course in course reserves, add an item to it. 2. confirm your item shows Available for its status on course-details.pl 3. edit your item to be withdrawn, lost, damaged, notforloan, and restricted use 4. reload course-details.pl, confirm it still shows available 5. apply patch 6. repeat step 3 with each of the statuses and make sure it correctly shows on course-details.pl Signed-off-by: Hayley Pelham <hayleypelham@catalyst.net.nz> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 163311 [details] [review] Bug 28762: handle notforloan better Signed-off-by: Hayley Pelham <hayleypelham@catalyst.net.nz> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 163312 [details] [review] Bug 28762: (follow-up) fix template logic with Available Signed-off-by: Hayley Pelham <hayleypelham@catalyst.net.nz> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 163313 [details] [review] Bug 28762: Use Koha::Course in course-details controller This patch updates Koha::Course to include the 'instructors' relation accessor and then update the course-details controller to use the Koha::Course object and pass it to the template instead of building a hash using GetCourse. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 163314 [details] [review] Bug 28762: Update item-status include This patch updates the item-status include so that it expects just an item object making if simpler and more widely re-usable. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 163315 [details] [review] Bug 28762: Move notforloan fallback into object Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 163316 [details] [review] Bug 28762: Add test for new 'instructors' accessor
Created attachment 163317 [details] [review] Bug 28762: Unit tests for not_for_loan accessor
Another rebase done.
there might be an issue found in comment 30
(In reply to Victor Grousset/tuxayo from comment #61) > there might be an issue found in comment 30 This patchset only touches the staff client and the comment appears to say the results here are correct. I think if we want to sync the OPAC back to follow the logic here we should do that in a follow-up bug rather than hold this patch back any longer.
Created attachment 165843 [details] [review] Bug 28762: add staff item-status.inc and better handle statues on course-details.tt This patch adds an item-status.inc to the staff side much like what is already in place on the OPAC. To test: 1. create a course in course reserves, add an item to it. 2. confirm your item shows Available for its status on course-details.pl 3. edit your item to be withdrawn, lost, damaged, notforloan, and restricted use 4. reload course-details.pl, confirm it still shows available 5. apply patch 6. repeat step 3 with each of the statuses and make sure it correctly shows on course-details.pl Signed-off-by: Hayley Pelham <hayleypelham@catalyst.net.nz> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 165844 [details] [review] Bug 28762: handle notforloan better Signed-off-by: Hayley Pelham <hayleypelham@catalyst.net.nz> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 165845 [details] [review] Bug 28762: (follow-up) fix template logic with Available Signed-off-by: Hayley Pelham <hayleypelham@catalyst.net.nz> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 165846 [details] [review] Bug 28762: Use Koha::Course in course-details controller This patch updates Koha::Course to include the 'instructors' relation accessor and then update the course-details controller to use the Koha::Course object and pass it to the template instead of building a hash using GetCourse. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 165847 [details] [review] Bug 28762: Update item-status include This patch updates the item-status include so that it expects just an item object making if simpler and more widely re-usable. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 165848 [details] [review] Bug 28762: Move notforloan fallback into object Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 165849 [details] [review] Bug 28762: Add test for new 'instructors' accessor
Created attachment 165850 [details] [review] Bug 28762: Unit tests for not_for_loan accessor
Just another quick rebase
Still looking here Marcel?
(In reply to Martin Renvoize from comment #72) > Still looking here Marcel? Feel free to take over if I didnt touch it very recently.
OK, I've reset the assignee. Can't take it on for QA as it looks like I was heavily involved in it's development in the end. :(
Created attachment 169172 [details] [review] Bug 28762: (QA follow-up): fix number of tests
Fixed number of tests mismatching and causing a failure. (maybe we put the number of tests for a reason and not just to fix it blindly?) --- Are the diag("item-level_itypes: 0"); and diag("item-level_itypes: 1"); still needed? (removed from QA queue pending the two previous points) ---- string change looks good [U+1F44D] +msgid "Lost" +msgid "Due: %s" +msgid "Pending hold" +msgid "Waiting at %s since %s." +msgid "Item-level hold (placed %s) for delivery at %s." +msgid "Restricted" -msgid "Restricted" -msgid "Lost"
Those 'diag' might be better as 'note' but I do like them in as I find they split up the output of a prove -vv more clearly personally.. I'm not especially worried though if you want them removed.
Created attachment 169273 [details] [review] Bug 28762: add staff item-status.inc and better handle statues on course-details.tt This patch adds an item-status.inc to the staff side much like what is already in place on the OPAC. To test: 1. create a course in course reserves, add an item to it. 2. confirm your item shows Available for its status on course-details.pl 3. edit your item to be withdrawn, lost, damaged, notforloan, and restricted use 4. reload course-details.pl, confirm it still shows available 5. apply patch 6. repeat step 3 with each of the statuses and make sure it correctly shows on course-details.pl Signed-off-by: Hayley Pelham <hayleypelham@catalyst.net.nz> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 169274 [details] [review] Bug 28762: handle notforloan better Signed-off-by: Hayley Pelham <hayleypelham@catalyst.net.nz> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 169275 [details] [review] Bug 28762: (follow-up) fix template logic with Available Signed-off-by: Hayley Pelham <hayleypelham@catalyst.net.nz> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 169276 [details] [review] Bug 28762: Use Koha::Course in course-details controller This patch updates Koha::Course to include the 'instructors' relation accessor and then update the course-details controller to use the Koha::Course object and pass it to the template instead of building a hash using GetCourse. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 169277 [details] [review] Bug 28762: Update item-status include This patch updates the item-status include so that it expects just an item object making if simpler and more widely re-usable. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 169278 [details] [review] Bug 28762: Move notforloan fallback into object Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 169279 [details] [review] Bug 28762: Add test for new 'instructors' accessor
Created attachment 169280 [details] [review] Bug 28762: Unit tests for not_for_loan accessor
Created attachment 169281 [details] [review] Bug 28762: (follow-up) Remove warning in tests Looks like a recent bug introduced a superflous warning, we just clean that up here whilst we're in the code.
(In reply to Victor Grousset/tuxayo from comment #76) > Fixed number of tests mismatching and causing a failure. > (maybe we put the number of tests for a reason and not just to fix it > blindly?) Rebase error introduced the count discrepancy.. just fixing the number was the right thing to do.. I've rebased it now anyway. > --- > > Are the > diag("item-level_itypes: 0"); > and > diag("item-level_itypes: 1"); Replaced with 'note' which makes the tests quieter Added a follow-up to clean up a superfluous warning introduced in another patch recently pushed.. no reason not to whilst were in the code. Back to NQA
Created attachment 169311 [details] [review] Bug 28762: Confirm return of not_for_loan in boolean context matches is_notforloan This patch is simply here to prove that is_notforloan is just not_for_loan but in a boolean context.. we'll remove it in the next patch.
Created attachment 169312 [details] [review] Bug 28762: Replace is_notforloan with not_for_loan This patch replaces the use of is_notforloan with not_for_loan and removes the older is_notforloan method and tests
Created attachment 169313 [details] [review] Bug 28762: Rename not_for_loan as effective_not_for_loan_status This patch updates the method name to follow current conventions.
Last three patches can happily be squashed.. they were submitted distinctly to help QA follow the flow of events.
Created attachment 169314 [details] [review] Bug 28762: Rename not_for_loan as effective_not_for_loan_status This patch updates the method name to follow current conventions.
Created attachment 169354 [details] [review] Bug 28762: (QA follow-up): fix number of tests Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 169355 [details] [review] Bug 28762: add staff item-status.inc and better handle statues on course-details.tt This patch adds an item-status.inc to the staff side much like what is already in place on the OPAC. To test: 1. create a course in course reserves, add an item to it. 2. confirm your item shows Available for its status on course-details.pl 3. edit your item to be withdrawn, lost, damaged, notforloan, and restricted use 4. reload course-details.pl, confirm it still shows available 5. apply patch 6. repeat step 3 with each of the statuses and make sure it correctly shows on course-details.pl Signed-off-by: Hayley Pelham <hayleypelham@catalyst.net.nz> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 169356 [details] [review] Bug 28762: handle notforloan better Signed-off-by: Hayley Pelham <hayleypelham@catalyst.net.nz> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 169357 [details] [review] Bug 28762: (follow-up) fix template logic with Available Signed-off-by: Hayley Pelham <hayleypelham@catalyst.net.nz> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 169358 [details] [review] Bug 28762: Use Koha::Course in course-details controller This patch updates Koha::Course to include the 'instructors' relation accessor and then update the course-details controller to use the Koha::Course object and pass it to the template instead of building a hash using GetCourse. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 169359 [details] [review] Bug 28762: Update item-status include This patch updates the item-status include so that it expects just an item object making if simpler and more widely re-usable. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 169360 [details] [review] Bug 28762: Move notforloan fallback into object Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 169361 [details] [review] Bug 28762: Add test for new 'instructors' accessor Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 169362 [details] [review] Bug 28762: Unit tests for not_for_loan accessor Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 169363 [details] [review] Bug 28762: (follow-up) Remove warning in tests Looks like a recent bug introduced a superflous warning, we just clean that up here whilst we're in the code. Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 169364 [details] [review] Bug 28762: Confirm return of not_for_loan in boolean context matches is_notforloan This patch is simply here to prove that is_notforloan is just not_for_loan but in a boolean context.. we'll remove it in the next patch. Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 169365 [details] [review] Bug 28762: Replace is_notforloan with not_for_loan This patch replaces the use of is_notforloan with not_for_loan and removes the older is_notforloan method and tests Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 169366 [details] [review] Bug 28762: Rename not_for_loan as effective_not_for_loan_status This patch updates the method name to follow current conventions. Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 169367 [details] [review] Bug 28762: (QA follow-up) Restore protection against unknown itemtypes Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Wait! ILSDI_Services.t fails (found while trying to check if the minor change in C4/ILSDI/Services.pm is covered (likely is)) kohadev-koha@kohadevbox:koha(testing-SO-QA)$ prove t/db_dependent/ILSDI_Services.t t/db_dependent/ILSDI_Services.t .. 12/14 # No tests run! # Failed test 'No tests run for subtest "GetAvailability itemcallnumber"' # at t/db_dependent/ILSDI_Services.t line 1045. The method Koha::Item->effective_notforloan is not covered by tests! Trace begun at /kohadevbox/koha/Koha/Object.pm line 971 Koha::Object::AUTOLOAD('Koha::Item=HASH(0x57fb9279d480)') called at /kohadevbox/koha/C4/ILSDI/Services.pm line 973 C4::ILSDI::Services::_availability(989) called at /kohadevbox/koha/C4/ILSDI/Services.pm line 122 C4::ILSDI::Services::GetAvailability('CGI=HASH(0x57fb901401d8)') called at t/db_dependent/ILSDI_Services.t line 1000 main::__ANON__ at /usr/share/perl/5.28/Test/Builder.pm line 339 eval {...} at /usr/share/perl/5.28/Test/Builder.pm line 339 Test::Builder::subtest('Test::Builder=HASH(0x57fb87b01678)', 'GetAvailability itemcallnumber', 'CODE(0x57fb92334470)') called at /usr/share/perl/5.28/Test/More.pm line 807 Test::More::subtest('GetAvailability itemcallnumber', 'CODE(0x57fb92334470)') called at t/db_dependent/ILSDI_Services.t line 1045 # Looks like your test exited with 255 just after 13. t/db_dependent/ILSDI_Services.t .. Dubious, test returned 255 (wstat 65280, 0xff00) ----- Thanks Nick for QAing the rest that was out of my reach [U+1F44D] ---- (In reply to Martin Renvoize from comment #77) > Those 'diag' might be better as 'note' but I do like them in as I find they > split up the output of a prove -vv more clearly personally.. I'm not > especially worried though if you want them removed. Nice, good to know about the difference between diag and note. No problem to have these, just needed to be sure it wasn't forgotten.
Created attachment 169393 [details] [review] Bug 28762: (follow-up) Fix missed not_for_loan_status rename
Thanks for all the hard work! Pushed to main for the next 24.11.00 release as RM Assistant
Backported to 24.05.x for upcoming 24.05.04
Oh big changes, I prefer not impact oldstable 23.11.x
Bug fix, no change to UI, no changes required to the documentation.