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

[Feature] Add option to control the "cleared state" in Rules #482

Merged
merged 11 commits into from
Jan 21, 2023

Conversation

shall0pass
Copy link
Contributor

This adds the ability to control the "cleared" flag from the rules dialog by setting the value to true or false.

image

@netlify
Copy link

netlify bot commented Jan 18, 2023

Deploy Preview for actualbudget ready!

Name Link
🔨 Latest commit a6f4bd9
🔍 Latest deploy log https://app.netlify.com/sites/actualbudget/deploys/63cc53b0d7812d0008c505ed
😎 Deploy Preview https://deploy-preview-482--actualbudget.netlify.app/
📱 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 settings.

@shall0pass shall0pass changed the title Cleared rules [Feature] Add option to control the "cleared state" in Rules Jan 18, 2023
@MatissJanis
Copy link
Member

I am able to create a rule that sets cleared to "hello". That does not seem right.

Screenshot 2023-01-18 at 22 09 55

@j-f1
Copy link
Contributor

j-f1 commented Jan 18, 2023

Maybe this should grab the checkbox from the transaction row and display itself as “set cleared ⏷ to ☑️”?

@shall0pass
Copy link
Contributor Author

shall0pass commented Jan 19, 2023

I've added a checkbox instead of a text box to toggle the state. Shouldn't have any data validation issues with that :)

@shall0pass
Copy link
Contributor Author

shall0pass commented Jan 20, 2023

I've setup a rule that "one of" "venmo, cash, or ca$happ" will result in a true cleared state. When I enter a transfer between two of the three accounts I've specified, only 1 of the transactions receives the rule. It appears the rules aren't applied to both transaction entries of a transfer between accounts.

Edit: The "notes" will update as expected, but not the cleared state. I clearly did something wrong. I'll need help 🙏.

Comment on lines 154 to 166
switch (field) {
case 'cleared':
content = (
<Checkbox
checked={value}
value={value}
onChange={e => onChange(!value)}
/>
);
break;

default:
}
Copy link
Member

Choose a reason for hiding this comment

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

❓ question: ‏why would we need a switch case here? IMO we can return the checkbox for all fields. And if sometime in the far future we need to have a different functionality for some other field - we can add it then. But until that time comes we can keep the code simple.

Suggested change
switch (field) {
case 'cleared':
content = (
<Checkbox
checked={value}
value={value}
onChange={e => onChange(!value)}
/>
);
break;
default:
}
content = (
<Checkbox
checked={value}
value={value}
onChange={e => onChange(!value)}
/>
);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a great point. I'll make the change.

@shall0pass
Copy link
Contributor Author

shall0pass commented Jan 21, 2023

I've learned a little more about the "cleared" state, at least as it pertains to transfers within Actual. The rules are applied upon entering a transaction, therefore any of the rules applied to the first transaction will be applied to the newly created transaction for the transfer. However, because the rules are already applied to the first transaction, they are never directly applied to the 2nd automatically created transaction. Because of this logic, I think it would be wiser to tackle this issue in a different PR.

I proved this logic to myself by using this test case:

  1. Create a rule that adds a note to a transaction if the Account name is "Account 1".
  2. Add a transfer from "Account 1" to "Account 2". Results: "Account 1" and "Account 2" both have the note.
  3. Add a transfer from "Account 2" to "Account 1". Results: The note is not applied to either transaction. Expected: Account 1 to have a note.

I'm not making a case that this is wrong or right. I can see a strong argument for the way it's currently implemented. In the case of wanting to control the cleared status of an "Instant cleared" type account such as Venmo or Cash, it's a bit unfortunate.

TLDR: This doesn't work as expected for transfers between two accounts that have a "cleared rule" applied.

@MatissJanis
Copy link
Member

@shall0pass your PR is not changing this functionality, right? So I'd say it's an existing limitation, but it should not block introducing the cleared field for rules.

Copy link
Member

@MatissJanis MatissJanis left a comment

Choose a reason for hiding this comment

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

Code looks good 👍

@shall0pass
Copy link
Contributor Author

@shall0pass your PR is not changing this functionality, right? So I'd say it's an existing limitation, but it should not block introducing the cleared field for rules.

No. This code doesn't change that functionality. It's just my observations when testing the different scenarios.

@MatissJanis
Copy link
Member

Got it! if you're interested to pursue this further, then I'd encourage to open a bug report issue. Otherwise: are you happy if I merge this PR?

@shall0pass
Copy link
Contributor Author

Got it! if you're interested to pursue this further, then I'd encourage to open a bug report issue. Otherwise: are you happy if I merge this PR?

I'm happy with it. Thanks!

@MatissJanis MatissJanis merged commit d36417e into actualbudget:master Jan 21, 2023
@trafico-bot trafico-bot bot added ✨ Merged Pull Request has been merged successfully and removed ✅ Approved labels Jan 21, 2023
@shall0pass shall0pass deleted the cleared_rules branch January 22, 2023 14:10
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.

None yet

3 participants