Bug 40982

Summary: Basket: Orders table — "Modify" and "Cancel order" columns missing or displayed incorrectly
Product: Koha Reporter: Slava Shishkin <slavashishkin>
Component: AcquisitionsAssignee: Jonathan Druart <jonathan.druart>
Status: Signed Off --- QA Contact:
Severity: normal    
Priority: P5 - low CC: jonathan.druart
Version: Main   
Hardware: All   
OS: All   
GIT URL: Change sponsored?: ---
Patch complexity: --- Documentation contact:
Documentation submission: Text to go in the release notes:
Version(s) released in:
Circulation function:
Bug Depends on: 33484, 40753    
Bug Blocks:    
Attachments: Bug 40982: Basket: Fix cached visibility for Modify/Cancel order columns
Bug 40982: (bug 40753 follow-up) Adjust conditions
Bug 40982: Allow to force visibility when needed
Bug 40982: Remove cannot_be_toggled for modify and cancel
Bug 40982: Do not raise Error when bKohaColumnsUseNames is not set
Bug 40982: (bug 40753 follow-up) Adjust conditions
Bug 40982: Allow to force visibility when needed
Bug 40982: Remove cannot_be_toggled for modify and cancel
Bug 40982: Do not raise Error when bKohaColumnsUseNames is not set

Description Slava Shishkin 2025-10-09 11:33:29 UTC
Testing notes (using KTD)

Note: before starting, set CancelOrdersInClosedBaskets = Allow.
/cgi-bin/koha/admin/preferences.pl?op=search&searchfield=CancelOrdersInClosedBaskets

Add an order to a basket:
1. Acquisitions → Search vendors → Search
2. Click the vendor name
3. In the Baskets column, click the link
4. Select a basket (or create a new one)
5. Add at least one order
6. In the open basket, verify that the Modify and Cancel order columns are absent.
7. Close the basket (you’ll return to the vendor’s basket list). Open the same closed basket and verify that the Cancel order column is absent, even though it should be present (because CancelOrdersInClosedBaskets = Allow).
Comment 1 Slava Shishkin 2025-10-09 11:37:16 UTC
Created attachment 187651 [details] [review]
Bug 40982: Basket: Fix cached visibility for Modify/Cancel order columns

Testing notes (using KTD)

Note: before starting, set CancelOrdersInClosedBaskets = Allow.
/cgi-bin/koha/admin/preferences.pl?op=search&searchfield=CancelOrdersInClosedBaskets

Add an order to a basket:
1. Acquisitions → Search vendors → Search
2. Click the vendor name
3. In the Baskets column, click the link
4. Select a basket (or create a new one)
5. Add at least one order
6. In the open basket, verify that the Modify and Cancel order columns are absent.
7. Close the basket (you’ll return to the vendor’s basket list). Open the same closed basket and verify that the Cancel order column is absent, even though it should be present (because CancelOrdersInClosedBaskets = Allow).
8. Apply the patch and repeat steps 1–7.
9. At step 6 (open basket), the Modify and Cancel order columns are displayed.
10. At step 7 (closed basket), the Cancel order column is visible according to the CancelOrdersInClosedBaskets setting.
Comment 2 Jonathan Druart 2025-10-09 14:59:59 UTC
Hum I thought we fixed this problem with bug 40753.

