Bug 32477

I can confirm this. 

The item I used was 39999000011135 from the sample data. It doesn't have source of acquisition set, but hiding the next column after holds, in this case collection, also made the bug visible.

+1

Apparently this happens because columns still have attribute "checked=checked" even if class "selected" is removed. Then we do this:

if ($(this).prop("checked")) {
    showColumn(num);
 } else {
    hideColumn(num);
}

So solution could simply be that we need to set checked attributed as false, not just remove class "selected".

(In reply to Emmi Takkinen from comment #3)
> Apparently this happens because columns still have attribute
> "checked=checked" even if class "selected" is removed. Then we do this:
> 
> if ($(this).prop("checked")) {
>     showColumn(num);
>  } else {
>     hideColumn(num);
> }
> 
> So solution could simply be that we need to set checked attributed as false,
> not just remove class "selected".

Hmm, I should have mentioned here that this is just part of problem. Another one  is that function "hideColumns" hides wrong columns when tool is launched again. Which is weird, code looks OK to me.

We launch function "hideColumns" too early. When page loads, dataTable still has "hidden" column with class "sorting_1" in it. That's why number of columns doesn't match to the columns which are actually present in the table and "hideColumns" hides column before next to the column we actually want to hide. Moving function after removing that extra column seems to solve the problem.

Created attachment 159206 [details] [review]
Bug 32477: Launch function hideColumns after changing datatables settings

Hiding columns in batch item modification breaks if page is
loaded again. If one hides e.g. column
"Source of classification or shelving scheme" and then loads
the page column "Holds" disappears. Also behaviour of checbox
changes for "Source of classification..." column. This happens
because we launch function "hideColumns" before changing
datatables settings and "hidden" column with class "sorting_1"
is still present.

To test:
1. Find items to modify and modify them with batch item
modification tool.
2. In modification page, hide column "Source of classification or
shelving scheme". Confirm correct column is hidden.
3. Reload the page or modify items again.
=> Note that column "Holds" is now hidden and checking checkbox
for column "Source of classification..." behaves incorrectly (unchecked
checkbox shows column, checked hides it)
4. Apply this patch.
5. Repeat steps 1, 2 and 3.
=> Confirm correct column is now hidden when page is loaded again
and checkbox works correctly.

Sponsored-by: Koha-Suomi Oy

Created attachment 159249 [details] [review]
Bug 32477: Launch function hideColumns after changing datatables settings

Hiding columns in batch item modification breaks if page is
loaded again. If one hides e.g. column
"Source of classification or shelving scheme" and then loads
the page column "Holds" disappears. Also behaviour of checbox
changes for "Source of classification..." column. This happens
because we launch function "hideColumns" before changing
datatables settings and "hidden" column with class "sorting_1"
is still present.

To test:
1. Find items to modify and modify them with batch item
modification tool.
2. In modification page, hide column "Source of classification or
shelving scheme". Confirm correct column is hidden.
3. Reload the page or modify items again.
=> Note that column "Holds" is now hidden and checking checkbox
for column "Source of classification..." behaves incorrectly (unchecked
checkbox shows column, checked hides it)
4. Apply this patch.
5. Repeat steps 1, 2 and 3.
=> Confirm correct column is now hidden when page is loaded again
and checkbox works correctly.

Sponsored-by: Koha-Suomi Oy
Signed-off-by: David Nind <david@davidnind.com>

Testing notes (using KTD)
=========================

I got a bit lost trying to follow the test plan using master:
1. Staff interface home > Cataloging > Batch editing > Batch item modification
2. Barcodes used:
    39999000006094
    39999000006117
    39999000011418
    39999000005134
3. Columns that you can show or hide: Collection, Home library, Current library, Shelvng location, Date acquired, Barcode, Date last seen, Price effective from, Koha item type
4. Columns shown in the table listing items to modify: Tick box column, Title, Holds, Collection, Home library, Current library, Shelving location, Date acquired, Barcode, Date last seen, Price effective from, Koha item type
4. Table options: Export (no options to configure table columns)
5. I don't see options to show or hide "Source of classification or shelving scheme", or a way to choose what columns are available to show/hide (I couldn't see anything under Administration > Other parameters > Table settings, or a system preference).
6. Maybe there have been some changes between 22.05 and master 8-)

Having said that, the behavour being fixed can be replicated using any of the columns actually displayed. I tested using the collection column (as per Katrin's comment#1 :
1. Show/hide columns: untick Collection
2. Refresh the page
3. Hold column is hidden, Collection column is shown (even though not ticked)
4. Tick Collection, refresh the page, Collection column is not displayed even though Collection is ticked.
5. Apply the patch
6. Check that the correct columns are displayed, and Show/hide options correctly match columns displayed, by:
   6.1 Hiding a column and refreshing the page
   6.2 Hiding multiple columns and refreshing the page
   6.3 Selecting one of the hidden columns to display and refreshing the page
   6.4 Any combination of 6.1-6.3

Created attachment 159258 [details] [review]
Bug 32477: Launch function hideColumns after changing datatables settings

Hiding columns in batch item modification breaks if page is
loaded again. If one hides e.g. column "Collection" and then loads
the page column "Holds" disappears. Also behaviour of checbox
changes for "Collection" column. This happens because we launch
function "hideColumns" before changing datatables settings and
"hidden" column with class "sorting_1" is still present.

To test:
1. Find items to modify and modify them with batch item
modification tool.
2. In modification page, hide column "Collection". Confirm
correct column is hidden.
3. Reload the page or modify items again.
=> Note that column "Holds" is now hidden and checking checkbox
for column "Collection" behaves incorrectly (unchecked checkbox
shows column, checked hides it)
4. Apply this patch.
5. Repeat steps 1, 2 and 3.
=> Confirm correct column is now hidden when page is loaded again
and checkbox works correctly.

Sponsored-by: Koha-Suomi Oy
Signed-off-by: David Nind <david@davidnind.com>

Updated test plan based on Davids comment.

There seems to be a minor problem with these. If one hides all columns and then reloads the page or launches tool again, "Holds" column isn't displayed and "Itemtype" column is. Also "Itemtype" columns checkbox is checked and works as it should.

So, still needs work?

(In reply to Victor Grousset/tuxayo from comment #12)
> So, still needs work?

A little yeah. Setting this back to assigned.

(In reply to Emmi Takkinen from comment #11)
> There seems to be a minor problem with these. If one hides all columns and
> then reloads the page or launches tool again, "Holds" column isn't displayed
> and "Itemtype" column is. Also "Itemtype" columns checkbox is checked and
> works as it should.

Problem is that we toggle columns instead of hiding them. Instead of doing this inside "hideColumns":

>$("#itemst td:nth-child(" + valCookie[i] + "),#itemst th:nth-child(" + valCookie[i] + ")").toggle();
we could to this:

>$("#itemst td:nth-child(" + valCookie[i] + "),#itemst th:nth-child(" + valCookie[i] + ")").nextAll().hide();
Or we could use this outside of for loop:

>$("#itemst td:nth-child(" + nb_cols + "),#itemst th:nth-child(" + nb_cols + ")").nextAll().hide();
I'm a bit unsure how to fix checkboxes. First we set number of columns inside a cookie as list that starts with value 3 and ends to the value 19. With method "guess_nb_colums" we receive value 3 and decrease this value from values received from cookie that list. So we end up with values from 0 to 16 which we use to determine which checkboxes we hide. However, "Itemtype" column has id "checkbox17" which is out of scope. 

My guess is that "guess_nb_columns" returns a wrong value when first column contains checkbox, but I don't now if changing them has some unwanted effect to task queue jobs. Another option is to add 1 to variable index but that's a bit ugly solution.

Hmm, it seems that the actual problem is how the variable "allColumns" is build. It doesn't take into account that there are more than two columns to skip.

Created attachment 160230 [details] [review]
Bug 32477: Fix hiding all columns in item batch mod tools

If one hides all columns in batch item modification tool
and then reloads the page or launches tool again, "Holds"
column isn't displayed and "Itemtype" column is. Also
"Itemtype" columns checkbox remains checked. This happens
because "Itemtype" columns id is out of scope of list of column
numbers set in cookies. This patch adds 3 to number of columns
in variable "allColums" to take account all 3 columns we don't
want to hide.

To test:
1. Find items to modify and modify them with batch item
modification tool.
2. Use "Hide all columns" checkbox to hide all columns.
3. Confirm all columns expect checkbox, "Title" and "Holds"
columns are hidden correctly.
4. Reload the page.
=> Note that "Holds" column is now hidden and "Itemtype"
column is displayed. Also "Itemtype" checkbox is checked.
5. Apply this patch.
6. Repeat steps 1 to 4.
=> "Holds" column should now be displayed and "Itemtype"
hidden. None of the checkbox should be checked.

Sponsored-by: Koha-Suomi Oy

Created attachment 160293 [details] [review]
Bug 32477: Launch function hideColumns after changing datatables settings

Hiding columns in batch item modification breaks if page is
loaded again. If one hides e.g. column "Collection" and then loads
the page column "Holds" disappears. Also behaviour of checbox
changes for "Collection" column. This happens because we launch
function "hideColumns" before changing datatables settings and
"hidden" column with class "sorting_1" is still present.

To test:
1. Find items to modify and modify them with batch item
modification tool.
2. In modification page, hide column "Collection". Confirm
correct column is hidden.
3. Reload the page or modify items again.
=> Note that column "Holds" is now hidden and checking checkbox
for column "Collection" behaves incorrectly (unchecked checkbox
shows column, checked hides it)
4. Apply this patch.
5. Repeat steps 1, 2 and 3.
=> Confirm correct column is now hidden when page is loaded again
and checkbox works correctly.

Sponsored-by: Koha-Suomi Oy
Signed-off-by: David Nind <david@davidnind.com>

Created attachment 160294 [details] [review]
Bug 32477: Fix hiding all columns in item batch mod tools

If one hides all columns in batch item modification tool
and then reloads the page or launches tool again, "Holds"
column isn't displayed and "Itemtype" column is. Also
"Itemtype" columns checkbox remains checked. This happens
because "Itemtype" columns id is out of scope of list of column
numbers set in cookies. This patch adds 3 to number of columns
in variable "allColums" to take account all 3 columns we don't
want to hide.

To test:
1. Find items to modify and modify them with batch item
modification tool.
2. Use "Hide all columns" checkbox to hide all columns.
3. Confirm all columns expect checkbox, "Title" and "Holds"
columns are hidden correctly.
4. Reload the page.
=> Note that "Holds" column is now hidden and "Itemtype"
column is displayed. Also "Itemtype" checkbox is checked.
5. Apply this patch.
6. Repeat steps 1 to 4.
=> "Holds" column should now be displayed and "Itemtype"
hidden. None of the checkbox should be checked.

Sponsored-by: Koha-Suomi Oy
Signed-off-by: David Nind <david@davidnind.com>

Created attachment 160904 [details] [review]
Bug 32477: Launch function hideColumns after changing datatables settings

Hiding columns in batch item modification breaks if page is
loaded again. If one hides e.g. column "Collection" and then loads
the page column "Holds" disappears. Also behaviour of checbox
changes for "Collection" column. This happens because we launch
function "hideColumns" before changing datatables settings and
"hidden" column with class "sorting_1" is still present.

To test:
1. Find items to modify and modify them with batch item
modification tool.
2. In modification page, hide column "Collection". Confirm
correct column is hidden.
3. Reload the page or modify items again.
=> Note that column "Holds" is now hidden and checking checkbox
for column "Collection" behaves incorrectly (unchecked checkbox
shows column, checked hides it)
4. Apply this patch.
5. Repeat steps 1, 2 and 3.
=> Confirm correct column is now hidden when page is loaded again
and checkbox works correctly.

Sponsored-by: Koha-Suomi Oy
Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Created attachment 160905 [details] [review]
Bug 32477: Fix hiding all columns in item batch mod tools

If one hides all columns in batch item modification tool
and then reloads the page or launches tool again, "Holds"
column isn't displayed and "Itemtype" column is. Also
"Itemtype" columns checkbox remains checked. This happens
because "Itemtype" columns id is out of scope of list of column
numbers set in cookies. This patch adds 3 to number of columns
in variable "allColums" to take account all 3 columns we don't
want to hide.

To test:
1. Find items to modify and modify them with batch item
modification tool.
2. Use "Hide all columns" checkbox to hide all columns.
3. Confirm all columns expect checkbox, "Title" and "Holds"
columns are hidden correctly.
4. Reload the page.
=> Note that "Holds" column is now hidden and "Itemtype"
column is displayed. Also "Itemtype" checkbox is checked.
5. Apply this patch.
6. Repeat steps 1 to 4.
=> "Holds" column should now be displayed and "Itemtype"
hidden. None of the checkbox should be checked.

Sponsored-by: Koha-Suomi Oy
Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Pushed for 24.05!

Well done everyone, thank you!

Pushed to 23.11.x for 23.11.02

Backported to 23.05.x for upcoming 23.05.09.