Bug 20312

Summary: Add a link towards the last consulted patron
Product: Koha Reporter: Maksim Sen <maksim.sen>
Component: PatronsAssignee: Maksim Sen <maksim.sen>
Status: CLOSED FIXED QA Contact: Martin Renvoize <martin.renvoize>
Severity: new feature    
Priority: P5 - low CC: caroline.cyr-la-rose, gmcharlt, jonathan.druart, josef.moravec, katrin.fischer, kyle.m.hall, maksim.sen, martin.renvoize, niamh.walker-headon, nick, oleonard, philippe.blouin, pierre-luc.lapointe, sandboxes
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
A first pass at adding a handy feature to allow quickly navigating back to the last searched user in the staff client.
Version(s) released in:
Bug Depends on:    
Bug Blocks: 21244, 21245, 21246, 24549    
Attachments: Bug 20312 - Add a link to last consulted patron
Bug 20312 - Add a link to last consulted patron
Screenshot of interface idea
Bug 20312: Add a link to last consulted patron
Bug 20312: Add a link to last consulted patron
Bug 20312: (follow-up) Minor CSS corrections
Bug 20312: Add a link to last consulted patron
Bug 20312: Add a link to last consulted patron
Bug 20312: Add a link to last consulted patron
Bug 20312: Add a link to last consulted patron
Bug 20312: Add a link to last consulted patron
Bug 20312: Add a link to last consulted patron
Bug 20312: (QA follow-up) Make display syspref driven
Bug 20312: (QA follow-up) Make display syspref driven

Description Maksim Sen 2018-02-28 18:58:11 UTC
This new feature will add a link (on the top-left of the site) to the last patron consulted in the staff site. The feature currently uses cookies to save this information. When a staff site user accesses a patron record, the ID, name and card number of the patron will be saved in the cookie.

(I'll add a patch and a test plan shortly)
Comment 1 Maksim Sen 2018-02-28 19:32:43 UTC Comment hidden (obsolete)
Comment 2 Biblibre Sandboxes 2018-03-12 10:34:11 UTC
Patch tested with a sandbox, by Axelle <axelle.clarisse@univ-amu.fr>
Comment 3 Biblibre Sandboxes 2018-03-12 10:34:32 UTC Comment hidden (obsolete)
Comment 4 Katrin Fischer 2018-03-12 15:49:10 UTC
Hi Maksim,

the patch contains a po file that should not be there, can you please fix?

I am also having doubts on the the placmeent of the link. This is not a link to a module, so I tihnk placing it in the top navigation is not a good spot.

Asking Owen for additional feedback.
Comment 5 Owen Leonard 2018-05-30 15:56:13 UTC
Created attachment 75672 [details]
Screenshot of interface idea

Something like this might work.
Comment 6 Pierre-Luc Lapointe 2018-07-25 20:40:46 UTC
Created attachment 77272 [details] [review]
Bug 20312: Add a link to last consulted patron

This is an update to Maksim's patch. The link "Last patron" is now on the top-right corner of the screen and has the same style as attachment 75672 [details]. Moreover, the "X" button is added to remove the link if clicked on.

To test:
1- Apply the patch
2- Go to a patron details' page
3- Verify that the link "Last patron" appears on the top-right corner of the screen
4- Go to another page
5- The link should still be here
6- Click on the link
7- Verify that you are at the patron's checkout page
8- Click on the "X" button next to the "Last patron" link
9- The link should be gone
10- Go to another page
11- The link shouldn't appear (until the next patron detail's page)
Comment 7 Pierre-Luc Lapointe 2018-07-25 20:41:53 UTC
Hi Owen, Katrin,

I created the file last-borrower.inc that contains the main code for the feature. It has a <style> tag for the link to appear like in attachment 75672 [details]. I would like to know if this tag is in the right place of it should be in another file.

Also, is the link's position appropriate?

