Bug 30123

Created attachment 164390 [details] [review]
Bug 30123: Set desk default to logged-in-desk-id on set library page

This patch refactors the javascript that runs on the set library page. If the logged in user branch code is the same as the seleceted library, default the desk selection to the current logged in desk id. Otherwise set the default desk selection to the first option for the current branch.

Test plan:

1. Turn on UseCirculationDesks
2. Create a few desks with unique names for a few different libraries
3. Set your library and desk
4. Reload set-library.pl and notice that the desk always defaults to the last option for the library selected
5. Apply patch and restart_all
6. Reload set-library.pl and notice that the desk selection defaults to your current logged in desk
7. Try changing the library selection to libraries with and without desks
8. Notice that the desk selection defaults to the first option available for the selected library
9. Notice that the desk selection defaults to the --- no desk for libraries without desks
10. Notice the desk selection defaults to the currenty logged in desk if you select the library you are currently logged into

For some reason when I made this commit the code got autoformatted in a way that I don't really like.

The code still works, but could be nicer to read if the whitespace is fixed.

Thanks for your work here Brendan! I have two questions:

1. Do we need all this JavaScript in desk_selection.js? Can't we just use what is already in set-library.tt?

[% PROCESS options_for_desks desks => Desks.all(), selected => desk_id  %]

2. Are there any circumstances where a user should be able to unset their desk?

