Bug 24201 - Attach desk to intranet session
Summary: Attach desk to intranet session
Status: In Discussion
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Nicolas Legrand
QA Contact: Josef Moravec
URL:
Keywords:
Depends on: 13881
Blocks: 14654 24412
  Show dependency treegraph
 
Reported: 2019-12-10 14:05 UTC by Nicolas Legrand
Modified: 2020-06-03 09:29 UTC (History)
9 users (show)

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


Attachments
Bug 24201: Attach desk to intranet session (19.33 KB, patch)
2019-12-10 14:07 UTC, Nicolas Legrand
Details | Diff | Splinter Review
Bug 24201: Attach desk to intranet session (19.28 KB, patch)
2019-12-10 15:11 UTC, Nicolas Legrand
Details | Diff | Splinter Review
Bug 24201: Attach desk to intranet session (19.29 KB, patch)
2019-12-10 15:53 UTC, Nicolas Legrand
Details | Diff | Splinter Review
Bug 24201: Attach desk to intranet session (20.30 KB, patch)
2019-12-10 16:34 UTC, Nicolas Legrand
Details | Diff | Splinter Review
Bug 24201: Attach desk to intranet session (20.79 KB, patch)
2020-03-23 14:36 UTC, Nicolas Legrand
Details | Diff | Splinter Review
Bug 24201: Attach desk to intranet session (20.79 KB, patch)
2020-03-24 13:37 UTC, Nicolas Legrand
Details | Diff | Splinter Review
Bug 24201: Attach desk to intranet session (20.84 KB, patch)
2020-03-25 11:15 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24201: (follow-up) Show desk information at lower screen widths (2.14 KB, patch)
2020-03-25 12:17 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 24201: Attach desk to intranet session (20.88 KB, patch)
2020-03-25 13:26 UTC, Nicolas Legrand
Details | Diff | Splinter Review
Bug 24201: (follow-up) Show desk information at lower screen widths (2.19 KB, patch)
2020-03-25 13:26 UTC, Nicolas Legrand
Details | Diff | Splinter Review
Bug 24201: (follow-up) Use GetLoggedInDeskName (5.94 KB, patch)
2020-03-26 13:51 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24201: (follow-up) Rework Desks Plugin (6.84 KB, patch)
2020-03-26 14:52 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24201: (follow-up) Rework Desks Plugin (6.86 KB, patch)
2020-03-26 14:59 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24201: (follow-up) Rework Desks Plugin (6.86 KB, patch)
2020-03-26 15:36 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24201: (follow-up) Use GetLoggedInDeskName (6.01 KB, patch)
2020-03-26 15:53 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 24201: (follow-up) Rework Desks Plugin (6.95 KB, patch)
2020-03-26 15:53 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 24201: Attach desk to intranet session (21.00 KB, patch)
2020-03-26 15:54 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 24201: (follow-up) Show desk information at lower screen widths (2.26 KB, patch)
2020-03-26 15:54 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 24201: (follow-up) Use GetLoggedInDeskName (6.08 KB, patch)
2020-03-26 15:54 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 24201: (follow-up) Rework Desks Plugin (7.01 KB, patch)
2020-03-26 15:54 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 24201: (follow-up) Rework Desks Plugin (7.72 KB, patch)
2020-04-28 10:12 UTC, Nicolas Legrand
Details | Diff | Splinter Review
Bug 24201: (follow-up) add desk choice with library choice (10.03 KB, patch)
2020-04-28 10:12 UTC, Nicolas Legrand
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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
Created attachment 96158 [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
Comment 2 Nicolas Legrand 2019-12-10 15:11:05 UTC
Created attachment 96164 [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
Comment 3 Nicolas Legrand 2019-12-10 15:53:36 UTC
Created attachment 96165 [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
Comment 4 Nicolas Legrand 2019-12-10 16:34:22 UTC
Created attachment 96166 [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
Comment 5 Nicolas Legrand 2020-03-23 14:36:43 UTC
Created attachment 101452 [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
Comment 6 Nicolas Legrand 2020-03-24 13:37:34 UTC
Created attachment 101564 [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
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
Created attachment 101684 [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>
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
Created attachment 101697 [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.
Comment 13 Nicolas Legrand 2020-03-25 13:26:08 UTC
Created attachment 101704 [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>
Comment 14 Nicolas Legrand 2020-03-25 13:26:11 UTC
Created attachment 101705 [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>
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
Created attachment 101848 [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.
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
Created attachment 101865 [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.
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
Created attachment 101866 [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.
Comment 26 Martin Renvoize 2020-03-26 15:36:56 UTC
Created attachment 101877 [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.
Comment 27 Andrew Fuerste-Henry 2020-03-26 15:53:24 UTC
Created attachment 101878 [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>
Comment 28 Andrew Fuerste-Henry 2020-03-26 15:53:27 UTC
Created attachment 101879 [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>
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
Created attachment 101883 [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 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
Created attachment 103840 [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. log in, you should see desks if they are defined. All are disabled.
3. when choosing a library, it should enabled all desks it has and
pick one.
4. the desk should be in your session (not working yet, I don't
undersand how to do it)
5. change library from intranet
6. you should have the same behaviours

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