Upstream/feature pdf import

[simpsus]

No description provided.

[simpsus]

Tut mir leid dass dieser branch so weit vornedran ist; den habe ich von master gebranched.
Er enthält einfach einen Extractor der aus den comdirect PDF files die relevanten Sachen holt.

Bei dem kleinsten Funken positiven Feedbacks baue ich das in das von @abuchen geschaffene Framework ein.

[buchen]

Die ganzen extra Commits tauchen nur auf, weil Du auf den "feature_pdf_import" branch pushen möchtest, aber vom master gebrancht hast. Und der master ist mittlerweile etwas weiter, das ist aber auch nicht schlimm.

Erstens bin ich überrascht wie wenig Zeilen Code es braucht... Hätte ich nicht gedacht.

Ich kenne jetzt den Code von @tamueller noch nicht im Detail. Darum kann ich nicht gut beurteilen wo man noch Ähnliches refactorn könnte. Aber im Prinzip habe ich nichts dagegen, dass in den "Extractor" zu übernehmen.

Mir ist aufgefallen, Du verwendest Java 7 Features. Momentan versuche ich noch zu Java 6 kompatibel zu bleiben, weil das auf verschiedenen Plattformen doch einfacher zur Verfügung steht (vor allem auf dem Mac).

[simpsus]

wenige Zeilen: Das Parsen ist wirklich nicht viel. Ich würde das Feature aber zuerst mal beta releasen, denn das parsen ist noch instabiler als HTML parsen. Es gibt einfach keine Tags. Im Moment arbeite ich mit Zeichenabständen in dem extrahierten Text. Bei meinen ~15 Beispiel PDFs funktioniert das. Keine Garantie dass das bei allen funktioniert. Man kann das bestimmt auch noch robuster machen (zB schauen ob danach Leerzeichen kommen).

Java7: welche Features sind das? Dann kann ich die Java6 kompatibel machen

Einbindung in Rahmen: Momentan hab ich keine Innere Klasse für das Resultat. Das passt logo nicht. Das hab ich nur gemacht weil ich das File in nem anderen Projekt entwickelt habe ... durch die Massen von Exceptions die ich in PP bekomme kann ich da nicht debuggen.
Mein Extractor verwendet im Moment auch den Dateinamen. Einfach weil da 3/4 der Informationen schon drin stehen und der Typ des PDFs hiermit einfachst zu erkennen ist. Das ist in dem momentanen Rahmen nicht so.
Ich parse auch nicht nur eine Datei, sondern scanne ein Verzeichnis. Der Aufwand eine Datei auszuwählen und dann die Transaktion daraus zu bestätigen ist genauso hoch wie die Transaktion einzeln anzulegen. Ich will die PDFs im Batch verarbeiten.

[buchen]

Ich parse auch nicht nur eine Datei, sondern scanne ein Verzeichnis. Der Aufwand eine Datei auszuwählen und dann die Transaktion daraus zu bestätigen ist genauso hoch wie die Transaktion einzeln anzulegen. Ich will die PDFs im Batch verarbeiten.

Momentan bekommst Du im Extractor auch einen Anzahl von Dateien:

https://github.com/buchen/portfolio/blob/feature_pdf_import/name.abuchen.portfolio/src/name/abuchen/portfolio/datatransfer/Extractor.java#L212

In einem Verzeichnis gerade Strg-A oder per Shift einen Anzahl von Dateien auswählen halte ich für vertretbar.

[simpsus]

Ah ja. Das hatte ich so garnicht gesehen. Dann gut, dass ich das intuitiv genauso gemacht hab...

[simpsus]

ComdirectPDFExtractor erstellt und dem Extractor Interface angepasst.
Komischerweise sehe ich garkeinen Menueintrag hier, also auch keinen für ING.
Dabei ist das in der Application.e4xmi zu sehen...

Mache ich hier was falsch?

[buchen]

Nach jeder Änderung an dem Application.e4xmi muss man einmal mit dem Flag clearPersistedState starten. Wenn das auch nicht hilft, dann muss man weitersuchen.

[buchen]

Der Travis Build schlägt wegen OSGi Abhängigkeiten fehl. Wenn Du auf dem allerletzten Master commit aufsetzt, sollte es wieder tun.

[simpsus]

So, also der Menueintrag ist jetzt da.
In einer Kopie von meinem Portfolio werden die Transactionen aus den PDFs erstellt. Funktioniert super!!

1. Das setzen der Stückzahl funktioniert nicht. Hier wird

purchase.setShares(stueck.longValue() \* Values.Share.factor()); benutzt. in stueck steht die richtige Zahl mit Komma. Danach kommt da aber kein Komma bei raus, weil longValue eine Ganzzahl rauswirft. Muss ich hier das Resultata von stueck.doubleValue() \* Values.Share.factor() in ein long parsen?

2. Wenn die Security nicht per ISIN gefunden wird, werde ich momentan ne RuntimeException. Ich fände es nicht soo geil wenn automatisch Securities angelegt werden.
   Generell sollte man dem User finde ich ein Feedback geben, was genau gemacht wurde, also welche Transaktion aus welcher Datei geparsed wurde und aus welchen Dateien nix geparsed wurde. Was meinst Du? Ich würde mir so eine Art log Bereich im Wizard vorstellen. Da könnte man das dann ja schreiben: "Kauf aus Datei XXX konnte nicht erstellt werden, weil Security mit der ISIN XYXYXX nicht vorhanden."

3. Gebühren parse ich momentan nicht raus. Muss ich noch rein machen.

[buchen]

zu 1)

Muss ich hier das Resultata von stueck.doubleValue() * Values.Share.factor() in ein long parsen?

Am besten noch runden: Math.round(stueck.doubleValue() * Values.Share.factor())

zu 2)

Wenn die Security nicht per ISIN gefunden wird, werde ich momentan ne RuntimeException. Ich fände es nicht soo geil wenn automatisch Securities angelegt werden.

Ein fehlendes Wertpapier sollte man anlegen. In dem Beispiel habe ich das auch so gemacht:

        // create a new security if not found in client.getSecurities()
        Security security = new Security("BASF", null, "BAS.DE", YahooFinanceQuoteFeed.ID); //$NON-NLS-1$ //$NON-NLS-2$
        answer.add(new SecurityItem(security));

In der Ergebnisliste sieht der Benutzer dann, dass ein neues Wertpapier angelegt wird.

Generell sollte man dem User finde ich ein Feedback geben, was genau gemacht wurde, also welche Transaktion aus welcher Datei geparsed wurde und aus welchen Dateien nix geparsed wurde.

Man könnte auch eine Spalte "Quelle" hinzufügen - da könnte dann der Dateiname stehen. Oder eben einen Log/Meldungsbereich. Im Prinzip habe ich da kein Problem mit wenn es den Dialog nicht unnötig kompliziert macht. Vielleicht "klappt" man den Meldungsbereich auch nur aus.

Von mir aus kannst Du den Teil auch erst mal weglassen und in einem separaten Pull Request machen. Diesen ersten Change würde ich erst mal drin haben. Und dann die Merges kreuz und quer etwas aufräumen so dass wir eine schöne Historie haben.

[buchen]

Der Build schlägt übrings fehlt wegen Java 7 Syntax:

[ERROR] catch (IOException | ParseException e)
[ERROR] ^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Die musst Du noch separat catchen. Ich baue PP immer noch für Java 6. Zum Beispiel auf Mac OS X haben die meisten nur Java 6, wenn überhaupt. Und man muss bei neueren Versionen zwingend das JDK installieren, das JRE reicht nicht. Ich überlege schon, das JRE im Download mitzuliefern. Wenn der dadurch nur nicht so groß werden würde... dann könnte man freier die Java Version ändern.

[simpsus]

mit 8bca4bd werden nun auch Gebühren aus Käufen geparsed, ich habe zwei Fehler beseitigt und das ganze ist deutlich robuster.
Jetzt fehlt nurnoch das Anlegen von Securities

