Bug 25765 - Replace LoginBranchname and LoginBranchcode with use of Branches template plugin
Summary: Replace LoginBranchname and LoginBranchcode with use of Branches template plugin
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Templates (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Fridolin SOMERS
QA Contact: Martin Renvoize
URL:
Keywords:
Depends on: 21746
Blocks:
  Show dependency treegraph
 
Reported: 2020-06-16 07:45 UTC by Fridolin SOMERS
Modified: 2020-08-03 23:37 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
20.11.00, 20.05.03


Attachments
Bug 25765: Add GetLoggedInBranchname method (3.23 KB, patch)
2020-06-16 12:07 UTC, Fridolin SOMERS
Details | Diff | Splinter Review
Bug 25765: Replace LoginBranchname and LoginBranchcode with use of Branches template plugin (17.56 KB, patch)
2020-06-16 12:07 UTC, Fridolin SOMERS
Details | Diff | Splinter Review
Bug 25765: Add GetLoggedInBranchname method (3.28 KB, patch)
2020-06-27 23:00 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 25765: Replace LoginBranchname and LoginBranchcode with use of Branches template plugin (18.02 KB, patch)
2020-06-27 23:00 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 25765: Add GetLoggedInBranchname method (3.35 KB, patch)
2020-07-21 15:12 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25765: Replace LoginBranchname and LoginBranchcode with use of Branches template plugin (18.23 KB, patch)
2020-07-21 15:12 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25765: (QA follow-up) Save a DB hit (1.79 KB, patch)
2020-07-21 15:12 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25765: (QA follow-up) Use method directly in cleanborrowers (1.06 KB, patch)
2020-07-21 15:12 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Fridolin SOMERS 2020-06-16 07:45:34 UTC
The template plugin Branches contains a method GetLoggedInBranchcode that returns branch code of the current loggedin user.
This is a bit off scope of the plugin and can easily be replaced by logged_in_user variable (a Koha::Patron object used is most templates) : logged_in_user.library.branchcode
Comment 1 Fridolin SOMERS 2020-06-16 08:33:36 UTC
Oups mistake, GetLoggedInBranchcode is correct and depends on loggin choice.

It is var LoginBranchname I want to correct.
Comment 2 Fridolin SOMERS 2020-06-16 12:07:23 UTC
Created attachment 105909 [details] [review]
Bug 25765: Add GetLoggedInBranchname method

Add GetLoggedInBranchname method to Branches templates plugin.

Also changes GetName method to use Koha::Library instead of direct SQL.

Test plan :
Run prove t/db_dependent/Template/Plugin/Branches.t
Comment 3 Fridolin SOMERS 2020-06-16 12:07:41 UTC
Created attachment 105910 [details] [review]
Bug 25765: Replace LoginBranchname and LoginBranchcode with use of Branches template plugin

The template plugin Branches contains a method GetLoggedInBranchcode that returns current branch code.
This patch adds GetLoggedInBranchname to get current branch name.
It is used to replace vars LoginBranchname and LoginBranchcode sent to all templates in C4/Auth.pm.

In labels and patrons cards modules, I choose to remove a unseless display of
current branch in a hint.

In acqui/acqui-home.tt, I choose to remove a useless display of current
branch and also because table of founds contains a filter on library.

Test plan:
Check pages source code to see branch code or name is correct.
Comment 4 Victor Grousset/tuxayo 2020-06-27 23:00:50 UTC
Created attachment 106357 [details] [review]
Bug 25765: Add GetLoggedInBranchname method

Add GetLoggedInBranchname method to Branches templates plugin.

Also changes GetName method to use Koha::Library instead of direct SQL.

Test plan :
Run prove t/db_dependent/Template/Plugin/Branches.t

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 5 Victor Grousset/tuxayo 2020-06-27 23:00:53 UTC
Created attachment 106358 [details] [review]
Bug 25765: Replace LoginBranchname and LoginBranchcode with use of Branches template plugin

The template plugin Branches contains a method GetLoggedInBranchcode that returns current branch code.
This patch adds GetLoggedInBranchname to get current branch name.
It is used to replace vars LoginBranchname and LoginBranchcode sent to all templates in C4/Auth.pm.

In labels and patrons cards modules, I choose to remove a unseless display of
current branch in a hint.

In acqui/acqui-home.tt, I choose to remove a useless display of current
branch and also because table of founds contains a filter on library.

Test plan:
Check pages source code to see branch code or name is correct.
list of the pages:
/cgi-bin/koha/acqui/acqui-home.pl
/cgi-bin/koha/catalogue/detail.pl?biblionumber=XXX
/cgi-bin/koha/circ/branchoverdues.pl
/cgi-bin/koha/circ/set-library.pl
/cgi-bin/koha/circ/offline.pl
/cgi-bin/koha/labels/label-edit-batch.pl?op=new
/cgi-bin/koha/labels/label-manage.pl
/cgi-bin/koha/patroncards/edit-batch.pl
/cgi-bin/koha/patroncards/manage.pl
OPAC:
/cgi-bin/koha/opac-detail.pl?biblionumber=XXX

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 6 Victor Grousset/tuxayo 2020-06-27 23:06:37 UTC
It works!

Test was done by opening the 10 test pages and comparing the source code before and after the patch with a diff tool.

The only actually changes are the 4 pages without the hint. And acqui/acqui-home.tt as advertised.
Comment 7 Martin Renvoize 2020-07-21 14:35:27 UTC
Throwing this out there.. I'm really not a fan of the method names in some of our template plugins.. a mixture of lower case accessors and mixed case ones.. and I'm also not entirely sure we need as many as we impliment.

Why not add a 'current' method to just return the current branches Koha::Library object.. perhaps unblessed.. and then you can refer to the relevant field via dot notation.. for example?

[% Branches.current.name %]
[% Branches.current.code %]
Comment 8 Martin Renvoize 2020-07-21 14:51:22 UTC
Also.. this introduces an additional DB hit.. you could fetch the branchname from context instead.

I feel caching of the logged in library object is ripe for performance.
Comment 9 Martin Renvoize 2020-07-21 15:12:40 UTC
Created attachment 107154 [details] [review]
Bug 25765: Add GetLoggedInBranchname method

Add GetLoggedInBranchname method to Branches templates plugin.

Also changes GetName method to use Koha::Library instead of direct SQL.

Test plan :
Run prove t/db_dependent/Template/Plugin/Branches.t

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 10 Martin Renvoize 2020-07-21 15:12:44 UTC
Created attachment 107155 [details] [review]
Bug 25765: Replace LoginBranchname and LoginBranchcode with use of Branches template plugin

The template plugin Branches contains a method GetLoggedInBranchcode that returns current branch code.
This patch adds GetLoggedInBranchname to get current branch name.
It is used to replace vars LoginBranchname and LoginBranchcode sent to all templates in C4/Auth.pm.

In labels and patrons cards modules, I choose to remove a unseless display of
current branch in a hint.

In acqui/acqui-home.tt, I choose to remove a useless display of current
branch and also because table of founds contains a filter on library.

Test plan:
Check pages source code to see branch code or name is correct.
list of the pages:
/cgi-bin/koha/acqui/acqui-home.pl
/cgi-bin/koha/catalogue/detail.pl?biblionumber=XXX
/cgi-bin/koha/circ/branchoverdues.pl
/cgi-bin/koha/circ/set-library.pl
/cgi-bin/koha/circ/offline.pl
/cgi-bin/koha/labels/label-edit-batch.pl?op=new
/cgi-bin/koha/labels/label-manage.pl
/cgi-bin/koha/patroncards/edit-batch.pl
/cgi-bin/koha/patroncards/manage.pl
OPAC:
/cgi-bin/koha/opac-detail.pl?biblionumber=XXX

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 11 Martin Renvoize 2020-07-21 15:12:47 UTC
Created attachment 107156 [details] [review]
Bug 25765: (QA follow-up) Save a DB hit

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 12 Martin Renvoize 2020-07-21 15:12:50 UTC
Created attachment 107157 [details] [review]
Bug 25765: (QA follow-up) Use method directly in cleanborrowers

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 13 Martin Renvoize 2020-07-21 15:15:35 UTC
Leaving to the RM's discretion.

I've added a couple of followups, one to restore fetching branchname from the userenv and save us a DB hit (this will be used of pretty much every page the loads and as such could be costly performance wise to always re-fetch from the DB).

I still like the idea of 'Branches.current' returning the currently logged in branch object at some point, but I feel that's out of scope for here.

Passing QA
Comment 14 Fridolin SOMERS 2020-07-23 09:45:56 UTC
(In reply to Martin Renvoize from comment #8)
> Also.. this introduces an additional DB hit.. you could fetch the branchname
> from context instead.
> 
> I feel caching of the logged in library object is ripe for performance.

Ah yep but the primary cause of this bug report is that we face encoding issues on branch name when session storage is on tmp file.
Its quite random but i think the performance difference is really minor, most pages use hundreds of SQL requests.
When session is in DB there is still a query ^^.

We should better add branche name in Memcached no ?
Comment 15 Jonathan Druart 2020-07-23 09:49:24 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 16 Jonathan Druart 2020-07-23 10:07:12 UTC
(In reply to Fridolin SOMERS from comment #14)
> (In reply to Martin Renvoize from comment #8)
> > Also.. this introduces an additional DB hit.. you could fetch the branchname
> > from context instead.
> > 
> > I feel caching of the logged in library object is ripe for performance.
> 
> Ah yep but the primary cause of this bug report is that we face encoding
> issues on branch name when session storage is on tmp file.
> Its quite random but i think the performance difference is really minor,
> most pages use hundreds of SQL requests.
> When session is in DB there is still a query ^^.
> 
> We should better add branche name in Memcached no ?

Sorry I missed you comment before pushing it. Do you mean there is an encoding issue in the current version?
Comment 17 Fridolin SOMERS 2020-07-30 16:03:37 UTC
(In reply to Jonathan Druart from comment #16)
> 
> Sorry I missed you comment before pushing it. Do you mean there is an
> encoding issue in the current version?

Yep when using tmp session storage, go to system preferences page, it breaks encoding in library name.
Comment 18 Lucas Gass 2020-07-30 17:40:22 UTC
backported to 20.05.x for 20.05.03
Comment 19 Jonathan Druart 2020-07-31 07:42:11 UTC
(In reply to Fridolin SOMERS from comment #17)
> (In reply to Jonathan Druart from comment #16)
> > 
> > Sorry I missed you comment before pushing it. Do you mean there is an
> > encoding issue in the current version?
> 
> Yep when using tmp session storage, go to system preferences page, it breaks
> encoding in library name.

Can you please detail the different steps to recreate the issue, I don't manage to see what you describe.
Comment 20 Aleisha Amohia 2020-08-03 23:37:12 UTC
enhancement, not backported to 19.11.x