gavin/maybe
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 28 Wiki Activity

Add rule option to change transaction name #2175

Merged
ahatzz11 merged 10 commits from add-transaction-name-rule into main 2025-05-07 00:11:56 +08:00
Conversation 10 Commits 10 Files Changed 6 +129 -29
ahatzz11 commented 2025-04-29 09:59:57 +08:00 (Migrated from github.com)
Copy Link

This PR allows someone to rename a transaction as part of a transaction rule. This was kind of inspired by #2174. I personally have been wanting a transaction rename, because the transaction list is often a mess of names.

I think this is probably a good candidate for some AI work in the future (e.g. autodetect transaction name), which would probably be similar to autodetecting merchant, but this would allow for a manual rule as well.

I'd love some feedback here on the changes I made to actions_controller.js - I had started making some of the showing/hiding more agnostic and cursor helped fill in some of the nuances of swapping select<>input types.

Here's a video showing off:

  • New rename transaction functionality
  • No regressions in changing the action between select/text types (improvement on some spacing)
    image
  • No regressions in deletion of an action

https://github.com/user-attachments/assets/a87492ca-5f7b-4c4b-a7d3-c79e8fdd4296

@zachgoll I'm curious - what is the future functionality for the function type?

update:
This PR has been refactored a bunch to include HTML templates that are cloned and injected by the stimulus controller.

This PR allows someone to rename a transaction as part of a transaction rule. This was kind of inspired by #2174. I personally have been wanting a transaction rename, because the transaction list is often a mess of names. I think this is probably a good candidate for some AI work in the future (e.g. autodetect transaction name), which would probably be similar to autodetecting merchant, but this would allow for a manual rule as well. I'd love some feedback here on the changes I made to `actions_controller.js` - I had started making some of the showing/hiding more agnostic and cursor helped fill in some of the nuances of swapping select<>input types. Here's a video showing off: - New rename transaction functionality - No regressions in changing the action between select/text types (improvement on some spacing) <img width="1267" alt="image" src="https://github.com/user-attachments/assets/6d902046-a1bb-4173-9e9d-9a3b03e3b662" /> - No regressions in deletion of an action https://github.com/user-attachments/assets/a87492ca-5f7b-4c4b-a7d3-c79e8fdd4296 ~@zachgoll I'm curious - what is the future functionality for the `function` type?~ --- **update:** This PR has been refactored a bunch to include HTML templates that are cloned and injected by the stimulus controller.
zachgoll (Migrated from github.com) reviewed 2025-05-02 19:17:02 +08:00
zachgoll (Migrated from github.com) left a comment
Copy Link

I think this rule idea makes sense. I'm thinking of cases like:

If amount > 1000 AND amount is positive

Set transaction name to: Paycheck: Employer Name

I think this rule idea makes sense. I'm thinking of cases like: If `amount > 1000` AND `amount is positive` Set transaction name to: `Paycheck: Employer Name`
app/javascript/controllers/rule/actions_controller.js
@@ -51,3 +49,1 @@
optionEl.value = option[1];
optionEl.textContent = option[0];
this.valueSelectEl.appendChild(optionEl);
#buildSelectFor(actionExecutor) {
zachgoll (Migrated from github.com) commented 2025-05-02 19:12:19 +08:00
Copy Link

This is definitely the direction we'll need to go to introduce a text input but I'd like to try and keep it consistent with the pattern I've built over in conditions_controller.js with the #build[FieldType]For(model) convention:

0946a1497a/app/javascript/controllers/rule/conditions_controller.js (L37-L41)

My ultimate goal with this pattern is to move all style + Rails form attribute building out of the Stimulus controller so the Stimulus controller can work with 100% generic HTML and have minimal coupling with the view template. I'm not totally sold on my existing pattern yet and I haven't found great documentation around using Stimulus for this type of thing.