[buchen]

Cool. Ich schaue mir das am WE an und merge das ggf. in den Branch. Ich hatte bei der Comdirect mal ein Tagesgeldkonto - die Auszüge müsste ich ja passen können.

[simpsus]

Tagesgeldkonto....
also momentan kann mein extractor gutschriften und kaufabrechnungen einlesen. Andere Dokumente habe ich nicht. Um das zu erweitern muss ich eigentlich nur ein beispieldokument haben.

Das hat dann zwei schritte:

• Art des Dokuments erkennen
• Daten rausparsen

Der erste Schritt macht das ganze robuster, denn es gibt die gefahr von false positives, also dass man versucht einen kauf aus einem verkaufsdokument zu lesen

[simpsus]

Fehlende Securities werden jetzt angelegt mit name, ISIN und WKN.
Ich habe das ganze versucht ein wenig robuster zu machen.
Dabei fällt eigentlich fast ein "StringWalker" ab, der einen String mit

getNextWord, getNextRow, getLastWordInCurrentRow usw...

durchläuft. Ich hab ein wenig gesucht aber nichts entsprechendes gefunden.
Falls die anderen PDF Extractoren davon auch profitieren könnten, dann kann ich das ja auslagern.
@buchen Wenn ich irgendwie meine Hände an andere Arten von PDFs bekomme (gerne auch mit Fantasiedaten) dann könnte ich die einbauen.

[buchen]

Ich bin jetzt dazu gekommen mir die Änderung mal anzuschauen. Leider habe ich selber keine PDF Auszüge von der comdirect. Und mein Konto habe ich vor einiger Zeit aufgelöst.

Das bringt mich auch direkt zum ersten Punkt: Könntest Du einen Test hinzufügen? Vielleicht könnte man den Text eines PDFs extrahieren und das als "Testdaten" einchecken. Den Text kannst Du dann anonymisieren.

Der zweite Punkt ist das Fehler Handling: Mir ist aufgefallen, dass es keine gute Möglichkeit gibt Fehlermeldungen an den Benutzer zurückzugeben. Ich schlage vor, die Methode List<Item> extract(List<File> files, List<Exception> errors); um das Feld errors zu erweitern. Dann kann man zentral alle Meldungen sammeln und anzeigen. Füge doch bitte das Feld hinzu - im Dialog kannst Du das einfach mal ignorieren, dass ändern wir mit einer anderen Change.

Wenn Du ein Wertpapier anlegst (hier security = new Security(name, isin, null, "MANUAL");), dann solltest Du Dir das merken damit weitere Transaktionen die sich auf das selbe Wertpapier beziehen (und nicht noch mal ein Wertpapier angelegt wird). Und es sollte der Ergebnisliste mit new SecurityItem(security) hinzufügt werden, damit der Benuzter sieht, dass Du ein neues Wertpapier anlegst.

Und dann noch einige Kleinigkeiten:

• Label für comdirect Import fehlt in ~/n.a.portfolio.ui/OSGI-INF/l10n/bundle.properties
• Java 7 Code, z.B. String filename = f.toPath().getFileName().toString()
• Visibilität der Attribute vom ComdirectPDFExtractor
• Lustiger Mix von Deutsch und Englisch in den Kommentaren: "Thesaurierend? Do nothing!" - Bitte Englisch
• Keine Meldung in die Console: System.err.println("Could not snif type from text " + filename); - entweder als Exception / Fehler bis zum Benutzer, oder per PortfolioPlugin.log() ins Fehlerprotokoll.
• Keine Exception auf die Console ausgeben (e.printStackTrace();) sondern zurückgeben.

Dabei fällt eigentlich fast ein "StringWalker" ab [...] Falls die anderen PDF Extractoren davon auch profitieren könnten, dann kann ich das ja auslagern.

Der nächste Extractor, der sowas braucht, sollte das dann extrahieren. Auf Verdacht hin sollte man da nix machen. Und mit den Tests sollte man das dann recht problemlos refactoren können.

