openjverein / jverein

Open JVerein - Open Source Vereinsverwaltung
https://openjverein.github.io
GNU General Public License v3.0
42 stars 15 forks source link

Feature alte Buchungen löschen #225

Closed JohannMaierhofer closed 1 month ago

JohannMaierhofer commented 4 months ago

Mit diesem Feature lassen sich alte Buchungen z.B. deren Aufbewahrungsfrist nach 10 Jahren abgelaufen ist, löschen. Gelöscht werden die Buchungen aller Konten vor einem vorgegebenen Datum. Damit bei den Mitgliedern im Mitgliedskonto keine Fehlbeträge entstehen können zugeordnete Sollbuchungen mit gelöscht werden. Der Button für das Löschen befindet sich im Buchungen View. Screenshot_20240507_093248 Der Dialog sieht so aus. Screenshot_20240507_094848

Buchungsdokument verweist auf eine Buchung. Diese werden automatisch gelöscht durch das Setzen der Referenz auf CASCADE.

Ich habe keine Beschränkung für das Datum eingebaut. Es steht nur der Hinweis im Dialog, die Aufbewahrungsfristen zu beachten. Mann kann also auch Buchungen löschen die jünger als die Aufbewahrungsfrist sind. Ich sehe darin kein Problem, weil ja niemand gezwungen ist die Kontoauszüge (Buchungen) und Jahresabschlüsse in JVerein aufzubewahren. Die Pflicht ist ja auch erfüllt wenn man die Kontoauszüge und Jahresabschlüsse als Papier oder elektronisch z.B. PDF aufbewahrt. In BuchungImpl habe ich beim deleteCheck den insertCheck auskommentiert. Es macht ja keinen Sinn auf die Insert Bedingungen zu prüfen. Da dürfte man älter als 10 Jahre nicht im Datum haben und könnte sie also auch nicht löschen.

PS: Wegen der Migration muss dieses Feature nach #224 übergeben werden.

SchachtnerTh commented 1 month ago

