No longer overwrite manually adjusted security prices with historical

[inv-trad]

Manually adjusted prices are flagged and not overwritten when fetching
historical prices from remote servers

[chirlu]

I sometimes add a fund price by hand that I happen to know before Ariva knows it. I would prefer not to have that marked as sacred and unchangeable in case I made a typo.

Under what circumstances will addPrice be called with the same value as before (and hence remove the “manual” flag)? Also when the prices are updated from an online source? That would actually be convenient for my use case, but not for those using an unreliable source that intermittently returns correct values (flag removed) and then again wrong ones.

[chirlu]

Another question … I don’t know yet how serialization into the XML file and back works in PP. Will the “manual” flag automatically be included?

[ghost]

Could you kindle revise the XML revision, please?

https://github.com/buchen/portfolio/blob/4b8891855c3bed3d8d972aa715285f21f57af8c0/name.abuchen.portfolio/src/name/abuchen/portfolio/model/Client.java#L27

https://github.com/buchen/portfolio/blob/4b8891855c3bed3d8d972aa715285f21f57af8c0/name.abuchen.portfolio/src/name/abuchen/portfolio/model/ClientFactory.java#L579-L586

Regarding the use case the meaning of "manually adjusted" and "not automatically updated" is different upon users expectations.

By the way, is there any reasons to use plain text for this hint rather than using a checkbox?

[inv-trad]

Thank you for your feedback. I will try to find a more coherent approach and bring it up here.

[inv-trad]

Now a checkbox is used and can be toggled. Identical prices from automatic sources don't remove the lock anymore.

Manual adjustment of a price still leads to the price being locked and the checkbox selected. Not sure, if this automatic is good. Feedback on this is welcome.

Put Current_version to 46 in Client, but what needs to be done in ClientFactory?

[inv-trad]

One more thing. Writing to the xml and reading it, seems to work.

[buchen]

One more thing. Writing to the xml and reading it, seems to work.

PP is using XStream to serialize objects to and from XML. If you add a new property, it will automatically be persisted.

The changes to Client and ClientFactory are needed so that PP knows a) when an older version of PP tries to load a newer file and b) when loading a older version of the file that (optionally) the data model can be migrated. Because a new "locked" property means old PP versions cannot read this file anymore, the version must be increased.

My thinking is: I try to limit the XML serialization to classes in the "model" package. Any change to those classes means a change to the XML. And any change is forever. PP can still read (and upgrade) the very first version of the XML from 2012. Therefore I am always super careful with any change to the model classes.

Now to the pull request:

Manually adjusted prices are flagged and not overwritten when fetching
historical prices from remote servers

Let's quickly recap how the quote provider behave:

• If new prices are loaded, the existing prices are overwritten
• If the API allows, the request for data is limited to the time period where no data is given
  • Portfolio Report, Quandl, Finnhub, etc. PP asks for specific dates because the API allows to do that
  • For Yahoo Finance, PP must ask in 1mo, 3mo, 6mo, 1y, 2y, and 5y intervals. The actual interval is picked based on the last available historical price and is as short as possible
  • For the generic importer (JSON, HTML) the quotes are imported that happen to be on the current page (response)

Of course, we can introduce the "lock" property and have the user maintain that property. I see the following disadvantages:

• the user has more things to maintain and that makes it more complex;
• the XML can grow significantly in size (we probably need a custom converter that writes the property only if it is true - as it is mostly false - my test XML file increases from 9,2 MByte to 18,5 MByte)

Just a thought:
Can't we simply change the behavior of the automatic download to not overwrite prices?

That would mean

• When updating prices in the background, an existing price is not overwritten. If the user manually change the price, then it stays that way.
• If one deletes prices, they are loaded again
• If the user explicitly imports prices (say via CSV or be extracting from HTML) then the prices are overridden

I see one argument against this implementation: If the quote provider corrects a price, then this correction would not make it into the list of historical prices. But how often does that happen? Does anybody have an idea?

From the code perspective, @inv-trad, it looks good. The only thing I would add (if we need this feature and the proposal above does not work) is to have custom XML converter that is not writing the boolean flag always, but only if it is set to "true").

[inv-trad]

Thank you for your thoughts, Andreas. I agree with you.
In fact, I was wondering why existing prices are update again and again and thought, there must be a specific reason to do so. That's why I didn't touch this side and went the "locked flag" way.

I too, saw, that the xml structure is growing dramatically and had the same thoughts.

Can't we simply change the behavior of the automatic download to not overwrite prices?

Not updating existing quotes would be rather easy to implement. And as you have written, that behavior is implicitly implemented in case Portfolio Report, Quandl, Finnhub, etc are used to retrieve historic quotes.
As we have more prices that we need, I think it would make sense to check if the old price is zero and the new price is different. And update in that case only.

If you agree, I will adjust the coding accordingly, remove the checkbox and the locked flag.

[chirlu]

One issue that I see with the no-update strategy: During a trading day, Ariva shows the most recent quote in its “close” column instead of the (not-yet-known) closing price, with a footnote saying so. Therefore, PP will store the then-current price from some random point in time and no longer update it after the trading day is over.

Example page; note the asterisk with the footnote saying “most recent price today”.

[buchen]

During a trading day, Ariva shows the most recent quote in its “close” column instead of the (not-yet-known) closing price, with a footnote saying so

Fair point.

