Do not filter out non-Transaction entries

[Evernight]

See #1623. The behaviour of filtering out non-Transaction entries along with Transaction ones is rather limiting at the moment. If advanced or account filters are specified, most of the times most of non-Transaction entries are filtered out from the ledger and at least for me this seemed counter-intuitive and potentially led to conclusions made on misleading data while browsing through various views on the ledger.

This is particularly prominent with the price entries when using multiple currencies in the ledger frequently and relying on automatic conversions. While using the popular https://github.com/andreasgerstmayr/fava-dashboards plugin or (for example) running BQL plugins from Fava that causes missing transactions (due to CONVERT(SUM(position), '{{currency}}', LAST(date)) not working).

As mentioned in the linked issue, I am not aware of any workaround to preserve (for example) price entries while applying filters to other transactions. One could imagine a filter option for entries like type:Price that you could add as an OR to the advanced filter (but not to the account filter), and it would be one way to address the problem. But then I'd need to add it to most of the queries I do since it would personally make sense for me all of the time. In any slice of my data there would be 2 or 3 different currencies that I'd need to convert to a common basis to do meaningful analysis.

These below are also a few statements that I believe to make sense, although of course I may miss some details and counter-arguments.

1. Preserving non-Transaction entries while applying filters to Transactions would not cause unexpected behaviour in overwhelming majority of the cases.
2. Filtering non-Transaction entries is not much needed in practice while working with the ledger. At least in my almost 2-year experience I haven't faced a practical case that required it.
3. Filtering by time for non-Transaction entries still may make sense, although I'm not sure and don't have a particular opinion about this. I still can't think of practical cases and maybe for consistency that would need to be changed as well.

Hence, I suggest this pull request as a RFC. I tested it locally for some time and it works better than the Fava's current behaviour for my personal needs at least. I'm able to set and interlink more useful dashboards for a personal ledger.

The tests have been adjusted accordingly. I had to remove 4 checks related to an open directive properties. If they were based on any practical use-case, would be happy to return / make adjustments that make sense.

[ssergiienko]

Hope my comment will add some priority to this 🙏
Accidentally hit this problem and my use-case I believe is even more common and doesn't require any plugin.

Just consider splitting investments into few categories which would be useful to see separately:

• stocks
• crypto

Each of them are in own include file and wrapped with relevant tag.
Now if you'd select tag in Fava filter - calculations becomes broken, e.g. on Holdings view price column becomes empty and -> Unrealized PnL becomes negative garbage. As I understand, its because prices directives were filtered out as well (is this a cause?)

This problem basically makes it impossible to use such flow and filtering in general(

[yagebu]

Thanks for the PR. I don't think only ever filtering on Transactions is a better choice. If filtering by account, I'd expect only documents of that account to show up in the document report; likewise if filtering by tag on documents; if filtering by time, I'd expect that to apply to the events report. To me, not applying the filters consistently seems very unexpected.

I don't think this is about "non-Transaction" directives - this is just about prices, right? All the examples given are only ever about prices here. And I think it's not even about the Price directives but just the conversions based on them, right? (so for example, I'd still expect the commodities report to be filterable by time). If it's just about the conversions based on them, we should consider simply always using the complete price map for conversions but leaving filtering as is.

[Evernight]

@yagebu Thanks for the reply!

If filtering by account, I'd expect only documents of that account to show up in the document report

Yes, I agree that would make more sense. I don't use documents much but sounds like that would be a behaviour to expect.

One problem that I see is that, for example, only Transaction, Document and Note types do have tags. Event, Pad, Price and Custom - do not. This is not Fava-specific and could be more a consistency question of the Beancount's API itself. For example, if I used Event directives more, tags could have come in handy in some cases, I imagine. Same for Custom directives that I use, for example, to configure plugins. Good thing is that the plugins run before the filters are applied so it only changes rows that are displayed (whereas for Price entries, the information in the rows is also affected).

Also not all types are explicitly related to accounts. I've only now noticed the custom logic in get_entry_accounts that addresses that for the account filter and yeah, the current PR state would contradict with that logic since I haven't considered that. I was wondering before how advanced filter by account was different from the account filter and I guess this is one part of the answer.

Most of the non-Transaction entries will be filtered out whenever advanced filter is used. Except date and metadata-based filters – they are present pretty much for all of the directives.

Then the question remains what to do by default with the entries that are not filterable using that filter in their interface (e.g. filter by tag applied to entries that cannot possibly have a tag). Right now they are filtered out by default, the intention of this RFC commit was to flip that logic (so, tend to leave these entries in the ledger). The specific implementation should probably be adjusted.

You're right that my primary concern at the moment is prices being filtered out, for the reasons described above. I would not mind even not applying time filters and always using complete price map with all the entries I think. I haven't thought about cases where that behaviour would be unwanted, maybe other users could have these, however not sure how frequently.
So the possible change you've mentioned would work for my use-cases as well indeed. But I'm not sure if that wouldn't be a considerably more complex change (adding ad-hoc logic to all price calculation callsites). Haven't looked into that yet, anyway.

I'm currently running Beancount setup with these changes for some time and I've noticed that Custom operations are more prominent in the Journal, for example, since they always stay when the filters are applied (while the transactions are heavily filtered down). I'm still not sure how I feel about that but, fortunately, they can be excluded using a button in Fava's UI which at least gives more options for the user (and always excluding doesn't give that option).

I'd say something along these lines for Events but they are only usable from the Events page at the moment. And, predictably, it would show "No events" if any filter except for the time filter is applied, which I'm not sure is more intuitive than just not applying filter to them.

I think that's all I have for now at least for practical considerations.

I agree that some changes are needed to the current PR at least to fix the account filter behaviour (and maybe more) but not sure if price_map solution would be easier or preferable. I feel like general direction of "when filter does not apply, leave the entry in instead of out" is less changes and addresses more issues even if they're cosmetic or theoretical. Don't feel strongly though, for most practical purposes using complete price_map would also work. I'm surprised this problem with prices hasn't come up earlier though.

[yagebu]

For conversions in Fava, the full price map is/was already being used actually, so this only affects BQL queries with conversions. See #2005 for a change to add all prices to the entries the BQL is run on to align the behaviour.