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 portfolio filter in Trades view #4005

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

mierin12
Copy link
Contributor

@mierin12 mierin12 commented May 8, 2024

Closes #1597 (except the Filter by date range (applied on the Close Date)..)
Issue: https://forum.portfolio-performance.info/t/weitere-filter-bei-trades-unter-dem-reiter-berichte-fur-einzelne-depots/21955
Issue: https://forum.portfolio-performance.info/t/trades-filtern/18171
Issue: https://forum.portfolio-performance.info/t/trades-nach-offen-vs-geschlossen-filtern/11208/5, fifth post

Hello this is a proposition to add to the Trades view a filter by account, in addition to the onlyOpen/onlyClose/onlyProfitable/onlyLoss already existing filtering options.

I have also added in the search box the possibility to search in the Portfolio column (second commit).

Note 1: I am thinking a little change to the clientFilterMenu, allowing to give the possibility to only fetch Portfolio account and custom filter account (instead of fetching Portfolio account + (Portfolio +Deposit account) +custom filter account) may be good, .
I do not see the value of a (Portfolio + Deposit account) filter here for example, since, if I am not mistaken, it is the same filter as the Portfolio account only. Same for the SecuritiesPerformanceView (which is the reference for this commit's code). So the Portfolio +Deposit account takes a lot of place in the menu for nothing in those two views.
tradefilter

Edit: updated as per fourth commit. Indeed, if we confirm that Ref accounts have no added value here, it may better to exclude them right from the start of the feature, in order to not have to deal with retrocompatibility for already selected "Portfolio+Ref Account".
2024-05-09 13_37_17

Note 2 : I kept the existing logic around "hasPreselectedTrades" , but I have no idea of what this is supposed to do. Are we supposed to get a additional filter action in the menu with it ? See lines 289-293 of the current before commits TradeDetailsView. I have never managed to get such menu line appear in the current release of PP.

as Portfolio+Reference Accounts are not needed in Trades
@mierin12 mierin12 marked this pull request as draft October 19, 2024 10:39
@mierin12
Copy link
Contributor Author

Note 2 : I kept the existing logic around "hasPreselectedTrades" , but I have no idea of what this is supposed to do. Are we supposed to get a additional filter action in the menu with it ? See lines 289-293 of the current before commits TradeDetailsView. I have never managed to get such menu line appear in the current release of PP.

OK I found what this it. It is the additional date filter when you jump into the Trades from the two widget "Trade profil loss" and "Number of trades". The combination of the two filters (date and portfolio) does not work perfectly. I will look into it. I put in Draft.

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.

Add filter to the Trades view
1 participant