Bug 26268 only drops the column items.paidfor if the column is empty. If there is a value, a warning message is displayed, but the database upgrade completes successfully. So you may end up with 47 columns in your items table in Koha 22.11 while the deleteditems table only has 46 columns. This will cause scripts like /cgi-bin/koha/reports/issues_stats.pl to bust with a 500 error since it can't do a * union on those two tables with a different number of columns.
What do you suggest? A blocking db rev?
(In reply to Jonathan Druart from comment #1) > What do you suggest? A blocking db rev? I'm not sure yet. A blocking db rev would work well if upgrading 1 Koha schema at a time, but probably wouldn't work well when upgrading all Koha schemas via a "apt install koha-common". One option would be to move the data from items.paidfor to somewhere else and then drop the column, but I don't think we have an easy option for that. I've just started to manually go through these databases, and so far I'm putting "items.paidfor" into non-public notes, but if the item already has a non-public note that probably won't work very well. For now, I think my suggestion is to show the problems, so that people can remedy them, and that's why I raised bug 34064
Maybe the easier way forward would be to bring back deleted.itemspaidfor, if * there is still an items.paidfor * items.paidfor still has data If items.paidfor has no data, we could delete it.
The easiest way forward is to simply drop the column.
(In reply to Jonathan Druart from comment #4) > The easiest way forward is to simply drop the column. True but that would lead to data loss. I'm scanning through databases now and most only have data in single/double digit numbers of items, but I see a few that have thousands of rows of data...
(In reply to David Cook from comment #5) > (In reply to Jonathan Druart from comment #4) > > The easiest way forward is to simply drop the column. > > True but that would lead to data loss. > > I'm scanning through databases now and most only have data in single/double > digit numbers of items, but I see a few that have thousands of rows of > data... That said, as I look through it, it mostly looks like garbage data...
Actually you know what... while the data might be in the database, I don't see a place where it gets surfaced, so for all intents and purposes... it doesn't really exist. So maybe we do just drop it.
Of course, as soon as I do that, I notice that some databases also still have deleteditems.paidfor... but some don't. Think we just need to re-blitz paidfor across both tables...
For what it's worth, I'm locally going to drop it.
Created attachment 161816 [details] [review] Bug 34063: Add system information warning If the table columns differ, we should at least provide a warning in the system information tab of the about page. Test plan: 1) Run the following SQL: alter table deleteditems add column paidfor longtext; 2) visit the issue stats page: /cgi-bin/koha/reports/issues_stats.pl 3) press 'Submit' 4) Notice it causes a 500 error. 5) Apply patch 6) Visit the about page and check the 'System information' tab 7) Notice the warning is there. 8) fix the issue, run alter table deleteditems drop column paidfor; 9) Repeat steps 2+3 confirm it no longer errors 10) Repeat step 6 and notice there's no longer a warning
Imo this issue needs to be handled by sys admins on systems that require it fixed. I think it's very unlikely to have data in this column, and even more unlikely for this data to be actually real and utilized data, however just dropping the column entirely would open a precedent and at the end of the day, someone may in fact be using it for some odd reason. As a compromise I've submitted a patch that shows a warning in the about -> system information page to help guide sys admins in fixing it. This is just an idea, let me know if it makes sense or follow up if it you think it can be improved!
(In reply to Pedro Amorim from comment #11) > Imo this issue needs to be handled by sys admins on systems that require it > fixed. > I think it's very unlikely to have data in this column, and even more > unlikely for this data to be actually real and utilized data, however just > dropping the column entirely would open a precedent and at the end of the > day, someone may in fact be using it for some odd reason. > > As a compromise I've submitted a patch that shows a warning in the about -> > system information page to help guide sys admins in fixing it. This is just > an idea, let me know if it makes sense or follow up if it you think it can > be improved! Yeah it's a tough one. The sad thing is that there's lots of possible discrepancies in the Koha database, especially for ones that have been upgraded many times over the years. That was my motivation for bug 34064 which adds an "audit_database.pl" script to Koha 23.11. I wonder if we could couple audit_database.pl with about.pl indirectly. Like a weekly cronjob that audits the database behind the scenes and updates something that about.pl could check/display without having to do the calculations itself in real time.
Ooh, I'd totally forgotten about that audit script, that's an awesome suggestion in my opinion.
(In reply to Martin Renvoize from comment #13) > Ooh, I'd totally forgotten about that audit script, that's an awesome > suggestion in my opinion. <3 I think we'd probably want to tweak it, or even do something different that uses SQL::Translator::Diff->compute_differences() without the produce_diff_sql(). It's been a little while since I've looked at the script (been so busy), but that could be interesting.
(In reply to Pedro Amorim from comment #11) > Imo this issue needs to be handled by sys admins on systems that require it > fixed. > I think it's very unlikely to have data in this column, and even more > unlikely for this data to be actually real and utilized data, however just > dropping the column entirely would open a precedent and at the end of the > day, someone may in fact be using it for some odd reason. There is some history there that might be worthwhile knowing: items.paidfor for quite some while was mapped by default in new installations to one of the item note fields (I don't remember if it was public or non public). IIRC we were not able to do a clean migration when we introduced a new column to fix that. So it's not super unlikely to have data especially for very old installations. > As a compromise I've submitted a patch that shows a warning in the about -> > system information page to help guide sys admins in fixing it. This is just > an idea, let me know if it makes sense or follow up if it you think it can > be improved! I like the direction :) I'd also like something that doesn't "stop" the update process, but can be handled after an update separately. So that sounds good.
Thanks for bringing bug 34064 to my attention, I was not aware of it. This is better than my patch as it detects all possible issues, not just a single one. In my opinion, the audit_database.pl is too valuable to just be a CLI script. My suggestions here: 1) Drop my patch as audit_database.pl will already catches it, and much more. 2) Consider running audit_database.pl and having its report viewable in the "about" page (maybe somewhere else, not sure). Performance implications of the "about" page could be alleviated by the work I just submitted to bug 32693.
(In reply to Pedro Amorim from comment #16) > 2) Consider running audit_database.pl and having its report viewable in the > "about" page (maybe somewhere else, not sure). Performance implications of > the "about" page could be alleviated by the work I just submitted to bug > 32693. +1
Closing this bug as what it's offering should be satisfied by bug 36039