Bug 30123 - On set library page, desk always defaults to last in list instead of desk user is signed in at
Summary: On set library page, desk always defaults to last in list instead of desk use...
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Staff interface (show other bugs)
Version: master
Hardware: All All
: P5 - low minor (vote)
Assignee: Brendan Lawlor
QA Contact: Testopia
URL:
Keywords:
Depends on: 36572 24201
Blocks: 36582
  Show dependency treegraph
 
Reported: 2022-02-17 16:48 UTC by Lucas Gass
Modified: 2024-04-12 12:52 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 30123: Set desk default to logged-in-desk-id on set library page (4.94 KB, patch)
2024-04-03 19:19 UTC, Brendan Lawlor
Details | Diff | Splinter Review
Bug 30123: Set desk default to logged-in-desk-id on set library page (4.94 KB, patch)
2024-04-03 19:26 UTC, Brendan Lawlor
Details | Diff | Splinter Review
Screenshot showing missing option (171.29 KB, image/png)
2024-04-12 12:47 UTC, Owen Leonard
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Lucas Gass 2022-02-17 16:48:19 UTC
TO recreate:

-Turn on  UseCirculationDesks 
-Create several desks for the library you are logged in as
-Set your desk to one you just created
-Reload set-library.pl and notice that no matter what desk you are set as the dropdown defaults to the last one in the list

It should default to your currently logged in desk
Comment 1 Brendan Lawlor 2024-04-03 19:19:47 UTC Comment hidden (obsolete)
Comment 2 Brendan Lawlor 2024-04-03 19:26:12 UTC
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
Comment 3 Brendan Lawlor 2024-04-03 19:56:26 UTC
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.
Comment 4 Lucas Gass 2024-04-03 21:57:10 UTC
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?
Comment 5 Brendan Lawlor 2024-04-04 14:34:10 UTC
(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.
Comment 6 Lucas Gass 2024-04-11 09:22:55 UTC
(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.
Comment 7 Andrew Auld 2024-04-11 10:58:22 UTC
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.
Comment 8 Lucas Gass 2024-04-11 13:53:20 UTC
Thanks Andrew! I think if we apply this one on top of Bug 36572 it will work?
Comment 9 Owen Leonard 2024-04-11 16:48:31 UTC
Why trigger change() after the on("change") event?
Comment 10 Brendan Lawlor 2024-04-11 17:20:04 UTC
(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.
Comment 11 Owen Leonard 2024-04-11 17:37:31 UTC
(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.
Comment 12 Owen Leonard 2024-04-12 12:47:12 UTC
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.
Comment 13 Owen Leonard 2024-04-12 12:52:19 UTC
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?