Bug 24201

Summary: Attach desk to intranet session
Product: Koha Reporter: Nicolas Legrand <nicolas.legrand>
Component: CirculationAssignee: Nicolas Legrand <nicolas.legrand>
Status: CLOSED FIXED QA Contact: Josef Moravec <josef.moravec>
Severity: enhancement    
Priority: P5 - low CC: f.demians, fridolin.somers, gmcharlt, jonathan.druart, josef.moravec, katrin.fischer, kyle.m.hall, lucas, martin.renvoize, oleonard, severine.queune
Version: master   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26037
Change sponsored?: --- Patch complexity: Medium patch
Documentation contact: Documentation submission:
Text to go in the release notes:
When `UseCirculationDesks` is enabled and desks are defined and attached to a library, this feature makes it possible to chose your current desk on login or when changing library from the intranet. The desk is then attached to the session. Future developments are planned to allow associating hold pickup locations with desks and other features.
Version(s) released in:
20.11.00
Bug Depends on: 13881    
Bug Blocks: 30123, 24412, 24786    
Attachments: Bug 24201: Attach desk to intranet session
Bug 24201: Attach desk to intranet session
Bug 24201: Attach desk to intranet session
Bug 24201: Attach desk to intranet session
Bug 24201: Attach desk to intranet session
Bug 24201: Attach desk to intranet session
Bug 24201: Attach desk to intranet session
Bug 24201: (follow-up) Show desk information at lower screen widths
Bug 24201: Attach desk to intranet session
Bug 24201: (follow-up) Show desk information at lower screen widths
Bug 24201: (follow-up) Use GetLoggedInDeskName
Bug 24201: (follow-up) Rework Desks Plugin
Bug 24201: (follow-up) Rework Desks Plugin
Bug 24201: (follow-up) Rework Desks Plugin
Bug 24201: (follow-up) Use GetLoggedInDeskName
Bug 24201: (follow-up) Rework Desks Plugin
Bug 24201: Attach desk to intranet session
Bug 24201: (follow-up) Show desk information at lower screen widths
Bug 24201: (follow-up) Use GetLoggedInDeskName
Bug 24201: (follow-up) Rework Desks Plugin
Bug 24201: (follow-up) Rework Desks Plugin
Bug 24201: (follow-up) add desk choice with library choice
Bug 24201: (follow-up) add desk choice with library choice
Bug 24201: (follow-up) add desk choice with library choice
Bug 24201: (follow-up) add desk choice with library choice
Bug 24201: (follow-up) add desk choice with library choice
Bug 24201: (follow-up) add desk choice with library choice
Bug 24201: (follow-up) add desk choice with library choice
Bug 24201: Attach desk to intranet session
Bug 24201: (follow-up) Show desk information at lower screen widths
Bug 24201: (follow-up) Use GetLoggedInDeskName
Bug 24201: (follow-up) Rework Desks Plugin
Bug 24201: (follow-up) add desk choice with library choice
Bug 24201: (QA follow-up) Only display desks picker if enabled
Bug 24201: (QA follow-up) Remove deprecated template file
Bug 24201: (QA follow-up) Hide header if desks disabled
Bug 24201: (QA follow-up) Re-indent helpers
Bug 24201: Attach desk to intranet session
Bug 24201: (follow-up) Show desk information at lower screen widths
Bug 24201: (follow-up) Use GetLoggedInDeskName
Bug 24201: (follow-up) Rework Desks Plugin
Bug 24201: (follow-up) add desk choice with library choice
Bug 24201: (QA follow-up) Only display desks picker if enabled
Bug 24201: (QA follow-up) Remove deprecated template file
Bug 24201: (QA follow-up) Hide header if desks disabled
Bug 24201: (QA follow-up) Re-indent helpers
Bug 24201: (QA follow-up) Do not unbless a list from search in Desks template plugin
Bug 24201: (QA follow-up) Remove unneeded use
Bug 24201: (QA follow-up) Fix desks select box selected attribute
Bug 24201: (QA follow-up) Remove setdesk variable and add desk selection script only on pages in need
Bug 24201: (QA follow-up) Make desk and branch selects on login page consistant
Bug 24201: (QA follow-up) Do not unbless a list from search in Desks template plugin
Bug 24201: (QA follow-up) Remove unneeded use
Bug 24201: (QA follow-up) Fix desks select box selected attribute
Bug 24201: (QA follow-up) Remove setdesk variable and add desk selection script only on pages in need
Bug 24201: (QA follow-up) Make desk and branch selects on login page consistant
Bug 24201: (QA follow-up) Only display desks picker if enabled
Bug 24201: (QA follow-up) Capitalisation fix
Bug 24201: (QA follow-up) Correct call to set_userenv
Bug 24201: (QA follow-up) Only display desks picker if enabled
Bug 24201: (QA follow-up) Capitalisation fix
Bug 24201: (QA follow-up) Correct call to set_userenv
Bug 24201: (QA follow-up) Another capitalization fix
Bug 24201: (QA follow-up) Add 'and desk' to menus and title
Bug 24201: (QA follow-up) IndependentBranches
Bug 24201: (QA follow-up) Make logic consistent
Bug 24201: (QA follow-up) Replace library select
Bug 24201: fix failing test
Bug 24201: fix failing tests