Hallo, ich wollte mir gerade das Löschen alter Buchungen anschauen. Die Änderungen sind für mich schlüssig... Beim Laden der Sources zum Ausprobieren fehlen bei mir aber wieder einige Migrations. Ich bin mir nicht sicher: Ist das tatsächlich ein Fehler und sollte das behoben werden oder soll ich die für das Review "reintricksen"? Außerdem fehlt eine Methode "createForeignKeyIfNotExistsNocheck", die ich in einem früheren PR (#223) gefunden habe. Heißt das, dass die Änderung aus dem vorherigen Merge dann wieder rausfliegt? Oder anders: Soll ich auch hier tricksen oder ist das ein Fehler?

Viele Grüße Tom

JohannMaierhofer commented 1 month ago

Ich habe das Feature auf blocked gesetzt weil da auch einige DB Migrationen nicht dabei sind. Erst muss das Feature für das Mitglied löschen übernommen werden, dann kann ich wieder den Main Branch reinmergen. Wenn du es schon testen willst musst du erst die Datenbank mit dem Mitglied löschen Feature migrieren und kannst dann das Feature hier testen. Dann sollte auch die hier enthalte Migration ablaufen, weil dann die DB auf 440 steht und 441 die nächste ist. Dass dann einige Migrationen nicht dabei sind macht nichts weil sie ja schon ausgeführt wurden.

JohannMaierhofer commented 1 month ago

PS: Wenn du mit #224 fertig bist solltest du das Review abschließen und akzeptieren. Ich brauche zwei Reviewer die akzeptieren damit es übernommen werden kann.

JohannMaierhofer commented 1 month ago

Nachdem #254 mit der Migration 440 übernommen wurde und das Review für #224 erst später möglich ist kann jetzt für dieses Feature ein Review gemacht werden. #224 kommt dann danach.

JohannMaierhofer commented 1 month ago

Hallo @SchachtnerTh , wenn du das schon gereviewt hast kannst du es jetzt genehmigen. Ich habe dieses jetzt vor Feature 224 vorgezogen. In Feature 224 musste ich sowieso die Migration nochmals anpassen und mbmueller kann dort auch erst später testen.

SchachtnerTh commented 1 month ago

Ich habe mir jetzt gerade die Diskussion zum DSGVO-konformen Löschen von (ehemaligen) Mitgliedern durchgelesen. Wäre es vielleicht sinnvoll, die ganze "Datenbereinigungsthematik" gebündelt zu behandeln? Dann könnte man hier nur einen Button zu dem allgemeineren Dialog setzen, in dem dann mehr gelöscht oder bereinigt werden könnte, z. B.

Ich weiß (a) nicht, wie viel Aufwand es ist, bei der Löschung noch mehrere Tabellen/Informationstypen mit zu berücksichtigen und (b) bin ich mir gar nicht sicher, ob es die zusätzlichen Punkte überhaupt braucht. Von der Funktionalität her könnt ich mir aber vorstellen, dass die Routinen ähnlich sein könnten.

Ein weiterer Vorteil wäre, dass diese gefährlichen Bereinigungs-Funktionen ein bisschen abseits der ständig benutzten Views sind. Aber auch hier: Wenn das keine gute Idee ist bzw. jetzt nicht gemacht werden kann/soll, dann approve ich diese Änderung natürlich auch gerne so.

lenilsas commented 1 month ago

Beim löschen muss zuerst die Buchung gelöscht werden und erst danach die Mitgliedskonto-Buchung sonst kommt es zu ForeingKey Fehlern. Ich würde vorschlagen als Standartdatum nicht 2000 einzutragen, sondern 10 Jahre vor dem jetzigen Jahr, das entspricht dann der Standart-Aufbewahrungsfrist.

JohannMaierhofer commented 1 month ago

Beim löschen muss zuerst die Buchung gelöscht werden und erst danach die Mitgliedskonto-Buchung sonst kommt es zu ForeingKey Fehlern. Ich würde vorschlagen als Standartdatum nicht 2000 einzutragen, sondern 10 Jahre vor dem jetzigen Jahr, das entspricht dann der Standart-Aufbewahrungsfrist.

Ich habe für das Mitglieder Löschen Feature die Foreign Keys geändert, so dass man Mitglieder löschen kann und die Referenzen in den Buchungen werden einfach auf null gesetzt.

JohannMaierhofer commented 1 month ago

Ich habe mir jetzt gerade die Diskussion zum DSGVO-konformen Löschen von (ehemaligen) Mitgliedern durchgelesen. Wäre es vielleicht sinnvoll, die ganze "Datenbereinigungsthematik" gebündelt zu behandeln? Dann könnte man hier nur einen Button zu dem allgemeineren Dialog setzen, in dem dann mehr gelöscht oder bereinigt werden könnte, z. B.

* das ganze Mitglied

* Buchungen, ggf. mit Sollbuchungen

* Lastschriften

* Spendenbescheinigungen

* Kursteilnehmer

* E-Mails (? da werden ja nur die ausgehenden verfügbar sein, aber vielleicht gibt es da auch Gründe, die löschen zu wollen/müssen nach einer bestimmten Zeit)

Ich weiß (a) nicht, wie viel Aufwand es ist, bei der Löschung noch mehrere Tabellen/Informationstypen mit zu berücksichtigen und (b) bin ich mir gar nicht sicher, ob es die zusätzlichen Punkte überhaupt braucht. Von der Funktionalität her könnt ich mir aber vorstellen, dass die Routinen ähnlich sein könnten.

Ein weiterer Vorteil wäre, dass diese gefährlichen Bereinigungs-Funktionen ein bisschen abseits der ständig benutzten Views sind. Aber auch hier: Wenn das keine gute Idee ist bzw. jetzt nicht gemacht werden kann/soll, dann approve ich diese Änderung natürlich auch gerne so.

Ich finde die Idee mit einem Bereinigungs View eine gute Idee. Ich hatte das auch schon einmal im Blick. Nachdem ich bei Mails die Filter eingebaut hatte und auch den Lastschrift View mit Filter, kann man da nach alten Einträgen filtern. Dann alle selektieren und gemeinsam löschen. Darum habe ich das wieder aus den Augen verloren.

Was meiner Meinung aber hier nicht rein passt sind Mitglieder und Kursteilnehmer. Hier handelt es sich ja nicht um alte Daten die man bereinigen möchte. Ein Mitglied tritt halt aus und dann wird es gelöscht. Das möchte ich direkt machen ohne einen Bereinigungs View zu bemühen. Dass dann natürlich auch alle Einträge gelöscht werden müssen die das Mitglied referenzieren ist klar, sonst gibt es Referenzen auf ein nicht existierendes Mitglied. Ich sehe darum das Feature #224 immer noch separat. Spendenbescheinigungen löschen ist auch nicht unbedingt gut. Wenn man dann wieder eine automatische Generierung startet werden sie ja dann neu erzeugt was man auch nicht will.

Mein Vorschlag ist jetzt, dass ich es auf einen View umbaue und dort die Möglichkeit für alte Buchungen, Lastschriften und Mails einbaue. Später kann man dann immer noch mehr aufnehmen.

SchachtnerTh commented 1 month ago

Dass Lastschriften und Mails noch mit dazukommen, war auch nur ein Vorschlag. Gib einfach kurz Bescheid, was ich machen soll. ;-) Ich approve das auch gerne so, wie es jetzt ist.

JohannMaierhofer commented 1 month ago

Dass Lastschriften und Mails noch mit dazukommen, war auch nur ein Vorschlag. Gib einfach kurz Bescheid, was ich machen soll. ;-) Ich approve das auch gerne so, wie es jetzt ist.