I will have a look at your patch later.
Comment 3 Slava Shishkin 2025-10-09 15:17:29 UTC
(In reply to Jonathan Druart from comment #2)
> Hum I thought we fixed this problem with bug 40753.
> 
> I will have a look at your patch later.

Bug 40753 didn’t cover this case. It didn’t touch acqui/basket.tt nor the last two TT-conditional columns of the Orders table. 

DataTables caches .visible in localStorage (DataTables_acqui_basket_orders) and reuses it across baskets; when switching open ↔ closed, the cached flags override current TT conditions, so Modify/Cancel order end up missing or shown incorrectly.

This patch synchronizes only columns [21] and [22] in the cached state on load to match TT logic (active && !closedate and !closedate || CancelOrdersInClosedBaskets). 

Repro in KTD still fails with 40753 applied; after this patch, it’s consistent.
Comment 4 Jonathan Druart 2025-10-10 04:16:53 UTC
(In reply to Slava Shishkin from comment #3)
> (In reply to Jonathan Druart from comment #2)
> > Hum I thought we fixed this problem with bug 40753.
> > 
> > I will have a look at your patch later.
> 
> Bug 40753 didn’t cover this case. It didn’t touch acqui/basket.tt nor the
> last two TT-conditional columns of the Orders table. 

Have a look at the patch, it is exactly the code it touches ;)

https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=186239

> DataTables caches .visible in localStorage (DataTables_acqui_basket_orders)
> and reuses it across baskets; when switching open ↔ closed, the cached flags
> override current TT conditions, so Modify/Cancel order end up missing or
> shown incorrectly.
> 
> This patch synchronizes only columns [21] and [22] in the cached state on
> load to match TT logic (active && !closedate and !closedate ||
> CancelOrdersInClosedBaskets). 

Yes, and IIRC it's exactly what bug 40753 tried to fix, or at least something similar.

I have spotted a couple of problemd with the code there, I will attach something here.
Comment 5 Jonathan Druart 2025-10-10 04:18:10 UTC
Created attachment 187711 [details] [review]
Bug 40982: (bug 40753 follow-up) Adjust conditions

The IF/ELSE block contents have been reversed in the JS code.
Comment 6 Jonathan Druart 2025-10-10 04:31:13 UTC
There is something else, I see what's going on. I will provide a fix later today.
Comment 7 Jonathan Druart 2025-10-10 05:04:30 UTC
Created attachment 187712 [details] [review]
Bug 40982: Allow to force visibility when needed

The _dt_visibility function is only called when the state is
initialized. Here we want to force the visibility of some columns even
if the state exists already.

The idea of this patch is to add a new 'force_visibility' flag to the
column when the visibility needs to be respected regardless what is in
the state. That should only be used when column cannot be toggled.
Comment 8 Jonathan Druart 2025-10-10 05:06:51 UTC
Hi Slava,

I think I've managed to fix the problem  where it needs to be fixed: in datatables.js

I am not very happy with the code, we are looping over the column list too much, but it seems to work...

We will need Cypress tests to make it ready for inclusion but I would like to know if you are happy with how those patches behave.
Comment 9 Slava Shishkin 2025-10-10 09:29:28 UTC
Hi Jonathan, 

Just tested everything. It works exactly as expected. Great work!
You were ahead with the logic fix; we had a second patch ready on our side too :)

One small thing. Do we really need

cancel_column.cannot_be_toggled = 1; 
and 
modify_column.cannot_be_toggled = 1;

These two columns already have those flags by default and can’t be toggled anyway.



However, everything works perfectly. Thank you!
Comment 10 Jonathan Druart 2025-10-10 10:09:43 UTC
Created attachment 187724 [details] [review]
Bug 40982: Remove cannot_be_toggled for modify and cancel

It's already set in the global config.

However it does make sense to have it for supplier_report: you don't
want to show it in the dropdown if the pref is off
Comment 11 Jonathan Druart 2025-10-10 10:09:51 UTC
(In reply to Slava Shishkin from comment #9)
> One small thing. Do we really need
> 
> cancel_column.cannot_be_toggled = 1; 
> and 
> modify_column.cannot_be_toggled = 1;
> 
> These two columns already have those flags by default and can’t be toggled
> anyway.

Yes, might not be needed. I've removed them.
Comment 12 Jonathan Druart 2025-10-10 10:26:25 UTC
Created attachment 187726 [details] [review]
Bug 40982: Do not raise Error when bKohaColumnsUseNames is not set

Return early if there is no visibility to force
Comment 13 Slava Shishkin 2025-10-10 11:16:16 UTC
I checked the patch with the latest changes again. The table displays correctly in all cases, whether the basket is open or closed. I also even manipulated (deleted it several times) with DataTables_acqui_basket_orders from local storage. No problems were found. Thanks!
Comment 14 Jonathan Druart 2025-10-10 13:47:42 UTC
(In reply to Slava Shishkin from comment #13)
> I checked the patch with the latest changes again. The table displays
> correctly in all cases, whether the basket is open or closed. I also even
> manipulated (deleted it several times) with DataTables_acqui_basket_orders
> from local storage. No problems were found. Thanks!

Great, please signoff! :)

I will write some Cypress tests next week.
Comment 15 Slava Shishkin 2025-10-10 14:02:24 UTC
Created attachment 187735 [details] [review]
Bug 40982: (bug 40753 follow-up) Adjust conditions

The IF/ELSE block contents have been reversed in the JS code.

Signed-off-by: Slava Shishkin <slavashishkin@gmail.com>
Comment 16 Slava Shishkin 2025-10-10 14:02:26 UTC
Created attachment 187736 [details] [review]
Bug 40982: Allow to force visibility when needed

The _dt_visibility function is only called when the state is
initialized. Here we want to force the visibility of some columns even
if the state exists already.

The idea of this patch is to add a new 'force_visibility' flag to the
column when the visibility needs to be respected regardless what is in
the state. That should only be used when column cannot be toggled.

Signed-off-by: Slava Shishkin <slavashishkin@gmail.com>
Comment 17 Slava Shishkin 2025-10-10 14:02:28 UTC
Created attachment 187737 [details] [review]
Bug 40982: Remove cannot_be_toggled for modify and cancel

It's already set in the global config.

However it does make sense to have it for supplier_report: you don't
want to show it in the dropdown if the pref is off

Signed-off-by: Slava Shishkin <slavashishkin@gmail.com>
Comment 18 Slava Shishkin 2025-10-10 14:02:30 UTC
Created attachment 187738 [details] [review]
Bug 40982: Do not raise Error when bKohaColumnsUseNames is not set

Return early if there is no visibility to force

Signed-off-by: Slava Shishkin <slavashishkin@gmail.com>