Description Nicolas Legrand 2019-12-10 14:05:10 UTC
Last step before attaching a waiting hold to a desk: attach the current desk to the librarians session.
Comment 1 Nicolas Legrand 2019-12-10 14:07:16 UTC Comment hidden (obsolete)
Comment 2 Nicolas Legrand 2019-12-10 15:11:05 UTC Comment hidden (obsolete)
Comment 3 Nicolas Legrand 2019-12-10 15:53:36 UTC Comment hidden (obsolete)
Comment 4 Nicolas Legrand 2019-12-10 16:34:22 UTC Comment hidden (obsolete)
Comment 5 Nicolas Legrand 2020-03-23 14:36:43 UTC Comment hidden (obsolete)
Comment 6 Nicolas Legrand 2020-03-24 13:37:34 UTC Comment hidden (obsolete)
Comment 7 Martin Renvoize 2020-03-25 11:14:17 UTC
Generally, this patch appears to work as expected so I'll sign it off.

However, I have some feedback:
1/ I'd like to see the whole 'Desks' functionality behind a system preference so libraries not interested in using the feature don't see it.
2/ If there are not desks defined for the logged-in library then a trailing `|` is left in the header menu
3/ Should you not be able to set a Desk from the Login screen?
4/ Jury is out regarding adding the Desk into the header bar menu title.. I like that it's immediately obvious, but for narrow screens, this could cause issues?

