This is definitely problematic when trying to do several different patron merges in a row. It remembers previous searches. Thus, after doing one merge, when you search and select two new patrons to merge, it tries to add in a third patron (the previous merge) to merge with them.

*** Bug 34682 has been marked as a duplicate of this bug. ***

This is an issue we discovered after migrating to 22.11.  I'm seeing the same behavior that Michael described.

Created attachment 159261 [details] [review]
Bug 34479: Move patron selection history to a re-useable file

This patch takes some of the functionality for maintaining patron selections and moves it to a new file that can be used in other template files. It also introduces a new method for determining whether to delete the history after an operation is complete, along with an .inc file containing the checkbox that manages this

Test plan:
1) Navigate to Patrons and run a search
2) Use the checkboxes to select some patrons and run the three different options in the menu bar: Add to patron list, Merge selected patrons, Batch patron modification.
3) For each operation, you should see a checkbox asking if you want to "Keep patrons selected for anew operation". N.B. For adding patrons to a list, you will only see this when selecting to add them to a new list
4) When you run the operations, if you select the checkbox to keep the patrons then when you return tho the patron search, those patrons should all be still selected.
5) If you don't check the box, when you return to the search, your patron selection history should be empty and no patrons should be selected
N.B. If you have run a merge operation and elected to keep the patron history, you will only keep the patron who was kept

Created attachment 159262 [details] [review]
Bug 34479: Add new files to templates

This patch adds two newly created files to the templates to allow the patron selection history to be wiped or persisted as appropriate

Test plan as per previous commit

The patches above now allow users to choose whether to keep the history between operations - the default behaviour is that the history will not be kept.

I've tried to refactor the code to allow us to use the same code in multiple places, the only difficulty is in adding patrons to an existing list as there is no easy place to add a checkbox to keep/delete the selection history. That may need breaking out in a separate bug to allow us to address the issue with the current patches

when merging more than 2 patrons and clicking "keep patrons selected for a new operation" does not result in keeping patrons.

when merging 2 patrons after clicking on "keep...." then doing a patron search for a new list the "Patrons Selected" button says 1 but none are selected and none are brought through for a new merge.

when using batch patron modification: after clicking "keep patrons selected for a new operation" and you do another patron search and decide you do not actually want to keep them, if you then click the button "clear selected" at top of the page, choose new patrons you are directed to a message "No patron card numbers or borrowernumbers given" When you then click OK you are directed to the Batch Patron Modification landing page.

when adding 3 patrons to a new list and merging 2 and clicking "keep..." the next page shows the "patrons selected" button as 1 but none are selected. If you then click on 2 new patrons to merge the "patrons selected" says 3 but only the 2 new ones are shown on the next page. I cannot tell if koha is also merging the unseen one (the one kept from the merge on the list)

Created attachment 159275 [details] [review]
Bug 34479: Move patron selection history to a re-useable file

This patch takes some of the functionality for maintaining patron selections and moves it to a new file that can be used in other template files. It also introduces a new method for determining whether to delete the history after an operation is complete, along with an .inc file containing the checkbox that manages this

Test plan:
1) Navigate to Patrons and run a search
2) Use the checkboxes to select some patrons and run the three different options in the menu bar: Add to patron list, Merge selected patrons, Batch patron modification.
3) For each operation, you should see a checkbox asking if you want to "Keep patrons selected for anew operation". N.B. For adding patrons to a list, you will only see this when selecting to add them to a new list
4) When you run the operations, if you select the checkbox to keep the patrons then when you return tho the patron search, those patrons should all be still selected.
5) If you don't check the box, when you return to the search, your patron selection history should be empty and no patrons should be selected
N.B. If you have run a merge operation and elected to keep the patron history, you will only keep the patron who was kept

Created attachment 159276 [details] [review]
Bug 34479: Add new files to templates

This patch adds two newly created files to the templates to allow the patron selection history to be wiped or persisted as appropriate

Test plan as per previous commit

Hi Sharon,

Thanks for testing! I have made some code changes as per your feedback:

(In reply to Sharon Dugdale from comment #7)
> when merging more than 2 patrons and clicking "keep patrons selected for a
> new operation" does not result in keeping patrons.

The patron you selected to keep should still be selected. However, this was only working in certain scenarios so I have fixed this and it should now work.
> 
> when merging 2 patrons after clicking on "keep...." then doing a patron
> search for a new list the "Patrons Selected" button says 1 but none are
> selected and none are brought through for a new merge.

This was the same error as above, just presenting itself in a different way.

> 
> when using batch patron modification: after clicking "keep patrons selected
> for a new operation" and you do another patron search and decide you do not
> actually want to keep them, if you then click the button "clear selected" at
> top of the page, choose new patrons you are directed to a message "No patron
> card numbers or borrowernumbers given" When you then click OK you are
> directed to the Batch Patron Modification landing page.

This is a good spot, I've run some tests and this is also happening in our master branch, not just with this new code I've added. This might need addressing in a separate bug or there may already be one. I'll have a look and see if one exists

> 
> when adding 3 patrons to a new list and merging 2 and clicking "keep..." the
> next page shows the "patrons selected" button as 1 but none are selected. If
> you then click on 2 new patrons to merge the "patrons selected" says 3 but
> only the 2 new ones are shown on the next page. I cannot tell if koha is
> also merging the unseen one (the one kept from the merge on the list)

As per the two comments above, this all seems to be stemming from the same error and should be fixed :)

Let me know if this improves things

Redid all my previous tests in a new sandbox and all work as expected

Great, thanks Sharon! If you change the bug status to "Signed off" it will move into the QA queue and you'll get your name in the release notes for the version that this patch is released in :)

I think you can do it via the sandbox using the actions dropdown - that will sign off all the patches and change the bug for you, just click "Sign off patches" from the dropdown

Created attachment 159295 [details] [review]
Bug 34479: Add new files to templates

This patch adds two newly created files to the templates to allow the patron selection history to be wiped or persisted as appropriate

Test plan as per previous commit

Signed-off-by: Sharon Dugdale <sharon.dugdale@cumberland.gov.uk>

Matt: Changed assignee to you

Sharon: You will have to change the status of the bug in Bugzilla (Status: Needs Signoff to Signed Off) - you can add your sign-off line to the patches using the sandboxes, but it doesn't change the status...

Created attachment 159451 [details] [review]
Bug 34479: Move patron selection history to a re-useable file

This patch takes some of the functionality for maintaining patron selections and moves it to a new file that can be used in other template files. It also introduces a new method for determining whether to delete the history after an operation is complete, along with an .inc file containing the checkbox that manages this

Test plan:
1) Navigate to Patrons and run a search
2) Use the checkboxes to select some patrons and run the three different options in the menu bar: Add to patron list, Merge selected patrons, Batch patron modification.
3) For each operation, you should see a checkbox asking if you want to "Keep patrons selected for anew operation". N.B. For adding patrons to a list, you will only see this when selecting to add them to a new list
4) When you run the operations, if you select the checkbox to keep the patrons then when you return tho the patron search, those patrons should all be still selected.
5) If you don't check the box, when you return to the search, your patron selection history should be empty and no patrons should be selected
N.B. If you have run a merge operation and elected to keep the patron history, you will only keep the patron who was kept

Signed-off-by: Sharon Dugdale <sharon.dugdale@cumberland.gov.uk>

I don't understand the original problem. This implementation seems more confusing to me.

There is an info at the top of the page: "Patrons selected: 2"

Here you are adding not-related checkboxes all around.

You are also changing the existing behaviour: prior to this patch you didn't have to do anything to keep the selection, now you need to tick the box.

I would like to collect more feedbacks on what you suggest with this approach.

The main problem is that adding the "saved patron" functionality was a behavior change in the first place, and if staff aren't aware of it or forget to pay attention to the "patrons selected" box, it can lead to data loss.