Der Pull Request ist mittlerweile schwer zu lesen. Wenn Du oben auf "Files Changed" wechselst, dann sieht man viele Änderungen die gar nicht von Dir gemacht wurden. Das liegt an den diversen Merges. Darum würde ich diesen Change relativ schnell abschliessen und einen neuen "drauf" setzen.

[simpsus]

Vielen Dank für Dein Feedback. Ich habe versucht alles zu machen.
Folgendes habe ich nicht geschafft:

Test: Es ist für mich etwas schwer fest zu stellen, was ich da testen soll. Dass aus einer "Testdatei" die richtigen Transaktionen geparsed werden? Hast Du ein Beispiel an dem ich mich orientieren kann? Mit welchen Parametern wird so ein Test aufgerufen?

Exceptions: Warum auch immer, bei mir kann er PortfolioPlugin nicht resolven. In anderen Dateien des selben Projekts ist der Aufruf drin und funktioniert, ohne import. Wenn ich den Fehler ignoriere beendet er zur Laufzeit die ganze Aktion.

Java 7 Syntax. Das eine Beispiel habe ich angepasst. Das dumme ist ich erkenne keine Java 7 Syntax wenn ich sie sehe. Ich google einfach wenn ich was nicht weiß und dann ist der neueste Hit halt von der neusten Version. Wenn noch was da is nehm ich mich dem gerne an.

Wir können diesen PR gerne zu machen und auf einen neuen von master aufmachen.

[simpsus]

Habe versucht einen Test hinzuzufügen.
Das mit den File Objekten klappt beim speichern von Textdateien nicht mehr so ganz. Auch PDFText verwendet man dann nicht mehr.

Leider hat es meinem Eclipse überhaupt nicht gefallen, das ich im Manifest des test Projektes die Dependency einbauen wollte (in dem Issue von @buchen geschrieben). Daher konnte ich den Test nicht laufen lassen. So könnte das aber aussehen.

Die beiden Textdateien sind sicherlich nicht an der optimalen Stelle. So konnte ich sie ohne kompliziertes und absoluten Pfad ansprechen. Die Strings in die Klasse zu machen wäre wegen den Zeilenumbrüchen ein großer Schmerz gewesen.

Zu guter Letzt habe ich noch den aktuellen Master gemerged damit der branch einfacher zu pullen ist.

[buchen]

Zu guter Letzt habe ich noch den aktuellen Master gemerged damit der branch einfacher zu pullen ist.

Vorsichtig! Schau Dir mal die Liste der "Files changed" von diesem Pull Request an. Da sind mittlerweile 94 Dateien aufgeführt. Klar muss man manchmal mergen (vor allem wenn der Build nicht tut)... das verstehe ich schon. Einfacher ist es, wenn man das Feature separat macht und den Merge beim "Merge pull request" macht!

Das mit den File Objekten klappt beim speichern von Textdateien nicht mehr so ganz. Auch PDFText verwendet man dann nicht mehr.

Klar - PDFText und das Einlesen aus der Datei würde man nicht mehr testen. Aber das Parsen des Textes - und da liegt die meiste Logik, oder? Und die ist ggf. recht fragil weil sie auf reinen Text angewiesen ist.

Der Test liegt doch an der richtigen Stelle. Ich würde noch

• die Textdateien (Gutschrift.txt + Wertpapierabrechnung_kauf.txt) in das selbe Package legen. Mit getResourceAsStream kann man die recht einfach einlesen.
• den Test auch ComdirectPDFExtractorTestnennen. Die Namenskonvention ist immer Klassennamen + "Test". Dann weiß man sofort was hier getestet wird.

Die Dependencies hast Du aber hinbekommen, oder? Der Test scheint ja wegen etwas anderem fehlzuschlagen:

Tests in error: 
  testKauf(name.abuchen.portfolio.datatransfer.ComdirectPDFImportTest): No match found

[simpsus]

