Bug 14783

Summary: Allow patrons to change pickup location for non-waiting holds
Product: Koha Reporter: Kyle M Hall <kyle.m.hall>
Component: Hold requestsAssignee: Björn Nylén <bjorn.nylen>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: acampbell, barbara.johnson, bjorn.nylen, caroline.cyr-la-rose, daniel.gaghan, fridolin.somers, gmcharlt, jonathan.druart, lucas, m.de.rooy, martin.renvoize, michael.hafen, nick, nkeener, techservspec, tomascohen, vctrzuniga
Version: master   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31855
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Caroline Cyr La Rose Documentation submission: https://gitlab.com/koha-community/koha-manual/-/merge_requests/661
Text to go in the release notes:
This enhancement allows patrons to change the pickup location of non-waiting holds from the opac. The new system preference "OPACAllowUserToChangeBranch" controls at what stages the pickup location can be changed (Pending, In-transit, Suspended).
Version(s) released in:
22.11.00
Bug Depends on: 30500    
Bug Blocks: 32470    
Attachments: Allow users to change pickup location for hold from the opac
Allow users to change pickup location for hold from the opac
Bug 14783: Change pickup location from opac.
Bug 14783: (follow-up) Move pickup library form into modal
Bug 14783: Change pickup location from opac.
Bug 14783: (follow-up) Move pickup library form into modal
(QA followup) Unit tests
Bug 14783: (follow-up revised) Move pickup library form into modal
(QA followup, suggestion) Change "Pick up location" to "Pickup location"
Bug 14783: Change pickup location from opac.
Bug 14783: (QA follow-up) Add unit tests.
Bug 14783: (follow-up) Move pickup library form into modal
Bug 14783: (QA follow-up) Change "Pick up location" to "Pickup location" in table.
Bug 14783: Change pickup location from opac.
Bug 14783: (follow-up) Move pickup library form into modal
(QA follow-up) Change "Pick up location" to "Pickup location" in table.
(QA follow-up) Add unit tests.
Bug 14783: Change pickup location from opac.
Bug 14783: (follow-up) Move pickup library form into modal
Bug 14783: (QA follow-up) Change "Pick up location" to "Pickup location" in table.
Bug 14783: (QA follow-up) Add unit tests.
Bug 14783: Change pickup location from opac.
Bug 14783: (follow-up) Move pickup library form into modal
Bug 14783: (QA follow-up) Change "Pick up location" to "Pickup location" in table.
Bug 14783: (QA follow-up) Add unit tests.
Bug 14783: (QA follow-up) Rename method and move tests
Bug 14783: Compiled CSS
Bug 14783: Update atomicupdate syntax
Bug 14783: (follow-up) MySQL doesn't like selecting from a table it is updating

Description Kyle M Hall 2015-09-03 18:43:12 UTC
This enhancement would allow patrons to change the pickup location for any hold that is not already waiting ( e.g. it would be allowable for in transit holds ).

For non-found holds the feature will not change much behavior. For in-transit holds the feature would trigger a completion of the current transfer and generate a new transfer from the previous target library to the new target library.

The feature would be controlled via a system preference.
Comment 1 NancyK. 2016-12-07 21:07:21 UTC
My staff is asking for this option.  We have a branch closed for remodeling and it would have been great to allow patrons to redirect their unfilled holds to another location.  Redirecting after it is in-transit isn't as important.
NancyK
Comment 2 Daniel Gaghan 2018-12-27 19:55:49 UTC
Just migrated over to Koha and our staff and users are asking for this option too.
Comment 3 Timothy Alexis Vass 2021-02-12 14:24:46 UTC
We are trying to understand how it makes sense to allow changing location for in transit holds. Could someone comment on this?
Comment 4 Katrin Fischer 2021-02-12 14:45:08 UTC
Hm it might be nice to control 'not pulled yet' and 'in transit' separately. But I guess patrons could realize they made a wrong setting after placing the hold and it could be useful then?
Comment 5 Timothy Alexis Vass 2021-02-12 15:57:30 UTC
I think we could have a multiple select in the system preference to set which statuses to allow it for.

Something like:
OpacAllowPickupLocationChange
Enable allowing patrons to change pickup location for these statuses: <multiple select element>

