my-flow / fintex

Elixir-based client library for HBCI 2.2 and FinTS 3.0
Other
27 stars 17 forks source link

HITANS: Allow omitting of separator for last element if optional #7

Closed Trundle closed 8 years ago

Trundle commented 8 years ago

The last field of a TAN scheme ("Verfahrensparameter Zwei-Schritt-Verfahren") is optional in some element versions. Some FinTS implementations in the wild seem to omit the separator for the last data element in case of absence (e.g. Deutsche Bank).

Note that I'm by no means a FinTS expert and I have no idea whether that's syntactically allowed by FinTS. I simply noticed it happen and it makes kind of sense.

my-flow commented 8 years ago

Thanks, @Trundle! Could this problem be solved by just adding an additional parameter [nil] as leftover to the Enum.chunk/4 function? Could we use something like Enum.drop(offset) |> Enum.chunk(params_count, params_count, [nil])?

Trundle commented 8 years ago

I'd say it depends on how malformed responses should be handled. Before the PR, if a TAN scheme has too few elements, it would get omitted. Although arguably that only helps if the last entry has too few elements - if all entries have too few elements, the result would be garbage anyway. The pull request in its current state keeps exactly that behaviour, but allows the very last element of the TAN scheme group to be absent if allowed by the FinTS specification.

With Enum.chunk(params_count, params_count, [nil]) it's possible that last TAN scheme of the return value has too few elements. With the current solution the last TAN scheme would get discarded silently (and the ones before are probably garbage). As both solutions have its flaws, it probably makes sense to go with the simpler code (i.e. Enum.chunk(params_count, params_count, [nil])).

Opinions?

my-flow commented 8 years ago

I would prefer to go with the solution that requires less code (or less code changes) and is easier to understand, so just defining a leftover parameter which would solve the new test case (which is based on a troubling real-world scenario, I assume).

Let’s fix other (so far theoretical) cases -- such as 2 or more trailing data elements were omitted, missing data elements in a TAN scheme other than the last one -- only if we really find them in the wild, not proactively.

Trundle commented 8 years ago

The test case is based on a response from a Deutsche Bank account.

I updated the pull request.

my-flow commented 8 years ago

Thank you!