Bug 37334

Summary: Cannot filter holdings table by status
Product: Koha Reporter: Lucas Gass (lukeg) <lucas>
Component: CirculationAssignee: Lucas Gass (lukeg) <lucas>
Status: Signed Off --- QA Contact: Testopia <testopia>
Severity: normal    
Priority: P5 - low CC: anni.rajala, barbara.johnson, caroline.cyr-la-rose, catrina, Chip.Halvorsen, danielle.elder, david, dcook, emmi.takkinen, gmcharlt, hattara, holly, jonathan.druart, kbecker, kebliss, koha, kyle.m.hall, lisette, martin.renvoize, mspinney, nick, rcoert, sbcornell, tomascohen, victor
Version: MainKeywords: regression
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38122
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
TODO, when this is merged if the situation hasn't changed, a ticket about measuring a performance (to maybe add cache) is need. See comment 99 and comment 100.
Version(s) released in:
Circulation function:
Bug Depends on: 33568, 38512    
Bug Blocks: 7012    
Attachments: Bug 37334: Allow embed _status for GET /biblio/{biblio_id}/items
Bug 37334: Allow a column filter to be shown even searchable is off
Bug 37334: Restore filtering holdings per status
Bug 37334: Allow embed _status for GET /biblio/{biblio_id}/items
Bug 37334: Allow a column filter to be shown even searchable is off
Bug 37334: Restore filtering holdings per status
Bug 37334: Allow embed _status for GET /biblio/{biblio_id}/items
Bug 37334: Allow a column filter to be shown even searchable is off
Bug 37334: Restore filtering holdings per status
Bug 37334: Fix logic for not available
Bug 37334: Fix restricted and notforloan filter logic
Bug 37334: Allow embed _status for GET /biblio/{biblio_id}/items
Bug 37334: Allow a column filter to be shown even searchable is off
Bug 37334: Restore filtering holdings per status
Bug 37334: Fix logic for not available
Bug 37334: Fix restricted and notforloan filter logic
Bug 37334: Return all non zero notforloan statuses
Bug 37334: filter_by_available should not return notforloan statues other than 0
Bug 37334: Add _status tests
Bug 37334: Add filter_by_checked_out tests
Bug 37334: Add POD
Bug 37334: Add filter_by_in_transit tests
Bug 37334: Add filter_by_has_holds tests
Bug 37334: Fix filter_by_has_recalls
Bug 37334: Allow embed _status for GET /biblio/{biblio_id}/items
Bug 37334: Allow a column filter to be shown even searchable is off
Bug 37334: Restore filtering holdings per status
Bug 37334: Fix logic for not available
Bug 37334: Fix restricted and notforloan filter logic
Bug 37334: Return all non zero notforloan statuses
Bug 37334: filter_by_available should not return notforloan statues other than 0
Bug 37334: Add _status tests
Bug 37334: Add filter_by_checked_out tests
Bug 37334: Add POD
Bug 37334: Add filter_by_in_transit tests
Bug 37334: Add filter_by_has_holds tests
Bug 37334: Fix filter_by_has_recalls
Bug 37334: Allow embed _status for GET /biblio/{biblio_id}/items
Bug 37334: Allow a column filter to be shown even searchable is off
Bug 37334: Restore filtering holdings per status
Bug 37334: Fix logic for not available
Bug 37334: Fix restricted and notforloan filter logic
Bug 37334: Return all non zero notforloan statuses
Bug 37334: filter_by_available should not return notforloan statues other than 0
Bug 37334: Add _status tests
Bug 37334: Add filter_by_checked_out tests
Bug 37334: Add POD
Bug 37334: Add filter_by_in_transit tests
Bug 37334: Add filter_by_has_holds tests
Bug 37334: Fix filter_by_has_recalls
Bug 37334: Allow embed _status for GET /biblio/{biblio_id}/items
Bug 37334: Allow a column filter to be shown even searchable is off
Bug 37334: Restore filtering holdings per status
Bug 37334: Fix logic for not available
Bug 37334: Fix restricted and notforloan filter logic
Bug 37334: Return all non zero notforloan statuses
Bug 37334: filter_by_available should not return notforloan statues other than 0
Bug 37334: Add _status tests
Bug 37334: Add filter_by_checked_out tests
Bug 37334: Add POD
Bug 37334: Add filter_by_in_transit tests
Bug 37334: Add filter_by_has_holds tests
Bug 37334: Fix filter_by_has_recalls
Bug 37334: Add ability to filter by in_bundle
Bug 37334: filter_by_in_bundle tests
Bug 37334: Add filter_by_recall test
Bug 37334: Add filter_by_available tests
Bug 37334: Add search tests
Bug 37334: Allow embed _status for GET /biblio/{biblio_id}/items
Bug 37334: Allow a column filter to be shown even searchable is off
Bug 37334: Restore filtering holdings per status
Bug 37334: Fix logic for not available
Bug 37334: Fix restricted and notforloan filter logic
Bug 37334: Return all non zero notforloan statuses
Bug 37334: filter_by_available should not return notforloan statues other than 0
Bug 37334: Add _status tests
Bug 37334: Add filter_by_checked_out tests
Bug 37334: Add POD
Bug 37334: Add filter_by_in_transit tests
Bug 37334: Add filter_by_has_holds tests
Bug 37334: Fix filter_by_has_recalls
Bug 37334: Add ability to filter by in_bundle
Bug 37334: filter_by_in_bundle tests
Bug 37334: Add filter_by_recall test
Bug 37334: Add filter_by_available tests
Bug 37334: Add search tests

Description Lucas Gass (lukeg) 2024-07-11 20:21:53 UTC
Currently one is not able to sort the holdings table by status. IN catalogue_detail.inc I see the following: 

451                 data: "me.lost_status", 
452                 className: "status",
453                 searchable: false, // FIXME We are losing the ability to search on the status
454                 orderable: false,
455                 render: function (data, type, row, meta) {

Setting orderable to true brings back the icons, but I am assuming since this wasn't fixed initially that it is not trivial.
Comment 1 Dani Elder 2024-07-15 19:17:03 UTC
We do use this feature regularly. Thanks for fixing it.
Comment 2 Jonathan Druart 2024-10-02 12:45:06 UTC
On bug 33568 I clearly stated that we were loosing the ability to search on the status.

"""
  commit 385048f27ccb5eaeeac2e90130d621305a537960
  Bug 33568: Status

    See the FIXME in the code, we are losing the ability to search on the status,
    but this is really impossible to implement if we are not storing the
    status (DB or search engine).
"""

Actually the search is not impossible, tricky but feasible.
However the sort is clearly not possible if we don't store the status (ie. if we keep calculating it on the fly).

I think the use cases and the needs are answered with the filtering restored. I am renaming the bug to highlight that, but feel free to open a separate one if it is still problematic. But keep in mind that we need to rethink how/when/where we store and calculate the status, and it's not a trivial change.
Comment 3 Jonathan Druart 2024-10-02 12:56:06 UTC
Created attachment 172300 [details] [review]
Bug 37334: Allow embed _status for GET /biblio/{biblio_id}/items
Comment 4 Jonathan Druart 2024-10-02 12:56:09 UTC
Created attachment 172301 [details] [review]
Bug 37334: Allow a column filter to be shown even searchable is off

We do not want to use the usual filtering method here, we are building
the query from "additional filters"
Comment 5 Jonathan Druart 2024-10-02 12:56:12 UTC
Created attachment 172302 [details] [review]
Bug 37334: Restore filtering holdings per status
Comment 6 Jonathan Druart 2024-10-02 12:56:43 UTC
I am not sure this is the best we can do, but it seems to work.

I would like, before continuing (or, better, getting help to get it done), to know if Lucas and Dani are happy with this change. Does it answer your needs?

There are more TODO:
1. Tests
2. POD
3. Discuss, rename "_status" (?)

This is ready for testing, not to be pushed however.
Comment 7 Jonathan Druart 2024-10-07 08:54:30 UTC
I am waiting for feedback.
Comment 8 Lucas Gass (lukeg) 2024-10-07 16:52:35 UTC
Thanks for your work here Jonathan, I really appreciate it. 

When I apply this patch and attempt to sort the column I get this 500 error: 


==> /var/log/koha/kohadev/plack-api-error.log <==
[2024/10/07 16:28:03] [ERROR] GET /api/v1/biblios/9/items: unhandled exception (DBIx::Class::Exception)<<DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st execute failed: Unknown column 'me' in 'order clause' at /kohadevbox/koha/Koha/Objects.pm line 403>>
Comment 9 Jonathan Druart 2024-10-08 07:15:36 UTC
(In reply to Lucas Gass from comment #8)
> Thanks for your work here Jonathan, I really appreciate it. 
> 
> When I apply this patch and attempt to sort the column I get this 500 error: 
> 
> 
> ==> /var/log/koha/kohadev/plack-api-error.log <==
> [2024/10/07 16:28:03] [ERROR] GET /api/v1/biblios/9/items: unhandled
> exception
> (DBIx::Class::Exception)<<DBIx::Class::Storage::DBI::_dbh_execute(): DBI
> Exception: DBD::mysql::st execute failed: Unknown column 'me' in 'order
> clause' at /kohadevbox/koha/Koha/Objects.pm line 403>>

Which column? Status is not sortable, and sort on other columns works for me.
Comment 10 Lucas Gass (lukeg) 2024-10-08 14:51:46 UTC
(In reply to Jonathan Druart from comment #9)
> (In reply to Lucas Gass from comment #8)
> > Thanks for your work here Jonathan, I really appreciate it. 
> > 
> > When I apply this patch and attempt to sort the column I get this 500 error: 
> > 
> > 
> > ==> /var/log/koha/kohadev/plack-api-error.log <==
> > [2024/10/07 16:28:03] [ERROR] GET /api/v1/biblios/9/items: unhandled
> > exception
> > (DBIx::Class::Exception)<<DBIx::Class::Storage::DBI::_dbh_execute(): DBI
> > Exception: DBD::mysql::st execute failed: Unknown column 'me' in 'order
> > clause' at /kohadevbox/koha/Koha/Objects.pm line 403>>
> 
> Which column? Status is not sortable, and sort on other columns works for me.

Sorry, this is some misunderstanding on my part. I testing the filtering of the Status column and it works great for me. I tested with all the listed statuses. 

I did file this additional bug for SORTING of the status column:

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38122
Comment 11 Jonathan Druart 2024-10-21 07:42:02 UTC
ping, Lucas?
Comment 12 David Cook 2024-10-21 22:54:34 UTC
I took a peek at the code, and this will
Comment 13 Jonathan Druart 2024-10-22 07:44:43 UTC
(In reply to David Cook from comment #12)
> I took a peek at the code, and this will

Slow down things? Yes maybe, but only if you filter on status.
Comment 14 Lucas Gass (lukeg) 2024-10-25 22:32:11 UTC
Everything works great here except for when filtering by not_for_loan status. In that case I seem to get a 500 error:

==> /var/log/koha/kohadev/plack-api-error.log <==
[2024/10/25 22:31:33] [ERROR] GET /api/v1/biblios/32/items: unhandled exception (Koha::Exceptions::Object::MethodNotCoveredByTests)<<The method Koha::ItemTypes->column is not covered by tests!
Comment 15 Jonathan Druart 2024-10-28 10:36:07 UTC
Created attachment 173499 [details] [review]
Bug 37334: Allow embed _status for GET /biblio/{biblio_id}/items
Comment 16 Jonathan Druart 2024-10-28 10:36:11 UTC
Created attachment 173500 [details] [review]
Bug 37334: Allow a column filter to be shown even searchable is off

We do not want to use the usual filtering method here, we are building
the query from "additional filters"
Comment 17 Jonathan Druart 2024-10-28 10:36:14 UTC
Created attachment 173501 [details] [review]
Bug 37334: Restore filtering holdings per status
Comment 18 Jonathan Druart 2024-10-28 10:36:26 UTC
(In reply to Lucas Gass (lukeg) from comment #14)
> Everything works great here except for when filtering by not_for_loan
> status. In that case I seem to get a 500 error:
> 
> ==> /var/log/koha/kohadev/plack-api-error.log <==
> [2024/10/25 22:31:33] [ERROR] GET /api/v1/biblios/32/items: unhandled
> exception (Koha::Exceptions::Object::MethodNotCoveredByTests)<<The method
> Koha::ItemTypes->column is not covered by tests!

Fixed!
Comment 19 Lucas Gass (lukeg) 2024-11-01 22:00:36 UTC
(In reply to Jonathan Druart from comment #18)
> (In reply to Lucas Gass (lukeg) from comment #14)
> > Everything works great here except for when filtering by not_for_loan
> > status. In that case I seem to get a 500 error:
> > 
> > ==> /var/log/koha/kohadev/plack-api-error.log <==
> > [2024/10/25 22:31:33] [ERROR] GET /api/v1/biblios/32/items: unhandled
> > exception (Koha::Exceptions::Object::MethodNotCoveredByTests)<<The method
> > Koha::ItemTypes->column is not covered by tests!
> 
> Fixed!

Yes, everything seems to work great now. Can we move this out of 'In discussion', happy to add my sign off at this point.
Comment 20 Lucas Gass (lukeg) 2024-11-04 21:26:53 UTC
Created attachment 173949 [details] [review]
Bug 37334: Allow embed _status for GET /biblio/{biblio_id}/items

Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Comment 21 Lucas Gass (lukeg) 2024-11-04 21:26:57 UTC
Created attachment 173950 [details] [review]
Bug 37334: Allow a column filter to be shown even searchable is off

We do not want to use the usual filtering method here, we are building
the query from "additional filters"

Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Comment 22 Lucas Gass (lukeg) 2024-11-04 21:27:01 UTC
Created attachment 173951 [details] [review]
Bug 37334: Restore filtering holdings per status

Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Comment 23 Lucas Gass (lukeg) 2024-11-04 21:27:34 UTC
Things are working, I am signing off to keep this moving.
Comment 24 Emmi Takkinen 2024-11-05 07:08:30 UTC
Tested this and while filtering seems to be working with every other status, available status returns either all items or items with other statuses. 

E.g. I have an item A with damaged value 2 and item B with all statuses (notforloan, damaged, withdrawn, itemlost, restricted and itype is also available for loan) as 0. Only item A is returned when I filter holdings table with "Available" status.
Comment 25 Jonathan Druart 2024-11-05 14:13:28 UTC
Created attachment 173982 [details] [review]
Bug 37334: Fix logic for not available

Several major changes here:
* the push @not_availab_itemnumbers for lost, damaged, etc. was totally
  wrong
* not for loan is only for positive values
* restricted can be NULL
Comment 26 Jonathan Druart 2024-11-05 14:13:46 UTC
(In reply to Emmi Takkinen from comment #24)
> Tested this and while filtering seems to be working with every other status,
> available status returns either all items or items with other statuses. 
> 
> E.g. I have an item A with damaged value 2 and item B with all statuses
> (notforloan, damaged, withdrawn, itemlost, restricted and itype is also
> available for loan) as 0. Only item A is returned when I filter holdings
> table with "Available" status.

Indeed, lot of wrong logics in the previous version, thanks!
Comment 27 Jonathan Druart 2024-11-05 14:15:21 UTC
I won't be able to work more on this soon.

Feel free to test, but it's not ready for inclusion (hence the previous "In discussion" status). It's missing a lot of tests. Please take over if you are willing to make it move forward. The tests can be interesting to write but I won't have the time during the next weeks.
Comment 28 Nick Clemens (kidclamp) 2024-11-07 15:45:43 UTC
*** Bug 38122 has been marked as a duplicate of this bug. ***
Comment 29 Lucas Gass (lukeg) 2024-11-08 23:35:40 UTC
(In reply to Emmi Takkinen from comment #24)
> Tested this and while filtering seems to be working with every other status,
> available status returns either all items or items with other statuses. 
> 
> E.g. I have an item A with damaged value 2 and item B with all statuses
> (notforloan, damaged, withdrawn, itemlost, restricted and itype is also
> available for loan) as 0. Only item A is returned when I filter holdings
> table with "Available" status.

I tried to recreate this exact scenario and it seems to work for me. However I did find flaws with the restricted and notforloan filters.
Comment 30 Lucas Gass (lukeg) 2024-11-08 23:37:08 UTC
Created attachment 174310 [details] [review]
Bug 37334: Fix restricted and notforloan filter logic
Comment 31 Lucas Gass (lukeg) 2024-11-08 23:38:29 UTC
I am going to put this back to into discussion.

Emmi, if you get a chance can you test again with my follow-up patches. 


Like Jonathan said, we'll still need to write a bunch of tests here.
Comment 32 Emmi Takkinen 2024-11-11 07:48:17 UTC
First patch needed a minor rebase. Filtering with at least "Available", "Checked out", "In transit", "Damaged" and "Lost" status works, but "Not for loan" status doesn't return results if its value starts with -. E.g. I have authorized values 7 "Added to bundle" and -7 "Auton varastossa". Filtering first one works, second doesn't.
Comment 33 Jonathan Druart 2024-11-19 12:54:03 UTC
(In reply to Emmi Takkinen from comment #32)
> First patch needed a minor rebase. Filtering with at least "Available",
> "Checked out", "In transit", "Damaged" and "Lost" status works, but "Not for
> loan" status doesn't return results if its value starts with -. E.g. I have
> authorized values 7 "Added to bundle" and -7 "Auton varastossa". Filtering
> first one works, second doesn't.

Yes, I think it's the correct behaviour.

C4/Reserves.pm:1322         $item->notforloan > 0  || # item with negative or zero notforloan value is holdable
Comment 34 Catrina Berka 2024-11-19 17:06:41 UTC
If I filter on Not for Loan, I would expect to see ALL values, positive or negative. Libraries using negative NFL values would want to see copies that may be on order or in processing, in addition to any that are not holdable. The filtering needs to account for any value where NFL != 0
Comment 35 Lucas Gass (lukeg) 2024-11-19 17:10:54 UTC
Created attachment 174798 [details] [review]
Bug 37334: Allow embed _status for GET /biblio/{biblio_id}/items

Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Comment 36 Lucas Gass (lukeg) 2024-11-19 17:10:57 UTC
Created attachment 174799 [details] [review]
Bug 37334: Allow a column filter to be shown even searchable is off

We do not want to use the usual filtering method here, we are building
the query from "additional filters"

Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Comment 37 Lucas Gass (lukeg) 2024-11-19 17:11:00 UTC
Created attachment 174800 [details] [review]
Bug 37334: Restore filtering holdings per status

Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Comment 38 Lucas Gass (lukeg) 2024-11-19 17:11:03 UTC
Created attachment 174801 [details] [review]
Bug 37334: Fix logic for not available

Several major changes here:
* the push @not_availab_itemnumbers for lost, damaged, etc. was totally
  wrong
* not for loan is only for positive values
* restricted can be NULL
Comment 39 Lucas Gass (lukeg) 2024-11-19 17:11:07 UTC
Created attachment 174802 [details] [review]
Bug 37334: Fix restricted and notforloan filter logic
Comment 40 Lucas Gass (lukeg) 2024-11-19 17:11:10 UTC
Created attachment 174803 [details] [review]
Bug 37334: Return all non zero notforloan statuses
Comment 41 Lucas Gass (lukeg) 2024-11-19 17:12:17 UTC
(In reply to Catrina Berka from comment #34)
> If I filter on Not for Loan, I would expect to see ALL values, positive or
> negative. Libraries using negative NFL values would want to see copies that
> may be on order or in processing, in addition to any that are not holdable.
> The filtering needs to account for any value where NFL != 0

Rebased and added a patch, based on Catrina's comment, so the notforloan filter will return ALL nfl statuses, positive or negative.
Comment 42 Lucas Gass (lukeg) 2024-11-19 17:59:41 UTC
I am noticing that filtering by available is returning items with a negative notforloan status when it should not.
Comment 43 Lucas Gass (lukeg) 2024-11-19 18:09:18 UTC
Created attachment 174804 [details] [review]
Bug 37334: filter_by_available should not return notforloan statues other than 0
Comment 44 Lucas Gass (lukeg) 2024-11-19 22:27:24 UTC
Created attachment 174813 [details] [review]
Bug 37334: Add _status tests
Comment 45 Lucas Gass (lukeg) 2024-11-19 22:32:13 UTC
There is a few more tests to write here for the 'filter_by*' routines.
Comment 46 Lucas Gass (lukeg) 2024-11-19 23:24:32 UTC
Created attachment 174814 [details] [review]
Bug 37334: Add filter_by_checked_out tests
Comment 47 Lucas Gass (lukeg) 2024-11-21 18:43:17 UTC
Created attachment 174901 [details] [review]
Bug 37334: Add POD
Comment 48 Lucas Gass (lukeg) 2024-11-21 18:47:32 UTC
Jonathan had an empty routine, I removed it. I don't think we need this?


sub filter_by_for_loan {

}


We can already filter by not_for_loan, available, and checked_out.
Comment 49 Lucas Gass (lukeg) 2024-11-21 19:07:23 UTC
Created attachment 174902 [details] [review]
Bug 37334: Add filter_by_in_transit tests
Comment 50 Lucas Gass (lukeg) 2024-11-21 21:31:22 UTC
Created attachment 174903 [details] [review]
Bug 37334: Add filter_by_has_holds tests
Comment 51 Lucas Gass (lukeg) 2024-11-21 21:32:27 UTC
Trying to write tests for filter_by_has_recalls made me realize filtering by recalls is working quite right.
Comment 52 Lucas Gass (lukeg) 2024-11-21 21:33:13 UTC
(In reply to Lucas Gass (lukeg) from comment #51)
> Trying to write tests for filter_by_has_recalls made me realize filtering by
> recalls is working quite right.

It is NOT working quite right, fix incoming.
Comment 53 Lucas Gass (lukeg) 2024-11-22 23:44:01 UTC
(In reply to Lucas Gass (lukeg) from comment #52)
> (In reply to Lucas Gass (lukeg) from comment #51)
> > Trying to write tests for filter_by_has_recalls made me realize filtering by
> > recalls is working quite right.
> 
> It is NOT working quite right, fix incoming.


filter_by_has_recalls needs to filter by item_level recalls only, anything else should be considered available.
Comment 54 Lucas Gass (lukeg) 2024-11-22 23:44:26 UTC
Created attachment 174955 [details] [review]
Bug 37334: Fix filter_by_has_recalls
Comment 55 Emmi Takkinen 2024-11-26 11:46:02 UTC
Is this ready for testing?
Comment 56 Lucas Gass (lukeg) 2024-12-02 17:44:46 UTC
(In reply to Emmi Takkinen from comment #55)
> Is this ready for testing?

It is blocked by Bug 38512. I think we need to make sure the display of recalls is correct before we can continue here, please test that one first!
Comment 57 Lucas Gass (lukeg) 2024-12-18 18:22:05 UTC
This can be unblocked because Bug 38512 is PQA. It needs rebased on top of that one now, setting to patch doesn't apply and will rebase asap.
Comment 58 Lucas Gass (lukeg) 2024-12-20 22:15:27 UTC
Created attachment 175859 [details] [review]
Bug 37334: Allow embed _status for GET /biblio/{biblio_id}/items

Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Comment 59 Lucas Gass (lukeg) 2024-12-20 22:15:31 UTC
Created attachment 175860 [details] [review]
Bug 37334: Allow a column filter to be shown even searchable is off

We do not want to use the usual filtering method here, we are building
the query from "additional filters"

Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Comment 60 Lucas Gass (lukeg) 2024-12-20 22:15:34 UTC
Created attachment 175861 [details] [review]
Bug 37334: Restore filtering holdings per status

Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Comment 61 Lucas Gass (lukeg) 2024-12-20 22:15:38 UTC
Created attachment 175862 [details] [review]
Bug 37334: Fix logic for not available

Several major changes here:
* the push @not_availab_itemnumbers for lost, damaged, etc. was totally
  wrong
* not for loan is only for positive values
* restricted can be NULL
Comment 62 Lucas Gass (lukeg) 2024-12-20 22:15:41 UTC
Created attachment 175863 [details] [review]
Bug 37334: Fix restricted and notforloan filter logic
Comment 63 Lucas Gass (lukeg) 2024-12-20 22:15:44 UTC
Created attachment 175864 [details] [review]
Bug 37334: Return all non zero notforloan statuses
Comment 64 Lucas Gass (lukeg) 2024-12-20 22:15:48 UTC
Created attachment 175865 [details] [review]
Bug 37334: filter_by_available should not return notforloan statues other than 0
Comment 65 Lucas Gass (lukeg) 2024-12-20 22:15:51 UTC
Created attachment 175866 [details] [review]
Bug 37334: Add _status tests
Comment 66 Lucas Gass (lukeg) 2024-12-20 22:15:54 UTC
Created attachment 175867 [details] [review]
Bug 37334: Add filter_by_checked_out tests
Comment 67 Lucas Gass (lukeg) 2024-12-20 22:15:58 UTC
Created attachment 175868 [details] [review]
Bug 37334: Add POD
Comment 68 Lucas Gass (lukeg) 2024-12-20 22:16:01 UTC
Created attachment 175869 [details] [review]
Bug 37334: Add filter_by_in_transit tests
Comment 69 Lucas Gass (lukeg) 2024-12-20 22:16:05 UTC
Created attachment 175870 [details] [review]
Bug 37334: Add filter_by_has_holds tests
Comment 70 Lucas Gass (lukeg) 2024-12-20 22:16:08 UTC
Created attachment 175871 [details] [review]
Bug 37334: Fix filter_by_has_recalls
Comment 71 Emmi Takkinen 2024-12-30 07:55:54 UTC
Patches no longer apply.

Applying: Bug 37334: Allow embed _status for GET /biblio/{biblio_id}/items
error: sha1 information is lacking or useless (koha-tmpl/intranet-tmpl/prog/en/includes/html_helpers/tables/items/catalogue_detail.inc).
error: could not build fake ancestor
Patch failed at 0001 Bug 37334: Allow embed _status for GET /biblio/{biblio_id}/items
hint: Use 'git am --show-current-patch=diff' to see the failed patch
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Bug-37334-Allow-embed-status-for-GET-bibliobiblioi-34QjuN.patch
Comment 72 Lucas Gass (lukeg) 2024-12-30 15:31:34 UTC
Created attachment 175999 [details] [review]
Bug 37334: Allow embed _status for GET /biblio/{biblio_id}/items

Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Comment 73 Lucas Gass (lukeg) 2024-12-30 15:31:37 UTC
Created attachment 176000 [details] [review]
Bug 37334: Allow a column filter to be shown even searchable is off

We do not want to use the usual filtering method here, we are building
the query from "additional filters"

Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Comment 74 Lucas Gass (lukeg) 2024-12-30 15:31:40 UTC
Created attachment 176001 [details] [review]
Bug 37334: Restore filtering holdings per status

Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Comment 75 Lucas Gass (lukeg) 2024-12-30 15:31:44 UTC
Created attachment 176002 [details] [review]
Bug 37334: Fix logic for not available

Several major changes here:
* the push @not_availab_itemnumbers for lost, damaged, etc. was totally
  wrong
* not for loan is only for positive values
* restricted can be NULL
Comment 76 Lucas Gass (lukeg) 2024-12-30 15:31:47 UTC
Created attachment 176003 [details] [review]
Bug 37334: Fix restricted and notforloan filter logic
Comment 77 Lucas Gass (lukeg) 2024-12-30 15:31:51 UTC
Created attachment 176004 [details] [review]
Bug 37334: Return all non zero notforloan statuses
Comment 78 Lucas Gass (lukeg) 2024-12-30 15:31:55 UTC
Created attachment 176005 [details] [review]
Bug 37334: filter_by_available should not return notforloan statues other than 0
Comment 79 Lucas Gass (lukeg) 2024-12-30 15:31:58 UTC
Created attachment 176006 [details] [review]
Bug 37334: Add _status tests
Comment 80 Lucas Gass (lukeg) 2024-12-30 15:32:02 UTC
Created attachment 176007 [details] [review]
Bug 37334: Add filter_by_checked_out tests
Comment 81 Lucas Gass (lukeg) 2024-12-30 15:32:06 UTC
Created attachment 176008 [details] [review]
Bug 37334: Add POD
Comment 82 Lucas Gass (lukeg) 2024-12-30 15:32:10 UTC
Created attachment 176009 [details] [review]
Bug 37334: Add filter_by_in_transit tests
Comment 83 Lucas Gass (lukeg) 2024-12-30 15:32:13 UTC
Created attachment 176010 [details] [review]
Bug 37334: Add filter_by_has_holds tests
Comment 84 Lucas Gass (lukeg) 2024-12-30 15:32:17 UTC
Created attachment 176011 [details] [review]
Bug 37334: Fix filter_by_has_recalls
Comment 85 Emmi Takkinen 2024-12-31 08:06:52 UTC
Created attachment 176023 [details] [review]
Bug 37334: Allow embed _status for GET /biblio/{biblio_id}/items

Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Comment 86 Emmi Takkinen 2024-12-31 08:07:25 UTC
Created attachment 176024 [details] [review]
Bug 37334: Allow a column filter to be shown even searchable is off

We do not want to use the usual filtering method here, we are building
the query from "additional filters"

Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Comment 87 Emmi Takkinen 2024-12-31 08:07:53 UTC
Created attachment 176025 [details] [review]
Bug 37334: Restore filtering holdings per status

Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Comment 88 Emmi Takkinen 2024-12-31 08:08:22 UTC
Created attachment 176026 [details] [review]
Bug 37334: Fix logic for not available

Several major changes here:
* the push @not_availab_itemnumbers for lost, damaged, etc. was totally
  wrong
* not for loan is only for positive values
* restricted can be NULL

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Comment 89 Emmi Takkinen 2024-12-31 08:08:51 UTC
Created attachment 176027 [details] [review]
Bug 37334: Fix restricted and notforloan filter logic

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Comment 90 Emmi Takkinen 2024-12-31 08:09:16 UTC
Created attachment 176028 [details] [review]
Bug 37334: Return all non zero notforloan statuses

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Comment 91 Emmi Takkinen 2024-12-31 08:09:48 UTC
Created attachment 176029 [details] [review]
Bug 37334: filter_by_available should not return notforloan statues other than 0

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Comment 92 Emmi Takkinen 2024-12-31 08:10:10 UTC
Created attachment 176030 [details] [review]
Bug 37334: Add _status tests

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Comment 93 Emmi Takkinen 2024-12-31 08:10:40 UTC
Created attachment 176031 [details] [review]
Bug 37334: Add filter_by_checked_out tests

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Comment 94 Emmi Takkinen 2024-12-31 08:11:02 UTC
Created attachment 176032 [details] [review]
Bug 37334: Add POD

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Comment 95 Emmi Takkinen 2024-12-31 08:11:27 UTC
Created attachment 176033 [details] [review]
Bug 37334: Add filter_by_in_transit tests

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Comment 96 Emmi Takkinen 2024-12-31 08:11:58 UTC
Created attachment 176034 [details] [review]
Bug 37334: Add filter_by_has_holds tests

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Comment 97 Emmi Takkinen 2024-12-31 08:12:21 UTC
Created attachment 176035 [details] [review]
Bug 37334: Fix filter_by_has_recalls

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Comment 98 Emmi Takkinen 2024-12-31 08:13:05 UTC
Tested and now filtering with status works as intended. Also tests pass.
Comment 99 Victor Grousset/tuxayo 2025-01-06 04:28:25 UTC
Comment on attachment 176023 [details] [review]
Bug 37334: Allow embed _status for GET /biblio/{biblio_id}/items

Review of attachment 176023 [details] [review]:
-----------------------------------------------------------------

::: Koha/Item.pm
@@ +1469,5 @@
> +    if ( $self->damaged ) {
> +        push @statuses, 'damaged';
> +    }
> +    if ( $self->notforloan || $self->item_type->notforloan  ) {
> +        # TODO on a big Koha::Items loop we are going to join with item_type too often, use a cache

Is the concern big enough to at least have a ticket opened to measure this? And eventually implement a cache.

@@ +1487,5 @@
> +    if ( $self->restricted ) {
> +        push @statuses, 'restricted';
> +    }
> +
> +    # TODO in_bundle?

Was that meant to be temporary or does it have a long term use?
Does it warrant at least opening a follow-up ticket or it's too minor?
Does it warrant some manual testing related to bundles?
Comment 100 Lucas Gass (lukeg) 2025-01-06 14:45:01 UTC
(In reply to Victor Grousset/tuxayo from comment #99)
> Comment on attachment 176023 [details] [review] [review]
> Bug 37334: Allow embed _status for GET /biblio/{biblio_id}/items
> 
> Review of attachment 176023 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: Koha/Item.pm
> @@ +1469,5 @@
> > +    if ( $self->damaged ) {
> > +        push @statuses, 'damaged';
> > +    }
> > +    if ( $self->notforloan || $self->item_type->notforloan  ) {
> > +        # TODO on a big Koha::Items loop we are going to join with item_type too often, use a cache
> 
> Is the concern big enough to at least have a ticket opened to measure this?
> And eventually implement a cache.

Yes, we should open a new bug report for this.  


> @@ +1487,5 @@
> > +    if ( $self->restricted ) {
> > +        push @statuses, 'restricted';
> > +    }
> > +
> > +    # TODO in_bundle?
> 
> Was that meant to be temporary or does it have a long term use?
> Does it warrant at least opening a follow-up ticket or it's too minor?
> Does it warrant some manual testing related to bundles?

in_bundle is a status, so that needs to be included in this bug, I think. I will add ASAP.
Comment 101 Victor Grousset/tuxayo 2025-01-06 20:28:56 UTC
To pass the tests, only these two commits are needed:
Bug 37334: Allow embed _status for GET /biblio/{biblio_id}/items
Bug 37334: Restore filtering holdings per status

Looking the diffs of the other commits, there are substantial changes that would have been expected to be necessary for tests to pass.

After that smoke test, let's look at proper coverage:
https://wiki.koha-community.org/wiki/Coding_Guidelines#PERL17:_Unit_tests_are_required


These are not covered by tests:
sub filter_by_has_recalls
sub filter_by_available

search() have very little coverage.
At least having the most complex case (not_for_loan) covered would be safer.
The "recalled" and "available" cases would also cover the above paragraph. Or maybe they should be tested directly and not via search(). No opinion on this.
Comment 102 Victor Grousset/tuxayo 2025-01-06 20:38:23 UTC
> Yes, we should open a new bug report for this.  

ok, I'm experimenting with hijacking the release notes field to keep track of this.


------


Thanks for the existing tests. coverage of _status is perfect and so is all the other filter_by_* subs.
Comment 103 Lucas Gass (lukeg) 2025-01-13 21:09:43 UTC
Created attachment 176475 [details] [review]
Bug 37334: Add ability to filter by in_bundle
Comment 104 Lucas Gass (lukeg) 2025-01-13 21:33:08 UTC
Created attachment 176477 [details] [review]
Bug 37334: filter_by_in_bundle tests
Comment 105 Lucas Gass (lukeg) 2025-01-13 22:37:41 UTC
Created attachment 176480 [details] [review]
Bug 37334: Add filter_by_recall test
Comment 106 Lucas Gass (lukeg) 2025-01-13 23:29:12 UTC
Created attachment 176481 [details] [review]
Bug 37334: Add filter_by_available tests
Comment 107 Lucas Gass (lukeg) 2025-01-13 23:30:27 UTC
I have added the ability to filter by bundles w/ tests. I have also included tests for filter_by_has_recalls and filter_by_available

Resetting to 'Signed off'
Comment 108 Jonathan Druart 2025-01-16 15:02:43 UTC
*** Bug 38686 has been marked as a duplicate of this bug. ***
Comment 109 Victor Grousset/tuxayo 2025-01-29 18:12:22 UTC
(In reply to Victor Grousset/tuxayo from comment #101)
> These are not covered by tests:
> sub filter_by_has_recalls
> sub filter_by_available

All good, thanks :)

> search() have very little coverage.
> At least having the most complex case (not_for_loan) covered would be safer.
> The "recalled" and "available" cases would also cover the above paragraph.
> Or maybe they should be tested directly and not via search(). No opinion on
> this.

Still the same issue with search() unfortunately.
Comment 110 Lucas Gass (lukeg) 2025-01-29 21:41:01 UTC
Created attachment 177321 [details] [review]
Bug 37334: Add search tests
Comment 111 Caroline Cyr La Rose 2025-01-31 20:45:11 UTC
There aren't any test plans that I can see on here... I tried applying the patches and using the search box at the top of the holdings table to search for statuses and it doesn't find anything. Is that what this is trying to solve?

To recreate:
1. Go to a record details page (e.g. http://localhost:8081/cgi-bin/koha/catalogue/detail.pl?biblionumber=285)

2. Add a couple of items with different statuses (or change the status of a couple of items)
   2.1. Click New > New item
   2.2. Change the Not for loan status to Staff collection (or another not for loan status)
   2.3. Click Add item
   2.4. Repeat 2.2 and 2.3 for various not for loan statuses
   2.5. Click Normal to go back to the record details page

3. Try to search for the status in the holdings
   3.1. Using the small search box right above the holdings table, search for one of the statuses (e.g. "Staff")
   --> No results
Comment 112 Lucas Gass (lukeg) 2025-01-31 21:51:12 UTC
Created attachment 177415 [details] [review]
Bug 37334: Allow embed _status for GET /biblio/{biblio_id}/items

To test;

0. APPLY PATCH, restart_all
1. Go to a record details page (e.g. http://localhost:8081/cgi-bin/koha/catalogue/detail.pl?biblionumber=285)
2. Click the 'Show filters' button above the table
3. Now, at the top of the Status column you should see a dropdown to filter by status
4. You'll have to create several item status to test everything.

5. Add some new items/or change some item status to: DAMAGED, WITHDRAWN, NOTFORLOAN, RESTRICTED, and LOST
6. Test the status filter for each of these statuses, ensuring the work correctly.
7. Create more new items and have some that are checked out, ensure that filter works
8. Put some items into transit and make sure the 'In transit' filter works
9. Put some items on hold, both item and bib level. Make sure the 'On hold' filter works.
10. Turn on the USERecalls system preference and make some recalls, both item and bib level. Make sure the 'Recalled' filter works
11. Create some bundles, make sure the 'In bundle' status works.
12. Also make sure that the available filter is working.

Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Comment 113 Lucas Gass (lukeg) 2025-01-31 21:51:17 UTC
Created attachment 177416 [details] [review]
Bug 37334: Allow a column filter to be shown even searchable is off

We do not want to use the usual filtering method here, we are building
the query from "additional filters"

Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Comment 114 Lucas Gass (lukeg) 2025-01-31 21:51:21 UTC
Created attachment 177417 [details] [review]
Bug 37334: Restore filtering holdings per status

Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Comment 115 Lucas Gass (lukeg) 2025-01-31 21:51:25 UTC
Created attachment 177418 [details] [review]
Bug 37334: Fix logic for not available

Several major changes here:
* the push @not_availab_itemnumbers for lost, damaged, etc. was totally
  wrong
* not for loan is only for positive values
* restricted can be NULL

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Comment 116 Lucas Gass (lukeg) 2025-01-31 21:51:29 UTC
Created attachment 177419 [details] [review]
Bug 37334: Fix restricted and notforloan filter logic

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Comment 117 Lucas Gass (lukeg) 2025-01-31 21:51:34 UTC
Created attachment 177420 [details] [review]
Bug 37334: Return all non zero notforloan statuses

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Comment 118 Lucas Gass (lukeg) 2025-01-31 21:51:38 UTC
Created attachment 177421 [details] [review]
Bug 37334: filter_by_available should not return notforloan statues other than 0

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Comment 119 Lucas Gass (lukeg) 2025-01-31 21:51:42 UTC
Created attachment 177422 [details] [review]
Bug 37334: Add _status tests

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Comment 120 Lucas Gass (lukeg) 2025-01-31 21:51:45 UTC
Created attachment 177423 [details] [review]
Bug 37334: Add filter_by_checked_out tests

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Comment 121 Lucas Gass (lukeg) 2025-01-31 21:51:50 UTC
Created attachment 177424 [details] [review]
Bug 37334: Add POD

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Comment 122 Lucas Gass (lukeg) 2025-01-31 21:51:54 UTC
Created attachment 177425 [details] [review]
Bug 37334: Add filter_by_in_transit tests

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Comment 123 Lucas Gass (lukeg) 2025-01-31 21:51:59 UTC
Created attachment 177426 [details] [review]
Bug 37334: Add filter_by_has_holds tests

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Comment 124 Lucas Gass (lukeg) 2025-01-31 21:52:03 UTC
Created attachment 177427 [details] [review]
Bug 37334: Fix filter_by_has_recalls

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Comment 125 Lucas Gass (lukeg) 2025-01-31 21:52:07 UTC
Created attachment 177428 [details] [review]
Bug 37334: Add ability to filter by in_bundle
Comment 126 Lucas Gass (lukeg) 2025-01-31 21:52:11 UTC
Created attachment 177429 [details] [review]
Bug 37334: filter_by_in_bundle tests
Comment 127 Lucas Gass (lukeg) 2025-01-31 21:52:15 UTC
Created attachment 177430 [details] [review]
Bug 37334: Add filter_by_recall test
Comment 128 Lucas Gass (lukeg) 2025-01-31 21:52:19 UTC
Created attachment 177431 [details] [review]
Bug 37334: Add filter_by_available tests
Comment 129 Lucas Gass (lukeg) 2025-01-31 21:52:23 UTC
Created attachment 177432 [details] [review]
Bug 37334: Add search tests
Comment 130 Lucas Gass (lukeg) 2025-01-31 21:53:30 UTC
(In reply to Caroline Cyr La Rose from comment #111)
> There aren't any test plans that I can see on here... I tried applying the
> patches and using the search box at the top of the holdings table to search
> for statuses and it doesn't find anything. Is that what this is trying to
> solve?

I attached a test plan to the first commit.
Comment 131 Victor Grousset/tuxayo 2025-02-02 23:10:41 UTC
For testing, one needs to run `yarn api:bundle` after applying the patch and before restarting the services.
Otherwise there is an error 400 when loading the record page.