Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement dikku2 #80

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Implement dikku2 #80

wants to merge 2 commits into from

Conversation

henryk
Copy link
Contributor

@henryk henryk commented Jun 10, 2019

This implements DKKKU/DIKKU for fetching credit card transactions from f.e. comdirect.

I'm not quite happy with the API yet: The bank response includes additional data (card number and balance) but also a list of transactions for which a touchdown_point may be in use. How do we want to return that? Concatenate the transaction lists and return the other information separately (secondary question: from the first response or from the last response?)? Create a special container object? Just discard the additional information?
(Currently the function just returns a list of DIKKU2 objects)

@henryk
Copy link
Contributor Author

henryk commented Jun 10, 2019

Fixes #73

@codecov
Copy link

codecov bot commented Jun 10, 2019

Codecov Report

Merging #80 into master will increase coverage by 0.07%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #80      +/-   ##
==========================================
+ Coverage   88.79%   88.87%   +0.07%     
==========================================
  Files          23       23              
  Lines        3035     3056      +21     
==========================================
+ Hits         2695     2716      +21     
  Misses        340      340
Impacted Files Coverage Δ
fints/segments/statement.py 100% <100%> (ø) ⬆️
fints/formals.py 98.55% <100%> (+0.04%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 089961c...4e53b41. Read the comment docs.

@raphaelm
Copy link
Owner

A special container object is probably the cleanest solution? I'm not totally sure either, though.

@Ferenc-
Copy link

Ferenc- commented Jul 3, 2021

This is a super useful PR, it would be nice to see this eventually included.
One suggestion though.
My observation is that, in case there are really no transactions, because the card is so new,
or the get_credit_card_transactions call is limited to a timeframe in which there were, no transactions,
then these placeholder fields:

    _unknown_2 = DataElementField(type='an')
    _unknown_3 = DataElementField(type='an')

are also missing not just the transactions field,
so one gets a warning:

fints/parser.py:163: FinTSParserWarning: Ignoring parser error and returning generic object: Required field DIKKU2._unknown_2 was not present. Turn off robust_mode to see Exception.

And then not even, the otherwise available balance field gets parsed, which would be nice to have in many scenarios,
like when there were no transactions in the queried period, but the balance is still a relevant value.

So I would suggest to go with:

   _unknown_2 = DataElementField(type='an', required=False)                                                                                                                          
   _unknown_3 = DataElementField(type='an', required=False)

This has been working well for me so far.

@mtremer
Copy link

mtremer commented Aug 22, 2024

I can also confirm that these changes work very well with Sparkasse.

Is there any chance this could be merged?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants