Bug 29768 - hidepatronname hides guarantor name on borrower edit screen
Summary: hidepatronname hides guarantor name on borrower edit screen
Status: ASSIGNED
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal
Assignee: Emmi Takkinen
QA Contact: Testopia
URL:
Keywords:
Depends on: 25879
Blocks:
  Show dependency treegraph
 
Reported: 2021-12-23 14:28 UTC by Benjamin Daeuber
Modified: 2024-05-02 09:13 UTC (History)
8 users (show)

See Also:
GIT URL:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
Circulation function:


Attachments
Bug 29768: Do not hide guarantor name from memberentry.pl (2.96 KB, patch)
2022-04-21 11:57 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 29768: Do not hide guarantor name from memberentry.pl (4.01 KB, patch)
2022-04-25 07:05 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 29768: Do not hide guarantor name from memberentry.pl (3.02 KB, patch)
2022-05-10 09:39 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 29768: Fix hyperlink to the guarantor detail page (1.78 KB, patch)
2022-05-10 09:40 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 29768: Do not hide guarantor name from memberentry.pl (3.04 KB, patch)
2022-08-16 12:04 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 29768: Fix patron name displays and hyperlinks (26.50 KB, patch)
2022-08-16 12:05 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 29768: Do not hide guarantor name from memberentry.pl (3.04 KB, patch)
2022-10-25 10:29 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 29768: Fix patron name displays and hyperlinks (27.70 KB, patch)
2022-10-25 10:29 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 29768: Fix patron name displays and hyperlinks (27.75 KB, patch)
2023-01-11 20:24 UTC, solene.ngamga
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Benjamin Daeuber 2021-12-23 14:28:28 UTC
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.
Comment 1 Emmi Takkinen 2022-04-21 11:57:50 UTC
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
Comment 2 Joonas Kylmälä 2022-04-23 17:25:47 UTC
Regression from bug 25879
Comment 3 Joonas Kylmälä 2022-04-23 17:43:31 UTC
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.
Comment 4 Joonas Kylmälä 2022-04-23 17:44:56 UTC
Changed the version to master as the bug happens with the latest version as well.
Comment 5 Emmi Takkinen 2022-04-25 07:05:42 UTC
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
Comment 6 Jonathan Druart 2022-05-02 09:37:30 UTC
Should this patch be back to needs signoff?
Comment 7 Emmi Takkinen 2022-05-02 09:38:08 UTC
(In reply to Jonathan Druart from comment #6)
> Should this patch be back to needs signoff?

Yes it should :D
Comment 8 Joonas Kylmälä 2022-05-04 16:15:47 UTC
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.
Comment 9 Emmi Takkinen 2022-05-06 12:36:17 UTC
(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.
Comment 10 Emmi Takkinen 2022-05-10 09:39:53 UTC
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
Comment 11 Emmi Takkinen 2022-05-10 09:40:18 UTC
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
Comment 12 Joonas Kylmälä 2022-05-10 16:58:31 UTC
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.
Comment 13 Joonas Kylmälä 2022-05-10 17:01:49 UTC
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
Comment 14 Emmi Takkinen 2022-05-11 08:37:33 UTC
(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 ) -%]
Comment 15 Emmi Takkinen 2022-05-12 09:38:35 UTC
Pyh.. I meant:

> > [%- IF no_html AND ( display_patron_name OR display_cardnumber ) -%]

not !no_hmtl..
Comment 16 Joonas Kylmälä 2022-05-16 17:27:28 UTC
(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.
Comment 17 Emmi Takkinen 2022-08-16 12:04:56 UTC
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
Comment 18 Emmi Takkinen 2022-08-16 12:05:23 UTC
Created attachment 139181 [details] [review]
Bug 29768: Fix patron name displays and hyperlinks

Sponsored-by: Koha-Suomi Oy
Comment 19 Joonas Kylmälä 2022-08-21 08:35:19 UTC
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.
Comment 20 Emmi Takkinen 2022-10-25 10:29:05 UTC
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
Comment 21 Emmi Takkinen 2022-10-25 10:29:41 UTC
Created attachment 142607 [details] [review]
Bug 29768: Fix patron name displays and hyperlinks

Sponsored-by: Koha-Suomi Oy
Comment 22 Emmi Takkinen 2022-10-25 12:08:23 UTC
(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.
Comment 23 Fridolin Somers 2022-12-22 07:30:28 UTC
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.
Comment 24 Fridolin Somers 2022-12-22 08:06:00 UTC
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
Comment 25 solene.ngamga 2023-01-10 15:38:24 UTC
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.
Comment 26 solene.ngamga 2023-01-11 20:24:57 UTC
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>
Comment 27 Emmi Takkinen 2023-06-19 09:04:38 UTC
Hmm, I'm bit confused here. Is status of this "Failed QA" or "Patch doesn't apply" or even "RESOLVED INVALID"?
Comment 28 Emmi Takkinen 2023-11-15 08:47:23 UTC
(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.
Comment 29 Emmi Takkinen 2024-04-26 10:49:57 UTC
Looking this again and trying to figure out a simpler solution to original problem.
Comment 30 Emmi Takkinen 2024-05-02 09:13:33 UTC
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 ) -%]