I like the idea of making it a timestamp instead of a boolean as suggested on other bugs (I think last discussed for recalls on bug 19532). Are there any reasons why one should be preferred over the other, for example for performance?

> Are there any reasons why one should be preferred over the other, for example for performance?

I dont't think so. As long as the column is indexed, querying for one or the other should perform with equivalent speed.

The benefit of using a timestamp is that it carries more information, and mirrors the current behaviour, where the timestamp in the deleted-tables denotes when the row was deleted.

I've started working on this and have migrated items, biblios, biblioitems and biblio_metadata successfully. it works fine on intra and seems ok in the administrative reports.

there is also a wiki page with more info here: https://wiki.koha-community.org/wiki/MergingOfTables

Created attachment 72300 [details] [review]
Bug 20271: merging delete biblio/items

NOTE: deleteditems/biblio on OAI
not sure how to test this, but the changes should be good enough

Comment on attachment 72300 [details] [review]
Bug 20271: merging delete biblio/items

Review of attachment 72300 [details] [review]:
-----------------------------------------------------------------

Just a quick look:

there is missing atomic database update

also the test suite should be adjusted

I am not sure about the changes in OAI, so need to investigate more later

::: C4/Biblio.pm
@@ -3182,5 @@
> -        my $bkup_sth = $dbh->prepare($query);
> -        $bkup_sth->execute(@bind);
> -        $bkup_sth->finish;
> -
> -        _koha_delete_biblio_metadata( $biblionumber );

this is missing in your patch

::: C4/Items.pm
@@ +1061,4 @@
>       FROM items
>       LEFT JOIN branches AS holding ON items.holdingbranch = holding.branchcode
>       LEFT JOIN branches AS home ON items.homebranch=home.branchcode
> +     LEFT JOIN biblio      ON      biblio.biblionumber     = items.biblionumber AND biblio.deleted_at IS NULL

I don't think this will work, that's condition should be in where clause

::: misc/export_records.pl
@@ +105,5 @@
>          push @record_ids, $_->{biblionumber} for @{
> +            $dbh->selectall_arrayref(q|
> +                SELECT biblionumber
> +                FROM biblioitems
> +             |, { Slice => {} }, ( $timestamp ) x 4 );

Please include the changes made by Bug 19730

(In reply to Josef Moravec from comment #5)
> there is missing atomic database update

It is on its way.

(It's fairly easy to do something reasonable, but there are some corner cases
which I would like to handle somehow better)

I would left the deleted* tables there with any clashing rows and let the
administrator deal with it in case there are problems. thoughts?


> I am not sure about the changes in OAI, so need to investigate more later

Thank you so much, I wish I knew more about OAI

> ::: C4/Biblio.pm
> @@ -3182,5 @@
> > -        my $bkup_sth = $dbh->prepare($query);
> > -        $bkup_sth->execute(@bind);
> > -        $bkup_sth->finish;
> > -
> > -        _koha_delete_biblio_metadata( $biblionumber );
> 
> this is missing in your patch

I might be lagging behind master, because I can't find it.

> ::: C4/Items.pm
> @@ +1061,4 @@
> >       FROM items
> >       LEFT JOIN branches AS holding ON items.holdingbranch = holding.branchcode
> >       LEFT JOIN branches AS home ON items.homebranch=home.branchcode
> > +     LEFT JOIN biblio      ON      biblio.biblionumber     = items.biblionumber AND biblio.deleted_at IS NULL
> 
> I don't think this will work, that's condition should be in where clause

I actually think it will be irrelevant. The FKs should make sure that if there is an item, there must be the biblio which it refers.

Or am I missing something?


> ::: misc/export_records.pl
> @@ +105,5 @@
> >          push @record_ids, $_->{biblionumber} for @{
> > +            $dbh->selectall_arrayref(q|
> > +                SELECT biblionumber
> > +                FROM biblioitems
> > +             |, { Slice => {} }, ( $timestamp ) x 4 );
> 
> Please include the changes made by Bug 19730

Is it in master, right?

TIA

I think we should try and provide at least a script to automate merging the two tables. You cannot assume that most admins know Koha well enough to fix the issues themselves. A problem might be the possible existence of duplicated rows. We could check that and then suggest a fix before continuing.

(In reply to Katrin Fischer from comment #7)
> I think we should try and provide at least a script to automate merging the
> two tables. You cannot assume that most admins know Koha well enough to fix
> the issues themselves. A problem might be the possible existence of
> duplicated rows. We could check that and then suggest a fix before
> continuing.

My idea is to move all the rows properly.

if any rows clash (for some unforeseen reason, i found a "orphaned" item referring to a non-existent biblio here, but can be anything!) then they won't be moved.

at the end, if the table is empty, it gets removed, otherwise it stays there.

does it sounds good?

Created attachment 72799 [details] [review]
Bug 20271: merging delete biblio/items

NOTE: deleteditems/biblio on OAI
not sure how to test this, but the changes should be good enough

Created attachment 72800 [details] [review]
Bug 20271 - merge deletedbiblio and -items back - add atomic update

Updated the failing tests, except for the weird GetItemsForInventory test which needs another look.

Ready for testing, we renamed the ticket to only concern tables deletedbiblio, deletedbiblioitems, deletedbiblio_metadata and deleteditems, since they are most crucial to avoid more data loss and corruption.

Suggest to rename deleted* tables temporarily before deleting them, so any merge conflicts can be examined first

Also suggest to run misc/devel/update_dbix_class_files.pl after, so schema/apis etc work as expected

Just a random thought to throw into this: would it be worth creating database views to replicate the old tables based on the new ones?  That way if any installations have pre-existing reports that use the old tables in their queries, they will still continue to work.

For example something like this:

CREATE VIEW old_issues AS SELECT * FROM issues WHERE deleted_at IS NOT NULL;

(In reply to Jon Knight from comment #12)
> Just a random thought to throw into this: would it be worth creating
> database views to replicate the old tables based on the new ones?  That way
> if any installations have pre-existing reports that use the old tables in
> their queries, they will still continue to work.
> 
> For example something like this:
> 
> CREATE VIEW old_issues AS SELECT * FROM issues WHERE deleted_at IS NOT NULL;

I thought about it, but you most likely need the view for "issues" than "old_issues", which means:

RENAME TABLE issues TO all_issues;
CREATE VIEW issues AS SELECT * FROM issues WHERE delted_at IS NULL;

but then, more code will need to be changed for updates/insert/deletes

OTOH, most reports UNION issues with old_issues, might be smart to take this opportunity to simplify them?

(note, this applies to deleteditems/biblios as well)

Created attachment 73091 [details] [review]
Bug 20271: merging delete biblio/items

NOTE: deleteditems/biblio on OAI
not sure how to test this, but the changes should be good enough

Created attachment 73092 [details] [review]
Bug 20271 - merge deletedbiblio and -items back - add atomic update

Created attachment 73093 [details] [review]
Bug 20271: fix remaining tests, add checks and report in atomicupdate

Ready for testing now, test plan:

1) make sure you have some deleted biblios and items
2) apply patch and run updatedatabase
3) if you have any clashing keys, they should be reported during updatedatabase and be left in renamed corresponding tables with _ (e.g. _deleteditems)
4) run update_dbix_class_files.pl (to make sure schemas and rest api are ok)
5) check that db_dependent tests concerning biblios and items pass OK
6) check that you can still delete items and biblio
7) for any deleted item or biblio, you should now have a timestamp in the 'deleted_at' column.

raised the importance to critical since it does in fact lead to data loss and possibly data corruption

thanks Benjamin!

(In reply to Benjamin Rokseth from comment #16)
> Created attachment 73093 [details] [review] [review]
> Bug 20271: fix remaining tests, add checks and report in atomicupdate

I think the OAI-PMH provider should now be changed to return all records in a single pass. It was built to return deleted records separately because that was much faster than joining tables on the fly, but when deleted and non-deleted records are in the same table this complication is not needed anymore. I think the whole ListBase class could be removed in this case.

Please let me know if you'd rather let me do the relevant changes in the OAI classes.

One more note: I couldn't find any changes to kohastructure.sql. Are they missing from the patches or did I just miss them?

And one more: there are still comments that refer to the deleted* tables.

Sorry for the comment spam... one more: the call to _koha_delete_biblio_metadata has been removed from _koha_delete_biblio. As far as I can see nothing takes care of marking the metadata record deleted.

Created attachment 73535 [details] [review]
Bug 20271: merging delete biblio/items

NOTE: deleteditems/biblio on OAI
not sure how to test this, but the changes should be good enough

Created attachment 73536 [details] [review]
Bug 20271 - merge deletedbiblio and -items back - add atomic update

Created attachment 73537 [details] [review]
Bug 20271: fix remaining tests, add checks and report in atomicupdate

Created attachment 73538 [details] [review]
Bug 20271: update kohastructure.sql

Created attachment 73539 [details] [review]
Bug 20271: delete biblio and biblio_metadata in one go

Created attachment 73540 [details] [review]
Bug 20271: remove comments regarding deleteditems and -biblio*

rebased against master and added fixes to comments except OAI.

Ere, since you voluntered to add the neccessary fixes to OAI, I leave them to you ;)

I'm working on it now. 


Looks like there may be a typo in the atomic update. I got the following error:

updatedatabase.pl: DBD::mysql::db do failed: Can't DROP FOREIGN KEY `deletedbiblio_metadata_fk_1`; check that it exists [for Statement "ALTER TABLE deletedbiblio_metadata DROP FOREIGN KEY deletedbiblio_metadata_fk_1"] at (eval 1389) line 8.

Looking at the old kohastructure.sql I believe it should have tried to drop 'deletedrecord_metadata_fk_1'.

