Add identifier field to custom filter item and use it as new link to reports (fix #2546)

[OnkelDok]

This new unique identifier for the custom filters should solve bug #2546 (link in forum: https://forum.portfolio-performance.info/t/bestehende-filter-erweitern/7355/14). Description/Analysis for the issue: https://github.com/buchen/portfolio/issues/2546#issuecomment-1191809609

Following points/actions are inlcuded in this PR (as of 10th Sep 2022):

• [x] Assign unique ident to each custom filter & move them from settings file into client file
• [x] Migrate filter references in settings file and move them into client file
• [x] Migrate filter references from widgets and charts
• [ ] Individual logic on PP destroy for the case that XML is not saved: last question for me: is the following differentiation needed? Or should we always skip saving settings file on PP destroy?
  1. When settings file stored next to client XML:
     • [x] reset migrated settings
     • [x] do not store settings file on destroy (to prevent losing filters when client XML is not saved)
  2. When central/common settings file is used:
     • [x] filters in settings file not resetted in case other client XML also use them
• [x] created/extracted method of common used code (95ab6f0357c74390406432137a7c7b4c5f158d49)

[OnkelDok]

After a look into the start up sequence I realized we have to migrate the portfolio-xml-file and the settings-file (ClientInput.preferencesStore). In the ClientFactory.upgradeModel method only the portfolio-xml-file is migrated and there is no access to the settings-file, because it is loaded afterwards in another class (ClientInput). Because the client filters are stored in the settings-file the migration can only be execute when both files are loaded.

@buchen: Do you see good places where to execute the migration of both files only in case when starting PP after the software update?

[buchen]

Do you see good places where to execute the migration of both files only in case when starting PP after the software update?

I wanted to merge the filter from the settings into the client for a very long time. Initially, the filter were just a pre-defined list, but now one can edit the filter, name them, etc.

The code needs some refactoring. At the moment, the filter are read in ClientFilterMenu. There we can enhance the logic: if the filter is part of the client -> use it; if the preference exist -> create new filter in client We must make sure not to delete the preferences: if the user does not save the file after migration, the filter would be lost. In a year from now, we can delete the code -> all active users should have migrated by then I think the ClientFilterMenu.Item should move to the model package (renamed, of course)

I realize there is one more thing we have to take care of: if we assign UUIDs, then we also have to migrated the data series previously picked... :-|

It makes sense to have a separate pr for this. I might find time tomorrow... (quite busy atm. sorry)

[OnkelDok]

With merging/relocating filter from settings into client, do you mean only the filter itself (ident, name, filterdata)? Or do you also want to merge the link to the filter, which is for some report stored in the settings file? So yes, it could be a seperate step. Is it important for you which step implemented first (relocation of filters from settings to client & introduce unqiue filter ident)?

Yesterday I found a spot where to do migration logic: In ClientInput.setClient. (https://github.com/buchen/portfolio/pull/2931/files#diff-eb6928a8bfee7050dd814a60da8948c2dcd3b52fe90064a73371ecd17242d252R700-R703)

I finished (hopefully) the migration of the settings file. Now I have to check how we can migrate all assignments in the client file (widgets, dataseries of all charts, ...). My biggest concerns are, that I miss some spots. Maybe I will create a sample file with custom filters and use it in all possible reports/charts/... and take a look into the client file where the link can be found.

[buchen]

Now I have to check how we can migrate all assignments in the client file (widgets, dataseries of all charts, ...). My biggest concerns are, that I miss some spots.

Maybe the logic can be:

• use the new id to identify
• if nothing is found AND it is a filter, fall back to the old identifier

For example, in DataSeriesSerializer#load, one could add a "legacyuuid2series" map and fallback. in 12 months, we delete this code again

[OnkelDok]

Maybe the logic can be:

* use the new id to identify

* if nothing is found AND it is a filter, fall back to the old identifier

For example, in DataSeriesSerializer#load, one could add a "legacyuuid2series" map and fallback. in 12 months, we delete this code again

Yes, maybe this (lazy migration = migrate when link is used) can be one solution. In my opinion the disadvanteage of this is, that the code for migration is spread to several spots. Currently I'm working on doing the migration for all filter links after startup.

[OnkelDok]

I made some additional changes. In current version of the PR following steps are executed:

• load existing filters and assign new unique guids
• migrate usages of filters in settings file
• migrate widgets
• migrate charts

I hope I didn't miss any other spots.

To check the migration logic I've created the following files (I had to append ".txt" extension that they are not blocked by github): AlteVersionMitFiltern.settings.txt AlteVersionMitFiltern.xml.txt

EDIT: Sorry for the many commits. I can squash them as soon as you are ok with the code. Or is it easier for you if I squash them earlier?

EDIT2: ~~In ClientFiltermenu#loadCustomItemsLegacy there is a legacy loading of filters from settings file. Should I also take this into account in the migration logic? If so, then I have to add the legacy loading and the convertion into the "@json" filters.~~

EDIT3: I added the legacy loading into the migration logic.

[buchen]

Hi @OnkelDok,

thanks for the change. You do not have to squash the commits, this is something I can easily do. And it is easier to see the history here. I will not more time to merge it - first I was very busy at work and now we are on vacation without devices beside my phone... (and I want to publish a new version before) Thanks for your patience.

I realize we have to do one more thing before merging: store the custom filters in the XML file itself. Right now, the data series have a UUID, but are still stored only in the settings file. The settings, however, are not necessarily in the user control. For example, I use PP on two computers (I replicate the data file via iCloud) but have different settings files. Then it will not work - because the UUID are only local and cannot be known to the second computer.

There is already a data structure we can use: ConfigurationSet. We would add a new configuration set for custom filters. Each filter is then a Configuration in the data set. We still need the Item class to hold the transient ClientFilter

[OnkelDok]

I hope you have a great vacation, @buchen! With 17a4ffe I moved the filters from settings file into client XML. I hope it is as you described.

One problem I realized while testing is, that if you start the software and close it without saving, then the settings file will still be updated (because it is saved automatically when exiting PP). That leads into missing filters on next software start, because they are missing in settings file and they were never saved into XML. So we have to find additional logic to determine if the old settings can be set to default (deleted) without hesitation.

One possible solution that comes in my mind is: only set old client filter settings to default when XML is saved and only when client filter migration was executed before (to not set to default on each save on newer versions). If I find more time I will try to find a good place in the code for this necessary logic. I set the PR to draft again until I made the changes.

[OnkelDok]

Sorry for the following long text. I just wrote down my ideas when implementing and testing. If you want you can skip the first part until the third solution at the bottom:

After thinking about the problem with not saving the XML I think we must not set the settings to default when move the filters from settings file to XML. Additionally we have two options:

1. We set the old filter settings to default in the settings file when the XML is saved.

   • Pros: setting file is clean
   • Cons: if setting file is used for other XML file, then for this the filters are lost. But do different XML files use the same filter? Because the filter contains UUIDs of the XML (depots, account, ...) and will not same.

2. We let the old filter settings in the settings file for the next year. Then (in a year) we include code to set the old filter settings entry in setings file to default.

   • Pros: if other XML are using the same settings file, then the filter is not lost for them.
   • Cons: setting file is not clean. Someone can wonder about the filter setting in the file. Additionally we have to remember to change the code in a year.

I decieded to commit the first solution. Please tell me which one do you prefer and I will make changes. Or maybe you have additional solutions for solving the problem.

Oh no, I did not commit, because: After implementing the first solution I realized while testing that if the XML is NOT saved but the settings file (automatically) then the reports which are storing their used filter in the settings file will lose their link to the filter, because the settings file is migrated to new filter ident. But the XML is not saved. Then on next start the migration is executed again, but the entries in the settings file are not the same old ones for a correct migration (they already have unique, but unknown idents and cannot be found and migrated by the migration logic). So I think following third option maybe can help:

3. We automatically save the XML file after doing the migration. With this we should have a valid XML. Are there any disadvantages with this?

What's the reason for saving the settings file even though the XML is not saved (in PortfolioPart#destroy? Because disabling the automatic saving of the settings file will also help to solve this.

EDIT: And last but not least, we can also move the used filter links (e.g. "SecuritiesPerformanceView-client-filter=...") from settings file into the XML (all in one new ConfigurationSet). Then all usages of the filters are saved in the XML just as the filters itself. So we have all at one place. Then additionaly we disable the automatic saving of the settings file on shutdown/destroy, and all should be fine (if no other reasons for the automatic saving exists). This is my final suggestion for solving the described problems when the user not saves the XML.

[OnkelDok]

In the initial post of this PR I've created a list of all functional changes. With the last three commits I moved the filter references (which so far where saved in the settings file) into the client XML and included logic to not save the settings file on destroy to prevent losing filters in case the client XML is not saved.

[OnkelDok]

With 5790b80 I removed the individual logic for handling the cases (XML not saved and settings file besides XML or central) differently. When XML is not saved then on next load the XML migration is still possible, of course. And when XML was saved and old filter data and links in settings file are still present they will not disturb because they are ignored after migration and can be deleted after some time.

From my side of view this PR is ready to go.

[OnkelDok]

After discussion in forum where @chirlu opened my eyes about the central settings file (strangely, i had thought that one central settings file is used for multiple client files) I got an idea for this PR about the cleaning-up-old-settings topic. In the current version of this PR all the settings in the settings file, which where moved into the XML on load, will remain in the settings file. I described the reason in one of my lasts comments. But now I have an idea how to handle this with cleaning up the settings file: The sequence has to be

1. on load: migrate/move filter data and filter reference/uses (like it's already done in this PR)
2. after saving XML and before saving settings: clean up moved filters and references in settings file

So when the user will not save the XML, the settings file will not be cleaned up and can be saved on destroy without hesitation.

So I will set this PR back to draft until I have access to eclipse and apply the described changes.

[OnkelDok]

As announced: With 7122a08b7df991df71738ae6253011fbdc03d516 I added removing of old filters and their references in the settings file only after the XML was saved. So in case the XML is not saved then these settings remain in the settings file so that on next load of the unsaved XML the filters can be migrated again.

@buchen, I hope the amount of my comments doesn't confuse too much. :blush:

[OnkelDok]

@buchen: I know this PR is a lot of code to check with a lot of cases to consider. Do you need additional information or more detailed description to review this PR?

[OnkelDok]

While rebase and resolving conflicts because of changes in master branch, I noticed that the filters of AllTransactionView still searches its filter in the settings-file. So I will set this PR to draft until I changed this. And I think because there are so many commits, I will see if I can squash them together.

[OnkelDok]

I will set the PR again into Ready for review state.

What I changed:

• All TransactionsView and PaymentsView now also use migrated filters in XML
• CLIENT_FILTER of widgets/dashboards are now also migrated correctly
• I moved all methods for accessing filter usages/references in the configuration into separate static class ClientFilterStorageHelper. I hope my choosen naming is OK to distinguish between the filter itself (now named Filter Definition) and the usages/reference of the filter (when a widget/chart/element have selected a filter).

EDIT: I added a short summary of the old/current and the new structure of filters and their usage in the first comment of this PR

[buchen]

Hi @OnkelDok,

first: apologies for this change to take that long. It is just a tricky one and I needed a block of time to look into it.

second: thanks for your excellent work. If you look at the commit, I changed a couple things here and there, but the main idea remains. Great work.

third: I changed a couple of things. But I merged both of our changes in order to keep the commit history clean. If you want to understand the changes, then I have added the patch of my changes on top of your changes to this pull request Patch.patch

What did I change?

• The selected filters must be migrated even if no custom filters are defined. Otherwise the selection of the default filters is gone
• The migration of filters must migrate the full identifier. The problem is if there are two filters, one including {a} and the other including {a, b}. Then the migration can accidentally do a partial migration and create unusable identifier
• payments view must always save the selected filter (even if none is selected); otherwise the deselection is not stored and next time around the use sees the filter again
• if the filter is changed, we must touch the client because the selected filter is stored in the file now

Other changes are

• Instead of calling "ident", I just called it "id"
• I introduces a WellKnownConfigurationSet enumeration - I think that will help in the future to know which keys are used and which are not
• I removed the UUID generation of custom filter chart series - the UUID is unique and identical filters have different UUIDs
• I removed the "-client-filter" postfix from the keys. It was needed in the preferences, but is not needed in the configuration set anymore
• I do not delete any keys from the settings - never did in the past - I feel it is safer to have it around and it does not cost much

@OnkelDok please have a look at it. Let's test drive this a little bit more before we publish it with the next release

[OnkelDok]

Hey @buchen, thanks for taking the time to look over my suggestion. I was aware that this would be a bit more work for you, so Idid'nt expect a quick answer.

At first rough view I canot see any issues. I will try to find time the next days to test it with a sample file, I created. But first I have to update eclipe etc and get the current version working.

[OnkelDok]

Hi @buchen, I tested it with my sample file and didn't found any problem.