One thing I started doing that seems to work pretty well is using HTML templates where we build a template in our ERB view (which has access to our app's styles, form structure, etc.), and then cloning the template and show/hiding it in the Stimulus controller. That might be helpful in this case too. Here's an example of doing that:

ERB defines HTML template (notice how we're heavily leveraging the Rails form builder, which is not available in a Stimulus controller):

0946a1497a/app/views/rule/conditions/_condition_group.html.erb (L30-L35)

Stimulus controller stays "dumb" and only has to know which target element to append the template to:

0946a1497a/app/javascript/controllers/rule/conditions_controller.js (L14-L21)

This is definitely the direction we'll need to go to introduce a text input but I'd like to try and keep it consistent with the pattern I've built over in `conditions_controller.js` with the `#build[FieldType]For(model)` convention: https://github.com/maybe-finance/maybe/blob/0946a1497a0865185ad943d0d82a21f71e171d31/app/javascript/controllers/rule/conditions_controller.js#L37-L41 My ultimate goal with this pattern is to move all style + Rails form attribute building out of the Stimulus controller so the Stimulus controller can work with 100% generic HTML and have minimal coupling with the view template. I'm not totally sold on my existing pattern yet and I haven't found great documentation around using Stimulus for this type of thing. One thing I started doing that seems to work pretty well is using HTML templates where we build a template in our ERB view (which has access to our app's styles, form structure, etc.), and then cloning the template and show/hiding it in the Stimulus controller. That _might_ be helpful in this case too. Here's an example of doing that: ERB defines HTML template (notice how we're heavily leveraging the Rails form builder, which is _not_ available in a Stimulus controller): https://github.com/maybe-finance/maybe/blob/0946a1497a0865185ad943d0d82a21f71e171d31/app/views/rule/conditions/_condition_group.html.erb#L30-L35 Stimulus controller stays "dumb" and only has to know which target element to append the template to: https://github.com/maybe-finance/maybe/blob/0946a1497a0865185ad943d0d82a21f71e171d31/app/javascript/controllers/rule/conditions_controller.js#L14-L21
app/views/rule/actions/_action.html.erb
@@ -2,7 +2,6 @@
zachgoll (Migrated from github.com) commented 2025-05-02 18:57:52 +08:00
Copy Link

To answer your question about the function type, it's actually being used already. A "function" executor would represent a type of action where the user doesn't assign any specific value, but rather, selects a function that will run automatically.

The two function types we currently have would be:

These get their designation through the inheritance chain here:

0946a1497a/app/models/rule/action_executor.rb (L16-L18)

To answer your question about the `function` type, it's actually being used already. A "function" executor would represent a type of action where the user doesn't _assign_ any specific value, but rather, selects a function that will run automatically. The two function types we currently have would be: - https://github.com/maybe-finance/maybe/blob/main/app/models/rule/action_executor/auto_categorize.rb - https://github.com/maybe-finance/maybe/blob/main/app/models/rule/action_executor/auto_detect_merchants.rb These get their designation through the inheritance chain here: https://github.com/maybe-finance/maybe/blob/0946a1497a0865185ad943d0d82a21f71e171d31/app/models/rule/action_executor.rb#L16-L18
zachgoll (Migrated from github.com) commented 2025-05-02 18:59:14 +08:00
Copy Link

Continuing my comment from above, this case statement will still need function added and we need to preserve the "hidden" => !needs_value so when a function is selected, we properly hide any input that is no longer needed.

Continuing my comment from above, this case statement will still need `function` added and we need to preserve the `"hidden" => !needs_value` so when a function is selected, we properly hide any input that is no longer needed.
ahatzz11 (Migrated from github.com) reviewed 2025-05-03 05:00:36 +08:00
app/javascript/controllers/rule/actions_controller.js
@@ -19,38 +24,67 @@ export default class extends Controller {
(executor) => executor.key === e.target.value,
);
ahatzz11 (Migrated from github.com) commented 2025-05-03 04:56:03 +08:00
Copy Link

Does this seem like the right way to ensure the "to" text (a child of the actionValue div) stays in place? Originally I just targeted the html span but I liked the idea of targeting it a bit more specifically.

