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

New dashboard widget to show the timeliness of security prices #2990

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

Conversation

ma4nn
Copy link
Contributor

@ma4nn ma4nn commented Sep 27, 2022

This PR suggests a new dashboard widget that shows how many security prices are older than 7 days.
This allows to better interpret the values on the dashboard and to find errors faster.

Issue: https://forum.portfolio-performance.info/t/dashboard-widget-kursqualitat-kursalter-kursvollstandigkeit/21903

@Nirus2000
Copy link
Member

Nirus2000 commented Sep 28, 2022

Hallo @ma4nn
Maybe you could still set the 7 days not statically, but dynamically under the usersetting?

Perhaps it can also be taken into account if the quotes are not retrieved on weekends or holidays (TradeCalendar.java / TradeCalendarManager.java)?

Alex

@ma4nn
Copy link
Contributor Author

ma4nn commented Sep 29, 2022

@Nirus2000 good points, thanks for your feedback 👍

My goal was to keep the implementation simple, that's why I chose the same logic as already applied in the SecuritiesTable view.

Speaking of making the interval configurable I see 2 options:

  1. Make the interval configurable in the PP preferences somewhere (I assume you meant this with "usersetting", right?).
    In this case the questions are a) where to put it (e.g. under a new preference page "Quotes"?) and b) should the SecuritiesTable view also be changed to use that new config value?
  2. Add a new configuration for the dashboard widget only. In this case the question is if we should reuse ReportingPeriodConfig for that or implement a completely new one.

Regarding the TradeCalendar I am not sure if we should really integrate it as it makes it way more complex and raises several questions, e.g. how to handle securities that do not have closed on weekends vs. the ones that do.
I think this can be easily mitigated by chosing a long enough time interval (like it's done in the SecuritiesTable).

Looking forward to your feedback on that.

@Nirus2000
Copy link
Member

Nirus2000 commented Sep 29, 2022

Hello @ma4nn

Make the interval configurable in the PP preferences somewhere (I assume you meant this with "usersetting", right?).
In this case the questions are a) where to put it (e.g. under a new preference page "Quotes"?) and b) should the SecuritiesTable view also be changed to use that new config value?

Yes, that's what I meant.
Under the settings could be entered the number of days exceeded maximum, without new quote.
"PrefTitleGeneral" -> "PresetsPrefPageTitle" --> new horizontal line and then below that the setting option.

With the "SecuritiesTable" it would be actually quite cool, if the date is exceeded, that an "event" is indicated.
I could imagine a color change or exclamation mark or a star.

Add a new configuration for the dashboard widget only. In this case the question is if we should reuse ReportingPeriodConfig for that or implement a completely new one.

I would leave "ReportingPeriodConfig" as it is and create a new one. If necessary, this can also be better adapted later.

Regarding the TradeCalendar I am not sure if we should really integrate it as it makes it way more complex and raises several questions, e.g. how to handle securities that do not have closed on weekends vs. the ones that do.
I think this can be easily mitigated by chosing a long enough time interval (like it's done in the SecuritiesTable).

That's a very good point. I didn't think of that at first.
However, what happens if the period is too small! So two days weekend and then a holiday?
Have a look here, maybe this will help you to implement the TradingCalender easier.
You could derive the associated calendar for the security here.

Greetings
Alex

@Morpheus1w3
Copy link
Contributor

@ma4nn
Just in case if you like to consider weekend or the configured trading calender:
https://github.com/buchen/portfolio/blob/master/name.abuchen.portfolio/src/name/abuchen/portfolio/snapshot/QuoteQualityMetrics.java#L88-L122

@ma4nn
Copy link
Contributor Author

ma4nn commented Oct 8, 2022

@Morpheus1w3 @Nirus2000 thanks for pointing in the right direction with the trade calendar, didn't know that part of the code yet 😊

While implementing the trade calendar change I came up with a little more advanced solution to display the timeliness for each widget and did implement the trade calendar adaptions there, but in case everybody likes the timeliness widget in this PR better I can also apply the changes back into this branch of course.

One still open point is the place where to put the configuration for "older than X days".
@Nirus2000 you suggested to use the "Presets" tab in the PP preferences, but I am not sure if this really makes sense for the settings because it is not a preset.
But unfortunately I do not see any other tab at the moment that would fit better (despite "General").
Any opinions on that? Perhaps create a new settings tab "Quotes" or something like that?

@Nirus2000
Copy link
Member

Nirus2000 commented Oct 9, 2022

Hello @ma4nn
I like your ideas. Good job!
With the percentage display I personally find a better solution. However, here I would not show as in your example 23 of 25 securities are currently equal to 8% but that 92% of the quotes are okay.

I would like it better if a list of securities is displayed, for which securities the quotes are not up to date since X days. This could then be displayed in the tooltip.

I would also do without the yellow dot. This creates NPE on the one hand and on the other hand you see this only well in DarkMode. (see video)

widget_ma4nn.mp4

I have thought about the preferences again. I think you are absolutely right that we should create a new subgroup in the settings. Here it would also make sense to move the setting PrefUpdateQuotesAfterFileOpen there. As name of the settings tab I would take simply " Quotes".

Greetings
Alex

@ma4nn
Copy link
Contributor Author

ma4nn commented Oct 9, 2022

@Nirus2000 thanks for your detailed feedback, Alex.

I would not show as in your example 23 of 25 securities are currently equal to 8% but that 92% of the quotes are okay.

That's already how it is implemented, i.e. 92% of the prices are not OK in this case (perhaps the example was not that good).

I would like it better if a list of securities is displayed, for which securities the quotes are not up to date since X days.

👍

I would also do without the yellow dot. This creates NPE on the one hand and on the other hand you see this only well in DarkMode. (see video)

Yes, the implementation is not final yet, just a first draft to test the feasibility. The color could be changed and the exception should not occur of course (though I was not able to reproduce it yet).

I think you are absolutely right that we should create a new subgroup in the settings. [..] As name of the settings tab I would take simply "Quotes".

👍


Ok so as a summary from this thread and the forum I think the best would be to have 2 separate PRs a) this one with the new single timeliness widget and b) a new PR for the "yellow dot" next to each widget.
For this PR I will backport the changes regarding the trade calendar made in the other branch and implement the new settings page.

I'll convert this PR into draft mode until that implementation is finished.

@ma4nn ma4nn marked this pull request as draft October 9, 2022 11:06
@ma4nn ma4nn force-pushed the feature/price_timeliness_dashboard_widget branch from 6fd2433 to ad73b3b Compare October 10, 2022 09:41
@ma4nn ma4nn marked this pull request as ready for review October 10, 2022 09:45
…unit tests, added new configuration tab for quotes and moved existing config setting to update quotes on startup there, included list of stale securities into tooltip
@ma4nn ma4nn force-pushed the feature/price_timeliness_dashboard_widget branch from 3f5fec6 to c879dd6 Compare August 5, 2023 13:39
@ma4nn
Copy link
Contributor Author

ma4nn commented Aug 5, 2023

@Nirus2000 rebased this PR to latest master, just let me know if there is something else that's holding this change back from being merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Todo
Development

Successfully merging this pull request may close these issues.

3 participants