The syspref hidepatronname is now hiding the guarantor name on memberentry.pl. This seems like a strange place to enforce that system preference since it's convenient for staff to be able to double check that info when editing patron information and personal information is already showing on that screen. It's not enforced on other patron details screens (such as moremember.pl) and I don't think it should impact patron edit screen either.
Created attachment 133542 [details] [review] Bug 29768: Do not hide guarantor name from memberentry.pl Syspref "hidepatronname" currently hides guarantors name on memberentry.pl. This is inconsistent since name isn't hidden e.g. on moremember.pl and staff can already see patrons other personal information during edit. To test: 1) Disable syspref "hidepatronname" 2) Edit guarantee patrons information => Field "Guarantor" only shows guarantors cardnumber (or is empty if guarantor has no cardnumber) 3) Apply patch => Guarantors name is now displayed Sponsored-by: Koha-Suomi Oy
Regression from bug 25879
The patch is certainly wanted, however it has a bug, it removes the hyperlink to the guarantor info. I'd say it should be fixed in intranet-tmpl/prog/en/includes/patron-title.inc The hyperlink should be added regardless of the hide_patron_infos_if_needed value. Also, please remove the hide_patron_infos_if_needed=0 parameter completely, since that's the default default.
Changed the version to master as the bug happens with the latest version as well.
Created attachment 133725 [details] [review] Bug 29768: Do not hide guarantor name from memberentry.pl Syspref "hidepatronname" currently hides guarantors name on memberentry.pl. This is inconsistent since name isn't hidden e.g. on moremember.pl and staff can already see patrons other personal information during edit. To test: 1) Disable syspref "hidepatronname" 2) Edit guarantee patrons information => Field "Guarantor" only shows guarantors cardnumber (or is empty if guarantor has no cardnumber) 3) Apply patch => Guarantors name is now displayed (as a hyperlink) Also confirm hidepatronname still works as expected on other pages (holds table, items detail page etc.). Sponsored-by: Koha-Suomi Oy
Should this patch be back to needs signoff?
(In reply to Jonathan Druart from comment #6) > Should this patch be back to needs signoff? Yes it should :D
The bug fix to patron-title.inc misses the closing tag for the link "</a>", thus it produces now invalid HTML. It would make more sense to replace > [%- IF display_patron_name OR display_cardnumber -%] with > [%- IF can_see_patron_infos -%] also, that would then allow to get rid of the following block above it: > [%- IF hide_patron_infos_if_needed %] [%# Should only be set if patron is set -%] > [%- SET can_see_patron_infos = logged_in_user.can_see_patron_infos( patron ) -%] > [%- UNLESS can_see_patron_infos -%] > [%- SET display_patron_name = 0 -%] > [%- SET display_cardnumber = 0 -%] > [%- ELSIF Koha.Preference('HidePatronName') -%] > [%- SET display_patron_name = 0 -%] > [%- END -%] > [%- END -%] Then similar bugs to miss a closing HTML tag would be avoided as everything would be under one if block. One more thing: could you please provide the bug fix in patron-title.inc in a separate commit so if it is needed to fix some other bug and/or we need to revert the fix you are doing here it would be possible.
(In reply to Joonas Kylmälä from comment #8) > The bug fix to patron-title.inc misses the closing tag for the link "</a>", > thus it produces now invalid HTML. > > It would make more sense to replace > > > [%- IF display_patron_name OR display_cardnumber -%] > > with > > > [%- IF can_see_patron_infos -%] > > also, that would then allow to get rid of the following block above it: > > > [%- IF hide_patron_infos_if_needed %] [%# Should only be set if patron is set -%] > > [%- SET can_see_patron_infos = logged_in_user.can_see_patron_infos( patron ) -%] > > [%- UNLESS can_see_patron_infos -%] > > [%- SET display_patron_name = 0 -%] > > [%- SET display_cardnumber = 0 -%] > > [%- ELSIF Koha.Preference('HidePatronName') -%] > > [%- SET display_patron_name = 0 -%] > > [%- END -%] > > [%- END -%] > > Then similar bugs to miss a closing HTML tag would be avoided as everything > would be under one if block. > > One more thing: could you please provide the bug fix in patron-title.inc in > a separate commit so if it is needed to fix some other bug and/or we need to > revert the fix you are doing here it would be possible. Weird, patch worked fine even when closing tag was behind > [%- IF hide_patron_infos_if_needed AND ( display_patron_name OR display_cardnumber ) -%]. And replacing > [%- IF display_patron_name OR display_cardnumber -%] with > [%- IF can_see_patron_infos -%] broke it :D so I don't think that's the best approache to this.
Created attachment 134806 [details] [review] Bug 29768: Do not hide guarantor name from memberentry.pl Syspref "hidepatronname" currently hides guarantors name on memberentry.pl. This is inconsistent since name isn't hidden e.g. on moremember.pl and staff can already see patrons other personal information during edit. To test: 1) Disable syspref "hidepatronname" 2) Edit guarantee patrons information => Field "Guarantor" only shows guarantors cardnumber (or is empty if guarantor has no cardnumber) 3) Apply patch => Guarantors name is now displayed (as a hyperlink) Also confirm hidepatronname still works as expected on other pages (holds table, items detail page etc.). Sponsored-by: Koha-Suomi Oy
Created attachment 134807 [details] [review] Bug 29768: Fix hyperlink to the guarantor detail page We should display hyperlink to the guarantor detail page regardless of the hide_patron_infos_if_needed value. To test confirm hyperlink in gurarantee edit page to the guarantor detail page works. Sponsored-by: Koha-Suomi Oy
The last patch would break a lot of pages, e.g. see the browser title field after applying the patch. I fix would be to go for now how other pages have handled this: by grepping "patron-title.inc" you can find for examples: > ./prog/en/modules/members/accountline-details.tt: <a href="/cgi-bin/koha/members/boraccount.pl?borrowernumber=[% patron.borrowernumber | uri %]">Account for [% INCLUDE 'patron-title.inc' %]</a> so basically add the link manually around the patron-title.inc include statement. However the ideal solution would be to: 1. Modify the last patch from here to support in addition no_html = 1 parameter correctly in patron-title.inc (the current support is only partly done...) 2. add the no_html = 1 tag to all the places where patron-title.inc is used and it cannot contain a link (for example in a browser title tag or inside another link) If you don't decide to go with the ideal solution then let's open another bug report for that.
err.. (In reply to Joonas Kylmälä from comment #12) > after applying the patch. I fix would be to go for now how other pages have I fix -> One fix
(In reply to Joonas Kylmälä from comment #12) > 1. Modify the last patch from here to support in addition no_html = 1 > parameter correctly in patron-title.inc (the current support is only partly > done...) Hmm, so should there be something like this: > [%- IF !no_html AND ( display_patron_name OR display_cardnumber ) -%]
Pyh.. I meant: > > [%- IF no_html AND ( display_patron_name OR display_cardnumber ) -%] not !no_hmtl..
(In reply to Emmi Takkinen from comment #14) > (In reply to Joonas Kylmälä from comment #12) > > > 1. Modify the last patch from here to support in addition no_html = 1 > > parameter correctly in patron-title.inc (the current support is only partly > > done...) > > Hmm, so should there be something like this: > > > [%- IF !no_html AND ( display_patron_name OR display_cardnumber ) -%] Yeah, and I think you got the if statement here right. IF no_html==0 basically, I don't quite remember the correct syntax for template toolkit.
Created attachment 139180 [details] [review] Bug 29768: Do not hide guarantor name from memberentry.pl Syspref "hidepatronname" currently hides guarantors name on memberentry.pl. This is inconsistent since name isn't hidden e.g. on moremember.pl and staff can already see patrons other personal information during edit. To test: 1) Disable syspref "hidepatronname" 2) Edit guarantee patrons information => Field "Guarantor" only shows guarantors cardnumber (or is empty if guarantor has no cardnumber) 3) Apply patch => Guarantors name is now displayed (as a hyperlink) Also confirm hidepatronname still works as expected on other pages (holds table, items detail page etc.). Sponsored-by: Koha-Suomi Oy
Created attachment 139181 [details] [review] Bug 29768: Fix patron name displays and hyperlinks Sponsored-by: Koha-Suomi Oy
The hyperlink is not shown in the latest patch version, the hide_patron_infos_if_needed check was reintroduced in this version of the patch, it was the one you removed earlier. Also, I'm thinking whether introducing a "no_link" variable or equivalent in addition to the "no_html" would be a good idea? Reading your last patch and patron-title.inc in more detail the patron's title (Mr, Dr, etc.) seems to be enclosed in a span, which would be valid inside hyperlink, however a hyperlink wouldn't be valid inside a hyperlink.
Created attachment 142606 [details] [review] Bug 29768: Do not hide guarantor name from memberentry.pl Syspref "hidepatronname" currently hides guarantors name on memberentry.pl. This is inconsistent since name isn't hidden e.g. on moremember.pl and staff can already see patrons other personal information during edit. To test: 1) Disable syspref "hidepatronname" 2) Edit guarantee patrons information => Field "Guarantor" only shows guarantors cardnumber (or is empty if guarantor has no cardnumber) 3) Apply patch => Guarantors name is now displayed (as a hyperlink) Also confirm hidepatronname still works as expected on other pages (holds table, items detail page etc.). Sponsored-by: Koha-Suomi Oy
Created attachment 142607 [details] [review] Bug 29768: Fix patron name displays and hyperlinks Sponsored-by: Koha-Suomi Oy
(In reply to Joonas Kylmälä from comment #19) > The hyperlink is not shown in the latest patch version, the > hide_patron_infos_if_needed check was reintroduced in this version of the > patch, it was the one you removed earlier. So it seems (how on earth it got there?), removed it again in latest patch. > Also, I'm thinking whether > introducing a "no_link" variable or equivalent in addition to the "no_html" > would be a good idea? Reading your last patch and patron-title.inc in more > detail the patron's title (Mr, Dr, etc.) seems to be enclosed in a span, > which would be valid inside hyperlink, however a hyperlink wouldn't be valid > inside a hyperlink. Hmm, I guess this could be useful. Feel free to add it if you have some sort idea how to implement it.
Looks like second patch is on a bit different and complex issue. Maybe this report can deal with guarantor name only and create a new bug report for second patch, with a test plan.
I've looked at code and tested. This causes a problem when syspref "HidePatronName" is "Show" and logged in staff user does not have permission "view_borrower_infos_from_any_libraries". When guarantor is from another library, it is displayed where it should not.* When syspref "HidePatronName" is "Don't show" only the cardnumber is shown which is odd. I think IF/ELSIF confidations should be revamped in patron-title.inc
I can't reproduce the problem. When I follow the steps, in step 2, I already get a hyperlink for the guarantor when I add it. The guarantor name is displayed (as a hyperlink) as desired in step 3. I did not need to apply the patch.
Created attachment 145220 [details] [review] Bug 29768: Fix patron name displays and hyperlinks Sponsored-by: Koha-Suomi Oy Signed-off-by: Solene Ngamga <solene.ngamga@inLibro.com>
Hmm, I'm bit confused here. Is status of this "Failed QA" or "Patch doesn't apply" or even "RESOLVED INVALID"?
(In reply to solene.ngamga from comment #25) > I can't reproduce the problem. > > When I follow the steps, in step 2, I already get a hyperlink for the > guarantor when I add it. The guarantor name is displayed (as a hyperlink) as > desired in step 3. I did not need to apply the patch. It does seem that second patch is mostly useless. Only part still valid is changes made to patron-title.inc.
Looking this again and trying to figure out a simpler solution to original problem.
As far as I can tell there isn't any simpler way to fix this. Althought, this could be probably also fixed with this line: [%- IF ( !no_html OR hide_patron_infos_if_needed ) AND ( display_patron_name OR display_cardnumber ) -%]