Bug 13518 - Table borrower_modifications is missing FK and not deleted with the patron
Summary: Table borrower_modifications is missing FK and not deleted with the patron
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: Database (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 5334
  Show dependency treegraph
 
Reported: 2015-01-05 16:28 UTC by Katrin Fischer
Modified: 2020-05-07 21:14 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
20.05.00


Attachments
Bug 13518: Delete patron's modifications along with the patron (3.81 KB, patch)
2020-04-17 09:25 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13518: Delete patron's modifications along with the patron (3.91 KB, patch)
2020-04-17 17:37 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 13518: Delete patron's modifications along with the patron (3.96 KB, patch)
2020-04-18 01:38 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Katrin Fischer 2015-01-05 16:28:27 UTC
The table borrower_modifications has no FK constraint on the borrowernumber and will remain untouched when the patron is deleted. If the borrowernumber doesn't exist in the database, the modification entry is no longer visible in Koha.
Comment 1 Katrin Fischer 2018-07-12 11:17:36 UTC
Upping severity a bit - storing patron information that is/can never be deleted outside of SQL is not nice.
Comment 2 Jonathan Druart 2018-12-07 16:10:32 UTC
The problem is that this table is used for the borrower modifications and the self-registration features.

So far borrowernumber is the PK (int(11) NOT NULL DEFAULT '0'), for the self-registration feature we can have borrowernumber that is not defined (0 is used)

Ideally we would like to have borrowernumber a DEFAULT NULL, and use NULL for self-reg, but then we will loose the PK (PK cannot be NULL).

I do not think we will be able to keep the correct constraints at DB level anyway, we will need to handle consistency at code-level.
Comment 3 Katrin Fischer 2020-04-15 21:27:24 UTC
I see what you by it being used for 2 features.

So we have:

`verification_token` varchar(255) NOT NULL DEFAULT '',
`borrowernumber` int(11) NOT NULL DEFAULT '0',
PRIMARY KEY (`verification_token` (191),`borrowernumber`),

Because we either have it's either or:

modification: 
borrowernumber != 0 or, no verfication token

self registration 
borrower = 0, verififcation token exists

The design, if I understand it correctly, also means that one patron can only have one modification request at a time.

And a PK column can not be NULL... as the resulting keys wouldn't be unique any more.

The main idea about the FK was to make sure we don't keep storing address and other personal data in there when the patron was long deleted.

Would it be simpler to solve that in the method deleting the patron? (where we move the data to deleted_borrowers?
Comment 4 Jonathan Druart 2020-04-16 10:12:53 UTC
It should be in Koha::Patron->delete, yes.
Comment 5 Jonathan Druart 2020-04-17 09:25:07 UTC
Created attachment 103130 [details] [review]
Bug 13518: Delete patron's modifications along with the patron

The table borrower_modifications has no FK constraint on the borrowernumber
and will remain untouched when the patron is deleted.
If the borrowernumber doesn't exist in the database, the modification entry is no longer visible in Koha.

The problem is that this table is used for the borrower modifications and the self-registration features.

So far borrowernumber is the PK (int(11) NOT NULL DEFAULT '0'), for the self-registration feature we can have borrowernumber that is not defined (0 is used)

Ideally we would like to have borrowernumber a DEFAULT NULL, and use NULL for self-reg, but then we will loose the PK (PK cannot be NULL).

As we cannot keep the correct constraints at DB level anyway, we will need to handle consistency at code-level.

Test plan:
Create a new patron
Do some modification at the OPAC
Delete the patron
Confirm that the modifications as been removed (directly in DB)
Comment 6 Bernardo Gonzalez Kriegel 2020-04-17 17:37:26 UTC
Created attachment 103176 [details] [review]
Bug 13518: Delete patron's modifications along with the patron

The table borrower_modifications has no FK constraint on the borrowernumber
and will remain untouched when the patron is deleted.
If the borrowernumber doesn't exist in the database, the modification entry is no longer visible in Koha.

The problem is that this table is used for the borrower modifications and the self-registration features.

So far borrowernumber is the PK (int(11) NOT NULL DEFAULT '0'), for the self-registration feature we can have borrowernumber that is not defined (0 is used)

Ideally we would like to have borrowernumber a DEFAULT NULL, and use NULL for self-reg, but then we will loose the PK (PK cannot be NULL).

As we cannot keep the correct constraints at DB level anyway, we will need to handle consistency at code-level.

Test plan:
Create a new patron
Do some modification at the OPAC
Delete the patron
Confirm that the modifications as been removed (directly in DB)

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Modifications removed, test pass
Comment 7 Katrin Fischer 2020-04-18 01:38:15 UTC
Created attachment 103187 [details] [review]
Bug 13518: Delete patron's modifications along with the patron

The table borrower_modifications has no FK constraint on the borrowernumber
and will remain untouched when the patron is deleted.
If the borrowernumber doesn't exist in the database, the modification entry is no longer visible in Koha.

The problem is that this table is used for the borrower modifications and the self-registration features.

So far borrowernumber is the PK (int(11) NOT NULL DEFAULT '0'), for the self-registration feature we can have borrowernumber that is not defined (0 is used)

Ideally we would like to have borrowernumber a DEFAULT NULL, and use NULL for self-reg, but then we will loose the PK (PK cannot be NULL).

As we cannot keep the correct constraints at DB level anyway, we will need to handle consistency at code-level.

Test plan:
Create a new patron
Do some modification at the OPAC
Delete the patron
Confirm that the modifications as been removed (directly in DB)

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Modifications removed, test pass

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 8 Katrin Fischer 2020-04-18 01:39:04 UTC
Thx Jonathan! :)
Comment 9 Martin Renvoize 2020-04-20 11:27:00 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 10 Joy Nelson 2020-05-07 21:14:01 UTC
does not apply to 19.11.x branch.  please rebase if needed.