search

NickvisionApps / Denaro

Manage your personal finances
https://flathub.org/apps/details/org.nickvision.money
MIT License
552 stars 37 forks source link

Fix inconsistent ids #758

Closed JoseBritto closed 6 months ago

JoseBritto commented 6 months ago

Fixes #750 Fixes #749

JoseBritto commented 6 months ago

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

nlogozzo commented 6 months ago

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

nlogozzo commented 6 months ago

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

JoseBritto commented 6 months ago

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

JoseBritto commented 6 months ago

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

Alright

JoseBritto commented 6 months ago

@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 commented 6 months ago

Should the date be Dec 1st or today?

Dec 1st please . So 2023-12-01

JoseBritto commented 6 months ago

Oops 😂 Fixed now

UrtsiSantsi commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

I didn't realize the date can be changed :sweat_smile: 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 commented 6 months ago

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.