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

[Bug]: Reconciled should imply cleared #2154

Closed
1 task done
tavlima opened this issue Jan 1, 2024 · 6 comments
Closed
1 task done

[Bug]: Reconciled should imply cleared #2154

tavlima opened this issue Jan 1, 2024 · 6 comments
Labels
bug Something isn't working transaction reconciliation Related to transaction reconciliation

Comments

@tavlima
Copy link

tavlima commented Jan 1, 2024

Verified issue does not already exist?

  • I have searched and found no existing issue

What happened?

This is not really a bug report, but more like a design issue (IMHO). Given cleared is a number (not a boolean), I think it would make more sense for that same field to be used as reconciled, instead of a separate attribute/column. The reason being:

  1. those states are dependent: reconciled implies cleared, while uncleared transactions should never be flagged as reconciled. I think this logic is easier to capture by using a single column rather than enforcing this logic in multiple places.

  2. I just found an issue where I filter by uncleared transactions and get some reconciled transactions as result, meaning my DB is in an inconsistent state. Can't tell what caused this, but this is what I see.

What error did you receive?

No response

Where are you hosting Actual?

Fly.io

What browsers are you seeing the problem on?

No response

Operating System

None

@tavlima tavlima added the bug Something isn't working label Jan 1, 2024
@jsehnoutka
Copy link

I think this depends on the use case.

I intentionally leave some transactions uncleared in case I need the entry as a placeholder, amount to be added later (I am following the accrual principle). Current behavior of Actual enables me to reconcile and still be able to filter out uncleared transactions later.

If reconciliation would change state on my uncleared transactions, I would have no means of getting back to them when cashflow happens (to clear & reconcile them when I need to).

My use case might be very specific, but the current behavior is OK from my POV.

@tavlima
Copy link
Author

tavlima commented Jan 4, 2024

Couldn’t you achieve the same results by either “tagging” the transactions (using some keyword in the notes) or just leaving these transactions uncleared (and thus not reconciled)? I honestly think reconciled transactions and “amount to be added later” are essentially incompatible, by the definition of reconciled. Actual actually have a bunch of warnings exactly to avoid changes to reconciled transactions.

@Jackenmen
Copy link
Contributor

Jackenmen commented Jan 4, 2024

Can you actually clear an uncleared transaction from UI once it's reconciled? 🤔

(sorry for the previous, deleted comment, I clicked the Comment button accidentally...)

@jsehnoutka
Copy link

... just leaving these transactions uncleared (and thus not reconciled)?...

Yes, that's exactly how I do it. I never modify reconciled transactions. I modify uncleared ones, then clear them and they get reconciled on the next reconcile task I run.

image

I see now that I probably misread the issue as functional - that reconciliation should overwrite all past uncleared transactions to reconciled. I see now that this is not the case - you are proposing a technical change in db which would prevent inconsistency, but shouldn't change functionality at all.

Sorry for the confusion!

@MatissJanis
Copy link
Member

MatissJanis commented Jan 11, 2024

Locked transactions should always be cleared. If they are locked and not cleared - it feels like a bug. Possibly because previously we allowed imports to update locked transactions (and also un-clear them). This has been since fixed. If there is another way to "un-clear" a transaction that is locked - I'd be interested to know how so we could patch it.

But if we take a step back to talk about the bigger architectural question - yes, we should have used a single field for cleared/locked status. But we didn't :( And column update migrations in local-first CRDTs are very cumbersome and fragile.. so I wouldn't really recommend doing that.

edit: s/reconciled/cleared/

@joel-jeremy joel-jeremy added the transaction reconciliation Related to transaction reconciliation label Jan 16, 2024
@youngcw
Copy link
Member

youngcw commented Feb 2, 2024

@tavlima Does this need to stay open as an issue or can this be closed?

@youngcw youngcw closed this as completed Feb 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working transaction reconciliation Related to transaction reconciliation
Projects
None yet
Development

No branches or pull requests

6 participants