Ich baue das um weil es schöner ist. Warte mit dem Review bis ich die Änderung übergebe. Es sollte nicht so kompliziert sein.

JohannMaierhofer commented 1 month ago

Ich habe jetzt den Dialog entfernt und eine View zum Löschen alter Daten erzeugt. Der Menüpunkt ist im linken Baum unter Administration/Erweitert. Ich habe dan gleich einen neuen Ordner Mitglieder erzeugt und alles was unter Administration direkt war da hineingeschoben. Jetzt schaut es aufgeräumter aus und korreliert mit dem Mitglieder Ordner oben. Da jetzt sowohl Buchungen als auch Lastschriften und Mails gelöscht werden können habe ich eine Background Task eingeführt. So kann ich einzelne Meldungen im Monitor ausgeben. Ich habe den Vorschlag bezuglich des Datums aufgenommen. Allerdings gehe ich 11 Jahre zurück. Bei den meisten werden es nämlich 11 Jahre sein die man aufheben muss. Der Grund ist der, dass man meist den Jahresabschluß im neuen Jahr macht. Diesen Abschluß muss man dann 10 Jahre aufbewahren. Um ihn zu belegen müssen aber dann auch die Belege dazu vorhanden sein und die sind halt ein Jahr älter. Ich wusste das auch nicht, habe es aber wo gelesen.

Bildschirmfoto_20240725_130413

Jetzt kann man reviewen.

JohannMaierhofer commented 1 month ago

@SchachtnerTh , ich habe es geschafft, schau dir es mal an. Mit gefällt es jetzt besser und man kann es zukünftig auch noch erweitern.

lenilsas commented 1 month ago

Mir gefällt das so schon sehr gut. In zwei Tabellen bleiben jedoch noch alte Einträge erhalten. Die sollten auch gelöscht werden können, finde ich: -abrechungslauf -zusatzabbuchung Bei den Zusatzabbuchungen muss jedoch auch auf Intervall und ggfs. Endedatum geprüft werden

lenilsas commented 1 month ago

Ich habe für das Mitglieder Löschen Feature die Foreign Keys geändert, so dass man Mitglieder löschen kann und die Referenzen in den Buchungen werden einfach auf null gesetzt.

Ich habe nicht gefunden wo du die Änderung gemacht hast. Ich denke die würde dann auch in diesen PR gehören.

JohannMaierhofer commented 1 month ago

Ich habe für das Mitglieder Löschen Feature die Foreign Keys geändert, so dass man Mitglieder löschen kann und die Referenzen in den Buchungen werden einfach auf null gesetzt.

Ich habe nicht gefunden wo du die Änderung gemacht hast. Ich denke die würde dann auch in diesen PR gehören.

Ja richtig, ich habe ja die Reihenfolge vertauscht. Ich muss den Teil der Migration hierher verschieben. Bei mit ist da ja schon gelaufen.

JohannMaierhofer commented 1 month ago

Mir gefällt das so schon sehr gut. In zwei Tabellen bleiben jedoch noch alte Einträge erhalten. Die sollten auch gelöscht werden können, finde ich: -abrechungslauf -zusatzabbuchung Bei den Zusatzabbuchungen muss jedoch auch auf Intervall und ggfs. Endedatum geprüft werden

Ja, Zusatzabbuchungen sollte man nur löschen wenn das Endedatum erreicht ist. Einen Abrechnungslauf darf man nur löschen wenn er von keiner Buchung, Lastschrift, Mitgliedskonto und Zusatzabrechnungslauf mehr referenziert wird.

Mir fallen auch selbst noch weitere Einträge ein z.B.

Können wir das so machen, dass wir das Feature jetzt erst mal so übernehmen, dann ist es auch nicht ganz so gross. Ich überlege mir dann bei schlechterem Wetter wie man den Rest dann hinzufügen kann und mache das mit eigenen PRs.

JohannMaierhofer commented 1 month ago

Habe die Migration um den fehlenden Teil erweitert.

lenilsas commented 1 month ago

Ja das stimmt, ich kann es auch erst mal so aprooven. Was auch noch fehlt sind die Buchungsdokumente. Die Einträge in der Datenbank werden gelöscht, die Dokumente selbst bleiben aber erhalten, da hier größere Datenmengen zusammenkommen können wäre es auch gut wenn man das irgendwann noch mit einbaut.

SchachtnerTh commented 1 month ago

Ich will das nicht jetzt noch sehr viel länger rauszögern, aber ich habe gerade eine mögliche Lösung für das Problem gefunden, dass es bei MySQL / MariaDB kein NOCHECK gibt. Da steht auf deren Seite:

grafik

Veilleicht könnten wir ja das (mit NOT) einbauen, bevor der FOREIGN KEY angelegt wird und danach dann das Gleiche nur halt ohne (NOT). Das würde die Exception vermeiden. Hab ich aber nicht ausprobiert. Fällt mir nur gerade auf.