We could also introduce a property on the Security that determines the behavior:

• overwrite prices with data from quote feed; or
• do not overwrite prices

The default would be "do not overwrite" but for some HTML pages like your example, the user could say "do overwrite prices all the time".

Personally, I would prefer one property on the Security over a configuration on each and every price.

If you agree, I will adjust the coding accordingly, remove the checkbox and the locked flag.

Yes. I think the Security#addAllPrices is a good starting point. And then add a boolean property to Security which has to be reflected in the EditSecurityModel. And this value you can "bind" in the HistoricalQuoteProviderPage. If the GUI part is too much (I know Eclipse SWT/JFace is overly complex...) then doing the "model" changes would already help.

[inv-trad]

Well, with that approach the issue, we want to solve isn't really fixed. For example, if Ariva delivers a wrong price for one day and the behavior is set to overwrite prices, because it shows the most recent quote in its “close” column, not the final one, one needs to wait for 30 days to have the corrected price not overwritten.

Yesterday, I was following a litte different approach, overloading Security#addPrice with an overwriteExisting flag and checking, what source the price comes from. With that logic it would be possible to distinguish between imports and automatic updates. Imports (manual, csv, html) overwrite existing prices with changed ones, while automatic updates don't and have exceptions like overwriting 0.0 prices (can happen during imports) or the last existing entry.

            if (!replaced.equals(price) && (replaced.getValue() == 0.0 || overwriteExisting
                            || replaced.getDate().equals(latest.getDate())))

I think, that could be a feasible approach and will update that you can see, what I have done.

[buchen]

Well, with that approach the issue, we want to solve isn't really fixed. For example, if Ariva delivers a wrong price for one day and the behavior is set to overwrite prices, because it shows the most recent quote in its “close” column, not the final one, one needs to wait for 30 days to have the corrected price not overwritten.

It would work like this:

• By default, one sets the configuration to not overwrite. So if Yahoo Finance delivers wrong prices, one can fix the prices and all is good. If the user deletes the price, then the prices will be inserted after the next update again (probably still the wrong one because Yahoo is slow to correct those prices)
• For quote providers such as the Ariva - knowing that it updates the prices in the table - one would have to say overwrite. But you are right, in this case the user cannot "fix" the price and would have to wait it out. I do not know how often Ariva reports wrong prices, but usually I read this for Yahoo. In this case, I would hope one property on the Security is good enough for 99% of the use cases. And Ariva is the only case (so far) I know of reporting the latest price in the table of historical prices.

But, ah, now I get your PR. Der Groschen ist gefallen.
With

replaced.getDate().equals(latest.getDate())

you basically handle the Ariva specific case of the last historical quote always to be updated. But depending on the jobs, the "latest" quote might already be up-to-date. Say the user opens PP every noon. If the user opens it today, then the "latest price" is first updated to today, but yesterdays historical price must be updated because it was just the value from noon. I think we cannot use the comparison to the latest day here (and "latest" can be null if no latest quote feed provider is configured).

Checking for LocalDate.now() does not work either because maybe the user has updated the prices yesterday at noon. And today, on Sunday, yesterdays prices must be update to the EOD price.

Maybe the rule is: if it is the most recent historical price, then it is updated. What do you think? But then it does not allow you to "fix" a historical price if it is the last one.

Do we have more options?

About your PR: Because Security#addPrice(p) is actually Security#addPrice(p, true) you do not have to change that many files. The only call to addPrice(p, false) is from addAllPrices.

[inv-trad]

Yes, comparing with the latest was meant to fix the Ariva specific behaviour. Not sure, if that applies for other quote sources as well.

During testing, I never deleted last Friday's price (latest), only changed it and everything worked fine, but it looks like the jobs provide the new latest before the historic prices. That means the old latest e.g. from Thursday noon (not Thursdays close price) is not updated, because it is no longer the latest. If the job for the historical quotes would run first, everything should be fine, but obviously it doesn't.

I will investigate further to see, if there are more options.

About the PR: I removed the unnecessary changes and left the CheckBoxEditingSupport class, in case someone else might need it later?!

[inv-trad]

I changed the sequence the jobs are initiated and it works the way it should do. The historic quotes are updated first and the latest existing is replaced, in case it is different. After that the latest is updated. When one now adjusts the last historical prices, it will not be touched again, because the new latest exists and the date is different.

Could there be side effects from changing the job sequence? So far I didn't realize any.

Can someone tell me, why the check fails giving an error in a class I didn't touch? What do I need to do to get it fixed?

[buchen]

Can someone tell me, why the check fails giving an error in a class I didn't touch? What do I need to do to get it fixed?

Because I committed code that did not build and I did not notice it... 😒 Nothing to do with your change.

[buchen]

Could there be side effects from changing the job sequence? So far I didn't realize any.

My thinking for loading the "latest" quotes first was because one wants to see the current valuation first. It is hard to say if that makes a difference. In my (bigger) file, I need ca. 5 seconds to update all quotes. That is not a big difference.

But what if the "latest" job ran, but the "historic" not. And the next day the data is not updated as expected. And why if Yahoo is wrong data for the last day? You can still not fix it if the "latest" quote happens to be for the same date.

On the other hand, what can go wrong? If this does not work, we can easily improve it with the next version of PP. But if we add a new property, we can never remove it again. Let me rebase and merge and we will see.

[buchen]

Squashed, rebased, merged.