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 amount filter to include both incoming and outgoing amounts #2643
Fix amount filter to include both incoming and outgoing amounts #2643
Conversation
✅ Deploy Preview for actualbudget ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Bundle Stats — desktop-clientHey 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
Changeset No files were changed View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger No assets were bigger Smaller No assets were smaller Unchanged
|
Bundle Stats — loot-coreHey 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
Changeset
View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger
Smaller No assets were smaller Unchanged No assets were unchanged |
Apologies, I'll need some time to address the E2E test failing. I would appreciate some comments on the approach if there are issues with this or concerns it might have negative effects on others |
I think as long as it only affects "amount" and the "inflow/outflow" are unchanged then you are fine. Also, it would need to be applied to all ops and not just "eq". |
I think that it probably would be good to maybe prevent negatives when looking at "amount" since the sign is ignored. Also, you cant save the different amount types as unique filters. They all show up as the same between "amount", inflow, or outflow. |
Thanks for the comments folks! I've updated the logic to apply to all operations. Note: This change does mean filtering with a negative number will show no results. If that's undesirable, I can add some logic to change that I still haven't had time to figure out the E2E tests yet, so I'll move this to draft for the time being |
…oFilterIncomingAndOutgoing
…updateFilterAmountToFilterIncomingAndOutgoing
Pulling in changes seemed to resolve the E2E test problem! So ready for review! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good to me
@youngcw It looks like this merge might have broken the filters: (the video is on another PR, but I get the same behavior on edge) |
Ok I see the behavior was intentionally changed. But this is super confusing for users now. Many will probably do as I did, select "Amount" and see there is a less than and try to use it. |
Also the terminology "inflow" and "outflow" does not match the use of "payment" and "deposit" elsewhere. |
I think it's a fair point that this may be a breaking change for existing users workflows. From the perspective of a new user, the old behavior was confusing and not intuitive. Is there a way we can clearly signal to users that this behavior has changed other than the release notes? |
I'm not sure it's all that confusing for users this way. There were complaints that it was more confusing the other way because there's no negatives in the app then we have to assume that payments should be treated as negative when using filters. It's a bit confusing for existing users because it's opposite of what it used to be but from the perspective of a new user it's much more intuitive this way. Less than can still be used. It doesn't have to mean negative. If you want transactions less than 5 you'd use it. |
I agree here, I'd be happy to see this changed. |
Correct me if I am wrong, but with the new change you can never match a transaction if you set the filter "Amount" to be "less than 0"? Maybe a simple warning message there would help educate users about the change? |
Hi folks, this address #1935, which had some confusing UX. I also added some simple tests to verify the behavior.
I want to acknowledge that changing it in the transaction rules may not be the best spot, I don't have a ton of context on this repo yet. I also considered updating the filter to allow using
oneOf
behind the scenes for the number type, but that seemed to have more knock on effects that weren't as obvious.Happy to update the logic if there is a better approach. Thank you!