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

update transactions view when reports are updated #137

Merged
merged 1 commit into from
Feb 15, 2017
Merged

Conversation

hrj
Copy link
Owner

@hrj hrj commented Feb 15, 2017

closes #1

remember the filter when showing list of transactions, and use it to
show fresh list of transactions when report is updated
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.9%) to 69.324% when pulling a80b211 on i1-update-txns into fbcf267 on master.

@hrj
Copy link
Owner Author

hrj commented Feb 15, 2017

cc @gerdreiss

Please review / test. In short,

  • A selection filter is saved which stores the group name or (account name + group name) for the list of transactions.
  • When reports are refreshed, we try to match the saved filter with the current register report entries
  • Transaction view is updated with matching entry
  • Register report title is also saved and matched

This is a relatively clean patch I think, and seems to work fine in my testing.

@hrj hrj added this to the Next release milestone Feb 15, 2017
@gerdreiss
Copy link
Contributor

gerdreiss commented Feb 15, 2017

Well, I'll have to admit, you put me to shame ;)
Your solution works just fine, and is simpler and more elegant (and, especially, more complete) than mine. I would've tried to work with date ranges next...
I hope my next pull request will be of more use ;)

Apropos, next pull request - since it's been so much fun so far, I'd like to continue working on the project, if that's fine with you. So which issue should I take on next, in your opinion? Maybe a couple of those low hanging fruits, so I have some more opportunity to learn the code?

@hrj
Copy link
Owner Author

hrj commented Feb 15, 2017

Well, I'll have to admit, you put me to shame ;)

Oh well, I am just a bit more familiar with my own hacks and requirements; so it's a bit easy 😄

Your solution works just fine, and is simpler and more elegant (and, especially, more complete) than mine. I would've tried to work with date ranges next...

To be honest, I also wanted to use date ranges and have proper filters. What I have coded is a little hacky, from that view point.

Apropos, next pull request - since it's been so much fun so far, I'd like to continue working on the project, if that's fine with you.

Yeah sure, of course!

So which issue should I take on next, in your opinion? Maybe a couple of those low hanging fruits, so I have some more opportunity to learn the code?

Certainly, that would be perfect.

I will merge this PR in, bundle a new release, and try to find an issue for you if you don't find one before me.

@hrj hrj merged commit a2716cd into master Feb 15, 2017
@gerdreiss
Copy link
Contributor

Sounds great! In the meantime I'll close my PR...

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.

Update transactions view when reports are updated
3 participants