Bug 34063 - items.paidfor and deleteditems.paidfor still sometimes exist
Summary: items.paidfor and deleteditems.paidfor still sometimes exist
Status: RESOLVED INVALID
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Bugs List
QA Contact: Testopia
URL:
Keywords:
Depends on: 26268
Blocks:
  Show dependency treegraph
 
Reported: 2023-06-20 01:14 UTC by David Cook
Modified: 2024-02-20 11:36 UTC (History)
6 users (show)

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


Attachments
Bug 34063: Add system information warning (8.18 KB, patch)
2024-02-07 15:33 UTC, Pedro Amorim
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description David Cook 2023-06-20 01:14:05 UTC
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.
Comment 1 Jonathan Druart 2023-06-20 06:34:09 UTC
What do you suggest? A blocking db rev?
Comment 2 David Cook 2023-06-20 23:32:00 UTC
(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
Comment 3 Katrin Fischer 2023-06-22 19:23:28 UTC
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.
Comment 4 Jonathan Druart 2023-07-04 15:13:20 UTC
The easiest way forward is to simply drop the column.
Comment 5 David Cook 2023-07-05 03:32:54 UTC
(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...
Comment 6 David Cook 2023-07-05 03:34:44 UTC
(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...
Comment 7 David Cook 2023-07-05 03:41:06 UTC
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.
Comment 8 David Cook 2023-07-05 03:55:19 UTC
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...
Comment 9 David Cook 2023-07-05 04:12:33 UTC
For what it's worth, I'm locally going to drop it.
Comment 10 Pedro Amorim 2024-02-07 15:33:54 UTC
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
Comment 11 Pedro Amorim 2024-02-07 15:38:45 UTC
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!
Comment 12 David Cook 2024-02-07 22:34:16 UTC
(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.
Comment 13 Martin Renvoize 2024-02-07 22:58:27 UTC
Ooh, I'd totally forgotten about that audit script, that's an awesome suggestion in my opinion.
Comment 14 David Cook 2024-02-07 23:14:59 UTC
(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.
Comment 15 Katrin Fischer 2024-02-08 10:49:53 UTC
(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.
Comment 16 Pedro Amorim 2024-02-08 11:47:26 UTC
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.
Comment 17 David Cook 2024-02-08 23:44:42 UTC
(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
Comment 18 Pedro Amorim 2024-02-20 11:36:59 UTC
Closing this bug as what it's offering should be satisfied by bug 36039