Bug 34479 - Clear saved patron search selections after certain actions
Summary: Clear saved patron search selections after certain actions
Status: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: Main
Hardware: All All
: P5 - low major (vote)
Assignee: Matt Blenkinsop
QA Contact: Testopia
URL:
Keywords:
: 34682 (view as bug list)
Depends on: 29971
Blocks:
  Show dependency treegraph
 
Reported: 2023-08-04 16:45 UTC by Owen Leonard
Modified: 2024-02-07 11:53 UTC (History)
19 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
This fixes issues with patron search, and remembering the patrons selected after performing an action (such as Add to patron list, Merge selected patrons, Batch patron modification). Remembering selected patrons was introduced in Koha 22.11, bug 29971. Previously, the patrons selected after running an action were kept, and this either caused confusion, or could result in data loss if other actions were taken with new searches. Now, after performing a search and taking one of the actions available, you are now prompted with "Keep patrons selected for a new operation". When you return to the patron search: - If the patrons are kept: those patrons should still be selected - If the patrons aren't kept: the patron selection history is empty and no patrons are selected
Version(s) released in:
24.05.00,23.11.03,23.05.09


Attachments
Bug 34479: Move patron selection history to a re-useable file (4.90 KB, patch)
2023-11-27 10:43 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 34479: Add new files to templates (6.50 KB, patch)
2023-11-27 10:43 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 34479: Move patron selection history to a re-useable file (5.02 KB, patch)
2023-11-27 15:31 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 34479: Add new files to templates (6.50 KB, patch)
2023-11-27 15:31 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 34479: Add new files to templates (6.54 KB, patch)
2023-11-28 13:56 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review
Bug 34479: Move patron selection history to a re-useable file (5.08 KB, patch)
2023-12-01 13:36 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 34479: Add new files to templates (6.62 KB, patch)
2024-01-12 09:54 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 34479: Move patron selection history to a re-useable file (5.14 KB, patch)
2024-01-12 09:54 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 34479: Move patron selection history to a re-useable file (5.18 KB, patch)
2024-01-12 15:20 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review
Bug 34479: (RMaint follow-up) Correct selectors for 23.05.x (1.30 KB, patch)
2024-02-07 11:53 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Owen Leonard 2023-08-04 16:45:51 UTC
+++ This bug was initially created as a clone of Bug #29971 +++

(In reply to Chris Slone from comment #17)
> I think this is a problematic implementation, as it seems to be retaining
> the selected patrons even after you perform a function (merge patrons,
> create a list, do a batch edit) and do a completely new search. This means
> that the previously selected patrons remain selected even when it doesn't
> really make logical sense. It's great to be able to select multiple patrons
> across different searches, but doing something like merging accounts should
> absolutely clear out that selection.
> 
> It seems to already be causing some issues, see this ticket:
> 
> https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34262
> 
> We've also had some errors at our library from this (before we realized what
> was happening).

I'm moving this issue into a new bug report because we don't re-open bugs for features which have been pushed. This will let us have a discussion about refining the behavior.
Comment 1 Michael Adamyk 2023-08-21 21:16:03 UTC
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.
Comment 2 Laura Escamilla 2023-09-08 15:38:40 UTC
*** Bug 34682 has been marked as a duplicate of this bug. ***
Comment 3 Rebecca Coert 2023-10-17 15:47:23 UTC
This is an issue we discovered after migrating to 22.11.  I'm seeing the same behavior that Michael described.
Comment 4 Matt Blenkinsop 2023-11-27 10:43:18 UTC
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
Comment 5 Matt Blenkinsop 2023-11-27 10:43:21 UTC
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
Comment 6 Matt Blenkinsop 2023-11-27 10:48:14 UTC
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
Comment 7 Sharon Dugdale 2023-11-27 13:42:52 UTC
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)
Comment 8 Matt Blenkinsop 2023-11-27 15:31:44 UTC
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
Comment 9 Matt Blenkinsop 2023-11-27 15:31:47 UTC
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
Comment 10 Matt Blenkinsop 2023-11-27 15:35:13 UTC
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
Comment 11 Sharon Dugdale 2023-11-28 12:35:04 UTC
Redid all my previous tests in a new sandbox and all work as expected
Comment 12 Matt Blenkinsop 2023-11-28 13:43:19 UTC
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 :)
Comment 13 Matt Blenkinsop 2023-11-28 13:44:32 UTC
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
Comment 14 PTFS Europe Sandboxes 2023-11-28 13:56:50 UTC
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>
Comment 15 David Nind 2023-11-29 10:00:56 UTC
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...
Comment 16 Matt Blenkinsop 2023-12-01 13:36:53 UTC
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>
Comment 17 Jonathan Druart 2023-12-06 09:21:03 UTC
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.
Comment 18 Emily Lamancusa 2023-12-06 15:29:08 UTC
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.
Comment 19 Michael Adamyk 2023-12-06 15:36:01 UTC
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.
Comment 20 Matt Blenkinsop 2024-01-11 09:37:18 UTC
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.
Comment 21 Matt Blenkinsop 2024-01-11 15:41:51 UTC
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 ));
});
Comment 22 Jonathan Druart 2024-01-12 09:54:05 UTC
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.
Comment 23 Jonathan Druart 2024-01-12 09:54:35 UTC
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>
Comment 24 Jonathan Druart 2024-01-12 09:54:39 UTC
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>
Comment 25 Biblibre Sandboxes 2024-01-12 15:20:10 UTC
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>
Comment 26 Katrin Fischer 2024-01-16 11:01:28 UTC
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)));
Comment 27 Katrin Fischer 2024-01-16 11:08:58 UTC
Pushed for 24.05!

Well done everyone, thank you!
Comment 28 David Nind 2024-01-16 22:14:35 UTC
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.
Comment 29 Matt Blenkinsop 2024-01-18 11:43:26 UTC
Thanks for adding the text - just had a look and it looks good to me!
Comment 30 David Nind 2024-01-18 19:44:27 UTC
(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!
Comment 31 Fridolin Somers 2024-01-31 12:22:12 UTC
Pushed to 23.11.x for 23.11.03
Comment 32 Lucas Gass 2024-02-02 18:19:27 UTC
Backported to 23.05.x for upcoming 23.05.09
Comment 33 Martin Renvoize 2024-02-07 11:53:12 UTC
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>
Comment 34 Martin Renvoize 2024-02-07 11:53:27 UTC
Follow-up pushed