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.

(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.

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?

(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.

(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

(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!

In my opinion, we should avoid adding LoginDeskname to get_template_and_user
and use the template plugin Desks by adding GetLoggedInDeskName().

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!

At first glance I'd say you don't need the "unfiltered" param in the TT plugin.

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 ;)

Hopefully, that'll help get this through the process for you Nicolas :)

(In reply to Martin Renvoize from comment #23)
> Hopefully, that'll help get this through the process for you Nicolas :)

sure! thanks!

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>

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>

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 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

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.

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 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

(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

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?

I'm still working on it, it should be ready soon :). In June.

Hi Nicolas,  is this ready again for testing? (Needs signoff) or is there still a question that needs to be resolved?

Still not sorry... I'm fighting with the login page. Hope it'll be ready today... If not, next week.

mostly working now, with still a bit missing : handling a library without desk when others have some.

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.

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.

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>

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>

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>

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>

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>

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>

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>

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>

Created attachment 107097 [details] [review]
Bug 24201: (QA follow-up) Re-indent helpers

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

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.

(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.

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

Created attachment 107677 [details] [review]
Bug 24201: (QA follow-up) Remove unneeded use

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

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>

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>

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>

(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

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>

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>

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>

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>

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>

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.

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.

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.

Created attachment 107751 [details] [review]
Bug 24201: (QA follow-up) Capitalisation fix

'Set desk' vs 'Set Desk'.

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.

Three more tiny QA followups.. would appreciate your eyes on them Josef for the final switch to PQA :)

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>

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>

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>

Created attachment 107818 [details] [review]
Bug 24201: (QA follow-up) Another capitalization fix

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

(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!

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"?

(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?

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>

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.

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>

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.

Pushed to master for 20.11, thanks to everybody involved!

Created attachment 107980 [details] [review]
Bug 24201: fix failing test

Created attachment 107981 [details] [review]
Bug 24201: fix failing tests

(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

enhancement will not be backported to 20.05.x series