Does this seem like the right way to ensure the "to" text (a child of the `actionValue` div) stays in place? Originally I just targeted the html `span` but I liked the idea of targeting it a bit more specifically.
app/javascript/controllers/rule/actions_controller.js
@@ -51,3 +49,1 @@
optionEl.value = option[1];
optionEl.textContent = option[0];
this.valueSelectEl.appendChild(optionEl);
#buildSelectFor(actionExecutor) {
ahatzz11 (Migrated from github.com) commented 2025-05-03 04:54:59 +08:00
Copy Link

Alright so I think I have this done. Again - I'm a ruby/rails novice here so if my second attempt is off the mark let me know, but I believe it's much closer to the pattern you've described. I do like this a lot better - all of the styling stays a lot more consistent. I have left a couple of questions/comments about a few things too.

Alright so I _think_ I have this done. Again - I'm a ruby/rails novice here so if my second attempt is off the mark let me know, but I believe it's much closer to the pattern you've described. I do like this a lot better - all of the styling stays a lot more consistent. I have left a couple of questions/comments about a few things too.
app/views/rule/actions/_action.html.erb
@@ -2,7 +2,6 @@
ahatzz11 (Migrated from github.com) commented 2025-05-03 01:45:22 +08:00
Copy Link

Ah I understand the function now - I didn't see that locally before since ai_enabled = false. I turned those on manually just to validate, and have the following:

  • I put needs_value back to ensure the to ___ is not shown when the type is not select/text (we can keep this hardcoded here, or do action.executor.type != "function", I kind of like the positive personally so I did that.
  • I don't have a function in the case since it ends up being hidden. If I'm misunderstanding this let me know
Ah I understand the `function` now - I didn't see that locally before since `ai_enabled = false`. I turned those on manually just to validate, and have the following: - I put `needs_value` back to ensure the `to ___` is not shown when the type is not select/text (we can keep this hardcoded here, or do `action.executor.type != "function"`, I kind of like the positive personally so I did that. - I don't have a `function` in the `case` since it ends up being hidden. If I'm misunderstanding this let me know
ahatzz11 (Migrated from github.com) commented 2025-05-03 01:46:04 +08:00
Copy Link

Oh got it! This makes sense, I missed it (per other comment below)

Oh got it! This makes sense, I missed it (per other comment below)
app/views/rule/actions/_action.html.erb
@@ -14,3 +13,3 @@
<%= tag.div class: class_names("min-w-1/2 flex items-center gap-2", "hidden" => !needs_value),
<%= tag.div class: class_names("min-w-1/2 flex items-center gap-2"),
data: { rule__actions_target: "actionValue" } do %>
ahatzz11 (Migrated from github.com) commented 2025-05-03 05:00:34 +08:00
Copy Link

Now that we have the templates and the injection of those templates from the stimulus controller, this case becomes a bit weird since it is only ever used for the initial render. The action type for that initial render is determined by the form, which currently has a hardcoded action (and I don't see this changing since we want a first item in the list):

441f436187/app/views/rules/_form.html.erb (L24)

I left a comment here explaining this since there is technically unused code here, and if more types come into play they could be put here, but thought I'd get your opinion on what you'd like to see for the initial load vs subsequent injections.

Now that we have the templates and the injection of those templates from the stimulus controller, this `case` becomes a bit weird since it is only ever used for the initial render. The action type for that initial render is determined by the form, which currently has a hardcoded action (and I don't see this changing since we want a first item in the list): https://github.com/maybe-finance/maybe/blob/441f436187231ae4a0a696d353dcb38bbbf1eed0/app/views/rules/_form.html.erb#L24 I left a comment here explaining this since there is technically unused code here, and if more types come into play they _could_ be put here, but thought I'd get your opinion on what you'd like to see for the initial load vs subsequent injections.
ahatzz11 commented 2025-05-03 05:02:52 +08:00 (Migrated from github.com)
Copy Link

A side effect of this PR is that it fixes some weird sizing issues that seem to exist in production:

CleanShot-002815 2025-05-02 at 16 02 06@2x

A side effect of this PR is that it fixes some weird sizing issues that seem to exist in production: ![CleanShot-002815 2025-05-02 at 16 02 06@2x](https://github.com/user-attachments/assets/4f1aea66-b1cd-40cc-a65c-625380ad9413)
🎉 1
ahatzz11 (Migrated from github.com) reviewed 2025-05-03 05:13:34 +08:00
app/views/rule/actions/_action.html.erb
@@ -14,3 +13,3 @@
<%= tag.div class: class_names("min-w-1/2 flex items-center gap-2", "hidden" => !needs_value),
<%= tag.div class: class_names("min-w-1/2 flex items-center gap-2"),
data: { rule__actions_target: "actionValue" } do %>
ahatzz11 (Migrated from github.com) commented 2025-05-03 05:13:33 +08:00
Copy Link

For sake of cleaner code without dead paths, I opted to remove the case here since that's only required for the initial render. This commit is the diff between the case and no case. This case isn't actually needed any more because of the template injection.

I'm happy to revert this if it makes more sense to have it, but I personally like the cleaner code with comment.

For sake of cleaner code without dead paths, I opted to remove the `case` here since that's only required for the initial render. [This commit](https://github.com/maybe-finance/maybe/pull/2175/commits/690daceba20b853fc26186048fcaf81f86f5719f) is the diff between the `case` and no `case`. This `case` isn't actually needed any more because of the template injection. I'm happy to revert this if it makes more sense to have it, but I personally like the cleaner code with comment.
ahatzz11 (Migrated from github.com) reviewed 2025-05-03 05:24:37 +08:00
app/views/rule/actions/_action.html.erb
@@ -14,3 +13,3 @@
<%= tag.div class: class_names("min-w-1/2 flex items-center gap-2", "hidden" => !needs_value),
<%= tag.div class: class_names("min-w-1/2 flex items-center gap-2"),
data: { rule__actions_target: "actionValue" } do %>
ahatzz11 (Migrated from github.com) commented 2025-05-03 05:24:37 +08:00
Copy Link

Another piece of cleanup we can do now that we have template injection on the entire action target (which also means we don't need the case), is we no longer have a need for action_type or needs_value:

Another piece of cleanup we can do now that we have template injection on the entire action target (which also means we don't need the `case`), is we no longer have a need for `action_type` or `needs_value`: - `action_type` was used to determine which one to show for each action. Now that we are injecting rules, this wasn't used. Since the initial load is now hardcoded, we don't really need this - `needs_value` was used to show/hide the entire action target for cases like `function`. This is also not needed anymore since the controller hides the entire action value for cases that don't need any input: https://github.com/maybe-finance/maybe/pull/2175/files#diff-cd0bc09d066aff1b98283823f34902bd4d57c03b6fd45a3bb2f08ea8f6fa0dadR36-R37
zachgoll (Migrated from github.com) reviewed 2025-05-06 00:34:49 +08:00
app/views/rule/actions/_action.html.erb
@@ -2,7 +2,6 @@
zachgoll (Migrated from github.com) commented 2025-05-06 00:34:49 +08:00
Copy Link

Yep, looks good! You're correct I misspoke on the case statement. We do not need the function type there.

Yep, looks good! You're correct I misspoke on the case statement. We do not need the `function` type there.
zachgoll (Migrated from github.com) approved these changes 2025-05-06 00:39:15 +08:00
zachgoll (Migrated from github.com) left a comment
Copy Link

Works great! Left one small suggestion around the templating but otherwise I think this is good to go.

Works great! Left one small suggestion around the templating but otherwise I think this is good to go.
app/javascript/controllers/rule/actions_controller.js
@@ -19,38 +24,67 @@ export default class extends Controller {
(executor) => executor.key === e.target.value,
);
zachgoll (Migrated from github.com) commented 2025-05-06 00:38:47 +08:00
Copy Link

I don't think it's a big deal either way, but I'd probably throw the "to" span directly in each of the templates. It's obviously slightly duplicative, but I think it increases clarity and simplifies things enough to warrant that. This way, the replacements all happen as a single string of markup:

<template data-rule--actions-target="selectTemplate">
  <span class="font-medium uppercase text-xs">to</span>
  <%= form.select :value, [], {} %>
</template>

<template data-rule--actions-target="textTemplate">
  <span class="font-medium uppercase text-xs">to</span>
  <%= form.text_field :value, placeholder: "Enter a value" %>
</template>
I don't think it's a big deal either way, but I'd probably throw the "to" span directly in each of the templates. It's obviously slightly duplicative, but I think it increases clarity and simplifies things enough to warrant that. This way, the replacements all happen as a single string of markup: ```erb <template data-rule--actions-target="selectTemplate"> <span class="font-medium uppercase text-xs">to</span> <%= form.select :value, [], {} %> </template> <template data-rule--actions-target="textTemplate"> <span class="font-medium uppercase text-xs">to</span> <%= form.text_field :value, placeholder: "Enter a value" %> </template> ```
ahatzz11 (Migrated from github.com) reviewed 2025-05-06 01:53:46 +08:00
app/javascript/controllers/rule/actions_controller.js
@@ -19,38 +24,67 @@ export default class extends Controller {
(executor) => executor.key === e.target.value,
);
ahatzz11 (Migrated from github.com) commented 2025-05-06 01:53:45 +08:00
Copy Link

Great idea - I like this a lot more than having a secondary piece of this be updated by the controller instead of inside the template. Definitely cleaner code this way.

Great idea - I like this a lot more than having a secondary piece of this be updated by the controller instead of inside the template. Definitely cleaner code this way.
ahatzz11 commented 2025-05-06 02:19:50 +08:00 (Migrated from github.com)
Copy Link

@zachgoll I agree about the to inside each template - that is a lot cleaner. When I was playing (since I had that PR that deleted all my tags) I found an edge case where no options for a specific thing (tags, merchants) was not a great UX, so I added a (none) disabled select box for that case, which matches the current drawer text when there is no tag to choose from.

I also found a few weird cases where when converting from a select to a text where a selection has been chosen already and now you're editing that same field, the text box would get pre-filled with some kind of identifier, so buildTextInputFor now clears the text.

Let me know what you think!

image
@zachgoll I agree about the `to` inside each template - that is a lot cleaner. When I was playing (since I had that PR that deleted all my tags) I found an edge case where no options for a specific thing (tags, merchants) was not a great UX, so I added a `(none)` disabled select box for that case, which matches the current drawer text when there is no tag to choose from. I also found a few weird cases where when converting from a select to a text where a selection has been chosen already and now you're editing that same field, the text box would get pre-filled with some kind of identifier, so `buildTextInputFor` now clears the text. Let me know what you think! <img width="593" alt="image" src="https://github.com/user-attachments/assets/8141e3a9-4952-4b39-833d-31f77ebf0b1b" />
zachgoll (Migrated from github.com) approved these changes 2025-05-07 00:11:44 +08:00
zachgoll (Migrated from github.com) left a comment
Copy Link

Yeah I think these improvements and changes all make sense! Looks great!

Yeah I think these improvements and changes all make sense! Looks great!
Sign in to join this conversation.
Reviewers
No Reviewers
ahatzz11
zachgoll
Labels
Clear labels
📆 Planned
Not ready to be worked on yet, but planned for future
 💻 Self Hosted only
Issues pertaining to self-hosted versions of Maybe
 ⚙️ Config
ℹ️ Needs info
Further information is requested
 📱 Mobile
Bugs exclusively happening on mobile devices
 1️⃣ High Priority
High priority issues will be worked on next
 🚨 Urgent
Issues that are urgent for app functionality
 3️⃣ Low priority
Contributions accepted, but Maybe team will not be working on this in the near term
 2️⃣ Medium Priority
Community contributions accepted, Maybe team only works on if there are no high priority items open
 ⚡ Performance
Performance issues to address
 🎨 UI / UX
Issues related to UI / UX interactions and design.
 💎 Bounty
💰 Rewarded
📊 Data issue
Issues where the app functionally works, but the data is missing or inaccurate.
 🙋 Bounty claim
$1.5K
Community
This is a great issue for community members to work on
 dependencies
Pull requests that update a dependency file
 Design
Hosted only
Hosted app only
 Required self hoster data migration
Self hosters must run data migrations to resolve this issue
 ruby
Pull requests that update Ruby code
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
gavin
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: gavin/maybe#2175
Delete Branch "add-transaction-name-rule"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?