Some early 23.11 adopters have spotted that the rooms reservations plugin already adds a 'bookings' table (without any form of table qualification which is modern best practice). We'll want to try and identify when this is the case and migrate the table. I'll contact the authors for the original plugin to discuss the next steps.. but there's lots of forks of this particular plugin so we'll want to somehow highlight issues to end users too.
The plugin has now been updated to prevent the conflict, however we still need to fix the database update
Created attachment 160827 [details] [review] Bug 35473: Handle room reservations plugin conflict We now look for the existance of the room reservations 'bookings' table prior to attempting to install the core bookings table and migrate it in the same way the upstream plugin does before proceeding with installing the core bookings tables.
Kyle.. I think we'll need an additional check in the plugin side to look for whether the table has already been migrated prior to upgrade (i.e check that the bookings table has booking_id vs bookingid column.) I'm also tempted to add a 'disable' call into here to disable the room reservations plugin if found to ensure people go and upgrade it prior trying to use it again.
Created attachment 160879 [details] [review] Bug 35473: Handle room reservations plugin conflict We now look for the existance of the room reservations 'bookings' table prior to attempting to install the core bookings table and migrate it in the same way the upstream plugin does before proceeding with installing the core bookings tables. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
(In reply to Martin Renvoize from comment #3) > Kyle.. I think we'll need an additional check in the plugin side to look for > whether the table has already been migrated prior to upgrade (i.e check that > the bookings table has booking_id vs bookingid column.) > > I'm also tempted to add a 'disable' call into here to disable the room > reservations plugin if found to ensure people go and upgrade it prior trying > to use it again. Sounds like a good plan to me!
Created attachment 161043 [details] [review] Bug 35473: Also disable the plugin
*** Bug 35697 has been marked as a duplicate of this bug. ***
WARN installer/data/mysql/db_revs/230600061.pl WARN tidiness The file is less tidy than before (bad/messy lines before: 0, now: 11)
Created attachment 161742 [details] [review] Bug 35473: Handle room reservations plugin conflict We now look for the existance of the room reservations 'bookings' table prior to attempting to install the core bookings table and migrate it in the same way the upstream plugin does before proceeding with installing the core bookings tables. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 161743 [details] [review] Bug 35473: Also disable the plugin
Tidied.. again.
I'm getting a bit tired of a tiny tidy requirement simply because master moved stopping a bug in it's tracks :(
This is a pretty darn critical bug for upgrades that can completely break Koha.. amazing it doesn't get any attention
(In reply to Martin Renvoize from comment #13) > This is a pretty darn critical bug for upgrades that can completely break > Koha.. amazing it doesn't get any attention Will have a look now. But you know about 34478 ;)
This is an interesting patch. I tend to think that we should not touch the tables of a plugin. And just abort here? But that may be a bit too formal response? I like to have bit more feedback from QA. Moving to ID and sending a mail. Do we choose for Pragmatic here and just move the tables, or do we choose to just bail out and let the plugin stuff be handled outside Koha? Note that the question also arises: What should we do for other plugins in the future that created tables in Koha and clash? I would normally not expect Koha to contain knowledge about plugin tables?
(In reply to Marcel de Rooy from comment #15) > Note that the question also arises: What should we do for other plugins in > the future that created tables in Koha and clash? I would normally not > expect Koha to contain knowledge about plugin tables? I think we should try to avoid using table names that are already used by known plugins, but if that happens (like in this bug) I think it's the plugin responsibility to move its own data. Now what should Koha do ? Stop the update process, and if the plugin is known add a message suggesting to update the plugin first ? Can the plugin update be done in the middle of the updatedb process ?
On bug 30650 (Curbside pickups) I took care of the plugin's data (the idea was to provide the same feature as the plugin). The patch created the same tables as the plugin, then adjusted the structure to reach the new one.
Hmm, it does raise a fiar question. In this case I worked with the plugin maintainers to get both sides of the equation done.. so, with this patch we don't just explode badly and you cna upgrade the plugin either before or after the upgrade and it works. Withouth this patch, if you don't happen to know you need to upgrade the plugin first, you'll end up with a nasty silent failure during upgrade which, without some code that's very much aware of the plugin table layout isn't at all easy to spot. I'm open to suggestions of course, but I couldn't see an alternative.
Plugins should always use: my $table_name = $self->get_qualified_table_name('name_you_want');
Do we have a proposed path forward here.. as it stands Koha just breaks silently at upgrade if room reservations was installed..
(In reply to Martin Renvoize from comment #20) > Do we have a proposed path forward here.. as it stands Koha just breaks > silently at upgrade if room reservations was installed.. IMO Koha continuing to function should trump all other things.
I think we have the best possible outcome here that will allow libraries to fix the issue without losing data.
Created attachment 163159 [details] [review] Bug 35473: Handle room reservations plugin conflict We now look for the existance of the room reservations 'bookings' table prior to attempting to install the core bookings table and migrate it in the same way the upstream plugin does before proceeding with installing the core bookings tables. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 163160 [details] [review] Bug 35473: Also disable the plugin Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
(In reply to Katrin Fischer from comment #22) > I think we have the best possible outcome here that will allow libraries to > fix the issue without losing data. No, I would not completely agree here. Certainly not the best possible outcome. I tend to allow it but it creates precedence for mixing specific plugin details and our codebase. Which is actually not nice. Theoretically, we should just abort the upgrade and leave plugin fixing to plugin support. But feeling the pressure (..) we might make yet another exception here :) Passing QA QA team: Please dont make such db revs. And dont allow them in the future ;)
(In reply to Kyle M Hall from comment #21) > IMO Koha continuing to function should trump all other things. The verb trump makes me think of someone :)
I believe the thinking was that if you are in the middle of an upgrade already, it's easier to make it continue and fix the plugin later from GUI. I can see that fixing "before" update could be another route. I DO hope that we won't run into more issues like this, but one can only hope. Should we adjust our checks for existing tables somehow?
Pushed for 24.05! Well done everyone, thank you!
Pushed to 23.11.x for 23.11.04
Not needed in 23.05.x, no backport.