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

✨ allow un-reconciling (unlocking) transactions #2252

Merged
merged 3 commits into from
Jan 22, 2024
Merged

Conversation

MatissJanis
Copy link
Member

In the past month I've found myself duplicating a locked transaction just so I could amend some details. Which is a bit annoying..

Hence my recommendation would be to allow unlocking transactions by clicking on the "lock" icon.

cc @youngcw @zachwhelchel LMK what you think

Copy link

netlify bot commented Jan 19, 2024

Deploy Preview for actualbudget ready!

Name Link
🔨 Latest commit 272a9c5
🔍 Latest deploy log https://app.netlify.com/sites/actualbudget/deploys/65ad34302ebc3000081306f7
😎 Deploy Preview https://deploy-preview-2252.demo.actualbudget.org/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Contributor

github-actions bot commented Jan 19, 2024

Bundle Stats — desktop-client

Hey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle.

As this PR is updated, I'll keep you updated on how the bundle size is impacted.

Total

Files count Total bundle size % Changed
10 5.09 MB → 5.09 MB (+530 B) +0.01%
Changeset
File Δ Size
src/components/modals/ConfirmTransactionEdit.tsx 📈 +273 B (+10.76%) 2.48 kB → 2.74 kB
src/components/transactions/TransactionsTable.jsx 📈 +257 B (+0.46%) 54.89 kB → 55.14 kB
View detailed bundle breakdown

Added

No assets were added

Removed

No assets were removed

Bigger

Asset File Size % Changed
static/js/index.js 2.64 MB → 2.64 MB (+273 B) +0.01%
static/js/wide.js 239.36 kB → 239.61 kB (+257 B) +0.10%

Smaller

No assets were smaller

Unchanged

Asset File Size % Changed
static/js/indexeddb-main-thread-worker-e59fee74.js 13.5 kB 0%
static/js/resize-observer.js 18.37 kB 0%
static/js/BackgroundImage.js 122.29 kB 0%
static/js/ButtonLink.js 379 B 0%
static/js/narrow.js 81.84 kB 0%
static/js/BalanceTooltip.js 6.06 kB 0%
static/js/FiltersMenu.js 28.92 kB 0%
static/js/ReportRouter.js 1.95 MB 0%

Copy link
Contributor

github-actions bot commented Jan 19, 2024

Bundle Stats — loot-core

Hey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle.

As this PR is updated, I'll keep you updated on how the bundle size is impacted.

Total

Files count Total bundle size % Changed
1 1.24 MB 0%

Changeset

No files were changed

View detailed bundle breakdown

Added

No assets were added

Removed

No assets were removed

Bigger

No assets were bigger

Smaller

No assets were smaller

Unchanged

Asset File Size % Changed
kcab.worker.js 1.24 MB 0%

@@ -1349,7 +1354,14 @@ const Transaction = memo(function Transaction(props) {
);
});

function TransactionError({ error, isDeposit, onAddSplit, onDistributeRemainder, style, canDistributeRemainder }) {
Copy link
Member Author

Choose a reason for hiding this comment

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

All of these other changes in the file: automatically applied by prettier. Just code formatting changes.

I wonder why linter was not breaking about them.. weird

@youngcw
Copy link
Member

youngcw commented Jan 19, 2024

I would prefer it to be a bit harder to unlock the transaction. It takes some work to lock, so it should also take some work to unlock imho.

I think it would work well to add an option in the transaction edit menu along with the bulk edit options

@MatissJanis
Copy link
Member Author

@youngcw I'm not quite sure I understand the rationale for explicitly adding user friction. Can you explain your thought process: why would adding more friction here be beneficial? Especially for actions that are undo-able by ctrl+z.

@zachwhelchel
Copy link
Contributor

@MatissJanis why do you need to duplicate a transaction to make changes? You can still make changes to a locked transaction it just pops up a confirm message. I'd think that's easier than duplicating? Am I missing something about your use case?

Screenshot 2024-01-19 at 3 42 28 PM

@MatissJanis
Copy link
Member Author

@zachwhelchel date cannot be changed on reconciled transactions (which makes sense). Also automated imports won't update reconciled transactions (which again: totally makes sense).

Is there a good reason why we would prohibit folks from unlocking? I've read a couple of support threads on Discord where the recommendation was to "just duplicate the transaction". Which doesn't scream to me like particularly friendly solution.

@Teprifer
Copy link

Teprifer commented Jan 19, 2024

My two cents, locking is about preventing unintentional or casual changes, such as imports, bulk actions(if not already, could add a prompt/notice if the selected transactions contain one or more that are locked, don't know current state), and drawing a line between a 'known good' point in time, and what's in flux.

But this is still user data they control, so I don't see why they shouldn't be to easily unlock it to make an intentional change.

@MatissJanis
Copy link
Member Author

cc also @shall0pass I think you had some strong opinions about this too.

--

Either way, even if I sound very direct here in my opinion - I don't have any objections to closing off this PR without merging and leaving the status-quo as is. Lets have a conversation and decide :)

--

Related:
#2110

@zachwhelchel
Copy link
Contributor

It's also worth nothing that we were kind of talking past each other because there is a bug. When I try to change the date on a locked transaction (maybe it's Safari?) it let's me with a warning. When @MatissJanis does the same it just doesn't do anything. So to him it felt like a ton of friction to have to "duplicate" it just to make that change. To me it didn't feel that heavy because mine is working without the bug.

@youngcw
Copy link
Member

youngcw commented Jan 19, 2024

@youngcw I'm not quite sure I understand the rationale for explicitly adding user friction. Can you explain your thought process: why would adding more friction here be beneficial? Especially for actions that are undo-able by ctrl+z.

I feel like if its too easy to undo a reconcile then it makes the reconcile lose its usefulness. I feel like the reconcile is about creating a psuedo fresh start and if I can unlock things too easy it makes my new foundation not mean anything.

A compromise might be to have one of the edit warnings for unlocking instead of hiding the option in a menu. Im not against being able to unlock, it is the users data after all, I just think it needs to be appropriately deliberate of an action.

@youngcw
Copy link
Member

youngcw commented Jan 19, 2024

@zachwhelchel fixing that bug does sound like a good place to start before making other changes

@shall0pass
Copy link
Contributor

I've come around on this a bit. I'm having a hard time justifying the difference between being able to unlock the transaction manually vs deleting the transaction and recreating it. I think if the warning were presented during an "unlock" action, that would be sufficient as "unlocking" a transaction could bring your reconciliation out of balance in the same way as deleting a transaction would.

However, I do think that the duplicate feature we have shouldn't be duplicating a locked or cleared status. Duplicating a reconciled transaction where it copies over as locked also brings the reconciliation out of balance.

That's my 2 cents. Overall, I'm in favor of the change to allow unlocking of a reconciled transaction.

@youngcw
Copy link
Member

youngcw commented Jan 19, 2024

Duplicating a reconciled transaction where it copies over as locked also brings the reconciliation out of balance.

I agree this needs fixed. There is an issue for it #2155. @zachwhelchel would you be willing to to look at it, and maybe the date issue?

@MatissJanis
Copy link
Member Author

Changes:

  • added a warning dialog when un-locking transactions
Screenshot 2024-01-21 at 15 10 03

@shall0pass
Copy link
Contributor

shall0pass commented Jan 21, 2024

I just have one more thought. Once this dialog is clicked through and the reconciliation is out of balance, I'm not sure it's entirely necessary to display it again until after the account is re-reconciled. Thanks for working on this Matiss.

@MatissJanis
Copy link
Member Author

I just have one more thought. Once this dialog is clicked through and the reconciliation is out of balance, I'm not sure it's entirely necessary to display it again until after the account is re-reconciled. Thanks for working on this Matiss.

That should already be the case in this PR, no? After unlocking the warning is no longer shown if you modify notes/payee/amount/other field.

@shall0pass
Copy link
Contributor

shall0pass commented Jan 21, 2024

That should already be the case in this PR, no? After unlocking the warning is no longer shown if you modify notes/payee/amount/other field.

I was thinking more globally for the account. If you edit one transaction and "void" the reconciliation, is there a purpose to the dialog on a different locked transaction. I'm not thinking that kind of change would be in this PR. Just a thought that occured to me.

@youngcw
Copy link
Member

youngcw commented Jan 21, 2024

Im fine with this state. I think trying to track a global reconciled state would just cause confusion when the warnings all of a sudden don't show anymore.

@MatissJanis MatissJanis merged commit a6e38ad into master Jan 22, 2024
19 checks passed
@MatissJanis MatissJanis deleted the matiss/unlock branch January 22, 2024 08:34
@trafico-bot trafico-bot bot added the ✨ Merged Pull Request has been merged successfully label Jan 22, 2024
FlorianLang06 pushed a commit to FlorianLang06/actual that referenced this pull request Mar 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✨ Merged Pull Request has been merged successfully
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants