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
Oups mistake, GetLoggedInBranchcode is correct and depends on loggin choice. It is var LoginBranchname I want to correct.
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
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.
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>
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>
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.
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 %]
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.
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>
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>
Created attachment 107156 [details] [review] Bug 25765: (QA follow-up) Save a DB hit Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
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>
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
(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 ?
Pushed to master for 20.11, thanks to everybody involved!
(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?
(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.
backported to 20.05.x for 20.05.03
(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.
enhancement, not backported to 19.11.x