Then each library can decide which statuses to allow it for at the same time as activating it.
Comment 6 Timothy Alexis Vass 2021-03-02 13:35:58 UTC
Should it be possible to change pick up location for suspended holds?
Comment 7 Katrin Fischer 2021-03-04 09:35:54 UTC
(In reply to Timothy Alexis Vass from comment #6)
> Should it be possible to change pick up location for suspended holds?

I'd say so. You suspend them before they have been filled, so this should be ok.
Comment 8 Björn Nylén 2021-03-12 13:16:13 UTC
Created attachment 118189 [details] [review]
Allow users to change pickup location for hold from the opac
Comment 9 Björn Nylén 2021-03-12 13:21:25 UTC
Timothy gave this bug to me so here's a patch. :)


Bug 14783: Change pickup location from opac.
    
    This patch allows users to change the  pickup location for their holds from
    the opac. A syspref (OPACAllowUserToChangeBranch) controls at what stage
    a hold can be changed.
    
    To test:
    1. Set up holds with diffrent statuses for a patron (pending, waiting, in transit, suspended)
    2. No pickup locations for any hold should be able to be changed.
    3. Turn on and off the different options under OPACAllowUserToChangeBranch. Make sure that
    only the corresponding holds can be changed from the opac. Check eg in the staff client that
    pickup location has changed.
    4. The available pickup locations should respect any transfer restrictions. (Same as the
    holds list for a biblio in staff client)
    5. For an in-transit hold: Check in the item at the original pickup location.
    6. Note that it will be redirected to the new location.
    
    Sponsored-by: Lunds Universitetsbibliotek
Comment 10 Björn Nylén 2021-03-15 15:17:47 UTC
Created attachment 118241 [details] [review]
Allow users to change pickup location for hold from the opac

Doh! Updated patch! Previous patch included some bits that wasn't supposed to be there. Same test case.
Comment 11 Owen Leonard 2021-07-26 17:27:00 UTC
Created attachment 123199 [details] [review]
Bug 14783: Change pickup location from opac.

This patch allows users to change the  pickup location for their holds
from the opac. A syspref (OPACAllowUserToChangeBranch) controls at what
stage a hold can be changed.

To test:
1. Set up holds with diffrent statuses for a patron (pending, waiting,
   in transit, suspended)
2. No pickup locations for any hold should be able to be changed.
3. Turn on and off the different options under
   OPACAllowUserToChangeBranch. Make sure that only the corresponding
   holds can be changed from the opac. Check eg in the staff client that
   pickup location has changed.
4. The available pickup locations should respect any transfer
   restrictions. (Same as the
   holds list for a biblio in staff client)
5. For an in-transit hold: Check in the item at the original pickup
   location.
6. Note that it will be redirected to the new location.

Sponsored-by: Lunds Universitetsbibliotek

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 12 Owen Leonard 2021-07-28 19:30:35 UTC
Created attachment 123271 [details] [review]
Bug 14783: (follow-up) Move pickup library form into modal

This patch attemps to simplify the list of holds by moving the form for
changing a hold's pickup library into a modal dialog.

This patch also makes minor tweaks to the CSS for the "X" close button
shown in modal in windows. Instances where modal markup used "x" have
been updated to use the Bootstrap 4 standard "&times;"

To test, apply the patch and rebuild the OPAC
SCSS
(https://wiki.koha-community.org/wiki/Working_with_SCSS_in_the_OPAC_and_staff_client)

- Turn on the OPACAllowUserToChangeBranch system preference.
- Log in to the OPAC as a user with holds.
- In the view of your holds, confirm that each hold which can be
  modified shows a "Change" link.
- Clicking the link should show a modal window with the title of the
  item on hold shown in the header.
- Confirm that changing the pickup location works correctly.
Comment 13 Biblibre Sandboxes 2021-08-10 16:34:15 UTC
Created attachment 123708 [details] [review]
Bug 14783: Change pickup location from opac.

This patch allows users to change the  pickup location for their holds
from the opac. A syspref (OPACAllowUserToChangeBranch) controls at what
stage a hold can be changed.

To test:
1. Set up holds with diffrent statuses for a patron (pending, waiting,
   in transit, suspended)
2. No pickup locations for any hold should be able to be changed.
3. Turn on and off the different options under
   OPACAllowUserToChangeBranch. Make sure that only the corresponding
   holds can be changed from the opac. Check eg in the staff client that
   pickup location has changed.
4. The available pickup locations should respect any transfer
   restrictions. (Same as the
   holds list for a biblio in staff client)
5. For an in-transit hold: Check in the item at the original pickup
   location.
6. Note that it will be redirected to the new location.

Sponsored-by: Lunds Universitetsbibliotek

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Barbara Johnson <barbara.johnson@bedfordtx.gov>
Comment 14 Biblibre Sandboxes 2021-08-10 16:34:18 UTC
Created attachment 123709 [details] [review]
Bug 14783: (follow-up) Move pickup library form into modal

This patch attemps to simplify the list of holds by moving the form for
changing a hold's pickup library into a modal dialog.

This patch also makes minor tweaks to the CSS for the "X" close button
shown in modal in windows. Instances where modal markup used "x" have
been updated to use the Bootstrap 4 standard "&times;"

To test, apply the patch and rebuild the OPAC
SCSS
(https://wiki.koha-community.org/wiki/Working_with_SCSS_in_the_OPAC_and_staff_client)

- Turn on the OPACAllowUserToChangeBranch system preference.
- Log in to the OPAC as a user with holds.
- In the view of your holds, confirm that each hold which can be
  modified shows a "Change" link.
- Clicking the link should show a modal window with the title of the
  item on hold shown in the header.
- Confirm that changing the pickup location works correctly.

Signed-off-by: Barbara Johnson <barbara.johnson@bedfordtx.gov>
Comment 15 Katrin Fischer 2021-10-21 21:42:50 UTC
Took a first look here and I think we will need unit tests for the new sub in Holds.pm: can_change_branch_opac
Comment 16 Katrin Fischer 2021-10-31 15:28:23 UTC
Taking another look at this - I had been hoping to see some unit tests here by now :(

1) There are some changes that appear unrelated to the patch sets intent - maybe local changes that slipped in? To me it looks like they should be moved to a separate bug:

-  <span aria-hidden="true">x</span>
+  <span aria-hidden="true">&times;</span>

2) For any change to a .pm we require unit tests. 

3) Being a really nit-picky librarian (and because I could not stand being told I had missed it) (not blocker)

('OPACAllowUserToChooseBranch','1','1','Allow the user to choose the branch they want to pickup their hold from','YesNo'),
+('OPACAllowUserToChangeBranch','','Pending, In-Transit, Suspended','Allow users to change the library to pick up a hold for these statuses:','multiple'),

Change should be before Choose :)