(In reply to Lucas Gass from comment #4)

These are some heavy questions so I'm in a little over my head but I'll try.

> 1. Do we need all this JavaScript in desk_selection.js? Can't we just use
> what is already in set-library.tt?
> 
> [% PROCESS options_for_desks desks => Desks.all(), selected => desk_id  %]

Maybe the javascript could be reduced, but as is I think it's required for this behavior. The template renders an option element for each desk and just sets properties to selected, hidden and disabled. The javascript listens for the user to change the set library selection and then updates hidden and disabled properties on the options to basically filter the list. You can see how PROCESS options_for_desks creates all the options in html_helpers.inc

I guess it might be possible to reduce the javascript here if set-library.pl was reloaded every time the set library selection is changed. I'm not sure how Koha usually handles this sort of thing, but I'm interested in learning.

 
> 2. Are there any circumstances where a user should be able to unset their
> desk?

This is a quirk that may also need to be addressed. The current behavior is that once a desk is set, it can't be unset to No desk. The javascript currently hides/disables the no desk <option id="nodesk" value="">---</option> when a library has desks, so it forces the user to select a desk. But if you change the javascript and submit the form with the desk value '' the page just reloads but nothing happens. I think there needs to be an update to set-library.pl to handle that no desk value as an update.

(In reply to Brendan Lawlor from comment #5)
> (In reply to Lucas Gass from comment #4)
> 
> These are some heavy questions so I'm in a little over my head but I'll try.
> 
> > 1. Do we need all this JavaScript in desk_selection.js? Can't we just use
> > what is already in set-library.tt?
> > 
> > [% PROCESS options_for_desks desks => Desks.all(), selected => desk_id  %]
> 
> Maybe the javascript could be reduced, but as is I think it's required for
> this behavior. The template renders an option element for each desk and just
> sets properties to selected, hidden and disabled. The javascript listens for
> the user to change the set library selection and then updates hidden and
> disabled properties on the options to basically filter the list. You can see
> how PROCESS options_for_desks creates all the options in html_helpers.inc
> 
> I guess it might be possible to reduce the javascript here if set-library.pl
> was reloaded every time the set library selection is changed. I'm not sure
> how Koha usually handles this sort of thing, but I'm interested in learning.

Thanks, for the detailed answer. I see that we need this JS. :) 

> > 2. Are there any circumstances where a user should be able to unset their
> > desk?
> 
> This is a quirk that may also need to be addressed. The current behavior is
> that once a desk is set, it can't be unset to No desk. The javascript
> currently hides/disables the no desk <option id="nodesk"
> value="">---</option> when a library has desks, so it forces the user to
> select a desk. But if you change the javascript and submit the form with the
> desk value '' the page just reloads but nothing happens. I think there needs
> to be an update to set-library.pl to handle that no desk value as an update.

We should deal with this in a follow-up bug, I think.

I applied this patch in a sandbox. In testing I noticed the following:
I had applied the patch on a sandbox on which I had previously applied patch 36096. I don't know if this is relevant but thought I would mention.
Once the patch was applied, I created the following desks:
desk1
desk2
desk3
desk4
The system would allow me to set the desk on login, but not change it on the 'setdesk.pl' screen. Only by logging off and setting it again at login could I change the desk. On 'setdesk.pl' it did default to the currently set desk, rather than the last one in the list. So that bit seems to work.
Thinking that it might be a conflict with the other bug I had applied to the sandbox, I loaded a new sandbox just with this patch applied.
This time I created desks as before. Before I started testing, I inadvertently created a duplicate desk (desk1) on the same library branch. I tried to delete it, but it created an error and couldn't delete the desk.
The confirmation of deletion screen didn't have any data under desk ID, desk and library, so this is probably why.
Again, it would only allow me to set the desk on logout, login. Not on the setdesk.pl screen.

Hope this is useful.

Thanks Andrew! I think if we apply this one on top of Bug 36572 it will work?

Why trigger change() after the on("change") event?

(In reply to Owen Leonard from comment #9)
> Why trigger change() after the on("change") event?

The javascript currently has a bit of logic that is repeated twice, because it has to run once when the page loads and then again whenever the library select is changed. I wanted to make it DRY, so change() triggers the on change event when the page first loads. Maybe it would be more readable if it was instead written as a function that gets called twice?

I wrote it this way so that when I was testing I only had to edit the logic in one place.

Maybe I should submit another patch that just updates the minimal number of lines that need to be changed? Instead of changing the whole file so that the change is easier to see. It's mostly just adding a conditional to set the current desk if it needs to be set.

(In reply to Brendan Lawlor from comment #10)
> (In reply to Owen Leonard from comment #9)
> > Why trigger change() after the on("change") event?
> so change() triggers the on change event when the page first loads

Ah, that makes sense. IMO you don't need to redo the patch again to change the number of updated lines.

Created attachment 164846 [details]
Screenshot showing missing option

This is working except for one weird issue:

If I try to switch to a library which has more than one desk, only one desk appears in the filtered dropdown. I'm attaching this screenshot which includes the DOM inspector because you can see that both options *should* be visible.

In my test, changing the markup in html_helpers.inc fixed the issue:

-  <option class="[% d.branchcode | html %]" value="[% d.desk_id | html %]" disabled hidden>[% d.desk_name | html %]</option>

+  <option class="[% d.branchcode | html %]" value="[% d.desk_id | html %]" disabled style="display: none">[% d.desk_name | html %]</option>

jQuery's show and hide don't play nice with the "hidden" attribute?

I've seen that jQuery fun before.. though I can't remember where or why it happens.. I'm not averse to the proposed change from Owen.

Created attachment 165331 [details] [review]
Bug 30123: Set desk default to logged-in-desk-id on sety library page

This patch refactors the javascript that runs on the set library page. If the logged in user branch code is the same as the seleceted library, default the desk selection to the current logged in desk id. Otherwise set the default desk selection to the first option for the current branch.

Test plan:

1. Turn on UseCirculationDesks
2. Create a few desks with unique names for a few different libraries
3. Set your library and desk
4. Reload set-library.pl and notice that the desk always defaults to the last option for the library selected
5. Apply patch and restart_all
6. Reload set-library.pl and notice that the desk selection defaults to your current logged in desk
7. Try changing the library selection to libraries with and without desks
8. Notice that the desk selection defaults to the first option available for the selected library
9. Notice that the desk selection defaults to the --- no desk for libraries without desks
10. Notice the desk selection defaults to the currenty logged in desk if you select the library you are currently logged into

This new patch changes the fewest number of lines and doesn't have the weird auto formatting issue so it's more clear to see what it does.

It also includes the markup fix that Owen suggested.

Created attachment 165339 [details] [review]
Bug 30123: Set desk default to logged-in-desk-id on sety library page

This patch refactors the javascript that runs on the set library page. If the logged in user branch code is the same as the seleceted library, default the desk selection to the current logged in desk id. Otherwise set the default desk selection to the first option for the current branch.

Test plan:

1. Turn on UseCirculationDesks
2. Create a few desks with unique names for a few different libraries
3. Set your library and desk
4. Reload set-library.pl and notice that the desk always defaults to the last option for the library selected
5. Apply patch and restart_all
6. Reload set-library.pl and notice that the desk selection defaults to your current logged in desk
7. Try changing the library selection to libraries with and without desks
8. Notice that the desk selection defaults to the first option available for the selected library
9. Notice that the desk selection defaults to the --- no desk for libraries without desks
10. Notice the desk selection defaults to the currenty logged in desk if you select the library you are currently logged into

Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>

Created attachment 165358 [details] [review]
Bug 30123: Set desk default to logged-in-desk-id on sety library page

This patch refactors the javascript that runs on the set library page. If the logged in user branch code is the same as the seleceted library, default the desk selection to the current logged in desk id. Otherwise set the default desk selection to the first option for the current branch.

Test plan:

1. Turn on UseCirculationDesks
2. Create a few desks with unique names for a few different libraries
3. Set your library and desk
4. Reload set-library.pl and notice that the desk always defaults to the last option for the library selected
5. Apply patch and restart_all
6. Reload set-library.pl and notice that the desk selection defaults to your current logged in desk
7. Try changing the library selection to libraries with and without desks
8. Notice that the desk selection defaults to the first option available for the selected library
9. Notice that the desk selection defaults to the --- no desk for libraries without desks
10. Notice the desk selection defaults to the currenty logged in desk if you select the library you are currently logged into

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

Thanks for your work here. The patch is clean and clear, the code works nicely and QA scripts are happy.

Passing QA

Pushed for 24.05!

Well done everyone, thank you!

Depends on Bug 36572 not in 23.11.x

Fixes an error in the circulation desk selection. No changes to the manual required (checked Administration > Circulation desks).