(In reply to Ere Maijala from comment #31)
> I'm working on it now. 
> 
> 
> Looks like there may be a typo in the atomic update. I got the following
> error:
> 
> updatedatabase.pl: DBD::mysql::db do failed: Can't DROP FOREIGN KEY
> `deletedbiblio_metadata_fk_1`; check that it exists [for Statement "ALTER
> TABLE deletedbiblio_metadata DROP FOREIGN KEY deletedbiblio_metadata_fk_1"]
> at (eval 1389) line 8.
> 
> Looking at the old kohastructure.sql I believe it should have tried to drop
> 'deletedrecord_metadata_fk_1'.

hmm, sad to say, but no, it's not a typo but db design issues.
this commit seems to have added to the confusion: https://github.com/Koha-Community/Koha/commit/739e2e0c5dc26502e195e2cc4c687ababdcaf381 (Bug 17196)

and following Bug 18284 you can see that most agree on deleted* tables being a bad idea in the first place.

Problem is, kohastructure.sql and updatedatabase.pl is not in sync and libraries having migrated from 16.05 and before would need to delete this foreign key. And this patch is most important for them anyways.

I suppose we could add a delete foreign key for both and ignore errors rather than die in the final version

Oh, that's a nasty one. But do you even have to drop the constraints to delete the table? I think it's enough to drop the table and all the constraints there would be dropped too, no?

Created attachment 73597 [details] [review]
Bug 20271 - Use a single pass in the OAI-PMH provider

Changes for the OAI-PMH provider have been attached. I'm really happy to make it so much more simple. Please feel free to rebase or whatever is necessary.

(In reply to Benjamin Rokseth from comment #32)
> (In reply to Ere Maijala from comment #31)
> > I'm working on it now. 
> > 
> > 
> > Looks like there may be a typo in the atomic update. I got the following
> > error:
> > 
> > updatedatabase.pl: DBD::mysql::db do failed: Can't DROP FOREIGN KEY
> > `deletedbiblio_metadata_fk_1`; check that it exists [for Statement "ALTER
> > TABLE deletedbiblio_metadata DROP FOREIGN KEY deletedbiblio_metadata_fk_1"]
> > at (eval 1389) line 8.
> > 
> > Looking at the old kohastructure.sql I believe it should have tried to drop
> > 'deletedrecord_metadata_fk_1'.
> 
> hmm, sad to say, but no, it's not a typo but db design issues.
> this commit seems to have added to the confusion:
> https://github.com/Koha-Community/Koha/commit/
> 739e2e0c5dc26502e195e2cc4c687ababdcaf381 (Bug 17196)
> 
> and following Bug 18284 you can see that most agree on deleted* tables being
> a bad idea in the first place.
> 
> Problem is, kohastructure.sql and updatedatabase.pl is not in sync and
> libraries having migrated from 16.05 and before would need to delete this
> foreign key. And this patch is most important for them anyways.
> 
> I suppose we could add a delete foreign key for both and ignore errors
> rather than die in the final version

fail to remember, but probably I put it there because otherwise it complained, perhaps due to the "rename table". the idea was that any leftovers from conflicts should be moved to temporary tables (prepended with _) so that one could check and delete them at convenience

I will remove the dies in the atomicupdate and see if it works for us

(In reply to Benjamin Rokseth from comment #36)
> fail to remember, but probably I put it there because otherwise it
> complained, perhaps due to the "rename table". the idea was that any
> leftovers from conflicts should be moved to temporary tables (prepended with
> _) so that one could check and delete them at convenience

We had to drop the constraint to avoid the delete on cascade. When we clean up the deletedbiblio andt able, we don't want to drop any items which were left behind because not properly merged.

Ok, fair enough. I think Benjamin's proposal of dropping both without die'ing makes sense.

Created attachment 73759 [details] [review]
Bug 20271: merging delete biblio/items

NOTE: deleteditems/biblio on OAI
not sure how to test this, but the changes should be good enough

Created attachment 73760 [details] [review]
Bug 20271 - merge deletedbiblio and -items back - add atomic update

Created attachment 73761 [details] [review]
Bug 20271: fix remaining tests

Created attachment 73762 [details] [review]
Bug 20271: update kohastructure.sql

Created attachment 73763 [details] [review]
Bug 20271: delete biblio and biblio_metadata in one go

Created attachment 73764 [details] [review]
Bug 20271: remove comments regarding deleteditems and -biblio*

Created attachment 73765 [details] [review]
Bug 20271 - Use a single pass in the OAI-PMH provider

Replaced die with warn in atomicupdate, and rebased and squashed a bit.
Should be ready for testing now

Comment on attachment 73537 [details] [review]
Bug 20271: fix remaining tests, add checks and report in atomicupdate

Forgot to obsolete this

Created attachment 74127 [details] [review]
Bug 20271: merging delete biblio/items

The present bug merges the following tables:
deletedbiblio          -> biblio
deletedbiblioitems     -> biblioitems
deletedbiblio_metadata -> biblio_metadata
deleteditems           -> items

and adds a column deleted_at to signify time of deletion, if deleted

it replaces all occurrences of the mentioned occurences and uses
of mentioned deleted* tables

upgrade is handled by moving rows from deleted* tables to the live
ones. If all rows are moved, the corresponding tables are dropped.
If there are duplicates or conflicts (duplicate barcodes e.g.) they
reminders are left for scrutiny and table renamed to _deleted*

Test plan:

1) Make sure you have a db with at least a few deleted biblios,
   biblioitems and items.
2) Apply patch and run updatedatabase.pl
3) Make note of the db upgrade messages, if you get a message like:
   There were x deleteditems that could not be moved, please check _deleteditems
   you should check the _deleteditems table and verify that they are
   actual conflicts
4) Click around in the web interface and make sure you can delete items
   and biblios
5) "Undelete" an item by NULLing the deleted_at column for an item
   and verify that it returns in interface (Not neccessary really)

Created attachment 74128 [details] [review]
Bug 20271: fix remaining tests

Created attachment 74129 [details] [review]
Bug 20271: update kohastructure.sql

Created attachment 74130 [details] [review]
Bug 20271: delete biblio and biblio_metadata in one go

Created attachment 74131 [details] [review]
Bug 20271: remove comments regarding deleteditems and -biblio*

Created attachment 74132 [details] [review]
Bug 20271: add atomic update

Created attachment 74133 [details] [review]
Bug 20271: Use a single pass in the OAI-PMH provider

Created attachment 74134 [details] [review]
Bug 20271: fix whitespace issues in C4::Items

Created attachment 74135 [details] [review]
Bug 20271: merging delete biblio/items

The present bug merges the following tables:
deletedbiblio          -> biblio
deletedbiblioitems     -> biblioitems
deletedbiblio_metadata -> biblio_metadata
deleteditems           -> items

and adds a column deleted_at to signify time of deletion, if deleted

it replaces all occurrences of the mentioned occurences and uses
of mentioned deleted* tables

upgrade is handled by moving rows from deleted* tables to the live
ones. If all rows are moved, the corresponding tables are dropped.
If there are duplicates or conflicts (duplicate barcodes e.g.) they
reminders are left for scrutiny and table renamed to _deleted*

Test plan:

1) Make sure you have a db with at least a few deleted biblios,
   biblioitems and items.
2) Apply patch and run updatedatabase.pl
3) Make note of the db upgrade messages, if you get a message like:
   There were x deleteditems that could not be moved, please check _deleteditems
   you should check the _deleteditems table and verify that they are
   actual conflicts
4) Click around in the web interface and make sure you can delete items
   and biblios
5) "Undelete" an item by NULLing the deleted_at column for an item
   and verify that it returns in interface (Not neccessary really)

Created attachment 74136 [details] [review]
Bug 20271: fix remaining tests

Created attachment 74137 [details] [review]
Bug 20271: update kohastructure.sql

Created attachment 74138 [details] [review]
Bug 20271: delete biblio and biblio_metadata in one go

Created attachment 74139 [details] [review]
Bug 20271: remove comments regarding deleteditems and -biblio*

Created attachment 74140 [details] [review]
Bug 20271: add atomic update

Created attachment 74141 [details] [review]
Bug 20271: Use a single pass in the OAI-PMH provider

Created attachment 74142 [details] [review]
Bug 20271: fix whitespace issues in C4::Items

Created attachment 74182 [details] [review]
Bug 20271: merging delete biblio/items

The present bug merges the following tables:
deletedbiblio          -> biblio
deletedbiblioitems     -> biblioitems
deletedbiblio_metadata -> biblio_metadata
deleteditems           -> items

and adds a column deleted_at to signify time of deletion, if deleted

it replaces all occurrences of the mentioned occurences and uses
of mentioned deleted* tables

upgrade is handled by moving rows from deleted* tables to the live
ones. If all rows are moved, the corresponding tables are dropped.
If there are duplicates or conflicts (duplicate barcodes e.g.) they
reminders are left for scrutiny and table renamed to _deleted*

Test plan:

1) Make sure you have a db with at least a few deleted biblios,
   biblioitems and items.
2) Apply patch and run updatedatabase.pl
3) Make note of the db upgrade messages, if you get a message like:
   There were x deleteditems that could not be moved, please check _deleteditems
   you should check the _deleteditems table and verify that they are
   actual conflicts
4) Click around in the web interface and make sure you can delete items
   and biblios
5) "Undelete" an item by NULLing the deleted_at column for an item
   and verify that it returns in interface (Not neccessary really)

Created attachment 74183 [details] [review]
Bug 20271: fix remaining tests

Created attachment 74184 [details] [review]
Bug 20271: update kohastructure.sql

Created attachment 74185 [details] [review]
Bug 20271: delete biblio and biblio_metadata in one go

Created attachment 74186 [details] [review]
Bug 20271: remove comments regarding deleteditems and -biblio*

Created attachment 74187 [details] [review]
Bug 20271: add atomic update

Bug 20271: disable foreign keys on deletedbiblio_metadata

Created attachment 74188 [details] [review]
Bug 20271: Use a single pass in the OAI-PMH provider

Created attachment 74189 [details] [review]
Bug 20271: fix whitespace issues in C4::Items

Created attachment 74190 [details] [review]
Bug 20271: print instead of warn unmovable items

Created attachment 74223 [details] [review]
Bug 20271: atomicupdate - support mysql < 5.7 and address foreign key issues