4) Table column has "pick up", but modal has "pickup" - what's correct?


This is a fail for 2) mostly - others I would have fixed myself. Please come back with this patch set, it's would be a great addition.
Comment 17 Owen Leonard 2021-11-03 14:38:50 UTC
(In reply to Katrin Fischer from comment #16)
> -  <span aria-hidden="true">x</span>
> +  <span aria-hidden="true">&times;</span>

This change is correct, so if it can be slipped in here I would approve.
Comment 18 Owen Leonard 2021-11-03 14:39:49 UTC
Haha because I did it. Of course I would approve. I have no memory of this patch!
Comment 19 Katrin Fischer 2021-11-03 22:29:59 UTC
(In reply to Owen Leonard from comment #18)
> Haha because I did it. Of course I would approve. I have no memory of this
> patch!

I didn't notice either when asking you to have look... what does that say about us?
Comment 20 Björn Nylén 2021-11-09 14:47:16 UTC
Created attachment 127488 [details] [review]
(QA followup) Unit tests

Got around to doing unit tests (a bit of a noob at this still)....
Comment 21 Björn Nylén 2021-11-09 15:26:07 UTC
Created attachment 127490 [details] [review]
Bug 14783: (follow-up revised) Move pickup library form into modal

Replacing the modal-followup, removing the change of x -> &times; as it is already done in a recent patch.
Comment 22 Björn Nylén 2021-11-09 15:52:57 UTC
Created attachment 127492 [details] [review]
(QA followup, suggestion) Change "Pick up location" to "Pickup location"

Adding a followup patch to change "Pick up location" to "Pickup location" in holds-table.inc I belive that's used elsewhere.

As for the syspref, from what I see the wording and syspref is already as suggested?
Comment 23 Björn Nylén 2021-11-09 15:54:56 UTC
Setting back to signed off, for another round of QA.
Comment 24 Katrin Fischer 2021-11-19 14:42:00 UTC
Thx, Björn!
Comment 25 Katrin Fischer 2022-01-31 22:03:46 UTC
Hi Björn,

there is a small conflict when I try to apply these patches that I had hoped I had fixed, but there is also this:

 FAIL	t/db_dependent/Hold.t
   FAIL	  valid
		"my" variable $dt masks earlier declaration in same scope 
		"my" variable $dt masks earlier declaration in same scope 

Could you please have a look?


Also we have some guidelines of how the patch subject line should be formatted (always starting with Bug xxxx: optionally followed by (QA follow-up) or (Follow-up). Let me know if I can help with fixing those:

Processing additional checks

	* Commit title does not contain 'follow-up' correctly spelt - 19baf3584b
	* Commit title does not contain '(QA follow-up)' correctly spelt - 19baf3584b
	* Commit title does not contain 'follow-up' correctly spelt - 65676a3ba0
	* Commit title does not contain '(QA follow-up)' correctly spelt - 65676a3ba0
Comment 26 Björn Nylén 2022-02-09 09:00:59 UTC
Created attachment 130344 [details] [review]
Bug 14783: Change pickup location from opac.

This patch allows users to change the  pickup location for their holds
from the opac. A syspref (OPACAllowUserToChangeBranch) controls at what
stage a hold can be changed.

To test:
1. Set up holds with diffrent statuses for a patron (pending, waiting,
   in transit, suspended)
2. No pickup locations for any hold should be able to be changed.
3. Turn on and off the different options under
   OPACAllowUserToChangeBranch. Make sure that only the corresponding
   holds can be changed from the opac. Check eg in the staff client that
   pickup location has changed.
4. The available pickup locations should respect any transfer
   restrictions. (Same as the
   holds list for a biblio in staff client)
5. For an in-transit hold: Check in the item at the original pickup
   location.
6. Note that it will be redirected to the new location.

Sponsored-by: Lunds Universitetsbibliotek

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Barbara Johnson <barbara.johnson@bedfordtx.gov>
Comment 27 Björn Nylén 2022-02-09 09:01:04 UTC
Created attachment 130345 [details] [review]
Bug 14783: (QA follow-up) Add unit tests.

This commit adds unit tests for Bug 14783.
Comment 28 Björn Nylén 2022-02-09 09:01:08 UTC
Created attachment 130346 [details] [review]
Bug 14783: (follow-up) Move pickup library form into modal

This patch attemps to simplify the list of holds by moving the form for
changing a hold's pickup library into a modal dialog.

This patch also makes minor tweaks to the CSS for the "X" close button
shown in modal in windows. Instances where modal markup used "x" have
been updated to use the Bootstrap 4 standard "&times;"

To test, apply the patch and rebuild the OPAC
SCSS
(https://wiki.koha-community.org/wiki/Working_with_SCSS_in_the_OPAC_and_staff_client)

- Turn on the OPACAllowUserToChangeBranch system preference.
- Log in to the OPAC as a user with holds.
- In the view of your holds, confirm that each hold which can be
  modified shows a "Change" link.
- Clicking the link should show a modal window with the title of the
  item on hold shown in the header.
- Confirm that changing the pickup location works correctly.

Signed-off-by: Barbara Johnson <barbara.johnson@bedfordtx.gov>
Comment 29 Björn Nylén 2022-02-09 09:01:13 UTC
Created attachment 130347 [details] [review]
Bug 14783: (QA follow-up) Change "Pick up location" to "Pickup location" in table.
Comment 30 Björn Nylén 2022-02-09 09:05:25 UTC
Got around to this. Rebased the patchset onto current master as there was some conflicts. 

Fixed the warning in the tests and commit-titles. Test and qa-tool passes for me now.

Setting back to Signed off as well.
Comment 31 Nick Clemens 2022-02-18 20:23:42 UTC
This works well, I do wonder if there should be a public holds API to allow the user to change their own holds, but the modal works nicely.

Two issues:
The test should be in their own subtest as they are covering a new subroutine

The opac-modrequest.pl script needs to check that the hold belongs to the signed in user, it is possible with these patches to alter another users hold via URL manipulation
Comment 32 Katrin Fischer 2022-10-02 11:31:03 UTC
Hi Björn, will you have time to work on the fixes Nick asked for? 
I'd really like to see this one in :)
Comment 33 Björn Nylén 2022-10-07 13:35:53 UTC
Created attachment 141499 [details] [review]
Bug 14783: Change pickup location from opac.
Comment 34 Björn Nylén 2022-10-07 13:36:44 UTC
Created attachment 141500 [details] [review]
Bug 14783: (follow-up) Move pickup library form into modal
Comment 35 Björn Nylén 2022-10-07 13:38:01 UTC
Created attachment 141501 [details] [review]
(QA follow-up) Change "Pick up location" to "Pickup location" in table.
Comment 36 Björn Nylén 2022-10-07 13:38:54 UTC
Created attachment 141502 [details] [review]
(QA follow-up) Add unit tests.
Comment 37 Björn Nylén 2022-10-07 13:47:53 UTC
Had forgotten about this. Rebased on to master as well. Bug 30500 already did part of this patch (in regards to intransit holds) so added to that code in some parts and replaces the syspref with this one. 

Moved the tests to a subtest.
User is checked in opac-modrequest.pl (from 30500)

Setting back to signed off for more qa.
Comment 38 Nick Clemens 2022-10-12 13:28:12 UTC
Created attachment 141727 [details] [review]
Bug 14783: Change pickup location from opac.

This patch allows users to change the  pickup location for their holds
from the opac. A syspref (OPACAllowUserToChangeBranch) controls at what
stage a hold can be changed.

To test:
1. Set up holds with diffrent statuses for a patron (pending, waiting,
   in transit, suspended)
2. No pickup locations for any hold should be able to be changed.
3. Turn on and off the different options under
   OPACAllowUserToChangeBranch. Make sure that only the corresponding
   holds can be changed from the opac. Check eg in the staff client that
   pickup location has changed.
4. The available pickup locations should respect any transfer
   restrictions. (Same as the
   holds list for a biblio in staff client)
5. For an in-transit hold: Check in the item at the original pickup
   location.
6. Note that it will be redirected to the new location.

Sponsored-by: Lunds Universitetsbibliotek

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Barbara Johnson <barbara.johnson@bedfordtx.gov>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 39 Nick Clemens 2022-10-12 13:28:16 UTC
Created attachment 141728 [details] [review]
Bug 14783: (follow-up) Move pickup library form into modal

This patch attemps to simplify the list of holds by moving the form for
changing a hold's pickup library into a modal dialog.

This patch also makes minor tweaks to the CSS for the "X" close button
shown in modal in windows. Instances where modal markup used "x" have
been updated to use the Bootstrap 4 standard "&times;"

To test, apply the patch and rebuild the OPAC
SCSS
(https://wiki.koha-community.org/wiki/Working_with_SCSS_in_the_OPAC_and_staff_client)

- Turn on the OPACAllowUserToChangeBranch system preference.
- Log in to the OPAC as a user with holds.
- In the view of your holds, confirm that each hold which can be
  modified shows a "Change" link.
- Clicking the link should show a modal window with the title of the
  item on hold shown in the header.
- Confirm that changing the pickup location works correctly.

Signed-off-by: Barbara Johnson <barbara.johnson@bedfordtx.gov>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 40 Nick Clemens 2022-10-12 13:28:21 UTC
Created attachment 141729 [details] [review]
Bug 14783: (QA follow-up) Change "Pick up location" to "Pickup location" in table.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 41 Nick Clemens 2022-10-12 13:28:26 UTC
Created attachment 141730 [details] [review]
Bug 14783: (QA follow-up) Add unit tests.

This commit adds unit tests for Bug 14783.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 42 Katrin Fischer 2022-10-14 18:44:32 UTC
Thanks, Nick!
Comment 43 Tomás Cohen Arazi 2022-10-17 19:00:34 UTC
Created attachment 142009 [details] [review]
Bug 14783: Change pickup location from opac.

This patch allows users to change the  pickup location for their holds
from the opac. A syspref (OPACAllowUserToChangeBranch) controls at what
stage a hold can be changed.

To test:
1. Set up holds with diffrent statuses for a patron (pending, waiting,
   in transit, suspended)
2. No pickup locations for any hold should be able to be changed.
3. Turn on and off the different options under
   OPACAllowUserToChangeBranch. Make sure that only the corresponding
   holds can be changed from the opac. Check eg in the staff client that
   pickup location has changed.
4. The available pickup locations should respect any transfer
   restrictions. (Same as the
   holds list for a biblio in staff client)
5. For an in-transit hold: Check in the item at the original pickup
   location.
6. Note that it will be redirected to the new location.

Sponsored-by: Lunds Universitetsbibliotek

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Barbara Johnson <barbara.johnson@bedfordtx.gov>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 44 Tomás Cohen Arazi 2022-10-17 19:00:41 UTC
Created attachment 142010 [details] [review]
Bug 14783: (follow-up) Move pickup library form into modal

This patch attemps to simplify the list of holds by moving the form for
changing a hold's pickup library into a modal dialog.

This patch also makes minor tweaks to the CSS for the "X" close button
shown in modal in windows. Instances where modal markup used "x" have
been updated to use the Bootstrap 4 standard "&times;"

To test, apply the patch and rebuild the OPAC
SCSS
(https://wiki.koha-community.org/wiki/Working_with_SCSS_in_the_OPAC_and_staff_client)

- Turn on the OPACAllowUserToChangeBranch system preference.
- Log in to the OPAC as a user with holds.
- In the view of your holds, confirm that each hold which can be
  modified shows a "Change" link.
- Clicking the link should show a modal window with the title of the
  item on hold shown in the header.
- Confirm that changing the pickup location works correctly.

Signed-off-by: Barbara Johnson <barbara.johnson@bedfordtx.gov>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 45 Tomás Cohen Arazi 2022-10-17 19:00:47 UTC
Created attachment 142011 [details] [review]
Bug 14783: (QA follow-up) Change "Pick up location" to "Pickup location" in table.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 46 Tomás Cohen Arazi 2022-10-17 19:00:55 UTC
Created attachment 142012 [details] [review]
Bug 14783: (QA follow-up) Add unit tests.

This commit adds unit tests for Bug 14783.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 47 Tomás Cohen Arazi 2022-10-17 19:01:03 UTC
Created attachment 142013 [details] [review]
Bug 14783: (QA follow-up) Rename method and move tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 48 Tomás Cohen Arazi 2022-10-17 19:01:19 UTC
Created attachment 142014 [details] [review]
Bug 14783: Compiled CSS

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 49 Tomás Cohen Arazi 2022-10-17 19:01:27 UTC
Created attachment 142015 [details] [review]
Bug 14783: Update atomicupdate syntax

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 50 Tomás Cohen Arazi 2022-10-17 20:48:15 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 51 Marcel de Rooy 2022-10-18 12:03:48 UTC
This was a very BAD dbrev. It clears all preferences !
Comment 52 Björn Nylén 2022-10-18 12:21:04 UTC
(In reply to Marcel de Rooy from comment #51)
> This was a very BAD dbrev. It clears all preferences !

Sorry about that one... I'm certain I tested the sql before. :blush:
Comment 53 Katrin Fischer 2022-10-18 13:44:20 UTC
(In reply to Björn Nylén from comment #52)
> (In reply to Marcel de Rooy from comment #51)
> > This was a very BAD dbrev. It clears all preferences !
> 
> Sorry about that one... I'm certain I tested the sql before. :blush:

Don't worry about it, we all broke something before.
Comment 54 Michael Hafen 2022-11-02 18:16:55 UTC
Created attachment 143025 [details] [review]
Bug 14783: (follow-up) MySQL doesn't like selecting from a table it is updating

I have a problem with the db_rev.  MySQL doesn't allow selecting from a table you are updating (which has tripped me up before too).  MySQL error message:

ERROR 1093 (HY000): You can't specify target table 'systempreferences' for update in FROM clause


The value needs to be selected separately.
Comment 55 Katrin Fischer 2022-11-25 14:03:03 UTC
Thank you for the release notes, Björn!
Comment 56 Katrin Fischer 2022-12-12 06:46:28 UTC
(In reply to Michael Hafen from comment #54)
> Created attachment 143025 [details] [review] [review]
> Bug 14783: (follow-up) MySQL doesn't like selecting from a table it is
> updating
> 
> I have a problem with the db_rev.  MySQL doesn't allow selecting from a
> table you are updating (which has tripped me up before too).  MySQL error
> message:
> 
> ERROR 1093 (HY000): You can't specify target table 'systempreferences' for
> update in FROM clause
> 
> 
> The value needs to be selected separately.

It looks like the follow-up might not completely fix the issue. Maybe the tables need to be named differently? s1 and s2?

See report on the mailing list:
https://lists.katipo.co.nz/pipermail/koha/2022-December/058794.html
Comment 57 Michael Hafen 2022-12-12 19:25:04 UTC
Is the Koha code from that email using the patch from my follow-up?  I don't think the follow-up has been applied to the release yet (maybe I should have changed the status when I posted that patch).
Comment 58 Katrin Fischer 2022-12-12 23:33:00 UTC
(In reply to Michael Hafen from comment #57)
> Is the Koha code from that email using the patch from my follow-up?  I don't
> think the follow-up has been applied to the release yet (maybe I should have
> changed the status when I posted that patch).

It was pushed, but no comment was left here:
https://git.koha-community.org/Koha-community/Koha/commit/ab1e0a5d6ad2eb31e4322ec11a1e971386144f5d
Comment 59 Michael Hafen 2022-12-12 23:37:32 UTC
Interesting.  But that's a different follow-up that isn't in the patch list here.  That's not my follow-up to make MySQL work.
Comment 60 Katrin Fischer 2022-12-12 23:40:50 UTC
You are correct, so we might have the fix on oour hands here then!
Comment 61 Tomás Cohen Arazi 2022-12-13 11:40:40 UTC
The only way for a late follow-up to be noticed, is by adding the 'additional_work_needed' keyword.

In this case, we noticed the breakage and talked about it on IRC, so I added an RM follow-up. But missed the follow-up added a few days later.
Comment 62 Tomás Cohen Arazi 2022-12-13 11:42:39 UTC
(In reply to Michael Hafen from comment #57)
> Is the Koha code from that email using the patch from my follow-up?  I don't
> think the follow-up has been applied to the release yet (maybe I should have
> changed the status when I posted that patch).

Michael, can you please post your follow-up on a new bug that depends on this one? So it has the required QA review? It doesn't apply right now.

Thanks!
Comment 63 Martin Renvoize 2022-12-16 15:51:11 UTC
We just got stung by this DB update too.. 
I'll schedule a bit more of a deep dive for Monday
Comment 64 Victor Zuniga 2023-01-11 06:20:11 UTC
Hello,

Is there a workaround for this issue? I ran into this issue when updating the structure of the database via the web installer. It's not letting me complete the installation.

Victor
Comment 65 Jonathan Druart 2023-01-11 07:51:02 UTC
(In reply to Victor Zuniga from comment #64)
> Hello,
> 
> Is there a workaround for this issue? I ran into this issue when updating
> the structure of the database via the web installer. It's not letting me
> complete the installation.
> 
> Victor

Hi Victor, see bug 32470. The fix is available in the new release of 21.11.x but the debian packages are not ready yet. They should be published very soon!
Comment 66 Victor Zuniga 2023-01-12 00:41:27 UTC
Hi Jonathan,

Thanks so much for the update! I just upgraded and was able to complete the DB schema upgrade.

We are up and running.

Best,

Victor


(In reply to Jonathan Druart from comment #65)
> (In reply to Victor Zuniga from comment #64)
> > Hello,
> > 
> > Is there a workaround for this issue? I ran into this issue when updating
> > the structure of the database via the web installer. It's not letting me
> > complete the installation.
> > 
> > Victor
> 
> Hi Victor, see bug 32470. The fix is available in the new release of 21.11.x
> but the debian packages are not ready yet. They should be published very
> soon!
Comment 67 Fridolin Somers 2023-05-03 20:16:57 UTC
I add dependancy with Bug 30500 because it deletes the preference 'OPACInTransitHoldPickupLocationChange' this bug added