Copying in a few more people to get opinions.
Comment 8 Martin Renvoize 2020-03-25 11:15:54 UTC Comment hidden (obsolete)
Comment 9 Nicolas Legrand 2020-03-25 11:34:33 UTC
(In reply to Martin Renvoize from comment #7)
> Generally, this patch appears to work as expected so I'll sign it off.
> 
> However, I have some feedback:

Thank you!

> 1/ I'd like to see the whole 'Desks' functionality behind a system
> preference so libraries not interested in using the feature don't see it.

I thought about not adding another syspref. If there is no desk defined it should not be seen apart from the links “desks” in admin home page and menu. If it's too much anyway I can surely add a syspref.

> 2/ If there are not desks defined for the logged-in library then a trailing
> `|` is left in the header menu

Oups, that should not happen, I'll fix it.

> 3/ Should you not be able to set a Desk from the Login screen?

Sure, but I wasn't sure how to do it, because it's necessary to change the desk menu when switching libraries, and I wasn't sure how to do it. I better liked the idea to attach IP addresses to a desk to automatically set a desk at logged-in.

> 4/ Jury is out regarding adding the Desk into the header bar menu title.. I
> like that it's immediately obvious, but for narrow screens, this could cause
> issues?

Ho, I'm terrible at web things. We are used to this display, but we certainly will conform to what suits best the community.
Comment 10 Katrin Fischer 2020-03-25 11:50:25 UTC
About IP adresses: I am not sure if that will always work. What if you have a hosted Koha but you only got one outgoing IP address instead of individual by workstation?
Comment 11 Owen Leonard 2020-03-25 12:00:55 UTC
(In reply to Nicolas Legrand from comment #9)

> I thought about not adding another syspref. If there is no desk defined it
> should not be seen apart from the links “desks” in admin home page and menu.

I think this mostly works. The goal of the system preferences is to prevent confusion from users about options they have which aren't relevant. In that sense this works as it should. The only problem I see right now is that the page is still accessible. It's still possible to navigate to /circ/selectdesk.pl and have no options. I would consider that a minor issue though.

> > 3/ Should you not be able to set a Desk from the Login screen?

I agree, this would be a logical and useful finishing touch. My idea of how this feature is useful is with libraries where there are multiple desks in one location and it's necessary to distinguish between them. In that situation the user wouldn't be using the change library option at all.

> > but for narrow screens, this could cause issues?

I think it's logical to display the desk if it is set. We can adjust the responsive behavior of the menu as a whole if necessary.
Comment 12 Owen Leonard 2020-03-25 12:17:35 UTC Comment hidden (obsolete)
Comment 13 Nicolas Legrand 2020-03-25 13:26:08 UTC Comment hidden (obsolete)
Comment 14 Nicolas Legrand 2020-03-25 13:26:11 UTC Comment hidden (obsolete)
Comment 15 Nicolas Legrand 2020-03-25 13:26:54 UTC
(In reply to Martin Renvoize from comment #7)

> preference so libraries not interested in using the feature don't see it.
> 2/ If there are not desks defined for the logged-in library then a trailing
> `|` is left in the header menu

fixed
Comment 16 Nicolas Legrand 2020-03-25 13:28:09 UTC
(In reply to Owen Leonard from comment #12)
> Created attachment 101697 [details] [review] [review]
> Bug 24201: (follow-up) Show desk information at lower screen widths
> 
> The staff client header menu adjusts responsively to narrower viewport
> widths, but for the logged-in-user menu this requires showing and hiding
> part of the template which duplicates information. This patch adds a
> copy of the current desk information to the part of the template shown
> at narrower widths.
> 
> To test, apply the patch and view any page in the staff client.
> 
>  - Narrow your browser so that the viewport is less than 800px wide.
>  - The logged-in-user menu should change from text to a single "user"
>    icon.
>  - Clicking the icon should display a menu which includes labeled
>    information about the current desk.

Nice! Thanks Owen!
Comment 17 Fridolin Somers 2020-03-26 12:16:11 UTC
In my opinion, we should avoid adding LoginDeskname to get_template_and_user
and use the template plugin Desks by adding GetLoggedInDeskName().
Comment 18 Nicolas Legrand 2020-03-26 13:29:54 UTC
Hoi Frido,

(In reply to Fridolin SOMERS from comment #17)
> In my opinion, we should avoid adding LoginDeskname to get_template_and_user
> and use the template plugin Desks by adding GetLoggedInDeskName().

I tried to mimic existing code and I'm not really confident on how it works and how it should work. If there are others thinking the way you hint to is better, I have nothing to say to oppose it and will be glad to adapt the patch!
Comment 19 Jonathan Druart 2020-03-26 13:39:36 UTC
At first glance I'd say you don't need the "unfiltered" param in the TT plugin.
Comment 20 Martin Renvoize 2020-03-26 13:51:32 UTC Comment hidden (obsolete)
Comment 21 Martin Renvoize 2020-03-26 14:14:27 UTC
So.. as yet the `Desks.GetName` and `Desks.all` functions aren't used within this patchset so we could probably just introduce them when they start to be used.

That could ease this through QA ;)
Comment 22 Martin Renvoize 2020-03-26 14:52:11 UTC Comment hidden (obsolete)
Comment 23 Martin Renvoize 2020-03-26 14:53:05 UTC
Hopefully, that'll help get this through the process for you Nicolas :)
Comment 24 Nicolas Legrand 2020-03-26 14:55:54 UTC
(In reply to Martin Renvoize from comment #23)
> Hopefully, that'll help get this through the process for you Nicolas :)

sure! thanks!
Comment 25 Martin Renvoize 2020-03-26 14:59:04 UTC Comment hidden (obsolete)
Comment 26 Martin Renvoize 2020-03-26 15:36:56 UTC Comment hidden (obsolete)
Comment 27 Andrew Fuerste-Henry 2020-03-26 15:53:24 UTC Comment hidden (obsolete)
Comment 28 Andrew Fuerste-Henry 2020-03-26 15:53:27 UTC Comment hidden (obsolete)
Comment 29 Andrew Fuerste-Henry 2020-03-26 15:54:16 UTC
Created attachment 101880 [details] [review]
Bug 24201: Attach desk to intranet session

When Desks are defined, a librarian can attach a desk to its session.

Test plan:

1. apply 13881 and create some desks
2. you should see “NO DESK SET” in the intranet header
3. go to circulation > Set desk
4. you should see your desk name in the header, whatever the page
5. you can also set desk with the header menu

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 30 Andrew Fuerste-Henry 2020-03-26 15:54:19 UTC
Created attachment 101881 [details] [review]
Bug 24201: (follow-up) Show desk information at lower screen widths

The staff client header menu adjusts responsively to narrower viewport
widths, but for the logged-in-user menu this requires showing and hiding
part of the template which duplicates information. This patch adds a
copy of the current desk information to the part of the template shown
at narrower widths.

To test, apply the patch and view any page in the staff client.

 - Narrow your browser so that the viewport is less than 800px wide.
 - The logged-in-user menu should change from text to a single "user"
   icon.
 - Clicking the icon should display a menu which includes labeled
   information about the current desk.

Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr>

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 31 Andrew Fuerste-Henry 2020-03-26 15:54:23 UTC
Created attachment 101882 [details] [review]
Bug 24201: (follow-up) Use GetLoggedInDeskName

This patch updates the templates to use the new TT plugin instead of
adding additional variables to the already crouded get_template_and_user.

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 32 Andrew Fuerste-Henry 2020-03-26 15:54:26 UTC Comment hidden (obsolete)
Comment 33 Josef Moravec 2020-04-21 14:13:27 UTC
Comment on attachment 101883 [details] [review]
Bug 24201: (follow-up) Rework Desks Plugin

Review of attachment 101883 [details] [review]:
-----------------------------------------------------------------

Hi Nicolas, good job

I just found a minor terminology issue from my point of view (see bellow)

And I have a logical problem with setting desks functionality:
Imaginewe have two libraries defined and each have some desks defined, for example
Library1: Desk 1A and Desk1B
Library2: Desk 2A and Desk2b

When I set Library1 and Desk 1A and then, I need to work with Library2. There is still Desk 1A set... I need to go at another page - setdesk.pl and set a correct desk (2A or 2B)

I maybe have one easy solution for this - instead of adding a setdesk page, implement the ability of setting desks into set-library page.

It also fixes the issue which Owen mentioned - using setdesk page even if it is useless

::: Koha/Template/Plugin/Desks.pm
@@ +76,5 @@
>  returns all desks existing at the library
>  
>  =cut
>  
> +sub ListForBranch {

Would be a bit better to use 'Library' instead 'Branch' - for consistant terminology
Comment 34 Martin Renvoize 2020-04-23 07:55:45 UTC
Time is running out for this, this cycle now.. I would love to have seen this and bug 24412 make it through alongside bug 13881 this cycle so we had a full feature.  There's still a small window of opportunity if we can get back to it quickly.
Comment 35 Nicolas Legrand 2020-04-28 10:12:16 UTC
Created attachment 103839 [details] [review]
Bug 24201: (follow-up) Rework Desks Plugin

This patch removes unused plugin methods (they can be re-introduced in
subsequent bugs where they are used) and merges the 'all' and 'defined'
methods into a ListForBranch method that returns a list of desks
associated with the logged in branch.

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 36 Nicolas Legrand 2020-04-28 10:12:19 UTC Comment hidden (obsolete)
Comment 37 Josef Moravec 2020-04-30 13:26:50 UTC
Comment on attachment 103840 [details] [review]
Bug 24201: (follow-up) add desk choice with library choice

Review of attachment 103840 [details] [review]:
-----------------------------------------------------------------

::: Koha/Template/Plugin/Desks.pm
@@ +95,5 @@
> +    for my $d ( @$desks ) {
> +        if (       defined $selected and $d->{desk_id} eq $selected
> +                   or not defined $selected and C4::Context->userenv and $d->{branchcode} eq ( C4::Context->userenv->{desk_id} // q{} )
> +            ) {
> +            $d->{selected} = 1;

You don't need selected here.

You can just return Koha::Desks->search();

::: koha-tmpl/intranet-tmpl/prog/en/includes/js_includes.inc
@@ +138,5 @@
>      </script>
>  [% END %]
> +
> +[% IF setdesk %]
> +    <script  type="text/javascript">

do not use type attribute, see https://wiki.koha-community.org/wiki/Coding_Guidelines#JS1:_Embedding_JavaScript_blocks_in_templates

Also, there is no need to have to have javascript code in template (you don't need template toolkit inside js code), so the code should be in js file and included using Asset template plugin

::: koha-tmpl/intranet-tmpl/prog/en/modules/circ/set-library.tt
@@ +78,5 @@
> +    <legend>Set Desk</legend>
> +    <ol>
> +        <li><label for="desk">Choose Desk:</label>
> +        <select name="desk_id" id="desk_id">
> +            [% PROCESS options_for_desks libraries => Branches.all( selected => branch ), desks => Desks.all( selected => desk_id ) %]

you pass the libraries variable to options_for_desks block, but not use it in the block itself
Comment 38 Josef Moravec 2020-04-30 13:48:36 UTC
(In reply to Nicolas Legrand from comment #36)
> 
> PROBLEMS:
> 
> 1. I don't know how to send desk from login page
> 2. What about multiple libraries with some not having desk? This patch
> assume that when you use desks, you use it for every libraries
> 3. when changing library, the bar displaying the library and the desk
> (up right) doesn't update the desk when arriving on the updated page
> of set-library.tt. It is changed only after having clicked on
> “continue”
> 4. should we keep a separate desk changing page or is it redundant?

ad 1.
You should use it in C4::Auth, look to the line 1113 and below, where is 'branch' parameter used

ad 2.
It should take it into account and use desks for the libraries with desks and not to show any desk for libraries with no desks defined... or you can do a workaround a set desk to fake "default" desk where there is no desk.

3. I dind't run the current code yet, so I am not sure about this


4. I think this is redundant now
Comment 39 Frédéric Demians 2020-06-06 06:49:02 UTC
What is exactly the status of this? Since bug 13881 is in master and 20.05, what is the plan to include the actual functionality?
Comment 40 Nicolas Legrand 2020-06-06 12:19:23 UTC
I'm still working on it, it should be ready soon :). In June.
Comment 41 Nicolas Legrand 2020-06-12 13:22:40 UTC Comment hidden (obsolete)
Comment 42 Katrin Fischer 2020-06-13 18:12:02 UTC
Hi Nicolas,  is this ready again for testing? (Needs signoff) or is there still a question that needs to be resolved?
Comment 43 Nicolas Legrand 2020-06-15 06:02:54 UTC
Still not sorry... I'm fighting with the login page. Hope it'll be ready today... If not, next week.
Comment 44 Nicolas Legrand 2020-06-15 08:18:12 UTC Comment hidden (obsolete)
Comment 45 Nicolas Legrand 2020-06-15 08:19:54 UTC
mostly working now, with still a bit missing : handling a library without desk when others have some.
Comment 46 Nicolas Legrand 2020-06-15 12:11:01 UTC Comment hidden (obsolete)
Comment 47 Nicolas Legrand 2020-06-15 12:13:29 UTC Comment hidden (obsolete)
Comment 48 Nicolas Legrand 2020-06-15 12:19:41 UTC
Created attachment 105875 [details] [review]
Bug 24201: (follow-up) add desk choice with library choice

You should be able to add desk choice when you are logging in or
changing library.

Test plan:

1. apply patch
2. have at least three libraries, one without desk, one with one and
one with a few.
3. At login, when choosing a library, it should enable all desks it
has. Pick one.
4. the desk id and name should be set in your session and appear in
the top right, next to the library name.
5. change library and desks from intranet (at the set-library.pl page)
6. you should have the same behaviours
7. if you have a library without a desk, it should prompt you a '---'
option and no desks will be attached to the session.
Comment 49 Nicolas Legrand 2020-07-06 12:13:22 UTC
Created attachment 106576 [details] [review]
Bug 24201: (follow-up) add desk choice with library choice

You should be able to add desk choice when you are logging in or
changing library.

Test plan:

1. apply patch
2. have at least three libraries, one without desk, one with one and
one with a few.
3. At login, when choosing a library, it should enable all desks it
has. Pick one.
4. the desk id and name should be set in your session and appear in
the top right, next to the library name.
5. change library and desks from intranet (at the set-library.pl page)
6. you should have the same behaviours
7. if you have a library without a desk, it should prompt you a '---'
option and no desks will be attached to the session.
Comment 50 Martin Renvoize 2020-07-20 12:46:16 UTC
Created attachment 107089 [details] [review]
Bug 24201: Attach desk to intranet session

When Desks are defined, a librarian can attach a desk to its session.

Test plan:

1. apply 13881 and create some desks
2. you should see “NO DESK SET” in the intranet header
3. go to circulation > Set desk
4. you should see your desk name in the header, whatever the page
5. you can also set desk with the header menu

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 51 Martin Renvoize 2020-07-20 12:46:20 UTC
Created attachment 107090 [details] [review]
Bug 24201: (follow-up) Show desk information at lower screen widths

The staff client header menu adjusts responsively to narrower viewport
widths, but for the logged-in-user menu this requires showing and hiding
part of the template which duplicates information. This patch adds a
copy of the current desk information to the part of the template shown
at narrower widths.

To test, apply the patch and view any page in the staff client.

 - Narrow your browser so that the viewport is less than 800px wide.
 - The logged-in-user menu should change from text to a single "user"
   icon.
 - Clicking the icon should display a menu which includes labeled
   information about the current desk.

Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr>
Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 52 Martin Renvoize 2020-07-20 12:46:24 UTC
Created attachment 107091 [details] [review]
Bug 24201: (follow-up) Use GetLoggedInDeskName

This patch updates the templates to use the new TT plugin instead of
adding additional variables to the already crouded get_template_and_user.

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 53 Martin Renvoize 2020-07-20 12:46:28 UTC
Created attachment 107092 [details] [review]
Bug 24201: (follow-up) Rework Desks Plugin

This patch removes unused plugin methods (they can be re-introduced in
subsequent bugs where they are used) and merges the 'all' and 'defined'
methods into a ListForBranch method that returns a list of desks
associated with the logged in branch.

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 54 Martin Renvoize 2020-07-20 12:46:33 UTC
Created attachment 107093 [details] [review]
Bug 24201: (follow-up) add desk choice with library choice

You should be able to add desk choice when you are logging in or
changing library.

Test plan:

1. apply patch
2. have at least three libraries, one without desk, one with one and
one with a few.
3. At login, when choosing a library, it should enable all desks it
has. Pick one.
4. the desk id and name should be set in your session and appear in
the top right, next to the library name.
5. change library and desks from intranet (at the set-library.pl page)
6. you should have the same behaviours
7. if you have a library without a desk, it should prompt you a '---'
option and no desks will be attached to the session.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 55 Martin Renvoize 2020-07-20 12:46:37 UTC
Created attachment 107094 [details] [review]
Bug 24201: (QA follow-up) Only display desks picker if enabled

Minor followup to add the 'UseCirculationDesks' check to the desk picker
option

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 56 Martin Renvoize 2020-07-20 12:46:40 UTC
Created attachment 107095 [details] [review]
Bug 24201: (QA follow-up) Remove deprecated template file

The controller associated to this template was deprecated earlier in
this patchset, but the template was left behind.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 57 Martin Renvoize 2020-07-20 12:46:44 UTC
Created attachment 107096 [details] [review]
Bug 24201: (QA follow-up) Hide header if desks disabled

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 58 Martin Renvoize 2020-07-20 12:46:48 UTC
Created attachment 107097 [details] [review]
Bug 24201: (QA follow-up) Re-indent helpers

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 59 Martin Renvoize 2020-07-20 12:53:07 UTC
Comment on attachment 107093 [details] [review]
Bug 24201: (follow-up) add desk choice with library choice

Review of attachment 107093 [details] [review]:
-----------------------------------------------------------------

Signing off the set.. this is a really nice and elegant solution, well done !

Minor note which I'm drawing the QAers attention to, but not a direct failure.. more of a slight code style wonder from my point.

::: koha-tmpl/intranet-tmpl/prog/en/includes/js_includes.inc
@@ +25,5 @@
>  [% Asset.js("lib/jquery/plugins/jquery.validate.min.js") | $raw %]
>  <!-- koha core js -->
>  [% Asset.js("js/staff-global.js") | $raw %]
> +[% IF setdesk %]
> +[% Asset.js("js/desk_selection.js") | $raw %]

I'm wondering about the logic here.. could we not just include the Asset line where it's required rather than setting a TT variable to then look for here?

::: koha-tmpl/intranet-tmpl/prog/en/modules/auth.tt
@@ +5,4 @@
>  [% USE Categories %]
>  [% SET footerjs = 1 %]
> +[% IF Desks.all %]
> +    [% SET setdesk = 1 %]

I think it would be clearer to just include the asset in the JS includes at the bottom of the template here rather than set a variable to be caught to add it in in the js_includes block.

::: koha-tmpl/intranet-tmpl/prog/en/modules/circ/set-library.tt
@@ +3,3 @@
>  [% USE Koha %]
> +[% IF Desks.all %]
> +    [% SET setdesk = 1 %]

I think it would be clearer to just include the asset in the JS includes at the bottom of the template here rather than set a variable to be caught to add it in in the js_includes block.
Comment 60 Owen Leonard 2020-07-24 17:59:01 UTC
(In reply to Martin Renvoize from comment #59)
> I'm wondering about the logic here.. could we not just include the Asset
> line where it's required rather than setting a TT variable to then look for
> here?

This patch doesn't apply for me so I'm not entirely sure what all is happening in the templates, but judging from the patch I think I agree... I don't really understand the point of setting the "setdesk" variable in some places and in others checking for "Desks.all" right in the template. The "setdesk" variable seems like an extra step.
Comment 61 Josef Moravec 2020-08-03 07:45:58 UTC
Created attachment 107667 [details] [review]
Bug 24201: Attach desk to intranet session

When Desks are defined, a librarian can attach a desk to its session.

Test plan:

1. apply 13881 and create some desks
2. you should see “NO DESK SET” in the intranet header
3. go to circulation > Set desk
4. you should see your desk name in the header, whatever the page
5. you can also set desk with the header menu

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 62 Josef Moravec 2020-08-03 07:46:04 UTC
Created attachment 107668 [details] [review]
Bug 24201: (follow-up) Show desk information at lower screen widths

The staff client header menu adjusts responsively to narrower viewport
widths, but for the logged-in-user menu this requires showing and hiding
part of the template which duplicates information. This patch adds a
copy of the current desk information to the part of the template shown
at narrower widths.

To test, apply the patch and view any page in the staff client.

 - Narrow your browser so that the viewport is less than 800px wide.
 - The logged-in-user menu should change from text to a single "user"
   icon.
 - Clicking the icon should display a menu which includes labeled
   information about the current desk.

Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr>
Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 63 Josef Moravec 2020-08-03 07:46:09 UTC
Created attachment 107669 [details] [review]
Bug 24201: (follow-up) Use GetLoggedInDeskName

This patch updates the templates to use the new TT plugin instead of
adding additional variables to the already crouded get_template_and_user.

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 64 Josef Moravec 2020-08-03 07:46:15 UTC
Created attachment 107670 [details] [review]
Bug 24201: (follow-up) Rework Desks Plugin

This patch removes unused plugin methods (they can be re-introduced in
subsequent bugs where they are used) and merges the 'all' and 'defined'
methods into a ListForBranch method that returns a list of desks
associated with the logged in branch.

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 65 Josef Moravec 2020-08-03 07:46:20 UTC
Created attachment 107671 [details] [review]
Bug 24201: (follow-up) add desk choice with library choice

You should be able to add desk choice when you are logging in or
changing library.

Test plan:

1. apply patch
2. have at least three libraries, one without desk, one with one and
one with a few.
3. At login, when choosing a library, it should enable all desks it
has. Pick one.
4. the desk id and name should be set in your session and appear in
the top right, next to the library name.
5. change library and desks from intranet (at the set-library.pl page)
6. you should have the same behaviours
7. if you have a library without a desk, it should prompt you a '---'
option and no desks will be attached to the session.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 66 Josef Moravec 2020-08-03 07:46:26 UTC
Created attachment 107672 [details] [review]
Bug 24201: (QA follow-up) Only display desks picker if enabled

Minor followup to add the 'UseCirculationDesks' check to the desk picker
option

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 67 Josef Moravec 2020-08-03 07:46:31 UTC
Created attachment 107673 [details] [review]
Bug 24201: (QA follow-up) Remove deprecated template file

The controller associated to this template was deprecated earlier in
this patchset, but the template was left behind.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 68 Josef Moravec 2020-08-03 07:46:37 UTC
Created attachment 107674 [details] [review]
Bug 24201: (QA follow-up) Hide header if desks disabled

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 69 Josef Moravec 2020-08-03 07:46:42 UTC
Created attachment 107675 [details] [review]
Bug 24201: (QA follow-up) Re-indent helpers

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 70 Josef Moravec 2020-08-03 07:46:48 UTC
Created attachment 107676 [details] [review]
Bug 24201: (QA follow-up) Do not unbless a list from search in Desks template plugin

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 71 Josef Moravec 2020-08-03 07:46:54 UTC
Created attachment 107677 [details] [review]
Bug 24201: (QA follow-up) Remove unneeded use

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 72 Josef Moravec 2020-08-03 07:46:59 UTC
Created attachment 107678 [details] [review]
Bug 24201: (QA follow-up) Fix desks select box selected attribute

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 73 Josef Moravec 2020-08-03 07:47:04 UTC
Created attachment 107679 [details] [review]
Bug 24201: (QA follow-up) Remove setdesk variable and add desk selection script only on pages in need

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 74 Josef Moravec 2020-08-03 07:47:10 UTC
Created attachment 107680 [details] [review]
Bug 24201: (QA follow-up) Make desk and branch selects on login page consistant

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 75 Josef Moravec 2020-08-03 08:10:45 UTC
(In reply to Martin Renvoize from comment #59)
> Comment on attachment 107093 [details] [review] [review]
> Bug 24201: (follow-up) add desk choice with library choice
> 
> Review of attachment 107093 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> Signing off the set.. this is a really nice and elegant solution, well done !
> 
> Minor note which I'm drawing the QAers attention to, but not a direct
> failure.. more of a slight code style wonder from my point.
> 
> ::: koha-tmpl/intranet-tmpl/prog/en/includes/js_includes.inc
> @@ +25,5 @@
> >  [% Asset.js("lib/jquery/plugins/jquery.validate.min.js") | $raw %]
> >  <!-- koha core js -->
> >  [% Asset.js("js/staff-global.js") | $raw %]
> > +[% IF setdesk %]
> > +[% Asset.js("js/desk_selection.js") | $raw %]
> 
> I'm wondering about the logic here.. could we not just include the Asset
> line where it's required rather than setting a TT variable to then look for
> here?
> 
> ::: koha-tmpl/intranet-tmpl/prog/en/modules/auth.tt
> @@ +5,4 @@
> >  [% USE Categories %]
> >  [% SET footerjs = 1 %]
> > +[% IF Desks.all %]
> > +    [% SET setdesk = 1 %]
> 
> I think it would be clearer to just include the asset in the JS includes at
> the bottom of the template here rather than set a variable to be caught to
> add it in in the js_includes block.
> 


I do agree with you Martin.

I rebased the patchset, and added some follow-ups, including removing setdesk variable...

Could you please look at my patches? I do not want to pass this QA until somebody else look at my follow-ups. Thanks
Comment 76 Martin Renvoize 2020-08-04 10:30:28 UTC
Created attachment 107728 [details] [review]
Bug 24201: (QA follow-up) Do not unbless a list from search in Desks template plugin

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 77 Martin Renvoize 2020-08-04 10:30:31 UTC
Created attachment 107729 [details] [review]
Bug 24201: (QA follow-up) Remove unneeded use

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 78 Martin Renvoize 2020-08-04 10:30:35 UTC
Created attachment 107730 [details] [review]
Bug 24201: (QA follow-up) Fix desks select box selected attribute

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 79 Martin Renvoize 2020-08-04 10:30:40 UTC
Created attachment 107731 [details] [review]
Bug 24201: (QA follow-up) Remove setdesk variable and add desk selection script only on pages in need

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 80 Martin Renvoize 2020-08-04 10:30:44 UTC
Created attachment 107732 [details] [review]
Bug 24201: (QA follow-up) Make desk and branch selects on login page consistant

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 81 Martin Renvoize 2020-08-04 10:32:18 UTC
Thanks for the additional followups and QA run Josef.. I've verified the followups all make sense so setting to PQA as it's had two QA team eyes on it now :)

Great work.
Comment 82 Martin Renvoize 2020-08-04 11:08:34 UTC
I decided to rebase bug 24786 atop this and is highlighted a few more minor things.. setting back to SO whilst I post some further followups.
Comment 83 Martin Renvoize 2020-08-04 12:30:28 UTC
Created attachment 107750 [details] [review]
Bug 24201: (QA follow-up) Only display desks picker if enabled

I missed a case on the authentication page with the prior patch of the
same name.
Comment 84 Martin Renvoize 2020-08-04 12:30:32 UTC
Created attachment 107751 [details] [review]
Bug 24201: (QA follow-up) Capitalisation fix

'Set desk' vs 'Set Desk'.
Comment 85 Martin Renvoize 2020-08-04 12:30:37 UTC
Created attachment 107752 [details] [review]
Bug 24201: (QA follow-up) Correct call to set_userenv

One call to set_userenv had been implimented incorrectly with the
parameters out of order.
Comment 86 Martin Renvoize 2020-08-04 12:31:24 UTC
Three more tiny QA followups.. would appreciate your eyes on them Josef for the final switch to PQA :)
Comment 87 Josef Moravec 2020-08-05 05:06:04 UTC
Created attachment 107815 [details] [review]
Bug 24201: (QA follow-up) Only display desks picker if enabled

I missed a case on the authentication page with the prior patch of the
same name.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 88 Josef Moravec 2020-08-05 05:06:11 UTC
Created attachment 107816 [details] [review]
Bug 24201: (QA follow-up) Capitalisation fix

'Set desk' vs 'Set Desk'.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 89 Josef Moravec 2020-08-05 05:06:17 UTC
Created attachment 107817 [details] [review]
Bug 24201: (QA follow-up) Correct call to set_userenv

One call to set_userenv had been implimented incorrectly with the
parameters out of order.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 90 Josef Moravec 2020-08-05 05:06:24 UTC
Created attachment 107818 [details] [review]
Bug 24201: (QA follow-up) Another capitalization fix

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 91 Josef Moravec 2020-08-05 05:08:30 UTC
(In reply to Martin Renvoize from comment #86)
> Three more tiny QA followups.. would appreciate your eyes on them Josef for
> the final switch to PQA :)

And one more from me again (capitalization fix)

I consider it PQA now, thanks!
Comment 92 Jonathan Druart 2020-08-07 08:46:04 UTC
Can I have an updated test plan please?

I followed the one from comment 1:
    1. apply 13881 and create some desks
    2. you should see “NO DESK SET” in the intranet header
    3. go to circulation > Set desk
=> I don't see that
    4. you should see your desk name in the header, whatever the page
    5. you can also set desk with the header menu
=> I did not see it until I figured out it was in "set library"

2 first questions then,
- Why "NO DESK SET" vs "No desk set" vs nothing display if no desk set?
- Why not "set library and desk"?
Comment 93 Martin Renvoize 2020-08-07 10:09:07 UTC
(In reply to Jonathan Druart from comment #92)
> Can I have an updated test plan please?
> 
> I followed the one from comment 1:
>     1. apply 13881 and create some desks
>     2. you should see “NO DESK SET” in the intranet header
>     3. go to circulation > Set desk
> => I don't see that
>     4. you should see your desk name in the header, whatever the page
>     5. you can also set desk with the header menu
> => I did not see it until I figured out it was in "set library"
> 
> 2 first questions then,
> - Why "NO DESK SET" vs "No desk set" vs nothing display if no desk set?
> - Why not "set library and desk"?

Updated test plan:

1. apply 13881 and create some desks
2. you should see “NO DESK SET” in the intranet header
> This will only appear upon first applying the patch, or if you enable 'UseDesks' during a session (If it was already enabled you will have had to have selected a desk at login and you cannot select 'no desk' at any point now I believe)
3. go to circulation > Set library
> 'Set desk' was merged into the 'Set library' page.
4. you should see your desk name in the header, whatever the page
5. you can also set desk with the header menu

'NO DESK SET'.. I think Nicholas felt it was very important to highlight that no desk is set so capitalised it.. I think in reality it will appear so infrequently that the capitalisation isn't a bad thing.

As for 'Set library and desk'.. I'm of two minds here, as mentioned on irc this morning.  With bug 24786 we also add the ability to set 'Cash register' from this page.. we could end up with 'Set library, desk and cash register' which feels a little verbose.. we could change it here for simply be 'Set library [and desk]' when desks are enabled and then perhaps update it on bug 24786 to read 'Session settings' or something like that when both preferences are enabled?
Comment 94 Martin Renvoize 2020-08-07 10:53:58 UTC
Created attachment 107951 [details] [review]
Bug 24201: (QA follow-up) Add 'and desk' to menus and title

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 95 Martin Renvoize 2020-08-07 12:38:59 UTC
Created attachment 107952 [details] [review]
Bug 24201: (QA follow-up) IndependentBranches

This patch updates the IndependentBranches template display logic such
that the 'Select desk' and 'Select library and desk' options appear
correctly when IndependentBranches is enabled.
Comment 96 Martin Renvoize 2020-08-07 14:22:37 UTC
Created attachment 107967 [details] [review]
Bug 24201: (QA follow-up) Make logic consistent

The display logic for the desk details in the header bar was
inconsistent between the narrow display view and the full width display
view.  This patch updates the former to match the latter.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 97 Martin Renvoize 2020-08-07 14:40:28 UTC
Created attachment 107968 [details] [review]
Bug 24201: (QA follow-up) Replace library select

Removing the library select entirely results in a JS failure for desk
selection when independant branches is enabled and the user does not
have permission to select a library.

This patch adds a hidden select element so we can still grab the current
branch from the selection.
Comment 98 Jonathan Druart 2020-08-07 14:55:44 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 99 Jonathan Druart 2020-08-10 07:57:48 UTC
Created attachment 107980 [details] [review]
Bug 24201: fix failing test
Comment 100 Jonathan Druart 2020-08-10 08:04:49 UTC
Created attachment 107981 [details] [review]
Bug 24201: fix failing tests
Comment 101 Jonathan Druart 2020-08-10 10:32:26 UTC
(In reply to Jonathan Druart from comment #100)
> Created attachment 107981 [details] [review] [review]
> Bug 24201: fix failing tests

Pushed to master for 20.11
Comment 102 Lucas Gass 2020-08-14 22:50:23 UTC
enhancement will not be backported to 20.05.x series