Bug 22087

Summary: Show city and state in patron search results
Product: Koha Reporter: Jessie Zairo <jzairo>
Component: PatronsAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: david.roberts, gmcharlt, jonathan.druart, katrin.fischer, kyle.m.hall, lucas
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
20.11.00
Bug Depends on: 25070    
Bug Blocks:    
Attachments: Bug 22087: It would be helpful to have City and State visible in member.pl
Bug 22087: It would be helpful to have City and State visible in member.pl
Bug 22087: Use include file to display patrons address in search result
Bug 22087: Add space between zipcode and country
Bug 22087: Add a no_line_break parameter to prevent new lines
Bug 22087: DO NOT PUSH - CSS changes
Mangled display of patrons
Bug 22087: Use include file to display patrons address in search result
Bug 22087: Add space between zipcode and country
Bug 22087: Add a no_line_break parameter to prevent new lines
Bug 22087: DO NOT PUSH - CSS changes
Screenshot
Bug 22087: Prevent variable names colision
Bug 22087: Remove padding on li's
Bug 22087: Move the email into the list
Bug 22087: DO NOT PUSH - CSS changes
Bug 22087: Use include file to display patrons address in search result
Bug 22087: Add space between zipcode and country
Bug 22087: Add a no_line_break parameter to prevent new lines
Bug 22087: Prevent variable names colision
Bug 22087: Remove padding on li's
Bug 22087: Move the email into the list
Bug 22087: Add missing filters
Bug 22087: Display address on different line
Bug 22087: Use include file to display patrons address in search result
Bug 22087: Add space between zipcode and country
Bug 22087: Add a no_line_break parameter to prevent new lines
Bug 22087: Prevent variable names colision
Bug 22087: Remove padding on li's
Bug 22087: Move the email into the list
Bug 22087: Add missing filters

Description Jessie Zairo 2019-01-09 18:26:16 UTC
When a staff member searches for a patron via the search patron box or filters a search through the patron module, they get results on members-home.pl or member.pl. The column for name includes firstname, surname, address and email. It would be beneficial to also see the city and state.
Comment 1 David Roberts 2020-03-29 00:56:07 UTC
Created attachment 102043 [details] [review]
Bug 22087: It would be helpful to have City and State visible in member.pl

This patch adds City, State and Zipcode to the display of a patron
search

To test:

1) Do a patron search that retrieves more than one user so you see the
results table
2) In the Names column, see that the patron address display without
city, state and zipcode.
3) Install the patch
4) Repeat the search. City, state and zipcode will now display in the
table.
5) Check that this is working from both member.pl and members-home.pl
Comment 2 David Roberts 2020-03-29 00:57:10 UTC
I've also added zipcode to this display, as there seemed little point in not displaying it if we are displaying all of the other address fields.
Comment 3 David Nind 2020-03-29 19:46:19 UTC
Created attachment 102064 [details] [review]
Bug 22087: It would be helpful to have City and State visible in member.pl

This patch adds City, State and Zipcode to the display of a patron
search

To test:

1) Do a patron search that retrieves more than one user so you see the
results table
2) In the Names column, see that the patron address display without
city, state and zipcode.
3) Install the patch
4) Repeat the search. City, state and zipcode will now display in the
table.
5) Check that this is working from both member.pl and members-home.pl