Thank you.
Comment 8 Katrin Fischer 2018-08-01 23:22:41 UTC
Hi Pierre-Luc,

taking a quick look. The QA script notes some issues:

 FAIL	koha-tmpl/intranet-tmpl/prog/en/includes/last-borrower.inc
   FAIL	  forbidden patterns
		forbidden pattern: tab char (line 44)
		forbidden pattern: intranet-tmpl should certainly replaced with [% interface %] (line 3)
		forbidden pattern: tab char (line 41)
		forbidden pattern: tab char (line 48)
		forbidden pattern: tab char (line 38)
		forbidden pattern: tab char (line 40)
		forbidden pattern: tab char (line 37)

Please note, that once this is pushed, the new cookie should be documented here:
https://wiki.koha-community.org/wiki/Use_of_Cookies

I will leave the question of where the Javascript should go to Owen, but feel like it might be better in a separate .js file.

I wonder if libraries would like to have this optional for privacy reasons. It might be safer to add a pref.
Comment 9 Owen Leonard 2018-08-13 12:35:04 UTC
Some suggestions:

 - Use <i class="fa fa-remove"></i> instead of an image for X.
 - Put the CSS in staff-global.scss
 - Put the JavaScript in staff-global.js
   - Add the JS to the existing $(document).ready() so you don't need 'addEventListener('DOMContentLoaded')';
   - Be consistent about whether you use single or double quotes

(In reply to Katrin Fischer from comment #8)

> I wonder if libraries would like to have this optional for privacy reasons.
> It might be safer to add a pref.

A lot of this already rests in the hands of the librarian and their browser. If I have my browser configured to restore my last session, my back button will take me back to the last patron without a handy link.

The way this patch does it, the cookie should disappear when the browser is closed. I would think that is adequate.
Comment 10 Pierre-Luc Lapointe 2018-08-13 20:42:11 UTC
Created attachment 77769 [details] [review]
Bug 20312: Add a link to last consulted patron

This new feature adds a link to the last patron consulted
in the staff site. It uses cookies to save this information.
When a staff site user accesses a patron record, the ID, name
and card number of the patron will be saved in the cookie.

To test:
1- Apply the patch
2- Go to a patron details' page
3- Verify that the link "Last patron" appears on the top-right
   corner of the screen
4- Go to another page
5- The link should still be here
6- Click on the link
7- Verify that you are at the patron's checkout page
8- Click on the "X" button next to the "Last patron" link
 9- The link should be gone
10- Go to another page
11- The link shouldn't appear (until the next patron detail's
    page)
Comment 11 Owen Leonard 2018-08-14 15:13:29 UTC
Created attachment 77796 [details] [review]
Bug 20312: (follow-up) Minor CSS corrections

This patch makes a couple of minor CSS corrections to match linter style
and to correctly color the "X" link.
Comment 12 Owen Leonard 2018-08-14 15:19:23 UTC
I noticed a problem when testing this today: The first time the "last patron" link appears, it doesn't contain any parameters (i.e. /cgi-bin/koha/circ/circulation.pl instead of /cgi-bin/koha/circ/circulation.pl?borrowernumber=XXX). It's only after I open a second patron record that the "last patron" link takes me to a valid patron record.
Comment 13 Pierre-Luc Lapointe 2018-08-15 15:12:18 UTC
Created attachment 77835 [details] [review]
Bug 20312: Add a link to last consulted patron

This new feature adds a link to the last patron consulted
in the staff site. It uses cookies to save this information.
When a staff site user accesses a patron record, the ID, name
and card number of the patron will be saved in the cookie.

To test:
1- Apply the patch
2- Go to a patron details' page
3- Verify that the link "Last patron" appears on the top-right
   corner of the screen
4- Go to another page
5- The link should still be here
6- Click on the link
7- Verify that you are at the patron's checkout page
8- Click on the "X" button next to the "Last patron" link
9- The link should be gone
10- Go to another page
11- The link shouldn't appear (until the next patron detail's
    page)
Comment 14 Owen Leonard 2018-08-17 13:34:38 UTC
This seems to be working well now. The only thing which seems unexpected to me was that the "Last patron" link changed immediately upon opening a new checkout screen.

- Check out to Patron A - "Last patron" = Patron A.
- Check out to Patron B - "Last patron" = Patron B.
  ...Isn't the "last patron" technically Patron A?

I think it's only when switching between patrons at checkout when this is potentially confusing.

I'm not sure if that justifies failing this. I'm going to defer to QA.
Comment 15 Owen Leonard 2018-08-17 13:35:23 UTC
Created attachment 77977 [details] [review]
Bug 20312: Add a link to last consulted patron

This new feature adds a link to the last patron consulted
in the staff site. It uses cookies to save this information.
When a staff site user accesses a patron record, the ID, name
and card number of the patron will be saved in the cookie.

To test:
1- Apply the patch
2- Go to a patron details' page
3- Verify that the link "Last patron" appears on the top-right
   corner of the screen
4- Go to another page
5- The link should still be here
6- Click on the link
7- Verify that you are at the patron's checkout page
8- Click on the "X" button next to the "Last patron" link
9- The link should be gone
10- Go to another page
11- The link shouldn't appear (until the next patron detail's
    page)

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 16 Martin Renvoize 2018-08-17 14:00:40 UTC
Created attachment 77979 [details] [review]
Bug 20312: Add a link to last consulted patron

This new feature adds a link to the last patron consulted
in the staff site. It uses cookies to save this information.
When a staff site user accesses a patron record, the ID, name
and card number of the patron will be saved in the cookie.

To test:
1- Apply the patch
2- Go to a patron details' page
3- Verify that the link "Last patron" appears on the top-right
   corner of the screen
4- Go to another page
5- The link should still be here
6- Click on the link
7- Verify that you are at the patron's checkout page
8- Click on the "X" button next to the "Last patron" link
9- The link should be gone
10- Go to another page
11- The link shouldn't appear (until the next patron detail's
    page)

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 17 Martin Renvoize 2018-08-17 14:02:27 UTC
Nice little feature addition here.. code looks reasonable and the feature works as expected for me.. I can't find any regressions and the qa scripts all pass.

Passing QA.
Comment 18 Martin Renvoize 2018-08-17 14:05:28 UTC
Minor note.. https://wiki.koha-community.org/wiki/Use_of_Cookies will need updating to reflect the additional cookie data added here.
Comment 19 Jonathan Druart 2018-08-17 14:12:51 UTC
Hi,

Now it looks like that: https://snag.gy/6vbjNS.jpg (a bit different that what is on the attached screenshot)

I am sorry to tell that, but I do not like it. I will list some things that are wrong and enhancement suggestions:

* #lastborrower-window ("window"?) is shown by default, then hidden when the page is loaded, it should be the reverse.

