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

Not possible to delete payee or category if present in deleted transactions #5296

Closed
whalley opened this issue Nov 1, 2022 · 12 comments
Closed
Assignees
Milestone

Comments

@whalley
Copy link
Member

whalley commented Nov 1, 2022

Following the implementation of #2963 it is not possible to delete:

  • A payee that in use no where but in deleted transactions
  • A category that in use no where but in deleted transactions

This may be confusing for users who are unfamiliar with this new feature or who never enable the view of deleted transactions.

Options:

  • Live with what we have, and accept the possible confusion.
  • Allow deletion of payees/categories that ONLY exist in deleted transactions (i.e. treat them as unused), and permanently delete any associated transactions
  • Warn the user that the payees/categories are in use in deleted transactions and ask for confirmation before deleting them (and the associated deleted transactions).
@whalley whalley added the bug label Nov 1, 2022
@whalley whalley added this to the v1.6.1 milestone Nov 1, 2022
@tactilis
Copy link

tactilis commented Nov 2, 2022

I think the safest and least surprising thing for the user, is your suggestion:

Warn the user that the payees/categories are in use in deleted transactions and ask for confirmation before deleting them (and the associated deleted transactions).




While on the topic of user confusion arising from the new Deleted Transactions functionality:

This may be confusing for users who are unfamiliar with this new feature or who never enable the view of deleted transactions.

I've raised #5298 , which discusses this.

@n-stein
Copy link
Contributor

n-stein commented Nov 3, 2022

Fixed in PR #5307.

Deleting a category or payee used only in deleted transactions will now show a warning confirmation dialog. If the user proceeds both the selected category/payee as well as the associated deleted transactions are removed permanently.

image
image
image

@n-stein
Copy link
Contributor

n-stein commented Nov 3, 2022

Do we need to add some kind of "Tip" saying the user can find the deleted transactions in the view found on the Nav Panel or enabled via View -> Show Deleted Transactions?

@vomikan
Copy link
Member

vomikan commented Nov 3, 2022

Do we need to add some kind of "Tip" saying the user can find the deleted transactions in the view found on the Nav Panel or enabled via View -> Show Deleted Transactions?

Let's wait for a request from users.

whalley added a commit that referenced this issue Nov 3, 2022
fix(#5296): Allow delete categories & payees used in deleted transactions
@whalley whalley added the fixed label Nov 4, 2022
@whalley whalley closed this as completed Nov 7, 2022
@n-stein
Copy link
Contributor

n-stein commented Nov 16, 2022

I think this needs to be reopened. On the v16 upgrade the columns are initialized with NULL, but the selection find(DELETEDTIME(wxEmptyString)) does not pick up NULL values, so you are able to delete categories which are used.

@n-stein
Copy link
Contributor

n-stein commented Nov 16, 2022

Possible to change the v16 upgrade script to default to empty string?

alter table CHECKINGACCOUNT_V1 add column LASTUPDATEDTIME text default '';
alter table CHECKINGACCOUNT_V1 add column DELETEDTIME text default '';

@whalley
Copy link
Member Author

whalley commented Nov 16, 2022

Will need to create a v17 and upgrade from v16 to v17 as v16 is out in the wild now.

@whalley whalley removed the fixed label Nov 16, 2022
@whalley whalley reopened this Nov 16, 2022
@n-stein
Copy link
Contributor

n-stein commented Nov 16, 2022

It can be fixed by code. Instead of pulling using DELETEDTIME(wxEmptyString) we would have to pull all entries then loop through and check DELETEDTIME one at a time and break when it finds a blank/null value. Would this be cleaner than a v17 migration?

@whalley
Copy link
Member Author

whalley commented Nov 16, 2022

That code change should be fine. It's obviously less elegant/efficient but the check is done quite rarely anyway.

@n-stein
Copy link
Contributor

n-stein commented Nov 16, 2022

PR #5336

@n-stein
Copy link
Contributor

n-stein commented Nov 16, 2022

Shouldn't be much performance hit since all it needs to find is the first non-deleted transaction which is very likely the first in the data set.

@n-stein
Copy link
Contributor

n-stein commented Nov 16, 2022

In the v17 upgrade script we can update NULL to '' then go back to the cleaner code in 1.6.2.

update CHECKINGACCOUNT_V1 set DELETEDTIME = '' where DELETEDTIME is null;
update CHECKINGACCOUNT_V1 set LASTUPDATEDTIME = '' where LASTUPDATEDTIME is null;

whalley added a commit that referenced this issue Nov 16, 2022
fix(#5296): fix for DELETEDTIME = NULL
@whalley whalley added the fixed label Nov 16, 2022
@whalley whalley closed this as completed Nov 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants