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

Add support for getting final opening / closing balance on each transaction #19

Conversation

benkonrath
Copy link
Contributor

@benkonrath benkonrath commented Mar 11, 2016

See #18 for explanation.

@WoLpH
Copy link
Owner

WoLpH commented Mar 11, 2016

For some reason the tests appear to be failing but I'm pretty sure that's not related to your changes. I'm a bit busy at the moment but I'll have a look within a few weeks :)

@WoLpH
Copy link
Owner

WoLpH commented Mar 11, 2016

Hmm, after running another test it does seem to be related to your changes but I'm not entirely sure how yet.

@benkonrath
Copy link
Contributor Author

benkonrath commented Mar 11, 2016

I actually think it is my code. When ever you have time to look into this, that's great.

This change seems to be working for me with an export from Knab and the monkey patch in #18 applied.

@benkonrath
Copy link
Contributor Author

benkonrath commented Mar 11, 2016

The issue with the current implementation (and the monkey patch) is that the opening balance isn't captured correctly because a new transaction is created with record 61 which comes after record 60F (opening balance).

I'm open to other solutions for this when you have time to look at it. Thanks.

@benkonrath
Copy link
Contributor Author

benkonrath commented Mar 11, 2016

I'm going to use this branch in my project and will update it if I have further changes. I'll try to update the tests as well.

@benkonrath
Copy link
Contributor Author

benkonrath commented Mar 11, 2016

I forgot to mention that an MT940 block with multiple 61 records will only have the final opening balance on the first transaction, and the final closing balance on the last transaction of the block. This is something that I can document if you want to merge this PR.

We could also generate these values if you think that's better for consistency.

@benkonrath benkonrath force-pushed the opening-closing-balance-per-transaction branch from 2d03193 to 5e750ed Compare Apr 15, 2016
@benkonrath
Copy link
Contributor Author

benkonrath commented Apr 16, 2016

Just a quick note to let you know that I'm still working on this so you shouldn't merge it. My other PRs come out of this work.

@benkonrath benkonrath force-pushed the opening-closing-balance-per-transaction branch from 1fd178d to eb3faf7 Compare Apr 16, 2016
@benkonrath benkonrath changed the title Create new transaction for TransactionReferenceNumber tag. Add support for getting final opening / closing balance on each transaction Apr 16, 2016
@coveralls
Copy link

coveralls commented Apr 16, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling eb3faf7 on benkonrath:opening-closing-balance-per-transaction into 03cfe0a on WoLpH:develop.

@benkonrath
Copy link
Contributor Author

benkonrath commented Apr 16, 2016

This is ready for review. There will be some merge conflicts with my other PRs so just let me know if you want me to rebase.

Some notes about the implementation:

  • I didn't make the transaction based final opening / closing balance the default behavior but instead provided directions on how to set it up.
  • The final opening and closing balance is not available on every transaction as it's not available in the STA file for each :61: tag.
  • You need to manually set the currency when using the final opening / closing balance on each transaction. This is related to how the currency is set on the Amount objects. The could be fixed but I'm not sure what the best approach would be.

@WoLpH
Copy link
Owner

WoLpH commented Apr 25, 2016

Thanks for the patch, I've merged it all and I'm running the tests now. As soon as all tests succeed I'll push out a new release :)

@WoLpH WoLpH closed this Apr 25, 2016
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.

None yet

3 participants