* A general pref should control it, you click 'x' then it displays again. It can be frustrating (yes it could be hidden with js, but here I think we need a pref (or better would have been a config at per user, but out of the scope).

* If you are on patron A detail view, you see the "last patron" that link to ... patron A. It is useless in that case. Either it points to the last patron, or is not displayed.

* I think it would have been more useless with the last X (5?) instead of the last one (not required)

You can wait for other QAers opinions before implementing these changes.
Comment 20 Jonathan Druart 2018-08-17 14:38:29 UTC
(In reply to Jonathan Druart from comment #19)
> * I think it would have been more useless with the last X (5?) instead of
> the last one (not required)

Ofc this was ^useless^useful^
Comment 21 Martin Renvoize 2018-08-17 14:49:20 UTC
Further comment.. it would be good to set the 'last patron' upon navigating away from a patron interaction rather than to it if that's possible.. else 'last patron' and 'current patron' end up matching immediately so if you want to return to the last patron you're stuck again at starting from scratch.
Comment 23 Pierre-Luc Lapointe 2018-08-17 20:50:15 UTC
Created attachment 78002 [details] [review]
Bug 20312: Add a link to last consulted patron

This new feature adds a link to the last patron consulted
in the staff site. It uses cookies to save this information.
When a staff site user accesses a patron record, the ID, name
and card number of the patron will be saved in the cookie.

To test:
1- Apply the patch
2- Go to a patron details' page
4- Go to another page
4- Verify that the link "Last patron" appears on the top-right
   corner of the screen
5- Click on the link
6- Verify that you are at the patron's checkout page
7- Click on the "X" button next to the "Last patron" link
8- The link should be gone
9- Go to another page
10- The link shouldn't appear (until the next patron detail's
    page)
Comment 24 Martin Renvoize 2018-08-18 07:27:58 UTC
Created attachment 78005 [details] [review]
Bug 20312: Add a link to last consulted patron

This new feature adds a link to the last patron consulted
in the staff site. It uses cookies to save this information.
When a staff site user accesses a patron record, the ID, name
and card number of the patron will be saved in the cookie.

To test:
1- Apply the patch
2- Go to a patron details' page
4- Go to another page
4- Verify that the link "Last patron" appears on the top-right
   corner of the screen
5- Click on the link
6- Verify that you are at the patron's checkout page
7- Click on the "X" button next to the "Last patron" link
8- The link should be gone
9- Go to another page
10- The link shouldn't appear (until the next patron detail's
    page)

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 25 Martin Renvoize 2018-08-18 07:31:10 UTC
Second times a charm.  Thankyou for making the requested changes, this looks very good to me now.

Passing QA
Comment 26 Nick Clemens 2018-09-19 12:35:38 UTC
(In reply to Jonathan Druart from comment #19)
> * A general pref should control it, you click 'x' then it displays again. It
> can be frustrating (yes it could be hidden with js, but here I think we need
> a pref

I agree with Jonathan here, I asked around at Kohacon, many libraries would like this feature, but a few had concerns about privacy/security and would like to have the option to disable.

Can you please add a syspref to disable the feature?
Comment 27 Martin Renvoize 2018-10-31 13:58:22 UTC
Created attachment 81726 [details] [review]
Bug 20312: Add a link to last consulted patron

This new feature adds a link to the last patron consulted
in the staff site. It uses cookies to save this information.
When a staff site user accesses a patron record, the ID, name
and card number of the patron will be saved in the cookie.

To test:
1- Apply the patch
2- Go to a patron details' page
4- Go to another page
4- Verify that the link "Last patron" appears on the top-right
   corner of the screen
5- Click on the link
6- Verify that you are at the patron's checkout page
7- Click on the "X" button next to the "Last patron" link
8- The link should be gone
9- Go to another page
10- The link shouldn't appear (until the next patron detail's
    page)

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 28 Martin Renvoize 2018-10-31 13:58:27 UTC
Created attachment 81727 [details] [review]
Bug 20312: (QA follow-up) Make display syspref driven

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 29 Martin Renvoize 2018-10-31 13:59:00 UTC
Quick rescue job done.
Comment 30 Martin Renvoize 2018-10-31 14:02:01 UTC
*** Bug 21244 has been marked as a duplicate of this bug. ***
Comment 31 Martin Renvoize 2018-10-31 14:22:47 UTC
Created attachment 81728 [details] [review]
Bug 20312: (QA follow-up) Make display syspref driven

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 32 Martin Renvoize 2018-10-31 14:23:43 UTC
Just reverse my last patch so the feature default to 'off' for now.. I want to get the refinements in before it's widely used ;)
Comment 33 Nick Clemens 2018-11-08 18:21:49 UTC
Awesome work all!

Pushed to master for 18.11
Comment 34 Martin Renvoize 2018-11-16 13:54:00 UTC
Enhancement will not be backported.