Die Tests sind bei mir nie gelaufen. Ich bekomme da classnotfound. Wenn ich das Manifest um das requirement von pdftext erweitere gibt er da nen fehler, dass der header mit ner newline beendete werden muss.
Auch irgendeine log libraray findet er beim testen nicht.

Deshalb sind die Tests auch so generisch. Man könnte das noch genauer testen als das 2 Werte in der Liste zurückkommen. Der Test ist aber meines Erachtens eh sehr wackeling:

Es stimmt vollkommen, dass die Logik nicht sehr robust ist. Aber sie ist ja gerade mit diesen Testdaten aufgebaut. Wenn dann kommt die Logik mit Dateien von anderen Personen ins Straucheln. Wenn sie schon diesen Test nicht schafft, dann ist eigentlich hopfen und Malz verloren.

[buchen]

Es stimmt vollkommen, dass die Logik nicht sehr robust ist. Aber sie ist ja gerade mit diesen Testdaten aufgebaut. Wenn dann kommt die Logik mit Dateien von anderen Personen ins Straucheln. Wenn sie schon diesen Test nicht schafft, dann ist eigentlich hopfen und Malz verloren.

Dafür braucht es ja den Test. Wenn die nächsten Beispiele kommen und gefixt werden, muss man sicherstellen, dass die alten Tests weiterhin funktionieren.

Außerdem habe ich bisher verstanden, dass Du PDF Dokumente von mehreren Jahren hast. Damit müsste doch der Code schon etwas generischer sein, oder?

Die Tests sind bei mir nie gelaufen.

Zentral scheint er zu laufen. Was machst Du lokal anders? Lässt Du das auch mit Maven laufen?

[simpsus]

Die Struktur der Dokumente ist halt konstant.
Die Logik arbeitet so: "Die ISIN ist das letzte Wort in der nächsten Zeile nach der Zeile in der 'Geschäftstag' vorkommt" So robust ist sie.

Ich muss gestehen ich habe in eclipse Run=>JUnit Test gemacht. Wie starte ich den maven Test?

[buchen]

Ich muss gestehen ich habe in eclipse Run=>JUnit Test gemacht. Wie starte ich den maven Test?

Aus Eclipse heraus musst Du "JUnit Plug-in Tests" laufen lassen (siehe auch das readme.md im Projekt).

Ansonsten von der Kommandozeile mvn clean install

[simpsus]

So jetzt sollte (hoffentlich) alles tun.

[buchen]

So jetzt sollte (hoffentlich) alles tun.

