-
Notifications
You must be signed in to change notification settings - Fork 719
Information pane shows filtered transactions but allows editing #5076
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
base: master
Are you sure you want to change the base?
Conversation
|
Hello, sorry for the long delay, I will try to look into it this week end. |
|
For the logo, I think I found a way to fix, with unwrap().getOwner(). The issue was actually from my initial commit with only the inject. I am still looking into the rest. I am wondering, should the internally derived transaction always be shown in the transaction lists ? PP needs the internally derived transaction for the internal calculation when a filter selection happens, but when it is only for visual listing is there still this need ?
Example : |
I see logos also for the "ReadOnlyAccount" which should work because the attributes are copied over into the ReadOnlyAccount (and the logo is an attribute). Can you share the patch? (I am not sure if you can create a pull request against my branch to have it included here)
Agree. About the logo: I was looking at Iconmonstr where I usually get the icons from. But maybe we use something that relates back to the filter icon? And, yes, adding a tooltip and a label to the context menu such as "virtual transaction" also makes sense.
Can you share an example? The filtering and the creation of 'derived' transactions happens in the PortfolioClientFilter. I could think of scenarios that are confusing: say for example, the user is not maintaining the cash account and always has a withdrawal for each dividend transaction. If now only the portfolio is added to the filter, then the dividend transactions are included in the filter (the original) but because the account is not included, there are virtual withdrawals created (which match the actual withdrawals but which are not included).
I tend to agree. The 'derived' transactions can be confusing. I am not sure anymore if it improves the user experience. About showing the "original transactions": We would need at least one more "annotation" to separate whether the original transaction was just converted (purchase -> delivery) or enhanced (add a corresponding removal for the dividend transaction). Let's go back to square one: initially, PP was showing all transactions and all transactions as the original. That list was showing the purchase (not the inbound delivery) and the cash transfer (and not the withdrawal). What was it showing "too much" in the case of an applied filter? |

This pull request includes and then extends #5025.
"Derived" transactions are now marked with a little calculator. For example, if there is a cash transfer from account A to B, but only account B is included in the filter, then this transfer is converted into a "derived" deposit.
Derived transactions cannot be edited - neither in-place in the table nor with the dialogs. All other options should work, e.g. create a new transaction for the given account, etc.
@mierin12 I would appreciate if you could help testing this change. Particularly that the all editing functions work. And test if no "derived" transaction leak into the actual model.