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 #134

Closed
wants to merge 3 commits into from
Closed

Update transactions view when reports are updated #134

wants to merge 3 commits into from

Conversation

gerdreiss
Copy link
Contributor

@gerdreiss gerdreiss commented Feb 11, 2017

Closes #1

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.9%) to 69.705% when pulling c05bce9 on gerdreiss:update-transactions-view into 97516f6 on hrj:master.

Copy link
Owner

@hrj hrj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for looking at this long-standing issue. I have noted one problem.

I don't know what the best solution is. Perhaps we could store the selection as a filter (date range + account name). When updating, we can then match all transactions against this filter. Seems inefficient, but reliable.

selectedItem = selectionModel().getSelectedItem
// show all transactions of the parent of the selected item if the latter has any transactions
// this will allow for an update of the transaction view when an input file changes
if (selectedItem.getValue.txns.nonEmpty) selectedItem = selectedItem.getParent
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When you change to parent, the transaction view includes more transactions than the selected one.

Try this example:

  • In "All" tab, select "Assets" and press Enter
  • In the register report for "Assets", select "2013 / Jan" / "Assets:Current:TDS" and press Enter.
  • Originally this showed transactions only for "Assets:Current:TDS", but now it will show all transactions in "2013 / Jan"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm aware of the problem, and you're right - it's not pretty. But I didn't know how to solve the problem that arose when selecting specific transactions for display - the update didn't work then. It only works for the root and the date range nodes.
So, your solution proposal isn't too bad at all - I'll look into it.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool.. you can use the TransactionFilter trait defined here.

@gerdreiss
Copy link
Contributor Author

Hi @hrj , I've fixed the problems you've mentioned before (although, I didn't use the TransactionFilter at the end) - it seems to work properly now, although the solution has gotten a bit complex. Maybe you'll see a way to simplify it, or to structure it in a better way. Just let me know if you think any more changes are necessary.
Cheers, gerdreiss

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.3%) to 69.224% when pulling 6056ba9 on gerdreiss:update-transactions-view into 97516f6 on hrj:master.

}
def filterSelectedDates(txns: Seq[DetailedPost]): Seq[DetailedPost] = {
if (selectedTxnDates.isEmpty) txns
else txns.filter(txn => selectedTxnDates.contains(txn.date))
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will only show newer transactions if they are on the exact same date as one of the previous transactions, right? That is not how I expect it to work. The transactions view is supposed to show all transactions for the selected date range and account name.

I am sorry if the expectation was not clear; the documentation is poor in this project.

@hrj
Copy link
Owner

hrj commented Feb 15, 2017

I think you are on the right track, in general.

But the implementation is not matching my expectations fully (probably because they are not well articulated). I will make a stab at implementing this now and set a pull request so that we can discuss my changes.

Thanks for inspiring me to work on this issue ✌️

@gerdreiss
Copy link
Contributor Author

Oh, ok, I understand. Embarrassingly, I haven't thought about adding new transactions at all! Well, at least I served as an inspiration ;)

@gerdreiss gerdreiss closed this Feb 15, 2017
@gerdreiss gerdreiss deleted the update-transactions-view branch February 15, 2017 19:05
@hrj
Copy link
Owner

hrj commented Feb 16, 2017

(Closed because it was superseded by #137 )

@gerdreiss I have created a new milestone and added few easier issues to it, if you wanna have a look.

@gerdreiss
Copy link
Contributor Author

Perfect, thanks! I'll take a closer look ;)

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.

3 participants