Most notably, if a staff member merges Patron A and Patron B (we'll call the resulting account Patron AB), the merged patron remains selected. If they don't remember or realize they need to clear selected patrons before trying to do a separate merge of Patron C and Patron D, then Koha will merge Patron AB, C, and D all together, and Patron AB will be lost, with no good way to undo the action.

The consequences to accidentally keeping an "old" selected patron when adding to a list or doing a batch modification are less permanent, but they will still lead to data errors that are likely to go unnoticed.

In our library system, staff have also reported confusion about going to the patron page and finding that there are already "mysteriously" selected patrons. (Presumably, those patrons are still saved from an earlier action.) This means that even if a staff member is conscientious about clearing selected patrons in between actions, they might still have a leftover selected patron on their "first" action when they're not expecting to have one, and therefore are less likely to notice it.

I think it's pretty reasonable to ask a staff member to actively confirm whether they still want to keep their selected patrons after performing an action on them. I imagine it's more common to perform one action on a patron set and then move on, rather than perform an additional, separate batch action on the exact same set. More importantly, it's safer to assume the staff member is finished with their selected patrons after performing an action, unless specified otherwise:

 -> If a staff member accidentally fails to keep their selected patrons, it may 
    cause them a bit more work re-selecting the patrons, but their attempted 
    action will fail and they will be able to correct the issue right away.

 ->If a staff member accidentally keeps their selected patrons when they don't 
   intend to, their attempted action will succeed with unintended side effects 
   that may go unnoticed and may not be easy to correct.

Emily, that's a fantastic explanation! I agree; the drawbacks of not keeping patrons selected is occasional inconvenience, while the current behavior can potentially cause major problems, as Emily noted. 

This is made worse because the selected patron doesn't ever "expire," so someone could be interrupted and do something else for an hour, return to the computer, and do another process without remembering that they still have patrons selected from their previous process. Again, potentially causing data loss. 

In my opinion, it's not very intuitive that selections are saved between processes. It perplexed me when I first noticed it, and I thought it was a problem, not a feature.

Thanks Emily and Michael for the input, I think its clear we need some sort of "protection" on this feature to prevent data loss. The alternative to the proposed solution is to put the patron selections behind a syspref to determine whether it should maintain the selections or not. I'm open to either, let me know any thoughts on whether a syspref would be a better option.

In the meantime, if any libraries / customers are struggling with this issue then the code below can be added to IntranetUserJS. This will wipe the selection history every time you navigate so it won't persist between patron searches

$(document).ready(function() {
   var patron_search_selections = [];
  localStorage.setItem('patron_search_selections', JSON.stringify( patron_search_selections ));
});

Just found one tiny unexpected behaviour:
1. Select x patrons
2. Add to patron list > new list
3. Tick "keep select patrons" box
4. Add to patron list > existing list

=> Patron selection is kept (we didn't ask for anything).

Let say it's not a bug.

Created attachment 160906 [details] [review]
Bug 34479: Add new files to templates

This patch adds two newly created files to the templates to allow the patron selection history to be wiped or persisted as appropriate

Test plan as per previous commit

Signed-off-by: Sharon Dugdale <sharon.dugdale@cumberland.gov.uk>

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

Created attachment 160907 [details] [review]
Bug 34479: Move patron selection history to a re-useable file

This patch takes some of the functionality for maintaining patron selections and moves it to a new file that can be used in other template files. It also introduces a new method for determining whether to delete the history after an operation is complete, along with an .inc file containing the checkbox that manages this

Test plan:
1) Navigate to Patrons and run a search
2) Use the checkboxes to select some patrons and run the three different options in the menu bar: Add to patron list, Merge selected patrons, Batch patron modification.
3) For each operation, you should see a checkbox asking if you want to "Keep patrons selected for anew operation". N.B. For adding patrons to a list, you will only see this when selecting to add them to a new list
4) When you run the operations, if you select the checkbox to keep the patrons then when you return tho the patron search, those patrons should all be still selected.
5) If you don't check the box, when you return to the search, your patron selection history should be empty and no patrons should be selected
N.B. If you have run a merge operation and elected to keep the patron history, you will only keep the patron who was kept

Signed-off-by: Sharon Dugdale <sharon.dugdale@cumberland.gov.uk>

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

Created attachment 160982 [details] [review]
Bug 34479: Move patron selection history to a re-useable file

This patch takes some of the functionality for maintaining patron selections and moves it to a new file that can be used in other template files. It also introduces a new method for determining whether to delete the history after an operation is complete, along with an .inc file containing the checkbox that manages this

Test plan:
1) Navigate to Patrons and run a search
2) Use the checkboxes to select some patrons and run the three different options in the menu bar: Add to patron list, Merge selected patrons, Batch patron modification.
3) For each operation, you should see a checkbox asking if you want to "Keep patrons selected for anew operation". N.B. For adding patrons to a list, you will only see this when selecting to add them to a new list
4) When you run the operations, if you select the checkbox to keep the patrons then when you return tho the patron search, those patrons should all be still selected.
5) If you don't check the box, when you return to the search, your patron selection history should be empty and no patrons should be selected
N.B. If you have run a merge operation and elected to keep the patron history, you will only keep the patron who was kept

Signed-off-by: Sharon Dugdale <sharon.dugdale@cumberland.gov.uk>

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

This is wrong since it was moved into a .js file:

$("#table_search_selections").show().find("span").text(_("Patrons selected: " + number));

It needs to be changed to a double underscore, but mixing the number into the translation string like that is also not good. Updated to:

$("#table_search_selections").show().find("span").text(__("Patrons selected: %s".format(number)));

Pushed for 24.05!

Well done everyone, thank you!

Could someone edit the "Text to go in the release notes"? I've had a go, but I wasn't involved in testing, so I'm not sure whether I am describing this correctly.

Thanks for adding the text - just had a look and it looks good to me!

(In reply to Matt Blenkinsop from comment #29)
> Thanks for adding the text - just had a look and it looks good to me!

Thanks Matt!

Pushed to 23.11.x for 23.11.03

Backported to 23.05.x for upcoming 23.05.09

Created attachment 161798 [details] [review]
Bug 34479: (RMaint follow-up) Correct selectors for 23.05.x

We renamed patron_search_selected to table_search_selections in
upstream, but missed that when backporting this bug.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Follow-up pushed