Travis sagt noch etwas anderes :-(
https://travis-ci.org/buchen/portfolio/builds/40585852

Ich habe es gerade mal bei mir ausprobiert. Wenn Du den Whitespace Fehler im Manifest fixed und das new Temp() wegnimmst, dann läuft der Test bei mir auch in der IDE. Unten der Patch dazu.

Wenn ich mir den Test anschaue, dann fallen mir noch ein paar Punkte auf:

• Der Methode buildClient ist doch überflüssig, oder? Ein new Client() würde reichen, da der Code doch überhaupt nicht auf ein existierendes Portfolio testet. Wenn, dann sollte man testen ob ein Wertpapier bei Bedarf angelegt wird oder nicht - also mal einen Client reinreichen, der ein Wertpapier hat oder nicht.
• assert (results.size() == 2); checkt ja relativ wenig - man könnte checken ob die richtigen Buchungen gefunden wurden.

Ansonsten bin ich die nächsten Tage geschäftlich unterwegs (devoXX in Antwerpen) und komme wohl eher nicht zu einem weiteren Review.

diff --git a/name.abuchen.portfolio.tests/META-INF/MANIFEST.MF b/name.abuchen.portfolio.tests/META-INF/MANIFEST.MF
index 1c0cb2a..e1e1902 100644
--- a/name.abuchen.portfolio.tests/META-INF/MANIFEST.MF
+++ b/name.abuchen.portfolio.tests/META-INF/MANIFEST.MF
@@ -14,4 +14,3 @@ Require-Bundle: org.junit;bundle-version="4.11.0",
  org.apache.pdfbox;bundle-version="1.8.7",
  org.apache.pdfbox.fontbox;bundle-version="1.8.7",
  org.apache.pdfbox.jempbox;bundle-version="1.8.7" 
- 
\ No newline at end of file
diff --git a/name.abuchen.portfolio.tests/src/name/abuchen/portfolio/datatransfer/ComdirectPDFExtractorTest.java b/name.abuchen.portfolio.tests/src/name/abuchen/portfolio/datatransfer/ComdirectPDFExtractorTest.java
index 957bed1..d22c385 100644
--- a/name.abuchen.portfolio.tests/src/name/abuchen/portfolio/datatransfer/ComdirectPDFExtractorTest.java
+++ b/name.abuchen.portfolio.tests/src/name/abuchen/portfolio/datatransfer/ComdirectPDFExtractorTest.java
@@ -1,11 +1,8 @@
 package name.abuchen.portfolio.datatransfer;

-import java.io.BufferedReader;
-import java.io.FileNotFoundException;
-import java.io.IOException;
-import java.io.InputStreamReader;
 import java.util.ArrayList;
 import java.util.List;
+import java.util.Scanner;

 import name.abuchen.portfolio.datatransfer.Extractor.Item;
 import name.abuchen.portfolio.model.Account;
@@ -23,36 +20,11 @@ public class ComdirectPDFExtractorTest

     public ComdirectPDFExtractorTest()
     {
-        try
-        {
-            BufferedReader reader = new BufferedReader(new InputStreamReader((new Temp()).getClass()
-                            .getResourceAsStream("/name/abuchen/portfolio/datatransfer/Gutschrift.txt")));
-            StringBuilder out = new StringBuilder();
-            int c;
-            while ((c = reader.read()) != -1)
-            {
-                out.append((char) c);
-            }
-            gutschriftText = out.toString();
-            reader.close();
-            reader = new BufferedReader(new InputStreamReader((new Temp()).getClass().getResourceAsStream(
-                            "/name/abuchen/portfolio/datatransfer/Wertpapierabrechnung_Kauf.txt")));
-            out = new StringBuilder();
-            while ((c = reader.read()) != -1)
-            {
-                out.append((char) c);
-            }
-            kaufText = out.toString();
-            reader.close();
-        }
-        catch (FileNotFoundException e)
-        {
-            e.printStackTrace();
-        }
-        catch (IOException e)
-        {
-            e.printStackTrace();
-        }
+        gutschriftText = new Scanner(getClass().getResourceAsStream("Gutschrift.txt"), "UTF-8").useDelimiter("\\A")
+                        .next();
+
+        kaufText = new Scanner(getClass().getResourceAsStream("Wertpapierabrechnung_Kauf.txt"), "UTF-8").useDelimiter(
+                        "\\A").next();
     }

     @Test

[buchen]

Hi @simpsus,

ich habe den Pull Request jetzt gemergt. Und zwar in den Branch "feature_pdf_import".

Um den mit dem Feature / Branch einfacher zu arbeiten, habe ich jetzt folgendes gemacht:

• Merge von master in feature_pdf_import
• Rebase Deiner Changes auf feature_pdf_import
• Squash Deiner einzelnen Commits in einen grossen Feature Commit

Auf dieser Change mache ich dann noch ein paar kleine Änderungen zum Exception Handling.

Und dann mache ich ein neues Ticket auf für Änderungen die notwendig sind, damit wir das überhaupt "ausliefern" können (Fehler anzeigen, etc.).

Diesen Pull Request können wir dann schliessen. Alle neuen Änderungen am einfachsten in einem Branch auf dem neuen Commit. Wenn die entsprechend klein sind, dann kann ich die auch direkt über das UI hier mergen. Bei diesem Pull Request war mir das zu groß und unübersichtlich geworden.

Nur damit das einigermassen Sinn macht, hier der Screenshot vor dem interactive rebase :-)