Typo: IsSuperLibraian/IsSuperLibrarian

N(In reply to Lucas Gass from comment #1)
> Typo: IsSuperLibraian/IsSuperLibrarian

Not just a typo though, with that fixed I am still unable to set a desk as a non-superlibrarian

Created attachment 164395 [details] [review]
Bug 36507: Allow non-superlibrarians to set desk and register

To test:

1. Turn on UseCirculationDesks
2. Create some desks
3. Login as a non-superlibrarian
4. Try to switch desks, get an error

5. Turn on UseCashRegisters
6. Create some registers
7. As a non-superlibrarian try to change your register, get an error.

8. APPLY PATCH and restart_all
9. Try changing your desk and register again, it should now work.

Created attachment 164396 [details] [review]
Bug 36507: Allow non-superlibrarians to set desk and register

To test:

1. Turn on UseCirculationDesks
2. Create some desks
3. Login as a non-superlibrarian
4. Try to switch desks, get an error

5. Turn on UseCashRegisters
6. Create some registers
7. As a non-superlibrarian try to change your register, get an error.

8. APPLY PATCH and restart_all
9. Try changing your desk and register again, it should now work.

Signed-off-by: David Nind <david@davidnind.com>

Existing patch is good and fixes what's supposed to fix. But I think additional work is required here:

When you set a desk and save, if you go back to the set desk screen, you'll notice the pre-selected value is not the one you just saved to.

I believe this is caused by something wrong happening in desk_selection.js. If you apply the below changes, the issue I mention above gets fixed (but I'm unsure what it breaks - if anything):

+++ b/koha-tmpl/intranet-tmpl/prog/js/desk_selection.js
@@ -2,7 +2,7 @@ $(document).ready(function() {
     $("#desk_id").children().each(function() {
         var selectedBranch = $("#branch"). children("option:selected"). val();
         if ($(this).attr('id') === "nodesk") { //set no desk by default, should be first element
-            $(this).prop("selected", true);
+            // $(this).prop("selected", true);
             $(this).prop("disabled", false);
             $(this).show();
         }
@@ -11,7 +11,7 @@ $(document).ready(function() {
             $('#nodesk').hide();
             $(this).prop("disabled", false);
             $(this).show();
-            $(this).prop("selected", true)
+            // $(this).prop("selected", true)
         } else {
         
         
---- FURTHER TESTING ----

Delete a desk:
/cgi-bin/koha/admin/desks.pl?op=delete_confirm&desk_id=1

Confirmation screen has all blank data

----

Following the above, click confirm delete, you get:
An error occurred when deleting this desk. Check the logs for details.

----

Edit an existing desk:
/cgi-bin/koha/admin/desks.pl?op=add_form&desk_id=1

You're unable to change library (unsure if this is intended or not)

-----

Some of the above may be considered their own separate bugs (the delete and edit(?) issues), but I feel like at least the first issue (wrong pre-selected desk) should be fixed here. Let me know if you disagree, and we can turn this to PQA and fix the others in their own bugs.

Another note: circulate permission is not required for the staff member to set a desk/cash register, but upon successful update the patron is redirected to /cgi-bin/koha/circ/circulation.pl and that page does require the circulate permission, i.e. patron is presented with a 403 screen after successfully setting a desk/cash_register in this specific use case.

(In reply to Pedro Amorim from comment #5)
> Existing patch is good and fixes what's supposed to fix. But I think
> additional work is required here:
> 
> When you set a desk and save, if you go back to the set desk screen, you'll
> notice the pre-selected value is not the one you just saved to.
> 
> I believe this is caused by something wrong happening in desk_selection.js.
> If you apply the below changes, the issue I mention above gets fixed (but
> I'm unsure what it breaks - if anything):

Pedro, have a look at Bug 30123, there is work been done to fix this particular issue there:

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30123

(In reply to Lucas Gass from comment #7)
> (In reply to Pedro Amorim from comment #5)
> > Existing patch is good and fixes what's supposed to fix. But I think
> > additional work is required here:
> > 
> > When you set a desk and save, if you go back to the set desk screen, you'll
> > notice the pre-selected value is not the one you just saved to.
> > 
> > I believe this is caused by something wrong happening in desk_selection.js.
> > If you apply the below changes, the issue I mention above gets fixed (but
> > I'm unsure what it breaks - if anything):
> 
> Pedro, have a look at Bug 30123, there is work been done to fix this
> particular issue there:
> 
> https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30123

Hey, cool! Thanks.
Should we PQA this one (36507) then and address the other issues in different bugs? i.e. "cannot delete a desk" and/or the others if they are indeed bugs?

I feel like this bug survived way too long undetected.

Please file bugs for the other issues detected!

Pushed for 24.05!

Well done everyone, thank you!

Looks like it depends on CSRF work, not in 23.11.x

Did the follow-up issues here get resolved/reported?  Can we drop the additional_work_needed keyword yet?

No changes required to the manual.

Response required to comment #13 about additional work required.

@Pedro: can you please review your comments and let us know which additional bugs need filing so that we can remove the keyword?

(In reply to Katrin Fischer from comment #15)
> @Pedro: can you please review your comments and let us know which additional
> bugs need filing so that we can remove the keyword?

I've done through my own notes, and I can't reproduce any of the 3 issues I listed at the time. All seems to have been fixed.

(In reply to Pedro Amorim from comment #16)
> (In reply to Katrin Fischer from comment #15)
> > @Pedro: can you please review your comments and let us know which additional
> > bugs need filing so that we can remove the keyword?
> 
> I've done through my own notes, and I can't reproduce any of the 3 issues I
> listed at the time. All seems to have been fixed.

Thank you!