Created attachment 74407 [details] [review]
Bug 20271: merging delete biblio/items

The present bug merges the following tables:
deletedbiblio          -> biblio
deletedbiblioitems     -> biblioitems
deletedbiblio_metadata -> biblio_metadata
deleteditems           -> items

and adds a column deleted_at to signify time of deletion, if deleted

it replaces all occurrences of the mentioned occurences and uses
of mentioned deleted* tables

upgrade is handled by moving rows from deleted* tables to the live
ones. If all rows are moved, the corresponding tables are dropped.
If there are duplicates or conflicts (duplicate barcodes e.g.) they
reminders are left for scrutiny and table renamed to _deleted*

Test plan:

1) Make sure you have a db with at least a few deleted biblios,
   biblioitems and items.
2) Apply patch and run updatedatabase.pl
3) Make note of the db upgrade messages, if you get a message like:
   There were x deleteditems that could not be moved, please check _deleteditems
   you should check the _deleteditems table and verify that they are
   actual conflicts
4) Click around in the web interface and make sure you can delete items
   and biblios
5) "Undelete" an item by NULLing the deleted_at column for an item
   and verify that it returns in interface (Not neccessary really)

Signed-off-by: Brendan A Gallagher <brendan@bywatersolutions.com>
Worked on two different databases.

Created attachment 74408 [details] [review]
Bug 20271: fix remaining tests

Signed-off-by: Brendan A Gallagher <brendan@bywatersolutions.com>

Created attachment 74409 [details] [review]
Bug 20271: update kohastructure.sql

Signed-off-by: Brendan A Gallagher <brendan@bywatersolutions.com>

Created attachment 74410 [details] [review]
Bug 20271: delete biblio and biblio_metadata in one go

Signed-off-by: Brendan A Gallagher <brendan@bywatersolutions.com>

Created attachment 74411 [details] [review]
Bug 20271: remove comments regarding deleteditems and -biblio*

Signed-off-by: Brendan A Gallagher <brendan@bywatersolutions.com>

Created attachment 74412 [details] [review]
Bug 20271: add atomic update

Bug 20271: disable foreign keys on deletedbiblio_metadata

Signed-off-by: Brendan A Gallagher <brendan@bywatersolutions.com>

Created attachment 74413 [details] [review]
Bug 20271: Use a single pass in the OAI-PMH provider

Signed-off-by: Brendan A Gallagher <brendan@bywatersolutions.com>

Created attachment 74414 [details] [review]
Bug 20271: fix whitespace issues in C4::Items

Signed-off-by: Brendan A Gallagher <brendan@bywatersolutions.com>

Created attachment 74415 [details] [review]
Bug 20271: print instead of warn unmovable items

Signed-off-by: Brendan A Gallagher <brendan@bywatersolutions.com>

Created attachment 74416 [details] [review]
Bug 20271: atomicupdate - support mysql < 5.7 and address foreign key issues

Signed-off-by: Brendan A Gallagher <brendan@bywatersolutions.com>

Looking here now, but I feel that this patch set will not make it anymore to 18.05.

Created attachment 75265 [details] [review]
Bug 20271: (follow-up) DBIx schema changes

Great job, Francesco
I did not finish my QA now, but want to report some issues I found already. And they unfortunately confirm that this patch set will not make it anymore into 18.05 ;)
But it would be good to have in master early.

qa tools: FAIL   Koha/Patron.pm, FAIL   pod, Spurious text after =cut
deleted_at should probably be better: deleted_on; we also have updated_on etc.
$dbh->do( "ALTER TABLE biblio ADD COLUMN deleted_at datetime DEFAULT NULL" ) or warn $DBI::errstr;
Probably it warns already and you should die here ?
Running OAI/Server.t
ok 12 - use Koha::OAI::Server::ResumptionToken;
DBD::mysql::db do failed: Table 'koha_dev.deletedbiblio' doesn't exist [for Statement "DELETE FROM deletedbiblio"] at t/db_dependent/OAI/Server.t line 68.
DBD::mysql::db do failed: Table 'koha_dev.deletedbiblioitems' doesn't exist [for Statement "DELETE FROM deletedbiblioitems"] at t/db_dependent/OAI/Server.t line 69.
DBD::mysql::db do failed: Table 'koha_dev.deleteditems' doesn't exist [for Statement "DELETE FROM deleteditems"] at t/db_dependent/OAI/Server.t line 70.
git grep deletedbiblio
about.pl:        q|select b.biblionumber from biblio b join deletedbiblio db on b.biblionumber=db.biblionumber|,
koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/web_services.pref (a few other refs too)
git grep deleteditems
about.pl again
koha-tmpl/intranet-tmpl/prog/en/modules/reports/catalogue_stats.tt:                <td><input type="radio" name="Line" value="deleteditems.timestamp" /></td>
might not be a problem?
misc/export_records.pl:            WHERE deleteditems.biblionumber = ? AND deleted_at IS NOT NULL
does not look good
reports/catalogue_stats.pl:    $linefilter[0] = @$filters[15] if ( $line =~ /deleteditems\.timestamp/ );
we should probably get rid of those things; they are confusing now

I am not sure if we should mark this one as critical btw. I would go for major when viewing it as a bugfix. But enh could be argued too.
Not so sure if the RMaints want to backport this one?

Marcel, thanks for thorough feedback!
will address the issues asap so it gets into master sooner than later.
Downgraded to major, enhancement it is definitely not ;)

Created attachment 75347 [details] [review]
Bug 20271: (QA follow-up): rename to deleted_on and various fixes

- renamed deleted_at => deleted_on
- removed failing sql in t/db_dependent/OAI/Server.t
- removed deprecated autoincrement checks in about.pl
- fixed errors in catalogue_stats.tt and catalogue_stats.pl

Adjusted to all qa feedback, except:
- left warn instead of die in atomicupdate - it would be removed by RM when moved to updatedatabase.pl anyways I suppose
- qa tools: FAIL   Koha/Patron.pm, FAIL   pod, Spurious text after =cut
I could not reproduce. Ran qa tools and it passed.

about.pl has compilation errors now.
Look at this line:

if ( @$patrons or @$biblios or @$items or @$checkouts or @$holds ) {

You removed $biblios and $items..

QA: Revisiting this one again

Created attachment 75897 [details] [review]
Bug 20271: (QA follow-up): rename to deleted_on and various fixes

- renamed deleted_at => deleted_on
- removed failing sql in t/db_dependent/OAI/Server.t
- removed deprecated autoincrement checks in about.pl
- fixed errors in catalogue_stats.tt and catalogue_stats.pl

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Removed the DBIx schema changes. Please keep them separated.
We missed changes for Biblioitem btw. Adding them in follow-up.

Created attachment 75898 [details] [review]
Bug 20271: (QA follow-up) DBIx schema changes

Please separate these changes from the other patches.
Note that Biblioitem was not in the last set of changes too.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 75899 [details] [review]
Bug 20271: (QA follow-up) Additional about cleanup

Still found references to biblios and items in script and template.

Test plan:
Check about page again.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Not ready yet !

t/db_dependent/Search.t
    not ok 44 - All records have at least one item available
    #   Failed test 'All records have at least one item available'
    #   at t/db_dependent/Search.t line 508.
    #          got: 'false'
    #     expected: 'true'

Pass on fresh db, so needs some digging..

Asked QA team for additional feedback; cant finish this one now.

(In reply to Marcel de Rooy from comment #86)

> deleted_at should probably be better: deleted_on; we also have updated_on

Not sure I agree with the logic here.. I would say the field within the patron (borrowers) table is misnamed rather than the other way around.. _on suggests date only precision whereas _at suggests a higher precision including time.  As the field is a datetime/timestamp rather than a simple date I'd go with _at here personally.

`installer/data/mysql/sysprefs.sql` still references deletedbiblio in the OAI-PMH:DeletedRecord preference.

I'm going to let the _at vs _on piece pass as although I feel both set should use _at rather than _on in the name of consistency _on is OK and switching to _at could be done consistently at a later date.

However.. the atomicupdate is not currently idempotent... I'll convert it as a QA followup.

I'm pretty happy with all this now. Well done to marcelr for the first QA run, he did a great job and made my life much easier to get it over the finishing line.

Please note that the manual should get a corresponding minor patch to update the OAI reference to the  deletedbiblio table that this removes: https://koha-community.org/manual/17.11/en/html/02_administration.html?highlight=oai

Passing QA

Thanks a lot to the QA team!

I found an issue now: we forgot to update CanBookBeIssued to check if item is deleted. So ATM it is possible to check out a deleted book (but it cannot be checked in again). We could either return UNKNOWN_BARCODE from CanBookBeIssued, or add a separate error code DELETED?. Opened a new bug for this, see bug #20940

Ack, How did I miss that!.. I'm going to give this a third pass today to make sure I've not missed anything else :(

In related news, I've added my proposed manual update as a merge request here https://gitlab.com/koha-community/koha-manual/merge_requests/135/diffs

(In reply to Petter Goksøyr Åsen from comment #122)
> Thanks a lot to the QA team!
> 
> I found an issue now: we forgot to update CanBookBeIssued to check if item
> is deleted. So ATM it is possible to check out a deleted book (but it cannot
> be checked in again). We could either return UNKNOWN_BARCODE from
> CanBookBeIssued, or add a separate error code DELETED?. Opened a new bug for
> this, see bug #20940

By that logic, we should also add handling to CanBookBeRenewed ;)

:( Looks like a missed a whole swathe of issues now I dig into this.

I believe we should NOT be changing the expected return of GetItem (i.e deleted items should not be returned from GetItem) to stay compliant with existing functions. I can't believe I missed this in the changed test file :(.  We are not consistently dealing with deleted_on after every invocation of GetItem currently and as such this patchset currently introduces a fairly significant number of untested bugs :(.

Thanks to Petter for spotting the issue that lead me to realise the extent of my failings as QA here :(.

C4::Items::ModItem should probably not be allowed to alter deleted_on as we have a DelItem routine.. save a future misuse.

Apart from the other issues that still need fixing, is there a way we could run a performance test on this? I know we got libraries around with many years (10+) of circulation data in these tables. Could this chance cause performance issues?

That'll be interesting to test.. the db update is certainly going to be painful at the very least.. it would be good to see if there's anything we could do about that.

Hi, regarding performance and db update/migrate:
we have it in production now and tested quite abit up front.

db migrate took ~40sec as I recall (1,5M items and 400k biblios) which is far less than the updates of statistics column took, for instance. We had 830 duplicate items that could not be moved. 

regarding performance there is nothing in this patchset that will degrade performance, unless, say, you have a report query for all items, which now would need to add 'WHERE deleted_on IS NULL' not to include deleted items

on the other hand, union queries of items and deleteditems are no longer neccessary so statistical queries will be faster

When we move on to merge issues and reserves, the question is more relevant, as 10 years of checkout history might lead to 100M+ rows, but still, MySql is capable of billions, and at that point its more a matter of indexes, RAM and physics.

Hi Benjamin, you are right, I was mostly thinking about issues/old_issues. But another question: When we moved marcxml we managed to add a nice conversion tool for the reports that seems to work quite well. Could we do the same for those tables? I think detecting the problematic reports and highlighting them should be no issue, converting might be harder.

I've done another round of testing and I'm reasonably happy that everything is caught now.. though I was happy the first time.. Hoping to get another pair of QA eye's on it and awaiting a followup with at least a warning during the update database about reports needing to change if not a more thorough handling to convert the simple cases automagically.

I started to look in to it too now

If somebody could test my follow-ups (last 4) and confirm they are ok, it would be great ;)

Created attachment 76507 [details] [review]
Bug 20271: (QA follow-up) Remove last occurrences of deleted_at

Trivial replace of deleted_at by deleted_on.

Test plan:
Run t/db_dependent/Koha/ItemTypes.t
Run t/db_dependent/Items_DelItemCheck.t
git grep -E "deleted_at\W"

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Created attachment 76508 [details] [review]
Bug 20271: (QA follow-up) Reword OAI-PMH:DeletedRecord systempref

The preference still refered to the deletedbiblio table made defunct by
this patchset.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Created attachment 76509 [details] [review]
Bug 20271: (QA follow-up) Make the atomicupdate idempotent

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Created attachment 76510 [details] [review]
Bug 20271: (QA follow-up) Restore original GetItem functionality

The return value of GetItem had been changed (as it would now return
deleted items as well as current items). This patch restores the
original functionality (as we've not catered for all calls to GetItem
independantly), restores the original test as well as adding a further
test to catch the correct setting of deleted_on by DelItem

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Created attachment 76511 [details] [review]
Bug 20271: (QA follow-up) Fix catalogue statistics in reports module

Test plan:
1) Have some deleted and some not deleted items
2) Use catalogue statistics in reports module and try to filter
total/deleted items
--> without this patch the deleted options returns not deleted items and
total returns deleted items
--> with this patch you should be able to filter the results to deleted,
    not deleted and all items and everyone of this filter should work

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Created attachment 76512 [details] [review]
Bug 20271: (QA follow-up) Zebra should not index deleted record when doing full reindex

Test plan:
Run koha-rebuild-zebra -f [instance]
--> without patch the deleted records are indexed and shown when
searching
--> with this patch the deleted records are not indexed

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Created attachment 76513 [details] [review]
Bug 20271: (QA follow-up) Does not return deleted record in GetMarcBiblio

Test plan:
Go to url /cgi-bin/koha/catalogue/detail.pl?biblionumber=[deleted
biblionumber]
--> without patch you see the deleted record
--> with patch you should see message "The record you requested does not
exist"

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Created attachment 76514 [details] [review]
Bug 20271: (QA follow-up): Fix test ModBiblioMarc

Test plan:
prove t/db_dependent/Biblio/ModBiblioMarc.t

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Rebased on master, there was conflict in C4/Items.pm because of bug 20702

Quick test:
- check an item out, then check it in
- delete the item

1. on /members/readingrec.pl?borrowernumber=51
a) before "This patron has no circulation history."
b) after we see a table with all item's info

2. on /catalogue/issuehistory.pl?biblionumber=42
a) before "Amy's diary, by Lee has never been checked out."
b) after "Checked out 1 times" with the table (and a barcode with a link to the item)

Same if we remove the bibliographic record.

3. Perform a search, /catalogue/search.pl?q=d
the search result will display "1 item, 1 available" in the "Location" column whereas there are no more items for this bibliographic record.

I did not see anything about these changes in the previous comments, and it seems that we lack a proper test plan.

What about acquisition module? Fines? Authority ("used in X"), etc?

I do a retest:

(In reply to Jonathan Druart from comment #182)
> Quick test:
> - check an item out, then check it in
> - delete the item
> 
> 1. on /members/readingrec.pl?borrowernumber=51
> a) before "This patron has no circulation history."
> b) after we see a table with all item's info

The same for me, you are right

> 2. on /catalogue/issuehistory.pl?biblionumber=42
> a) before "Amy's diary, by Lee has never been checked out."
> b) after "Checked out 1 times" with the table (and a barcode with a link to
> the item)

You are right

> 
> Same if we remove the bibliographic record.
>

True

> 
> 3. Perform a search, /catalogue/search.pl?q=d
> the search result will display "1 item, 1 available" in the "Location"
> column whereas there are no more items for this bibliographic record.

When I delete an item, it can not be anymore seen in search results - so ok for me.

> I did not see anything about these changes in the previous comments, and it
> seems that we lack a proper test plan.
>
> What about acquisition module? Fines? Authority ("used in X"), etc?

Authority "used in X records" is done by search engine, so  when it works, it gets the correct number - I just tested it and it is ok when i remove record with authority linked.

Fines linked with items remains linked and show the name of item in description - but it is the question if we do wan't to show it or not then - it could be useful information even if the item/biblio no longer "exists".

So I agree, there is more things to test and it is not easy to find all places which should be tested...

virtualshelfcontents entries are not deleted when a bibliographic record is removed:
- Add 3 biblios to a list
- Remove one
- /virtualshelves/shelves.pl?op=list&category=1
says "3 item(s)"
- click on the list
=> There are only 2

Should we move this to FQA for now?

(In reply to Katrin Fischer from comment #185)
> Should we move this to FQA for now?

Yeah I think so.
Is anyone planning to work on the issues mentioned by Jonathan/Josef ?

I register this stopped before summer and awaits some discussion. As to the issues mentioned, here's my 2c:
> - check an item out, then check it in
> - delete the item
> 
> 1. on /members/readingrec.pl?borrowernumber=51
> a) before "This patron has no circulation history."
> b) after we see a table with all item's info

> 2. on /catalogue/issuehistory.pl?biblionumber=42
> a) before "Amy's diary, by Lee has never been checked out."
> b) after "Checked out 1 times" with the table (and a barcode with a link to
> the item)

both these are related to history and not items. Deleting an item "unlinks" it, meaning the copy no longer exists, so it should disappear from search and circulation, but should not affect history. Anonymization though is another concern, but that has to do with issues and fines, not items and biblios. Many things were broken due to the splitting of tables, and these are examples of things that now actually can be fixed.

As to Virtualshelves I know little of what they are supposed to do. There is a link from virtualshelfcontents table to biblionumber. If the intended use is for keeping lists I see no reason in deleting it. If it is used for browsing or some virtual display it should probably be deleted.

Removing Academy again ;)

Can we split this into different bugs? One for biblio, one for holds, etc...

I am working on a rebased version of these patches, on top of bug 23463.

Note for myself:

* comment 182

* comment 183

* comment 184

* deal with reports

* my $item_object = Koha::Items->find({barcode => $barcode });
=> We need to remove the barcode unique index

* Prevent regression and deal with Koha::Items->find Koha::Items->search
Ideas:
  - ->find returns only non-deleted items when ->find_deleted returns only deleted items
  - same for ->search, ->search_deleted

Or keep Koha::Old::Items that could inherit from Koha::Items

Or update all the occurrences (how many?)

Up-to-date patches are on https://gitlab.com/joubu/Koha/commits/bug_20271

