Fix Sepa Check

[JohannMaierhofer]

Diese PR korrigiert den falschen SEPA Check wie in #568 beschrieben.

Den Code gab es an zwei Stellen.

Es gibt noch eine kleine Unschönheit. Wenn man manuell eine Lastschrift erzeugt wird diese direkt in Hibiskus eingetragen. Es gibt keine Entsprechung in JVerein. Wenn man also später wieder Lastschriften macht werden die manuellen Lastschriften beim Sepa Check nicht berücksichtigt.
Man kann auch Lastschriften löschen.
Evtl. wäre es besser wenn das Datum der letzten Lastschrift als Attribut des Mitglied gespeichert wird. Aber das wäre dann ein neuer PR.

[tolot27]

Warum nicht gleich den Code zu einer Helper Funktion zusammenführen, so dass wir Redundanzen reduzieren?

[JohannMaierhofer]

Warum nicht gleich den Code zu einer Helper Funktion zusammenführen, so dass wir Redundanzen reduzieren?

Der Code für die manuelle Lastschrift Erstellung ist etwas anders als beim Abrechnungslauf. Hier kann der User jeweils im Dialog auswählen, dass er trotzdem weiter machen will.
Im Abrechnungslauf gibt es auch keine Dialoge für die Fehlermeldung.

Wäre es gleich hätte ich es zusammen gefasst.

[JohannMaierhofer]

Ich habe jetzt die sepagültigkeit verschoben und den Text geändert.
Das mit dem Mitglied verschieben finde ich persönlich etwas übertrieben. Selbst der zu verschiebende Teil ist ja nicht gleich. Einmal wird gelogged und das andere Mal gibt es einen Dialogtext. Und nur wegen der zwei kleinen If muss das ja nicht wirklich sein.

[tolot27]

Ich habe jetzt die sepagültigkeit verschoben und den Text geändert. Das mit dem Mitglied verschieben finde ich persönlich etwas übertrieben. Selbst der zu verschiebende Teil ist ja nicht gleich. Einmal wird gelogged und das andere Mal gibt es einen Dialogtext. Und nur wegen der zwei kleinen If muss das ja nicht wirklich sein.

Der Code ist bis auf das confirm nahezu identisch:

Loggen könnte man eigentlich immer oder auch gar nicht.
Beides ließe sich mit einem CallbackHandler realisieren.

Es geht ja darum, dass es wartbarer wird und man bei einem fix nicht nach weiteren Code Duplikaten suchen muss. So wird es zumindest in den meisten Projekten gehandhabt, die ich kenne. Müssen wir hier aber nicht so machen.

Wichtiger wäre mir, warum der erste Check in AbrechnungSEPA true zurück liefert und der gleiche check in einer identisch benannten Funktion (checkSEPA) in MitgliedLastschriftAction false zurück liefert.

Außerdem verstehe ich nicht, was mit dem confirmDialog bezweckt werden soll. Ich dachte, es muss ein Mandatsdatum vorliegen und es muss eine Lastschrift innerhalb der letzten 3 Jahre geben, falls ein Mandat älter als 3 Jahre ist. Wie kann man dann "Fortsetzen" und so ein true erzeugen. Ist dann so eine manuell erzeugte Lastschrift auf einmal gültig und bei einem Abrechnungslauf nicht, wo der identische Check zu false wird. Das finde ich inkonsistent.

PS: Hier wird SEPA auch automatisch mit Lastschrift assoziiert. 😏

[tolot27]

Das Refactoring von checkSEPA können wir auch später machen. Es hätte sich nur gerade angeboten.

[JohannMaierhofer]

Wichtiger wäre mir, warum der erste Check in AbrechnungSEPA true zurück liefert und der gleiche check in einer identisch benannten Funktion (checkSEPA) in MitgliedLastschriftAction false zurück liefert.

Beim Abrechnungslauf wird der Check immer aufgerufen unabhängig von der Zahlart . Wenn es also keine Lastschrift ist ist alles ok und man kann weiter machen. Nur bei Lastschrift muss man prüfen.
Beim manuellen Lastschrift erzeugen will man eine Lastschrift und es wird abgebrochen wenn das Mitglied nicht per Lastschrift bezahlt.

Außerdem verstehe ich nicht, was mit dem confirmDialog bezweckt werden soll. Ich dachte, es muss ein Mandatsdatum vorliegen und es muss eine Lastschrift innerhalb der letzten 3 Jahre geben, falls ein Mandat älter als 3 Jahre ist. Wie kann man dann "Fortsetzen" und so ein true erzeugen. Ist dann so eine manuell erzeugte Lastschrift auf einmal gültig und bei einem Abrechnungslauf nicht, wo der identische Check zu false wird. Das finde ich inkonsistent.

Ich denke, der Unterschied kommt daher, dass die manuelle Lastschrift direkt in Hibiscus eingetragen wird und nicht über JVerein verwaltet wird. Darum kann der User wohl auch selbst entscheiden ob es ein Mandat gibt oder nicht und ob es eine Lastschrift in den letzten 3 Jahren gibt. Da diese Lastschriften ja nicht in JVerein gespeichert werden kann JVerein das ja auch nicht abprüfen. Das ist aber alter Code und hat nichts mit meiner Fehlerkorrektur zu tun. Ich habe auf dieses Problem ja oben schon hingewiesen, wollte aber erst einmal den Fehler beheben. Ein Redesign ist da erst einmal nicht mein Scope.

Es ist halt dann generell die Frage ob man die manuelle Lastschrift überhaupt braucht. Die kann man ja auch gleich in Hibiscus anlegen. Dann gibts auch keinen doppelten Code.

[tolot27]

Okay, vielen Dank für die Erklärungen. So lerne ich mehr über den Code und JVerein kann zukünftig mehr contributen.