ccavanaugh / jgnash

jGnash Personal Finance
http://ccavanaugh.github.io/jgnash/
Other
139 stars 80 forks source link

mergeFiles skips last entry #73

Closed stefanhendriks closed 4 years ago

stefanhendriks commented 5 years ago

https://github.com/ccavanaugh/jgnash/blob/f38a9c4ed4343a0700f5e0e5bafaa44677401e29/mt940/src/main/java/net/bzzt/swift/mt940/parser/Mt940Parser.java#L113

I'm trying to use this implementation in my own software. I noticed that when passing 3 recordLines it ends up with 2 mergedLines. basically omitting the last line. This only is visible if you run it within a unit test (or you don't end with a -} recordLine).

Ie, this is a case where it happens (I made parseRecord public):

    @Test
    public void parseStatementLine() {
        Mt940Record mt940Record = Mt940Parser.parseRecord(list(
                "{1:F01KNABNL2HAXXX0000000000}{2:I940KNABNL2HXXXXN3020}{4:",
                ":20:B9A02MSEE326KVMR",
                ":61:1901011231C1,53N650NONREF//B8L31INTBB8ZCND1"  // this is skipped?
        ));

        Assert.assertEquals(1, mt940Record.getEntries().size());
    }

I am aware this is not a valid MT940 record.

I am not proficient in the MT940 format, still figuring things out myself. Perhaps you could clarify a bit what the mergeLines intents to do? I already figured it merges indeed some lines not starting with : (which seems to be correct behaviour).

At this point it looks like it basically skips the last line. Ie there is no retVal.add(currentString.toString()); happening for the very last line, be cause of:

if (inMessage) {
                if (string.startsWith(":")) {
                    retVal.add(currentString.toString());
                    currentString = new StringBuilder();
                }
                currentString.append(string);
            } else ....

This adds the previous line, (in currentString), sets the new currentString, but it never gets added to retVal.

Adding:

        retVal.add(currentString.toString());

just before returning retVal in the mergeLines function seems to fix the test I describe above. I'm not sure if that is correct.

Just wanted to let you know I found this. For my implementation I need other fields. Perhaps I can offer some kind of PR when things are fixed on my end? I also try to translate any german verbs along the way.

ccavanaugh commented 5 years ago

I wouldn't say I'm the most proficient with MT940 either. The majority of the code was contributed by Arnout Engelen per the Copyright. I've done a few bits here and there to maintain it, but I've not dug very deep into the workings of it.

Improvements are always welcome.

Are you encountering invalid MT940 from online / banking sources?

ccavanaugh commented 4 years ago

Closing due to inactivity

stefanhendriks commented 4 years ago

@ccavanaugh sorry, I haven't been able to spend time on this. I have a working MT940 implementation for our cases. I use integration tests to verify it works as expected. Although I did refactor a bit to make the code more testable. If you want - let me know - then I can send you some of my changes.