(In reply to Jonathan Druart from comment #193)
> Note for myself:
> 
> * comment 182
> 
> * comment 183
> 
> * comment 184
> 
> * deal with reports

This should be done using DB views. What is the 'deleted' column name we picked?

> * my $item_object = Koha::Items->find({barcode => $barcode });
> => We need to remove the barcode unique index

We need to rely on the DB to handle barcode uniqueness... we should move the barcode to another column, say... deleted_barcode.

> * Prevent regression and deal with Koha::Items->find Koha::Items->search
> Ideas:
>   - ->find returns only non-deleted items when ->find_deleted returns only
> deleted items
>   - same for ->search, ->search_deleted
> 
> Or keep Koha::Old::Items that could inherit from Koha::Items
> 
> Or update all the occurrences (how many?)

On the API I would expect the endpoint to return non-deleted ones as default, and I would prefer to handle it explicitly: manually adding deleted => 0 to the query. And have ?include_deleted=true and/or ?deleted=true be mandatory to get the deleted ones.

Maybe we could encapsulate this pattern in Koha::Objects::Deleted, which extended Koha::Objects with new generic methods: ->search_without_deleted

I haven't made up my mind yet, but I feel it smells to have ->search have a weird behaviour other than the expected.

I'm also worried how we would handle:

my $items = $biblio->items;

so it doesn't return deleted ones.

(In reply to Tomás Cohen Arazi from comment #195)
> (In reply to Jonathan Druart from comment #193)
> > Note for myself:
> > 
> > * comment 182
> > 
> > * comment 183
> > 
> > * comment 184
> > 
> > * deal with reports
> 
> This should be done using DB views. What is the 'deleted' column name we
> picked?

I wanted to try and add an automatic conversion, like we did for biblio_metadata (bug 17898).
The column is a datetime and is named deleted_on (deleted_at in the first patches).

> > * my $item_object = Koha::Items->find({barcode => $barcode });
> > => We need to remove the barcode unique index
> 
> We need to rely on the DB to handle barcode uniqueness... we should move the
> barcode to another column, say... deleted_barcode.

If we want to keep the DBMS handle barode uniqueness we will need to discuss how to deal with that correctly then :)
I do not like moving the column, but maybe we could add a unique constraint on (barcode, deleted_on).

> > * Prevent regression and deal with Koha::Items->find Koha::Items->search
> > Ideas:
> >   - ->find returns only non-deleted items when ->find_deleted returns only
> > deleted items
> >   - same for ->search, ->search_deleted
> > 
> > Or keep Koha::Old::Items that could inherit from Koha::Items
> > 
> > Or update all the occurrences (how many?)
> 
> On the API I would expect the endpoint to return non-deleted ones as
> default, and I would prefer to handle it explicitly: manually adding deleted
> => 0 to the query. And have ?include_deleted=true and/or ?deleted=true be
> mandatory to get the deleted ones.
> 
> Maybe we could encapsulate this pattern in Koha::Objects::Deleted, which
> extended Koha::Objects with new generic methods: ->search_without_deleted
> 
> I haven't made up my mind yet, but I feel it smells to have ->search have a
> weird behaviour other than the expected.

I like you idea, I will try to have a look at how we can implement it.
Also I think we will need a separate find method.

> I'm also worried how we would handle:
> 
> my $items = $biblio->items;
> 
> so it doesn't return deleted ones.

We deal with that already:
406 sub items {
407     my ($self) = @_;
408 
409     my $items_rs = $self->_result->items->search({ deleted_on => undef });
410 
411     return Koha::Items->_new_from_dbic( $items_rs );
412 }

(In reply to Jonathan Druart from comment #196)
> If we want to keep the DBMS handle barode uniqueness we will need to discuss
> how to deal with that correctly then :)
> I do not like moving the column, but maybe we could add a unique constraint
> on (barcode, deleted_on).

Oops, that cannot work. Unique constraint allows multiple NULL values.

As we are going to face this situation for some other tables as well (at least columns borrowers.cardnumber, borrowers.userid, issues.itemnumber), I think we need an approach that will work and be named identically for them.

We could have a boolean flag is_deleted, in addition of deleted_on that contains an important info. The problem is that it will not work either, as:
The following situation should be valid but will fail the unique constraint:
(id, is_deleted)
(42, 1)
(42, 0)
(42, 0)

So we could have a nullable is_not_deleted:
(id, is_not_deleted)
(42, 1)
(42, NULL)
(42, NULL)
The work but it will not be a boolean, as 0 will not be a "valid" value (not a big deal anyway).

"is_not_deleted" is not ideal, maybe is_alive? is_current? naming suggestions?

We will not be able to confirm the data integrity between this new column and deleted_on. But that seems easy to handle at code level.

Note that Postgres can deal with a WHERE clause on a unique constraint. Also that CHECK could be helpful here, but we need it to be present in the stable versions of the DBMS we support.

Something else to suggest?

Stuff something like '$$DELETED$$' . <id field> or UUID::uuid() in any unique fields upon deletion?

> We could have a boolean flag is_deleted, in addition of deleted_on that
> contains an important info. The problem is that it will not work either, as:
> The following situation should be valid but will fail the unique constraint:
> (id, is_deleted)
> (42, 1)
> (42, 0)
> (42, 0)

I don't think I understand what scenario would create a situation such as this, could you elaborate?

(In reply to Kyle M Hall from comment #200)
> > We could have a boolean flag is_deleted, in addition of deleted_on that
> > contains an important info. The problem is that it will not work either, as:
> > The following situation should be valid but will fail the unique constraint:
> > (id, is_deleted)
> > (42, 1)
> > (42, 0)
> > (42, 0)
> 
> I don't think I understand what scenario would create a situation such as
> this, could you elaborate?

Sorry, wrong copy/paste, it was the reverse:
(id, is_deleted)
(42, 0)
(42, 1)
(42, 1)
So barcode=42 can appear several times, but only once with is_deleted=0.
Result is the same, the constraint will fail.

Use cases:
- Items: reused barcode stickers or barcode slips, maybe for ILL items?
- Cardnumbers: reusing expensive chip cards for different guest users

I don't think invalidating with a prefix for something would be very elegant, it also gets more complicated when things are reused more than one time. Also would make writing reports harder and the data harder to interpret for the libraries.

Hmm.. CHECK constraints come to mind.. but to use them we would need to up our minimum MySQL and MariaDB versions.. and carefully test how their implementations of CHECK constraints compare.

I think i like your suggestion of 'is_alive'/'is_current' the best at the moment.. it feels like the cleanest approach so far and does not require a DB version lift or additional linked tables etc.

Additional question: what's the point of biblioitem.deleted_on and biblio_metadata.deleted_on?
I think we only need items.deleted_on and biblio.deleted_on

(In reply to Jonathan Druart from comment #205)
> Additional question: what's the point of biblioitem.deleted_on and
> biblio_metadata.deleted_on?
> I think we only need items.deleted_on and biblio.deleted_on

As long as we have biblioitems, a last change can be helpful. But I agree we are actually using biblioitems as an overflow for biblio only. The columns could be moved up.

(In reply to Marcel de Rooy from comment #206)
> (In reply to Jonathan Druart from comment #205)
> > Additional question: what's the point of biblioitem.deleted_on and
> > biblio_metadata.deleted_on?
> > I think we only need items.deleted_on and biblio.deleted_on
> 
> As long as we have biblioitems, a last change can be helpful. But I agree we
> are actually using biblioitems as an overflow for biblio only. The columns
> could be moved up.

Having a timestamp (last modified), created and deleted is what would be helpful. We have some sync functionality with our unoin catalog, where the dates are needed. 

We might want the newest of them to survive, because: depending on the change, not every of the 3 tables in question is updated and the timestamps might be out of sync.

> Having a timestamp (last modified), created and deleted is what would be
> helpful. We have some sync functionality with our unoin catalog, where the
> dates are needed. 
> 
> We might want the newest of them to survive, because: depending on the
> change, not every of the 3 tables in question is updated and the timestamps
> might be out of sync.

In case of you wondering why I am not making sense... I was one step ahead - merging all the things (which we are not doing here...)

I have just pushed some follow-ups implementing the last bit we talked about.

I did not test the whole changes as I am waiting for bug 23463 to be integrated first.

Early tests and reviews would be appreciated however.

Patches have been rebased, ready for testing!

Please use the remote branch at https://gitlab.com/joubu/Koha/commits/bug_20271

I intend to hit this one hard at the beginning of the next cycle.

Nice work everyone! I've been looking at the code and so far would like to mention:
- I would prefer 'archived' instead of 'deleted' in the chosen terminology. And reserve 'delete' for permanent deletion.
- I'd suggest we split this bug into separate ones for each case to ease testing and have more people onboard. I know it might be frustrating to read this, but I think it is the best.
- In light of the work on the *reserves tables that was announced today, I'd suggest we review the column names (this could be done in a follow-up bug, but worth thinking about.
- With the same spirit as the above comment, having views for the 'old tables' might be a good idea.
- Can we merge biblio and biblioitems here?

(In reply to Tomás Cohen Arazi from comment #212)
> - In light of the work on the *reserves tables that was announced today, I'd
> suggest we review the column names (this could be done in a follow-up bug,
> but worth thinking about.

Do you mean this announcement?
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25260

> - With the same spirit as the above comment, having views for the 'old
> tables' might be a good idea.

That could be a good idea, although it might mean that some SQL reports don't fail when they should, and end up including everything from biblio_metadata as well as deletedbiblio_metadata, which could be problematic. 

That said, we don't take as much advantage of views as we could/should...

I am not sure about the use of archived - for libraries archived might have a different meaning and be counter-intuitive. We are talking about items and records that have been removed from their collections permanently here, I feel deleted is better and would match the GUI functionalities.

(In reply to Tomás Cohen Arazi from comment #212)
> - I would prefer 'archived' instead of 'deleted' in the chosen terminology.
> And reserve 'delete' for permanent deletion.
I am with Katrin here. Deleted is fine. Permanently deleted is gone ;)

> - I'd suggest we split this bug into separate ones for each case to ease
> testing and have more people onboard. I know it might be frustrating to read
> this, but I think it is the best.
> - Can we merge biblio and biblioitems here?
Here means another report probably :)
This move is huge already.

(In reply to Tomás Cohen Arazi from comment #212)
> Nice work everyone! I've been looking at the code and so far would like to
> mention:
> - I would prefer 'archived' instead of 'deleted' in the chosen terminology.
> And reserve 'delete' for permanent deletion.

There is deleted_on and is_current (comment 198 and later for the discussion).

> - I'd suggest we split this bug into separate ones for each case to ease
> testing and have more people onboard. I know it might be frustrating to read
> this, but I think it is the best.

There is 2 years history in those commits, splitting them will break everything and add lot of work. I won't do that. I can help to push this one until the finish line but as it.

> - In light of the work on the *reserves tables that was announced today, I'd
> suggest we review the column names (this could be done in a follow-up bug,
> but worth thinking about.

That's definitely something different.

> - With the same spirit as the above comment, having views for the 'old
> tables' might be a good idea.

For the reports then you mean?
I would personally prefer to add a warning in the report table if the deleted tables are used, but we could add a view.
Adding views mean that we will keep them forever? If yes it will add work to maintain them, otherwise when are we going to remove them? Like 1 year after? 2 years?

> - Can we merge biblio and biblioitems here?

Definitely out of the scope.

> For the reports then you mean?
> I would personally prefer to add a warning in the report table if the
> deleted tables are used, but we could add a view.
> Adding views mean that we will keep them forever? If yes it will add work to
> maintain them, otherwise when are we going to remove them? Like 1 year
> after? 2 years?

Maybe we can keep them for 2 or 3 release cycles with some kind of deprecation warning in release notes

I rebased the remote branch against master and added the view for backward compatibility.

biblio_legacy
items_legacy
biblioitems_legacy
biblio_metadata_legacy

deletedbiblio
deletedbiblioitems
deleteditems
deletedbiblio_metadata

On the branch https://gitlab.com/joubu/Koha/commits/bug_20271
7 tests are failing while they pass in the master commit on which the branch is based.

pastebin:
https://copycat.drycat.fr/?c17f8b259b275bf6#7MLWpfeTftSKTwT8Rs3ke6rP9Srau9jXXQzUroJNYcZH

Ok, I tested all I could find and added a couple follow-ups for the API (tests and spec).

My only concern is the atomic update doesn't seem to be idempotent. Not sure it is a big deal as this is not a target for backporting. But worth mentioning.

My SO branch with the follow-up commits:

https://gitlab.com/thekesolutions/Koha/-/commits/qa_20271

(In reply to Tomás Cohen Arazi from comment #220)
> Ok, I tested all I could find and added a couple follow-ups for the API
> (tests and spec).

Hi Tomas, can you explain a bit more so people can get creativ about other things to test?

I'm working on a performance improvement. Gimme one hour or so to submit.

I submitted my work to the mentioned branch [1]

What I tested was:
- Do a reset_all on master
- Run updatedatabase on that branch
- Tested adding biblios/items and deleting them works without changes.
- Rebuilding zebra
- Tests need to pass

I fixed the API so it acknowledges the new deleted_on and deleted fields. Fetching items should keep the current behaviour (no deleted items retrieved by default, we could add the feature later if we wanted to be able to retrieve them).

I noticed we merge the tables and now have a WHERE condition all over the place, that has an impact on performance, unless we have an index for the compared columns. As we are not making real use of the date the record was deleted, I made sure the index is created against a boolean (size, probably speed) so I changed all item-related queries to do

items.is_current=1 instead of items.deleted_on IS NULL
items.is_current=0 instead of items.deleted_on IS NOT NULL

and for biblios:

biblio.deleted=0 instead of biblio.deleted_on IS NULL
biblio.deleted=1 instead of biblio.deleted_on IS NOT NULL

I put my signature on this patchset as-is, but am still checking the atomic update edge cases.

[1] https://gitlab.com/thekesolutions/Koha/-/commits/qa_20271

Tomas, with your patches we now have:
  biblio.deleted_on, biblio.deleted
and
  items.deleted_on, items.is_current
I think it's confusing.

(In reply to Jonathan Druart from comment #225)
> Tomas, with your patches we now have:
>   biblio.deleted_on, biblio.deleted
> and
>   items.deleted_on, items.is_current
> I think it's confusing.

Yes. I think is_current is an anomaly and shouldn't be the rule. But I wouldn't block this for that.

(In reply to Tomás Cohen Arazi from comment #226)
> (In reply to Jonathan Druart from comment #225)
> > Tomas, with your patches we now have:
> >   biblio.deleted_on, biblio.deleted
> > and
> >   items.deleted_on, items.is_current
> > I think it's confusing.
> 
> Yes. I think is_current is an anomaly and shouldn't be the rule. But I
> wouldn't block this for that.

Can you explain? I think if possible we should be consistent, thinking of the many report writeres out there.

(In reply to Katrin Fischer from comment #227)
> (In reply to Tomás Cohen Arazi from comment #226)
> > (In reply to Jonathan Druart from comment #225)
> > > Tomas, with your patches we now have:
> > >   biblio.deleted_on, biblio.deleted
> > > and
> > >   items.deleted_on, items.is_current
> > > I think it's confusing.
> > 
> > Yes. I think is_current is an anomaly and shouldn't be the rule. But I
> > wouldn't block this for that.
> 
> Can you explain? I think if possible we should be consistent, thinking of
> the many report writeres out there.

Comment #198 explains it. What really makes sense is a 'deleted' flag, but because of this very special case in which we want to preserve the UNIQUE constraint on the items.barcode column which some libraries would like to reuse, and the DBMS doesn't allow us to have the constraint on non-NULL values, we need to do it in reverse (i.e. instead of 'deleted' we do 'not_deleted' and set NULL to represent deleted). So we are forcing the DB structure because of our limitations on the DB features we are allowed to use, or the versions of the DB we want to support (all for good reasons, not arguing).
I would just add a FIXME on the items table and leave the other tables saner.

I don't think items is an exception, and so I don't think biblio should have another naming.

For instance (as noted in comment 198):
"""As we are going to face this situation for some other tables as well (at least columns borrowers.cardnumber, borrowers.userid, issues.itemnumber)"""

(In reply to Jonathan Druart from comment #229)
> I don't think items is an exception, and so I don't think biblio should have
> another naming.
> 
> For instance (as noted in comment 198):
> """As we are going to face this situation for some other tables as well (at
> least columns borrowers.cardnumber, borrowers.userid, issues.itemnumber)"""

You're 100% right. I'll tweak my working branch so this moves forward, in a consistent way! Thanks for the clarification Jonathan!

Tomas, I cherry-picked 2 of your patches and added a commit to fix the atomic update process bug you reported me (pm)

(In reply to Jonathan Druart from comment #231)
> Tomas, I cherry-picked 2 of your patches and added a commit to fix the
> atomic update process bug you reported me (pm)

I tested your follow-up, and it works great! I amended my last patch to catch a typo. You can consider this as SO by me, picking my branch. It includes a commit from Martin, but better ask him about his SO status!

Good job team!

My turn, thanks for leading the way Tomas.. I'm taking over and giving it my once over.. :)

Remote branch updated. Tomas's signed-off-by lines have been added.

I've added a couple of additional followups with fixes to minor issues I found.. generally it's looking great, performs well and I've not found any regressions.

I'm not 100% sure about the introduced *_legacy views as we're not using them here for updating reports.. but I do like the deleted* views as a way to lower the requirement for people to immediately update reports.  Having spoken to Jonathan about this I think we were in agreement they were OK to stay with a later followup working on automating updating reports to use them.

All in all, I've added my SO stamp to the patchset on another local branch: https://gitlab.com/mrenvoize/Koha/-/commits/qa_20271 ready for Jonathan to merge in.

Thanks Martin, I've cherry-picked your 2 QA follow-ups and added your stamp on the whole stack!

Reran the tests (branch joubu/Koha/commits/bug_20271), the issues of comment 219 are gone.

New failing test:

kohadev-koha@50ed3ff6e267:/kohadevbox/koha$ prove t/db_dependent/Items.t
t/db_dependent/Items.t .. 4/15 
    #   Failed test 'found 1 item with itemnotes = "foobar"'
    #   at t/db_dependent/Items.t line 613.
    # Looks like you failed 1 test of 15.
t/db_dependent/Items.t .. 7/15 
#   Failed test 'SearchItems test'
#   at t/db_dependent/Items.t line 638.
t/db_dependent/Items.t .. 13/15 # Looks like you failed 1 test of 15.

When running the tests, is there a way to disable the output of «running "CREATE VIEW "items_legacy"» ?
There are more than 100 of those outputs when running the tests.

I commented all instances of CREATE VIEW
- installer/data/mysql/kohastructure.sql
- installer/data/mysql/atomicupdate/bug_20271_-_merge_deletedbiblio_and_deleteitems_tables_with_their_alive_cousins.perl
And reset the DB but the messages are still outputted in a high number.

Before that: is that relevant to run the tests with and without the patchset to compare the output and check if there are additional warnings?

> is there a way to disable the output

Found. The declarations of the views can be found with `git grep "ResultSource::View"`


Beside from me removing them now to check for other warnings. We might want later to not have the tests logs cluttered with this right?
Should I create a followup bug about hiding the output of view creations? (happens more than 100 times in the full suite)


So the results: no suspicious difference found in the output between running the test suite without and with the patch set.


Typo found:
installer/data/mysql/atomicupdate/bug_20271_-_merge_deletedbiblio_and_deleteitems_tables_with_their_alive_cousins.perl
> # Create the views for backward compatibility in reportrs
«reportrs»

Tomas and I have been discussing the possibilities to rename the tables here so we can reuse the current table names for the views and thus nicely sidestep the reports challenges.

I didn't get the failures you mention Victor... Wonder how our setups differ.

with koha-testing-docker (images up to date, including DB and ES)


Debian 9 with MariaDB 10.1
Debian 10 with MariaDB 10.3 (and ES 6 in case that matters)

prove t/db_dependent/Items.t

From what I understand cleanup_database.pl (and maybe other things) still rely on  the original table names, so with the patchset, the views.

It would be in a follow up to use the new tables right? (and check if deleted) To work incrementally and not further add to this patchset.

Or maybe that's a good use of views that it would be better to keep?

The failure from t/db_dependent/Items.t should be fixed by bug 25964.

(In reply to Victor Grousset/tuxayo from comment #242)
> From what I understand cleanup_database.pl (and maybe other things) still
> rely on  the original table names, so with the patchset, the views.
> 
> It would be in a follow up to use the new tables right? (and check if
> deleted) To work incrementally and not further add to this patchset.
> 
> Or maybe that's a good use of views that it would be better to keep?

We missed it, should be corrected in this patchset.
Fixed in a new commit "Bug 20271: Correct occurrences in cleanup_database.pl"

fatal: repository 'https://gitlab.com/joubu/Koha/commits/bug_20271/' not found
??

(In reply to Marcel de Rooy from comment #245)
> fatal: repository 'https://gitlab.com/joubu/Koha/commits/bug_20271/' not
> found
> ??

When do you get this error?

% git remote add joubu https://gitlab.com/joubu/koha.git
% git fetch joubu
% git checkout -b bug_20271

(In reply to Jonathan Druart from comment #246)
> (In reply to Marcel de Rooy from comment #245)
> > fatal: repository 'https://gitlab.com/joubu/Koha/commits/bug_20271/' not
> > found
> > ??
> 
> When do you get this error?
> 
> % git remote add joubu https://gitlab.com/joubu/koha.git
> % git fetch joubu
> % git checkout -b bug_20271

Yeah when adding the remote bug_20271 and doing the fetch.
Did you see the warnings on your gitlab:
This project is mirrored from https://*****@gitlab.com/koha-community/Koha.git.  Pull mirroring failed 1 year ago.
Repository mirroring has been paused due to too many failed attempts, and can be resumed by a project maintainer.
Last successful update 1 year ago.

All Koha gitlab projects have this problem (because of the size). I will deal with that in the next couple of months. The remote branch is up-to-date however ;)

Potentially incorrect remaining occurrences of the old tables:

misc/cronjobs/cleanup_database.pl
>  (from tables deleteditems, deletedbiblioitems, deletedbiblio_metadata and deletedbiblio).

It's the usage string for the option --deleted-catalog.

-

Nothing else suspicious found when greping deleteditems, deletedbiblioitems, deletedbiblio and deletedbiblio_metadata

For what it's worth. Nothing suspicious found after rereading the whole diff.

Also, deleted an item when using ES and restored it manually via the DB.

Does anyone know something worth to test with UNIMARC?

(In reply to Victor Grousset/tuxayo from comment #251)
> Does anyone know something worth to test with UNIMARC?

Not UNIMARC specific, but did you see: https://lite.framacalc.org/9hdw-bug_20271 ? Collected ideas on what to test there.

Bug found: it's possible to check out a deleted item.
- find a record
- copy the barcode of an item
- delete it
- check it out
- it works :o

(In reply to Katrin Fischer from comment #252)
> (In reply to Victor Grousset/tuxayo from comment #251)
> > Does anyone know something worth to test with UNIMARC?
> 
> Not UNIMARC specific, but did you see:
> https://lite.framacalc.org/9hdw-bug_20271 ? Collected ideas on what to test
> there.

Yes, that's what I was doing but also wondered if something could impact only UNIMARC (or NORMARC)

(In reply to Victor Grousset/tuxayo from comment #253)
> Bug found: it's possible to check out a deleted item.
> - find a record
> - copy the barcode of an item
> - delete it
> - check it out
> - it works :o

Should be fixed now by "Bug 20271: Handle items marked as deleted in the circ module"

Bug found: OPAC results lists deleted items

- delete items from a record
- do a search that returns the record among other ones
- in the results you should see «Items available for loan: MyLibrary (2). »
  - this is wrong
- click on the result
- «No physical items for this record»: as expected
- add in a list
- go to the list
- you should see «Items available for loan: MyLibrary (2). »
  - this is wrong (should be the same cause)

(In reply to Jonathan Druart from comment #255)
> Should be fixed now by "Bug 20271: Handle items marked as deleted in the
> circ module"

Confirmed :)

(In reply to Victor Grousset/tuxayo from comment #256)
> Bug found: OPAC results lists deleted items
> 
> - delete items from a record
> - do a search that returns the record among other ones
> - in the results you should see «Items available for loan: MyLibrary (2). »
>   - this is wrong
> - click on the result
> - «No physical items for this record»: as expected
> - add in a list
> - go to the list
> - you should see «Items available for loan: MyLibrary (2). »
>   - this is wrong (should be the same cause)

Should be fixed by "Bug 20271: Fix XSLT display"

Patches have been rebased against master.

(In reply to Jonathan Druart from comment #259)
> Patches have been rebased against master.

Looking here:
1 - Foreign key constraints on items table 'ON DELETE' updates are affected here - for instance a hold is not deleted if an item is deleted (can be done from additem.pl)
2 - Batch mod and batch delete should not allow altering deleted items
3 - CanBookBeReserved should not check deleted items (missing is_current => 1)

Maybe it is too late to ask this question: Rather than joining items and deleteditems, can we join them into a new table and provide views for both of the old tables? This would prevent breaking reports across the board.

biblio, deletedbiblios => biblios?
items,deleteditems => all_items?

This is going to be a big problem for existing users, it is going to be many reports, and it is going to cause disruption. Havign views that keep the reports working would be a huge boon.

Otherwise:

I mostly tested functionality in this pass rather than reading the code, there are some areas that need attention.

Acquisitions - Can delete ordered items and recieve them

Can edit deleted items by forcing URL in additem.pl

Can add to list using deleted barcodes

Can add deleted items to label batches and rotating collections

Batch record modification: Can load deleted records - then modification fails

Can attach images to deleted records

Canned reports - I don't think these were checked:
grep items /kohadevbox/koha/reports/* -c
grep is_current /kohadevbox/koha/reports/* -c

(In reply to Nick Clemens from comment #261)
> Maybe it is too late to ask this question: Rather than joining items and
> deleteditems, can we join them into a new table and provide views for both
> of the old tables? This would prevent breaking reports across the board.
> 
> biblio, deletedbiblios => biblios?
> items,deleteditems => all_items?
> 
> This is going to be a big problem for existing users, it is going to be many
> reports, and it is going to cause disruption. Havign views that keep the
> reports working would be a huge boon.

It's an interesting idea. It would be nice to preserve that backwards compatibility. 

I think MySQL supports read/write for views, so in theory it could work...

--

I imagine another issue would be changing the tables used in the DBIC definitions?

However, I don't think Biblio or Item were extensively used directly...

C4/Circulation.pm:            my @itemnumbers = $schema->resultset('Item')->search(
circ/renew.pl:    $item = $schema->resultset("Item")->single( { barcode => $barcode } );
Koha/Edifact/Order.pm:            my $i_obj = $schema->resultset('Item')->find( $item->itemnumber );
Koha/EDI.pm:        my $item = $schema->resultset('Item')->find( $ilink->itemnumber );
Koha/EDI.pm:                my $rs = $schema->resultset('Item')->search(
Koha/EDI.pm:        my $item     = $schema->resultset('Item')->find( $ilink->itemnumber );

Koha/BiblioUtils.pm:      $schema->resultset('Biblio')->search(

So then it would just be updating Koha/Item.pm and Koha/Biblio.pm with the new ResultSet module name.

(In reply to Nick Clemens from comment #260)
> (In reply to Jonathan Druart from comment #259)
> > Patches have been rebased against master.
> 
> Looking here:
> 1 - Foreign key constraints on items table 'ON DELETE' updates are affected
> here - for instance a hold is not deleted if an item is deleted (can be done
> from additem.pl)

Indeed.

I listed itemnumber FK here:
(I am adding FIX when this patchset will actually fix the problem for this column ie. we won't lose anymore the FK):
* branchtransfers.itemnumber => must be deleted when marked as deleted
* issues.itemnumber => there is a restrict at DB level and a FIXME in the code, I am opening a new bug report
* items_last_borrower => FIX
* creator_batches => ? TODO?
* reserves => follow-up is coming
* old_reserves => no change expected
* serialitems => ? TODO?
* tmp_holdsqueue => follow-up is coming
* accountlines => I need help on this one, hard to tell for me
* course_items => follow-up is coming
* hold_fill_targets => ? TODO?
* article_requests => FIX? (as there is a on delete set null)
* club_holds => I did not manage to have clubs.item_id different than NULL. How do I do that? (see bug 19618 comment 38)
* stockrotationitems => I am not familiar with stock rotation, but I am expecting change in behaviour here - TODO?
* return_claims => FIX (?)

biblionumber FK:
* biblioitems => OK
* items => It's handled in an ugly way in DelBiblio. Must be fixed later with a move to Koha::Biblio
* reserves => Done in DelBiblio
* old_reserves => no change expected here
* reviews => FIX
* subscription => Done in DelBiblio (Note that maybe we could remove that to have a "undo" ft)
* serial => will be deleted on cascade when the subscription will be deleted
* subscriptionhistory => same as before
* tags_all, tags_index => I think the tags must be deleted when the biblio is marked as deleted TODO?
* virtualshelfcontents => TODO! We will deleted to restore the existing behaviour
* suggestions => TODO, restoring existing behaviour
* aqorders => FIX (?) see also bug 10758
* biblioimages => FIX (?) this will take disk space, but can be reused for "undo". Can be done to restore the existing behaviour if needed
* ratings => FIX (?) same as biblioimages for "undo"
* hold_fill_targets => TODO?
* article_requests => TODO?
* biblio_metadata => OK
* club_holds => TODO

I have opened a pad at https://annuel2.framapad.org/p/koha_bug_20271_fk-9im9?lang=en
Please discuss there and I will copy it back here once we are done.

(In reply to Nick Clemens from comment #260)
> (In reply to Jonathan Druart from comment #259)
> > Patches have been rebased against master.
> 
> Looking here:
> 1 - Foreign key constraints on items table 'ON DELETE' updates are affected
> here - for instance a hold is not deleted if an item is deleted (can be done
> from additem.pl)

See previous comment and the pad for discussion

> 2 - Batch mod and batch delete should not allow altering deleted items

Done!

> 3 - CanBookBeReserved should not check deleted items (missing is_current =>
> 1)

Done!

(In reply to Nick Clemens from comment #261)
> Maybe it is too late to ask this question: Rather than joining items and
> deleteditems, can we join them into a new table and provide views for both
> of the old tables? This would prevent breaking reports across the board.
> 
> biblio, deletedbiblios => biblios?
> items,deleteditems => all_items?
> 
> This is going to be a big problem for existing users, it is going to be many
> reports, and it is going to cause disruption. Havign views that keep the
> reports working would be a huge boon.

Yes Nick, this will be addressed on bug 25921.
However, I don't think it's a good idea to rename the table to have an inconsistent name. We could rename biblio to biblio, but items is correct.
My opinion is that it should be quite trivial to fix the report in an update DB.
* \sitems can be replaced with items_legacy
* \sbiblio => biblio_legacy
* and so on

What do you think?

> Otherwise:
> 
> I mostly tested functionality in this pass rather than reading the code,
> there are some areas that need attention.
> 
> Acquisitions - Can delete ordered items and recieve them
> 
> Can edit deleted items by forcing URL in additem.pl
> 
> Can add to list using deleted barcodes
> 
> Can add deleted items to label batches and rotating collections
> 
> Batch record modification: Can load deleted records - then modification fails
> 
> Can attach images to deleted records

All fixed.

> Canned reports - I don't think these were checked:
> grep items /kohadevbox/koha/reports/* -c
> grep is_current /kohadevbox/koha/reports/* -c

That one sounds tricky, will try to fix later.

New commits are:

Bug 20271: Prevent cover image to be attached to a deleted record
Bug 20271: Prevent biblio to be modified in batch
Bug 20271: Prevent marked as deleted items to be added to lists
Bug 20271: (follow-up) Rely on indexed columns
Bug 20271: Prevent edition of marked as deleted items
Bug 20271: Make Orders->items return current items only
Bug 20271: CanBookBeReserved must not check marked as deleted items
Bug 20271: Prevent marked as delete items to be batch edited/deleted
Bug 20271: Delete attached holds when an item is deleted

(In reply to Jonathan Druart from comment #264)
> (In reply to Nick Clemens from comment #261)
> > Maybe it is too late to ask this question: Rather than joining items and
> > deleteditems, can we join them into a new table and provide views for both
> > of the old tables? This would prevent breaking reports across the board.
> > 
> > biblio, deletedbiblios => biblios?
> > items,deleteditems => all_items?
> > 
> > This is going to be a big problem for existing users, it is going to be many
> > reports, and it is going to cause disruption. Havign views that keep the
> > reports working would be a huge boon.
> 
> Yes Nick, this will be addressed on bug 25921.
> However, I don't think it's a good idea to rename the table to have an
> inconsistent name. We could rename biblio to biblio, but items is correct.
> My opinion is that it should be quite trivial to fix the report in an update
> DB.
> * \sitems can be replaced with items_legacy
> * \sbiblio => biblio_legacy
> * and so on

That's a good point.

I wouldn't trust the report DB update to be 100% effective, but effective enough. Even if someone had to manually update a couple reports here and there with items_legacy or biblio_legacy... having the ability to use those legacy views would be good.

Remote branch has been rebased against master, few tweaks made:
* new commit "Bug 20271: Fix - is_current is nullable"
* Only 1 "DBIC changes" commit and a separate "Declare is_current as boolean in schema" to deal with is_boolean.

This is now "in discussion".

More eyes and feedback are needed urgently.

Regarding the lack of interest in the time-frame I expected, I am removing myself as assignee of this one.

I will be happy to help anybody else who would be willing to work on it however.

I see this bug stalled back in 2020.  I LOVE the concept of it, and hope it can pick up momentum again.  I think leaving deleted things in their original tables with a date to flag deletion is a positively brilliant idea.  It would leave links in tact where needed, and make it so easy to implement an undo feature of deletion down the road.

If this project is too big, maybe it can be done in phases?  Just a shame it hasn't move in a couple of years.  This would be a HUGE step forward for Koha.

After discussing this a bit with Christopher I have to agree that I think this would make life a lot easier for libraries.  Periodically we have accidental patron deletes, and it is not easy getting the toothpaste back in the tube once things move to the deleted items table.  This seems like a far easier approach to managing this data I would be very interested in this moving forward. - Bob Bennhoff

At this point the best hope would be for a few user libraries to pool funding together and contract a support company to revive this.

The best hope is to find people to focus on it at the same time. Having someone to rebase it is not enough, I have rebased it and adjusted it dozens of times already.

What would it mean to have many people focus on it?  Previously you asked for more eyes and feedback.  I'm not setup to get into the development side of things, but if there is anything several non-programmers can do to help and give feedback, I'm sure we can generate interest and people to do what they can.  Can you be more specific on what is needed?

If I recall correctly testing was most of what this patch needed.
To check if stuff was broken in many parts of Koha.
At the time, it was possible to test it (except crons) using the sandboxes, so non-programmers could have helped a lot without much being limited.

(In reply to Victor Grousset/tuxayo from comment #275)
> If I recall correctly testing was most of what this patch needed.
> To check if stuff was broken in many parts of Koha.
> At the time, it was possible to test it (except crons) using the sandboxes,
> so non-programmers could have helped a lot without much being limited.

If that is the case, we can certainly rally people to test in sandboxes, if the sandboxes are still functioning.  :)

Adding my support for this enhancement.  Hope we can get it moving again.

Is this at a place for testing, or does more work need to be done?  If there are funding issues, please let us know what funds are needed.

> Is this at a place for testing, or does more work need to be done?

Testing became impossible after a few weeks after the last code update long ago.
It needs a really big work to be reapplied to the current code, since this touches many parts of Koha, as the time passes there a many places where there have been diverging changes. And likely some new places using the old tables that would need to be found and the tables merged. Overall it's like partially redoing the work over again.

(In reply to Victor Grousset/tuxayo from comment #279)
> > Is this at a place for testing, or does more work need to be done?
> 
> Testing became impossible after a few weeks after the last code update long
> ago.
> It needs a really big work to be reapplied to the current code, since this
> touches many parts of Koha, as the time passes there a many places where
> there have been diverging changes. And likely some new places using the old
> tables that would need to be found and the tables merged. Overall it's like
> partially redoing the work over again.

We would really like to see this development through to completion.

I've asked before, with no response, is this being funded?  Does anyone have a bid on this development?  We would be probably be interested in contributing to this cause.

Securing funding is one part, but the other part is to come to a decision as community about these change and ensure that there will be enough time/committment to see it through, if it's decided to do this. 

If that's the case I could imagine freezing a version for all other changes (but bug fixes) to ensure there is no distraction, proper testing and we are not working against an ever changing codebase.

I am not fully persuaded on benefits vs. work/risk yet, but would be open to be persuaded.

When reviving this one, it might be wise to do it one by one table instead?

(In reply to Marcel de Rooy from comment #282)
> When reviving this one, it might be wise to do it one by one table instead?

If doing it table by table would help move it along, I would go for that.  The first one could be proof of concept, at least.  Maybe pick something that would have the least impact, or maybe even try something we haven't done deletions on yet.  Maybe circ rules?

(In reply to Christopher Brannon from comment #283)
> (In reply to Marcel de Rooy from comment #282)
> > When reviving this one, it might be wise to do it one by one table instead?
> 
> If doing it table by table would help move it along, I would go for that. 
> The first one could be proof of concept, at least.  Maybe pick something
> that would have the least impact, or maybe even try something we haven't
> done deletions on yet.  Maybe circ rules?

But circ rules don't need merging?

No, but if you want to start small, and at least do a proof of concept, you might try out the delete flagging technique on something that doesn't have a delete table, then if that goes well, try a table that does have a delete table.  I was trying to offer a suggestion of an easier proof of concept.  Something that people could see in operation and get behind.  I threw circ rules at this, because eventually it might be useful to be able to delete (and recover) deleted rules.  If you have a better suggestion, go for it.

The problem is not to test the concept of delete flagging. The patches on this ticket well demonstrated that.

Even without that already done, not sure that was ever a question. The difficulty is that biblio, biblioitems, biblio_metadata, and items (and their counterpart) are used in a lot of places in Koha. Even taken individually that is still **a lot** of places to change, **test** and review.
Really a lot of workflows to check for regression. Automated test coverage doesn't seem enough. Not sure there is any way to be sure enough to not miss many usages of biblio, biblioitems, biblio_metadata, and items. And thus avoid shipping a few regressions in edge cases. That needs to be weighted against the benefits of this refactoring.

(In reply to Katrin Fischer from comment #281)
> Securing funding is one part, but the other part is to come to a decision as
> community about these change and ensure that there will be enough
> time/committment to see it through, if it's decided to do this. 
[...]
> I am not fully persuaded on benefits vs. work/risk yet, but would be open to be persuaded.

+1 , money can buy a provider to have someone do the painful task of changing a lot of the table usage everywhere. But it still needs other people in the community to test and review it. And the benefits that refactoring need to be weighted against the huge cost in testing and QA. Given the current huge bottleneck on signoff (141 waiting tickets (25 bugfixes)) and QA (168 tickets, (26 bugfixes)) including other refactorings, the benefits of this one need to be big also. In the long run, which alternate timeline Koha **can be expected** to have the less bugs, cleaner code, most features? That depends on the benefits of the refactoring here.

Even finding a way to do that incrementally (something with DB views maybe?) to avoid the need of pulling out of massive effort spike won't make the problem of overall testing and review effort go away.

A way to make the question way easier is to get literally a dozen or more **weekly active** librarians and Koha sysadmins to do some patch testing. Making the bottleneck mostly go away. Librarians, rise up!
(and library executives: let the workers improve their tools!)

See also bug 30888. It introduces deletedauth_header now for deleted authorities. In its current form it is a fairly simple patch set. But if we would want to introduce deleted authorities in the current auth_header table, it would be a completely different story for all the reasons already mentioned in the earlier comments.

So let me ask this.

Instead of trying to overhaul the existing deletion tables, would it be easier to try to insert an option between - as a starting point?

So, instead of doing away with a deleted table (which might resolve the question of what to do with said tables if we changed how things are deleted), how about we just add a new option - like "Mark for deletion".  We could add a deleted column to the table in which a date is posted if this option is marked on a record.  Then we could start having Koha omit records in searches that have a date in that field.  The record would be there, and in tact, it just wouldn't appear in any regular searches.  

We could allow the batch tools to still interact with these records, and move them to the deleted tables if desired.  Everything from that point would behave the same as it does currently.

We could also introduce a new tool to search for these "Marked deleted" records, and undo the mark for deletion, restoring the record.

Taking a step back on that topic was done in bug 30888. See bug 30888 comment 16 and comments before and after for a full context.

> But before that, the question is, should we even avoid
> introducing a tombstone table?
> That's suddenly not as clear.
> [...]
> > Please read this on dba.stackexchange.com:
> >https://dba.stackexchange.com/questions/14402/tombstone-table-vs-deleted-flag-in-database-syncronization-soft-delete-scenari/14419
> From the discussions and attempts to switch to delete flags in the
> last years, it seemed a given that a tombstone table was an
> anti-pattern but it turns out totally not ﷐[U+1F631]﷑

That question also applies to removing an existing tombstone table. If someone has additional thoughs, please add grist for the mill in bug 30888 to help unblock it and that will also be useful for the direction of this very ticket.