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

Fix inconsistent ids #758

Merged
merged 9 commits into from
Dec 12, 2023
Merged

Fix inconsistent ids #758

merged 9 commits into from
Dec 12, 2023

Conversation

JoseBritto
Copy link
Member

@JoseBritto JoseBritto commented Dec 11, 2023

Fixes #750
Fixes #749

If sort by is set to amount, try sorting by date if amounts are same.
For sorting by date, if dates are equal sort by id.
This should hopefully make the transactions order consistent.

This also means that SortTransactions will almost never return zero which is a good thing I believe.
@JoseBritto JoseBritto requested a review from nlogozzo December 11, 2023 18:52
@JoseBritto
Copy link
Member Author

@UrtsiSantsi can you check if this pr fixes those issues.

Copy link
Member

@nlogozzo nlogozzo left a comment

Choose a reason for hiding this comment

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

Overall looks good to me!

Maybe update changelog too?

NickvisionMoney.Shared/Models/Account.cs Show resolved Hide resolved
NickvisionMoney.Shared/Models/Account.cs Show resolved Hide resolved
Currently there when pressing "Today" button the marks disappear. This commit fixes that
@nlogozzo
Copy link
Member

Once @UrtsiSantsi confirms the fixes, it's good to go!

@nlogozzo
Copy link
Member

Feel free to update the version to V2023.12.0-next and then I will re-approve

@JoseBritto
Copy link
Member Author

@nlogozzo Updated the changelog. Let me know if wording needs to be changed

@JoseBritto
Copy link
Member Author

Feel free to update the version to V2023.12.0-next and then I will re-approve

Alright

@JoseBritto
Copy link
Member Author

JoseBritto commented Dec 11, 2023

@nlogozzo Is it just in the metainfo file? And what to do with the date?
Edit: Nvm, updated the version in MainWindowController as well. Should the date be Dec 1st or today?

@nlogozzo
Copy link
Member

Should the date be Dec 1st or today?

Dec 1st please . So 2023-12-01

Copy link
Member

@nlogozzo nlogozzo left a comment

Choose a reason for hiding this comment

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

On last grammar thing 😅

NickvisionMoney.GNOME/Program.cs Outdated Show resolved Hide resolved
@JoseBritto
Copy link
Member Author

Oops 😂 Fixed now

@nlogozzo nlogozzo self-requested a review December 12, 2023 00:10
@UrtsiSantsi
Copy link
Contributor

UrtsiSantsi commented Dec 12, 2023

It is better now, but still not perfect:

  1. When I delete the last few transactions and create new one, the new one will have number bigger by one compared to the last remaining.
  2. When I delete in the middle of the list gaps in numbers will remain.

So I wonder what this numbers represents - they are not the index in the list of all transactions ever created and they are not the index in the current list of transactions?

@JoseBritto
Copy link
Member Author

When I delete in the middle of the list gaps in numbers will remain.

That is intentional. The numbers are the ids of the transactions. We don't want to modify the id of an existing transaction.

When I delete the last few transactions and create new one, the new one will have number bigger by one compared to the last remaining.

I'm not sure I understand. If the biggest id remaining is 6 the new one picked will be 7. Isn't that how its supposed to be?

@UrtsiSantsi
Copy link
Contributor

UrtsiSantsi commented Dec 12, 2023

What I mean is if I delete 7 and the remaining one is 6, when I create new one it will be 7 again. So it is not the id of the transaction, since there was already transaction 7 before.

@nlogozzo
Copy link
Member

What I mean is if I delete 7 and the remaining one is 6, when I create new one it will be 7 again. So it is not the id of the transaction, since there was already transaction 7 before.

Yeah but you deleted transaction 7. So it is no more. Thus the id can be reused...that's the whole point, no? That's the problem u mentioned in #750

@UrtsiSantsi
Copy link
Contributor

Following that logic if I delete 2 instead of 7, the next one should be 2, not 8? After all 2 can be reused.

@UrtsiSantsi
Copy link
Contributor

From a user point of view why do we have these numbers? They are not unique and can be reused, so the only information we get from them is the order of creation, but we can just order transactions by date and achieve the same result.

@JoseBritto
Copy link
Member Author

Following that logic if I delete 2 instead of 7, the next one should be 2, not 8? After all 2 can be reused.

We do not reuse any id numbers. When creating a new transaction Denaro assigns a new permanent unique id number for that transaction. For that it checks what all transactions are present and adds one to the biggest id so that we can get nice sequential numbers. When the last transaction is deleted, it isn't stored anywhere and Denaro doesn't consider its id as "used" (as we don't have any info on that anymore) so when its time to create a new transaction, its id gets reused, but still in the account's database, it is a unique number.

If we use the numbers that are unused in between, the ids that will be assigned doesn't seem to be sequential anymore. It will more or less feel random to the user.

These ids also help to sort based on creation time.

but we can just order transactions by date and achieve the same result

Not really. You can have multiple transactions in the same day. Here ids can be used to see which transaction happened first, if the user has entered them in that order. Also since you can create transactions that are dated in the past and future, dates can't be used for order of creation.

@UrtsiSantsi
Copy link
Contributor

I didn't realize the date can be changed 😅
As a developer I understand the logic behind the numbering, but as a user I find it odd. I will suggest one of 2 options:

  1. Save the last number and always increase that number - this way the user can track deletions
  2. When deleting to renumber transactions so they are consecutive - this is more natural

I will prefer option 2. Even without my suggestions this pull request improves the consistency greatly, fixes #749 and mostly #750.

@JoseBritto
Copy link
Member Author

I didn't realize the date can be changed 😅 As a developer I understand the logic behind the numbering, but as a user I find it odd. I will suggest one of 2 options:

1. Save the last number and always increase that number - this way the user can track deletions

2. When deleting to renumber transactions so they are consecutive - this is more natural

I will prefer option 2. Even without my suggestions this pull request improves the consistency greatly, fixes #749 and mostly #750.

Number 1 makes more sense to me tbh. Regardless I don't think that change makes sense here, as the point of this PR was to improve the consistency of whatever denaro is currently doing. Maybe move that to a separate issue?

In that case this pr is ready to be merged unless @nlogozzo disagrees.

@nlogozzo nlogozzo merged commit 1e84896 into main Dec 12, 2023
4 checks passed
@nlogozzo nlogozzo deleted the consistent-ids branch December 12, 2023 16:36
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.

Inconsistent transaction numbers Strange order of transactions
4 participants