|
Description
Katrin Fischer
2018-07-12 11:30:22 UTC
on delete set null? or set to anon patron? I think this is a real evil bug - can we please prioritize it a bit with the accounts rework? It means you can't rely on reports that libraries need - like statistics on what was paid for certain fee types during a certain time period. There is no way to get this information from Koha in a reliable way, which is a real issue. This is a data loss bug... I am upping severity to get more attention. Created attachment 83220 [details] [review] Bug 21065: Add tests Created attachment 83221 [details] [review] Bug 21065: Add koha_object(s)_class methods to accountline Created attachment 83222 [details] [review] Bug 21065: Set ON DELETE SET NULL on accountlines.borrowernumber Note: Why do we have ON UPDATE SET NULL on accountlines.itemnumber? Created attachment 83223 [details] [review] Bug 21065: Update DBIC Schema change No test plan provided, on purpose. Created attachment 83224 [details] [review] Bug 21065: Update DBIC Schema change I find the ON UPDATE SET NULL odd too.. think it's a mistake and should also be CASCADE for itemnumber.. would be happy to sneek that in here. I'm currently running the entire test suit against this as if memory serves there's something odd going on with koha_object_class stuff in this area which fell out once pushed to Jenkins last time I pushed a similar change. Created attachment 83235 [details] [review] Bug 21065: Add tests Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 83236 [details] [review] Bug 21065: Add koha_object(s)_class methods to accountline Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 83237 [details] [review] Bug 21065: Set ON DELETE SET NULL on accountlines.borrowernumber Note: Why do we have ON UPDATE SET NULL on accountlines.itemnumber? Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 83238 [details] [review] Bug 21065: Update DBIC Schema change Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> It works as described and I've run the entire test suit against it. I believe this is a solid improvement to an important deficit.. Going straight for PQA It should, however, be noted that this action, depending on the size of your accountlines table, may take some time to run and may get executed multiple times if it is backported... RMaints, RM.. perhaps we could come up with a way of preventing this re-execution as part of backporting? I think we should start using a more descriptive naming scheme for foreign keys.. instead of table_ibfk_int (where int is a running number) we should make them more descriptive and have table1_fk_table2.. or {tablename}_{columnname(s)}_{suffix} (as per postgres defaults perhaps).. that would mean in this case we could check for the old _1 key before deleting it and check for the new _table2 constraint before creating it.
(In reply to Martin Renvoize from comment #18) > I think we should start using a more descriptive naming scheme for foreign > keys.. instead of table_ibfk_int (where int is a running number) we should > make them more descriptive and have table1_fk_table2.. or > {tablename}_{columnname(s)}_{suffix} (as per postgres defaults perhaps).. > that would mean in this case we could check for the old _1 key before > deleting it and check for the new _table2 constraint before creating it. Makes sense for me. Created attachment 83366 [details] [review] Bug 19850: (QA follow-up) Make the update idempotent Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> (In reply to Martin Renvoize from comment #20) > Created attachment 83366 [details] [review] [review] > Bug 19850: (QA follow-up) Make the update idempotent > > Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Not much important, but the existing occurrences are either the automatic one ${table}_ibfk_$i or ${table}_${column}, not ${table}_${related_table} So I would have named it accountlines_borrowernumber. Not considering this as blocker, just noting ;) Awesome work all! Pushed ot master for 19.05 Pushed to 18.11.x for 18.11.01 backported to 18.05 for 18.05.08 *** Bug 18323 has been marked as a duplicate of this bug. *** Hello everyone! I noticed this bug was pushed but the wrong one is mentioned in the release notes... https://gitlab.com/koha-community/koha-release-notes/blob/master/release_notes_18_12_00.md Under Enhancements > Acquisitions it says that bug [19850] Enhance invoicing functionality for each line item is pushed, but there is nothing so far in this bug. (In reply to Caroline Cyr La Rose from comment #26) > Hello everyone! > I noticed this bug was pushed but the wrong one is mentioned in the release > notes... > > https://gitlab.com/koha-community/koha-release-notes/blob/master/ > release_notes_18_12_00.md > > Under Enhancements > Acquisitions it says that bug [19850] Enhance invoicing > functionality for each line item is pushed, but there is nothing so far in > this bug. I think it might have happened here: http://git.koha-community.org/gitweb/?p=koha.git&a=search&h=6d44f9dac8235dfe0259ea5b935c5a8908d2e441&st=commit&s=21065 Sorry I missed this one for 17.11.x I prefer not push now at branch end life. Did I mess up here.. should we be replacing the query to INFORMATION SCHEMA with foreign_key_exists? Jonathan Druart, I'd appreciate your thoughts here... I'm happy to code it up as a followup or a new bug. (In reply to Martin Renvoize from comment #29) > Did I mess up here.. should we be replacing the query to INFORMATION SCHEMA > with foreign_key_exists? > > Jonathan Druart, I'd appreciate your thoughts here... I'm happy to code it > up as a followup or a new bug. I do not think so. We are dropping the FK to recreate it with a different ON DELETE clause value. So we could have used foreign_key_exists but rename the FK. |