Description
Slava Shishkin
2025-10-09 11:33:29 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. Hum I thought we fixed this problem with bug 40753. I will have a look at your patch later. (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. (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. 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. There is something else, I see what's going on. I will provide a fix later today. 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. 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. 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! 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 (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. 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 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! (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. 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> 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> 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> 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> |