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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix update cash balance #3193

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

rodriguestiago0
Copy link
Contributor

No description provided.

@@ -57,7 +57,7 @@ export class AccountService {
> {
const { include = {}, skip, take, cursor, where, orderBy } = params;

include.balances = { orderBy: { date: 'desc' }, take: 1 };
include.balances = { orderBy: { createdAt: 'desc' }, take: 1 };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will not work if you have created a balance today for a previous day.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean by creating a balance for today?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my opinion, cash balance table should only have one entry per account.
And use postgres capability on conflict to update the existing balance with the change.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my opinion, cash balance table should only have one entry per account.

No, we need multiple entries to show the progress of the account balance.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, when creating an account balance in the past, it needs to be inserted in order or be recalculated.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I don't think so.
However, your comment leads me to the conclusion that the current implementation for update cash balance only works if the activity (e.g. interest) is today. In the GUI, the checkbox must be disabled if the date differs from today.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They need to be newer than the last account balance date.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the best approach is to insert on this table the balance change and use the a view to calculate the accumulated values based on the date.

I will do this change and fix this

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will do this change and fix this

I have some doubts that it will work and would suggest to solely disable the checkbox based on the date.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR: #3229

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

Successfully merging this pull request may close these issues.

None yet

2 participants