Signed-off-by: David Nind <david@davidnind.com>
Comment 4 Jonathan Druart 2020-03-31 11:21:37 UTC
I think we should follow the same pattern we have already (in circ-menu.inc) and use the member-display-address-style-* include files instead, to match the AddressFormat syspref's value.
Comment 5 David Roberts 2020-04-03 12:45:15 UTC
I think I'm going to have to unassign this one. I'm competent to extend the functionality that is already in use, but not to change it completely, especially on the basis of a rather cryptic suggestion. :(
Comment 6 Jonathan Druart 2020-04-06 14:34:34 UTC
Created attachment 102448 [details] [review]
Bug 22087: Use include file to display patrons address in search result

In order to display the city and country in the patron search result, we
need to use the member-display-address-style include file, to satisfy
AddressFormat.

Test plan:
Modify some patrons and fill the different address info
Search for them and notice that the city, zipcode and country are now
displayed.
Modify the value of AddressFormat and confirm that the display is
modified according to its value
Comment 7 Jonathan Druart 2020-04-06 14:34:38 UTC
Created attachment 102449 [details] [review]
Bug 22087: Add space between zipcode and country
Comment 8 Jonathan Druart 2020-04-06 14:34:42 UTC
Created attachment 102450 [details] [review]
Bug 22087: Add a no_line_break parameter to prevent new lines
Comment 9 Jonathan Druart 2020-04-06 14:34:45 UTC
Created attachment 102451 [details] [review]
Bug 22087: DO NOT PUSH - CSS changes
Comment 10 Jonathan Druart 2020-04-06 14:35:26 UTC
Hello David, could you test these patches?
Comment 11 David Roberts 2020-04-06 19:26:09 UTC
Created attachment 102466 [details]
Mangled display of patrons
Comment 12 David Roberts 2020-04-06 19:26:38 UTC
Hi, I've tested but the search results screen is mangled. Firstly, there are no spaces between the address elements. This is also true when you display a single patron. Secondly, my search returns two users, but the usernames of each have got confused (see screenshot). Also, when I go to edit one of the users, I get an error:

Software error:
Template process failed: file error - member-alt-contact-style-us: not found at /home/david/kohaclone/C4/Templates.pm line 122.
For help, please send mail to the webmaster (webmaster@ptfs-europe.co.uk), giving this error message and the time and date of the error.
Comment 13 Jonathan Druart 2020-04-07 08:53:02 UTC
Created attachment 102482 [details] [review]
Bug 22087: Use include file to display patrons address in search result

In order to display the city and country in the patron search result, we
need to use the member-display-address-style include file, to satisfy
AddressFormat.

Test plan:
Modify some patrons and fill the different address info
Search for them and notice that the city, zipcode and country are now
displayed.
Modify the value of AddressFormat and confirm that the display is
modified according to its value
Comment 14 Jonathan Druart 2020-04-07 08:53:06 UTC
Created attachment 102483 [details] [review]
Bug 22087: Add space between zipcode and country
Comment 15 Jonathan Druart 2020-04-07 08:53:09 UTC
Created attachment 102484 [details] [review]
Bug 22087: Add a no_line_break parameter to prevent new lines
Comment 16 Jonathan Druart 2020-04-07 08:53:13 UTC
Created attachment 102485 [details] [review]
Bug 22087: DO NOT PUSH - CSS changes
Comment 17 Jonathan Druart 2020-04-07 08:55:14 UTC
(In reply to David Roberts from comment #12)
> Hi, I've tested but the search results screen is mangled. Firstly, there are
> no spaces between the address elements. This is also true when you display a
> single patron. Secondly, my search returns two users, but the usernames of
> each have got confused (see screenshot). Also, when I go to edit one of the
> users, I get an error:
> 
> Software error:
> Template process failed: file error - member-alt-contact-style-us: not found
> at /home/david/kohaclone/C4/Templates.pm line 122.
> For help, please send mail to the webmaster (webmaster@ptfs-europe.co.uk),
> giving this error message and the time and date of the error.

I fixed the software error (on bug 25070), and the space problems. I got confused by the sample data: the street number is in the address field.

However I do not understand your second point, what is confusing with the usernames (ie. what are you expecting to be displayed instead)?
Comment 18 David Roberts 2020-04-07 08:58:28 UTC
In my example, there are two users. David Roberts (dir) and Glyn Roberts (gdr). If you look at my screenshot, both users display the name David Roberts, and the second user is showing the cardnumber dir in brackets whereas it should be gdr
Comment 19 David Roberts 2020-04-07 09:47:09 UTC
I've re-tested, and the address format looks much better now. There are still problems with the username. I've added a 3rd user to my system now, and it seems like each user in the list displays the name of the previous user. See screenshot.
Comment 20 David Roberts 2020-04-07 09:47:45 UTC
Created attachment 102486 [details]
Screenshot
Comment 21 Jonathan Druart 2020-04-07 10:04:40 UTC
Created attachment 102492 [details] [review]
Bug 22087: Prevent variable names colision

We should not use PROCESS:
http://www.template-toolkit.org/docs/manual/Directives.html

" The PROCESS directive is slightly faster than INCLUDE because it
avoids the need to localise (i.e. copy) the variable stash before
processing the template. "

The 'patron' variable is reused by patron-title.inc
Comment 22 Jonathan Druart 2020-04-07 10:05:53 UTC
(In reply to David Roberts from comment #19)
> I've re-tested, and the address format looks much better now. There are
> still problems with the username. I've added a 3rd user to my system now,
> and it seems like each user in the list displays the name of the previous
> user. See screenshot.

Right, good catch! That was a very unexpected behaviour, it is fixed now.

The address parts do not look correct on your screenshot, you should not see the bullets.
Refresh correctly your browser cache to get the latest css version.

https://snipboard.io/sALU86.jpg
Comment 23 David Roberts 2020-04-07 10:15:22 UTC
Looks much better, although I quite liked the bullets! However, the email address is out of alignment now:

https://snipboard.io/pX2kli.jpg
Comment 24 Jonathan Druart 2020-04-07 10:41:35 UTC
Created attachment 102505 [details] [review]
Bug 22087: Remove padding on li's
Comment 25 Jonathan Druart 2020-04-07 10:41:39 UTC
Created attachment 102506 [details] [review]
Bug 22087: Move the email into the list
Comment 26 Jonathan Druart 2020-04-07 10:41:44 UTC
Created attachment 102507 [details] [review]
Bug 22087: DO NOT PUSH - CSS changes
Comment 27 Jonathan Druart 2020-04-07 10:43:32 UTC
(In reply to David Roberts from comment #23)
> Looks much better, although I quite liked the bullets!

I wanted to keep the view as compact as before. We could see later how to improve the display.

> However, the email address is out of alignment now:

Fixed.
Comment 28 David Roberts 2020-04-07 10:47:05 UTC
Looks great. Happy to sign off.
Comment 29 Katrin Fischer 2020-06-11 12:23:31 UTC
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/css/staff-global.css
Comment 30 Owen Leonard 2020-06-11 13:20:49 UTC
(In reply to Katrin Fischer from comment #29)
> CONFLICT (content): Merge conflict in
> koha-tmpl/intranet-tmpl/prog/css/staff-global.css

I obsoleted the patch containing compiled CSS. Instead a "yarn build" step should be added to the test plan.
Comment 31 Katrin Fischer 2020-06-11 13:23:47 UTC
Thx, I was a little too fast here as I had to also take care of the dependency bug.
Comment 32 Katrin Fischer 2020-07-04 21:35:01 UTC
1) 
 FAIL	koha-tmpl/intranet-tmpl/prog/en/includes/member-display-address-style.inc
   FAIL	  filters
		missing_filter at line 48 (            [%~ IF ( patron.zipcode ) ~%][%~ patron.zipcode | html %] [% END ~%][%~ patron.city | html ~%][%~ IF ( patron.state ) ~%][% line_break %][%~ patron.state | html ~%][%~ END ~%])
		missing_filter at line 49 (            [%~ IF ( patron.country ) ~%][% line_break %][%~ patron.country | html ~%][%~ END ~%])
		missing_filter at line 72 (            [%~ IF ( patron.zipcode ) ~%][%~ patron.zipcode | html %] [% END ~%][%~ patron.city | html ~%][%~ IF ( patron.state ) ~%][% line_break %][%~ patron.state | html ~%][%~ END ~%])
		missing_filter at line 73 (            [%~ IF ( patron.country ) ~%][% line_break %][%~ patron.country | html ~%][%~ END ~%])

2) 
I'd also like to suggest a visual change. At the moment the information is shown as a list, but I think as this is address information just a line break would work better. It will allow to just easily copy the addresses for use in a letter head or similar.
Comment 33 Jonathan Druart 2020-07-27 13:54:29 UTC
Created attachment 107420 [details] [review]
Bug 22087: Use include file to display patrons address in search result

In order to display the city and country in the patron search result, we
need to use the member-display-address-style include file, to satisfy
AddressFormat.

Test plan:
Modify some patrons and fill the different address info
Search for them and notice that the city, zipcode and country are now
displayed.
Modify the value of AddressFormat and confirm that the display is
modified according to its value
Comment 34 Jonathan Druart 2020-07-27 13:54:32 UTC
Created attachment 107421 [details] [review]
Bug 22087: Add space between zipcode and country
Comment 35 Jonathan Druart 2020-07-27 13:54:37 UTC
Created attachment 107422 [details] [review]
Bug 22087: Add a no_line_break parameter to prevent new lines
Comment 36 Jonathan Druart 2020-07-27 13:54:41 UTC
Created attachment 107423 [details] [review]
Bug 22087: Prevent variable names colision

We should not use PROCESS:
http://www.template-toolkit.org/docs/manual/Directives.html

" The PROCESS directive is slightly faster than INCLUDE because it
avoids the need to localise (i.e. copy) the variable stash before
processing the template. "

The 'patron' variable is reused by patron-title.inc
Comment 37 Jonathan Druart 2020-07-27 13:54:45 UTC
Created attachment 107424 [details] [review]
Bug 22087: Remove padding on li's
Comment 38 Jonathan Druart 2020-07-27 13:54:49 UTC
Created attachment 107425 [details] [review]
Bug 22087: Move the email into the list
Comment 39 Jonathan Druart 2020-07-27 13:54:53 UTC
Created attachment 107427 [details] [review]
Bug 22087: Add missing filters
Comment 40 Jonathan Druart 2020-07-27 13:54:57 UTC
Created attachment 107428 [details] [review]
Bug 22087: Display address on different line
Comment 41 Jonathan Druart 2020-07-27 13:55:34 UTC
(In reply to Katrin Fischer from comment #32)
> 1) 
>  FAIL
> koha-tmpl/intranet-tmpl/prog/en/includes/member-display-address-style.inc
>    FAIL	  filters
> 		missing_filter at line 48 (            [%~ IF ( patron.zipcode ) ~%][%~
> patron.zipcode | html %] [% END ~%][%~ patron.city | html ~%][%~ IF (
> patron.state ) ~%][% line_break %][%~ patron.state | html ~%][%~ END ~%])
> 		missing_filter at line 49 (            [%~ IF ( patron.country ) ~%][%
> line_break %][%~ patron.country | html ~%][%~ END ~%])
> 		missing_filter at line 72 (            [%~ IF ( patron.zipcode ) ~%][%~
> patron.zipcode | html %] [% END ~%][%~ patron.city | html ~%][%~ IF (
> patron.state ) ~%][% line_break %][%~ patron.state | html ~%][%~ END ~%])
> 		missing_filter at line 73 (            [%~ IF ( patron.country ) ~%][%
> line_break %][%~ patron.country | html ~%][%~ END ~%])

Done.

> 2) 
> I'd also like to suggest a visual change. At the moment the information is
> shown as a list, but I think as this is address information just a line
> break would work better. It will allow to just easily copy the addresses for
> use in a letter head or similar.

As I said in comment 27, "I wanted to keep the view as compact as before"

I added a follow-up but I don't think it's the way to go.
Comment 42 Jonathan Druart 2020-07-27 13:56:46 UTC
Display is now: https://snipboard.io/MBu8cI.jpg
Comment 43 Jonathan Druart 2020-07-28 07:03:41 UTC
vs without the last patch: https://snipboard.io/qEsVt4.jpg
Comment 44 Katrin Fischer 2020-07-29 22:12:11 UTC
Created attachment 107543 [details] [review]
Bug 22087: Use include file to display patrons address in search result

In order to display the city and country in the patron search result, we
need to use the member-display-address-style include file, to satisfy
AddressFormat.

Test plan:
Modify some patrons and fill the different address info
Search for them and notice that the city, zipcode and country are now
displayed.
Modify the value of AddressFormat and confirm that the display is
modified according to its value

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 45 Katrin Fischer 2020-07-29 22:12:15 UTC
Created attachment 107544 [details] [review]
Bug 22087: Add space between zipcode and country

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 46 Katrin Fischer 2020-07-29 22:12:20 UTC
Created attachment 107545 [details] [review]
Bug 22087: Add a no_line_break parameter to prevent new lines

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 47 Katrin Fischer 2020-07-29 22:12:24 UTC
Created attachment 107546 [details] [review]
Bug 22087: Prevent variable names colision

We should not use PROCESS:
http://www.template-toolkit.org/docs/manual/Directives.html

" The PROCESS directive is slightly faster than INCLUDE because it
avoids the need to localise (i.e. copy) the variable stash before
processing the template. "

The 'patron' variable is reused by patron-title.inc

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 48 Katrin Fischer 2020-07-29 22:12:29 UTC
Created attachment 107547 [details] [review]
Bug 22087: Remove padding on li's

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 49 Katrin Fischer 2020-07-29 22:12:33 UTC
Created attachment 107548 [details] [review]
Bug 22087: Move the email into the list

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 50 Katrin Fischer 2020-07-29 22:12:37 UTC
Created attachment 107549 [details] [review]
Bug 22087: Add missing filters

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 51 Katrin Fischer 2020-07-29 22:13:13 UTC
Hi Jonathan, you were right - the last patch was not needed. I had forgotten to do a yarn build before when testing :(
Comment 52 Jonathan Druart 2020-07-30 15:46:27 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 53 Lucas Gass 2020-08-06 20:18:08 UTC
skipping enhancement for 20.05.x, no backport