|
Description
Jessie Zairo
2019-01-09 18:26:16 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 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. 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> 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. 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. :( 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 Created attachment 102449 [details] [review] Bug 22087: Add space between zipcode and country Created attachment 102450 [details] [review] Bug 22087: Add a no_line_break parameter to prevent new lines Created attachment 102451 [details] [review] Bug 22087: DO NOT PUSH - CSS changes Hello David, could you test these patches? Created attachment 102466 [details]
Mangled display of patrons
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. 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 Created attachment 102483 [details] [review] Bug 22087: Add space between zipcode and country Created attachment 102484 [details] [review] Bug 22087: Add a no_line_break parameter to prevent new lines Created attachment 102485 [details] [review] Bug 22087: DO NOT PUSH - CSS changes (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)? 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 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. Created attachment 102486 [details]
Screenshot
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 (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 Looks much better, although I quite liked the bullets! However, the email address is out of alignment now: https://snipboard.io/pX2kli.jpg Created attachment 102505 [details] [review] Bug 22087: Remove padding on li's Created attachment 102506 [details] [review] Bug 22087: Move the email into the list Created attachment 102507 [details] [review] Bug 22087: DO NOT PUSH - CSS changes (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. Looks great. Happy to sign off. CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/css/staff-global.css (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. Thx, I was a little too fast here as I had to also take care of the dependency bug. 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. 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 Created attachment 107421 [details] [review] Bug 22087: Add space between zipcode and country Created attachment 107422 [details] [review] Bug 22087: Add a no_line_break parameter to prevent new lines 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 Created attachment 107424 [details] [review] Bug 22087: Remove padding on li's Created attachment 107425 [details] [review] Bug 22087: Move the email into the list Created attachment 107427 [details] [review] Bug 22087: Add missing filters Created attachment 107428 [details] [review] Bug 22087: Display address on different line (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. Display is now: https://snipboard.io/MBu8cI.jpg vs without the last patch: https://snipboard.io/qEsVt4.jpg 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> Created attachment 107544 [details] [review] Bug 22087: Add space between zipcode and country Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> 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> 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> Created attachment 107547 [details] [review] Bug 22087: Remove padding on li's Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 107548 [details] [review] Bug 22087: Move the email into the list Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 107549 [details] [review] Bug 22087: Add missing filters Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Hi Jonathan, you were right - the last patch was not needed. I had forgotten to do a yarn build before when testing :( Pushed to master for 20.11, thanks to everybody involved! skipping enhancement for 20.05.x, no backport |
