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

Improve rules - add name, allow sorting, improve UI #2177

Merged
ahatzz11 merged 38 commits from rule-name into main 2025-05-14 03:53:14 +08:00
Conversation 29 Commits 38 Files Changed 15 +176 -79
ahatzz11 commented 2025-04-30 08:22:31 +08:00 (Migrated from github.com)
Copy Link

This PR adds and improves a bunch of stuff around rules! I've been playing around with these more recently and thought they could use a few upgrades

  • Add ability to name a rule - this is currently an optional field. I thought about doing some kind of database seed to give all current rules a name, something like "Custom Rule" and then require a name, but this felt a bit heavy handed. Happy to add this if others feel like its a good idea.
  • Add sorting for rules, by name and date. I'd like to add some filtering options here eventually as well.
    • Something I went back and forth on here is, when sorting desc name, if it should always show named rules first or not. This ended up requiring a bit more custom sql with Arel(), so I opted to just keep the default behavior, which is null values first. Also happy to switch this up if that makes more sense. A checkbox option as a filter might be a middle ground here.
  • Improve rule page and form design - On top of adding the name to the main card, I also added some new styling to kind of show the format of the rules of IF this, THEN this, FOR this period of time with some icons. This carries over to the form as well.

As a side note - I see that #2154 is in progress and will probably change a bunch of things (like the icon() helper). If it makes more sense to wait for this (and my other PRs) until after that is merged, I can handle the changes to cut down on conflicts/ongoing changes.

Rule page:
CleanShot-002672 2025-04-29 at 18 41 32@2x

Form:
CleanShot-002676 2025-04-29 at 19 15 08@2x

Video showing creation of a rule + sorting:

https://github.com/user-attachments/assets/da47dd8b-2ad1-4834-bc97-3aec9af7bb29

This PR adds and improves a bunch of stuff around rules! I've been playing around with these more recently and thought they could use a few upgrades - Add ability to name a rule - this is currently an optional field. I thought about doing some kind of database seed to give all current rules a name, something like "Custom Rule" and then require a name, but this felt a bit heavy handed. Happy to add this if others feel like its a good idea. - Add sorting for rules, by name and date. I'd like to add some filtering options here eventually as well. - Something I went back and forth on here is, when sorting desc name, if it should always show named rules first or not. This ended up requiring a bit more custom sql with `Arel()`, so I opted to just keep the default behavior, which is null values first. Also happy to switch this up if that makes more sense. A checkbox option as a filter might be a middle ground here. - Improve rule page and form design - On top of adding the name to the main card, I also added some new styling to kind of show the format of the rules of `IF` this, `THEN` this, `FOR` this period of time with some icons. This carries over to the form as well. As a side note - I see that #2154 is in progress and will probably change a bunch of things (like the `icon()` helper). If it makes more sense to wait for this (and my other PRs) until after that is merged, I can handle the changes to cut down on conflicts/ongoing changes. Rule page: ![CleanShot-002672 2025-04-29 at 18 41 32@2x](https://github.com/user-attachments/assets/127f7aaf-5a22-4bb2-8de5-b1e5f9317b53) Form: ![CleanShot-002676 2025-04-29 at 19 15 08@2x](https://github.com/user-attachments/assets/fd5ae2a3-ef29-491d-804c-24422db1e0cc) Video showing creation of a rule + sorting: https://github.com/user-attachments/assets/da47dd8b-2ad1-4834-bc97-3aec9af7bb29
ahatzz11 (Migrated from github.com) reviewed 2025-04-30 08:52:35 +08:00
app/views/rules/index.html.erb
@@ -35,4 +32,2 @@
<div class="bg-container shadow-border-xs rounded-xl p-4">
<% if @rules.any? %>
ahatzz11 (Migrated from github.com) commented 2025-04-30 08:52:35 +08:00
Copy Link

For some reason brakeman started warning about these lines so instead of a dynamic render I'm doing it a bit more explicitly. This isn't something I changed directly, although maybe the sorting changed how this works here. I don't quite understand this - I'd love some more insight here from someone who more ruby/rails than I do.

Dynamic Render Path: 1

== Warnings ==

Confidence: Weak
Category: Dynamic Render Path
Check: Render
Message: Render path contains parameter value
Code: render(action => Current.family.rules.order(validate_sort_param(params[:sort_by], "name") => validate_direction_param(params[:direction], "asc")), {})
File: app/views/rules/index.html.erb
Line: 68
For some reason brakeman started warning about these lines so instead of a dynamic render I'm doing it a bit more explicitly. This isn't something I changed directly, although maybe the sorting changed how this works here. I don't quite understand this - I'd love some more insight here from someone who more ruby/rails than I do. ``` Dynamic Render Path: 1 == Warnings == Confidence: Weak Category: Dynamic Render Path Check: Render Message: Render path contains parameter value Code: render(action => Current.family.rules.order(validate_sort_param(params[:sort_by], "name") => validate_direction_param(params[:direction], "asc")), {}) File: app/views/rules/index.html.erb Line: 68 ```
zachgoll commented 2025-04-30 20:13:04 +08:00 (Migrated from github.com)
Copy Link

@ahatzz11 overall looking good! I'll take a closer look at this PR and #2174, #2175 once I get our pre-launch stuff done today/tomorrow!

@ahatzz11 overall looking good! I'll take a closer look at this PR and #2174, #2175 once I get our pre-launch stuff done today/tomorrow!
🎉 1
zachgoll (Migrated from github.com) reviewed 2025-05-02 19:38:41 +08:00
zachgoll (Migrated from github.com) left a comment
Copy Link

I agree the sorting isn't the best in the existing UI. The Brakeman warning you're getting is because you're dynamically constructing the sort query. Even though you're safe-listing values, Brakeman seems to always flag anything custom passed to ActiveRecord query builders.

That aside, I'd actually propose a much simpler sorting convention that will naturally clear out the brakeman warning—what if we just sorted by "most recently edited" (order(updated_at: :desc))? I think that's probably more helpful than sorting alphabetically based on my own usage patterns of this feature.

In regards to the design changes + name addition, I'll have to defer to @justinfar on that. I know he has an official Figma spec for this and I want to stick with that whenever possible. That said, we've had a few things in motion around the design of this feature so I'll let @justinfar speak to that!

I agree the sorting isn't the best in the existing UI. The Brakeman warning you're getting is because you're dynamically constructing the sort query. Even though you're safe-listing values, Brakeman seems to always flag anything custom passed to ActiveRecord query builders. That aside, I'd actually propose a much simpler sorting convention that will naturally clear out the brakeman warning—what if we just sorted by "most recently edited" (`order(updated_at: :desc)`)? I think that's probably more helpful than sorting alphabetically based on my own usage patterns of this feature. In regards to the design changes + name addition, I'll have to defer to @justinfar on that. I know he has an official Figma spec for this and I want to stick with that whenever possible. That said, we've had a few things in motion around the design of this feature so I'll let @justinfar speak to that!
ahatzz11 commented 2025-05-03 08:03:53 +08:00 (Migrated from github.com)
Copy Link

Good to know about Brakeman!

I do like like updated_at over created_at as the default, I always like seeing more recently touched things as well - that has been updated now.

Personally, I think being able to sort alphabetically, especially when tied to a name, would be super helpful. Some of this comes down to how people use the naming convention, but I can see myself having a lot of rules and being able to give them meaningful order other than a timestamp would be great, e.g.

Auto - merchants
Auto - categories
Income - {job1}
Income - {job2}
Category - {something}
Category - {something}

I think it's probably a bit hard to see how alphabetical sorting would fit into usage today since there aren't any names at this point. There was also a feature request for this today - #2198, so I think others would have similar cases. Having ability to search would be an awesome additional feature.

Would love to hear design feedback! I think it looks great, but don't want to veer too far off a larger vision 🙂

Good to know about Brakeman! I do like like `updated_at` over `created_at` as the default, I always like seeing more recently touched things as well - that has been updated now. Personally, I think being able to sort alphabetically, especially when tied to a name, would be super helpful. Some of this comes down to _how_ people use the naming convention, but I can see myself having a lot of rules and being able to give them meaningful order other than a timestamp would be great, e.g. ``` Auto - merchants Auto - categories Income - {job1} Income - {job2} Category - {something} Category - {something} ``` I think it's probably a bit hard to see how alphabetical sorting would fit into usage today since there aren't any names at this point. There was also a feature request for this today - #2198, so I think others would have similar cases. Having ability to _search_ would be an awesome additional feature. Would love to hear design feedback! I think it looks great, but don't want to veer too far off a larger vision 🙂
zachgoll commented 2025-05-06 00:20:24 +08:00 (Migrated from github.com)
Copy Link

@ahatzz11 those are some valid points. I do agree if we had a name property in there then alphabetical sorting could allow users to create their own naming convention for organization.

@justinfar thoughts on all of this?

@ahatzz11 those are some valid points. I do agree if we had a `name` property in there then alphabetical sorting could allow users to create their own naming convention for organization. @justinfar thoughts on all of this?
justinfar commented 2025-05-06 02:17:00 +08:00 (Migrated from github.com)
Copy Link

Fantastic work here @ahatzz11, I made some slight adustments in terms of the UI (so that it's more consistent with other views across the product) but other than that not much else from my end in terms of feedback.

Changes made:

  • Removed icons for if/then/for
  • Changed icon for condition group
  • Changed sorting by name/date to dropdown selection and then ascending/descending is seperated onto an icon button
  • Followed your format for how rules are displayed, but they're seperated by a divider (rather than seperate containers)
  • Disabled rules are given a text/secondary color

Link to the the Figma file:
https://www.figma.com/design/lonJmVk3HYkwZoIO7xYP2w/Maybe-App--Community-?node-id=6370-1960

Preview of the changes made:

image
image

Fantastic work here @ahatzz11, I made some _slight adustments_ in terms of the UI (so that it's more consistent with other views across the product) but other than that not much else from my end in terms of feedback. Changes made: - Removed icons for if/then/for - Changed icon for condition group - Changed sorting by name/date to dropdown selection and then ascending/descending is seperated onto an icon button - Followed your format for how rules are displayed, but they're seperated by a divider (rather than seperate containers) - Disabled rules are given a `text/secondary` color Link to the the Figma file: https://www.figma.com/design/lonJmVk3HYkwZoIO7xYP2w/Maybe-App--Community-?node-id=6370-1960 Preview of the changes made: ![image](https://github.com/user-attachments/assets/fc6dcbab-5f50-4456-a42c-de4eecfce7ec) ![image](https://github.com/user-attachments/assets/e7ed69fd-33af-4c98-87d2-d3c72c1f0267)
ahatzz11 commented 2025-05-06 02:31:59 +08:00 (Migrated from github.com)
Copy Link

@justinfar thanks for the feedback! I'll work on this over the next couple of days. Can you link the figma file? I think a copy/paste got missed in your comment.

One question I see right away - on your screenshot the font-mono on the IF/THIS/FOR is gone, do you want that to be normal font on the edit screen? Or should we keep those on the edit to match the list page? (I personally like the font-mono, but again happy to do whatever you think is best for the bigger vision!)

@justinfar thanks for the feedback! I'll work on this over the next couple of days. Can you link the figma file? I think a copy/paste got missed in your comment. One question I see right away - on your screenshot the `font-mono` on the `IF/THIS/FOR` is gone, do you want that to be normal font on the edit screen? Or should we keep those on the edit to match the list page? (I personally like the font-mono, but again happy to do whatever you think is best for the bigger vision!)
justinfar commented 2025-05-06 02:36:35 +08:00 (Migrated from github.com)
Copy Link

@ahatzz11 Ah thanks for letting me know, just updated the original comment, here's the link again just in case:
https://www.figma.com/design/lonJmVk3HYkwZoIO7xYP2w/Maybe-App--Community-?node-id=6370-1960

One question I see right away - on your screenshot the font-mono on the IF/THIS/FOR is gone, do you want that to be normal font on the edit screen? Or should we keep those on the edit to match the list page? (I personally like the font-mono, but again happy to do whatever you think is best for the bigger vision!)

Yep I'd like us to keep using the normal font on the edit rules view because I'd like us to use mono only for cases where we have numericals/data viz/value comparison etc (where it makes sense)

@ahatzz11 Ah thanks for letting me know, just updated the original comment, here's the link again just in case: https://www.figma.com/design/lonJmVk3HYkwZoIO7xYP2w/Maybe-App--Community-?node-id=6370-1960 > One question I see right away - on your screenshot the `font-mono` on the `IF/THIS/FOR` is gone, do you want that to be normal font on the edit screen? Or should we keep those on the edit to match the list page? (I personally like the font-mono, but again happy to do whatever you think is best for the bigger vision!) Yep I'd like us to keep using the normal font on the edit rules view because I'd like us to use mono only for cases where we have numericals/data viz/value comparison etc (where it makes sense)
👍 1
ahatzz11 commented 2025-05-07 02:44:44 +08:00 (Migrated from github.com)
Copy Link

@justinfar While I'm working on this I've got a few design questions!

  1. I can make the form.select match your design with bg-transparent, but do we want to keep that consistent with other dropdowns and have it look something like this? I can't seem to find another example. The transparent does look nice, but without cursor pointer I think is tough to notice it's a clickable area.
    CleanShot-002888 2025-05-06 at 13 41 56@2x
  2. Right now I noticed that none of our selects have cursor-pointer - is that something we should have across the board?
@justinfar While I'm working on this I've got a few design questions! 1. I can make the form.select match your design with `bg-transparent`, but do we want to keep that consistent with other dropdowns and have it look something like this? I can't seem to find another example. The transparent does look nice, but without cursor pointer I think is tough to notice it's a clickable area. ![CleanShot-002888 2025-05-06 at 13 41 56@2x](https://github.com/user-attachments/assets/26ef026e-0efe-4602-8b59-61d1f1217b2f) 2. Right now I noticed that none of our selects have `cursor-pointer` - is that something we should have across the board?
ahatzz11 commented 2025-05-07 08:17:16 +08:00 (Migrated from github.com)
Copy Link

Alright the current code is now updated with most of the design changes:

  • Removed icons for if/then/for
  • Changed icon for condition group
  • Changed sorting by name/date to dropdown selection and then ascending/descending is separated onto an icon button
    • Still waiting for some answers from Justin above
  • Followed your format for how rules are displayed, but they're seperated by a divider (rather than separate containers)
  • Disabled rules are given a text/secondary color

@zachgoll I could use some help/advice on the implementation on displaying the rules with a divider. I messed around with this for quite a while and couldn't get to anything perfect or that I really liked, but the current code is about as close as I could get it. The shadow-border-xs utility is great for being around a single thing (like the div that holds all the cards), but once I add it to each card with 0 spacing between it, I get a sloppy border because the border is in the margin and it becomes 2× bigger than intended. I also tried to make some new shadow-divider-{size} utilities, but didn't want to start making new utilities if we didn't think there would be other reasons for them. I ended up landing on using the divider from the sidebar. It isn't either (although close enough that most eyes wouldn't notice) <div class="h-px bg-alpha-black-100 w-full"></div>

Big edit: I decided to forego the shadow-box here because we're not really making a shadow around a card anymore, but instead having a border around each rule. I ended up just using <div class="border-b border-subdued"></div> to make the divider line, utilizing the existing border styling from the border-utils. I think this turned out excellent:

CleanShot-002915 2025-05-06 at 20 17 17@2x

One note - the figma says the border is subdued but that color seemed to not match very well, so I opted for border-secondary. I'm not sure if this is just a mismatch between figma and the design system or if I'm missing something.

Additionally, I updated some text- and bg- attributes and rules now has a much nicer dark mode

image

One weird thing I've found with sorting - because it's within a form right now, changing either option submits the form. A side effect of this, because of the direction toggle, is that the direction always toggles. This means if you're on name/asc and you move to updated_at, the direction becomes desc. If you're on name/desc and you move to updated_at, the direction becomes asc.

This is a bit strange and not quite what I expect to happen. I've got a couple ways to solve this, and wanted some feedback:

  1. Move from a button and a form.hidden_field to a LinkComponent that just changes the route:

    <%= render LinkComponent.new(
      icon: "arrow-up-down",
      size: :sm,
      title: "Toggle sort direction",
      href: rules_path(sort_by: @sort_by, direction: @direction == "asc" ? "desc" : "asc"),
      variant: :icon
      ) do %>

    This has a slightly odd side effect that it's a link, so browsers show the link on hover. I'm not too bothers by this (obviously there are a lot of other links in the app).

  2. Add a new controller (sort_direction_controller):

    import { Controller } from "@hotwired/stimulus";

export default class extends Controller {
  static targets = ["direction"];

  toggle(event) {
    event.preventDefault();
    const directionInput = this.directionTarget;
    directionInput.value = directionInput.value === "asc" ? "desc" : "asc";
    this.element.requestSubmit();
  }
}

    In theory this could be reused in other places that had sorting direction, but I don't think that exists anywhere else at the moment.

Alright the current code is now updated with most of the design changes: - [x] Removed icons for if/then/for - [x] Changed icon for condition group - [x] Changed sorting by name/date to dropdown selection and then ascending/descending is separated onto an icon button - Still waiting for some answers from Justin above - [x] Followed your format for how rules are displayed, but they're seperated by a divider (rather than separate containers) - [x] Disabled rules are given a text/secondary color ~@zachgoll I could use some help/advice on the implementation on displaying the rules with a divider. I messed around with this for quite a while and couldn't get to anything perfect or that I _really_ liked, but the current code is about as close as I could get it. The `shadow-border-xs` utility is great for being around a single thing (like the div that holds all the cards), but once I add it to each card with 0 spacing between it, I get a sloppy border because the border is in the margin and it becomes 2× bigger than intended. I also tried to make some new `shadow-divider-{size}` utilities, but didn't want to start making new utilities if we didn't think there would be other reasons for them. I ended up landing on using the divider from the sidebar. It isn't either (although close enough that most eyes wouldn't notice) [`<div class="h-px bg-alpha-black-100 w-full"></div>`](https://github.com/ahatzz11/maybe/blob/02a14d09607d50ec9512c1a2ef1af891ea1b6d33/app/views/settings/_settings_nav.html.erb#L58)~ Big edit: I decided to forego the shadow-box here because we're not really making a shadow _around_ a card anymore, but instead having a border around each rule. I ended up just using `<div class="border-b border-subdued"></div>` to make the divider line, utilizing the existing border styling from the border-utils. I think this turned out excellent: ![CleanShot-002915 2025-05-06 at 20 17 17@2x](https://github.com/user-attachments/assets/9dbf3fc8-9292-426c-ba1a-d5d54ab476be) One note - the figma says the border is `subdued` but that color seemed to not match very well, so I opted for `border-secondary`. I'm not sure if this is just a mismatch between figma and the design system or if I'm missing something. Additionally, I updated some `text-` and `bg-` attributes and rules now has a much nicer dark mode <img width="2040" alt="image" src="https://github.com/user-attachments/assets/5f35d535-382f-47c9-8c28-13fca8b5bef4" /> --- One weird thing I've found with sorting - because it's within a form right now, changing either option submits the form. A side effect of this, because of the direction toggle, is that the direction _always_ toggles. This means if you're on name/asc and you move to updated_at, the direction becomes desc. If you're on name/desc and you move to updated_at, the direction becomes asc. This is a bit strange and not quite what I expect to happen. I've got a couple ways to solve this, and wanted some feedback: 1. Move from a button and a `form.hidden_field` to a `LinkComponent` that just changes the route: ```ruby <%= render LinkComponent.new( icon: "arrow-up-down", size: :sm, title: "Toggle sort direction", href: rules_path(sort_by: @sort_by, direction: @direction == "asc" ? "desc" : "asc"), variant: :icon ) do %> ``` This has a slightly odd side effect that it's a link, so browsers show the link on hover. I'm not too bothers by this (obviously there are a lot of other links in the app). 2. Add a new controller (`sort_direction_controller`): ```javascript import { Controller } from "@hotwired/stimulus"; export default class extends Controller { static targets = ["direction"]; toggle(event) { event.preventDefault(); const directionInput = this.directionTarget; directionInput.value = directionInput.value === "asc" ? "desc" : "asc"; this.element.requestSubmit(); } } ``` In theory this could be reused in other places that had sorting direction, but I don't _think_ that exists anywhere else at the moment.
justinfar commented 2025-05-07 18:32:30 +08:00 (Migrated from github.com)
Copy Link

@justinfar While I'm working on this I've got a few design questions!

  1. I can make the form.select match your design with bg-transparent, but do we want to keep that consistent with other dropdowns and have it look something like this? I can't seem to find another example. The transparent does look nice, but without cursor pointer I think is tough to notice it's a clickable area.
    CleanShot-002888 2025-05-06 at 13 41 56@2x
  2. Right now I noticed that none of our selects have cursor-pointer - is that something we should have across the board?

@ahatzz11

Would rather keep it matching the design (as it'll stand out a bit too much within this context if we use the same input style).

As for the selects not having cursor-pointer that's definitely something we should address and have across the board, as it's an affordance that lets people know they can interact.

> @justinfar While I'm working on this I've got a few design questions! > > 1. I can make the form.select match your design with `bg-transparent`, but do we want to keep that consistent with other dropdowns and have it look something like this? I can't seem to find another example. The transparent does look nice, but without cursor pointer I think is tough to notice it's a clickable area. > ![CleanShot-002888 2025-05-06 at 13 41 56@2x](https://private-user-images.githubusercontent.com/6256032/440915713-26ef026e-0efe-4602-8b59-61d1f1217b2f.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3NDY2MTQwNzgsIm5iZiI6MTc0NjYxMzc3OCwicGF0aCI6Ii82MjU2MDMyLzQ0MDkxNTcxMy0yNmVmMDI2ZS0wZWZlLTQ2MDItOGI1OS02MWQxZjEyMTdiMmYucG5nP1gtQW16LUFsZ29yaXRobT1BV1M0LUhNQUMtU0hBMjU2JlgtQW16LUNyZWRlbnRpYWw9QUtJQVZDT0RZTFNBNTNQUUs0WkElMkYyMDI1MDUwNyUyRnVzLWVhc3QtMSUyRnMzJTJGYXdzNF9yZXF1ZXN0JlgtQW16LURhdGU9MjAyNTA1MDdUMTAyOTM4WiZYLUFtei1FeHBpcmVzPTMwMCZYLUFtei1TaWduYXR1cmU9YTI2MjhmYWFhZGFiNzU5MWFmZmI0MDJjNWY4NTlmYTE0ZWNmZjQzYTFhYWNmNThjZDJiMzcwM2EyN2I4OTNmZCZYLUFtei1TaWduZWRIZWFkZXJzPWhvc3QifQ.Nr46-rre2bHyJvvBnstM0MlOiDd0h_9K2CrShn0XrrQ) > 2. Right now I noticed that none of our selects have `cursor-pointer` - is that something we should have across the board? @ahatzz11 Would rather keep it matching the design (as it'll stand out a bit too much within this context if we use the same input style). As for the selects not having `cursor-pointer` that's definitely something we should address and have across the board, as it's an affordance that lets people know they can interact.
👍 1
zachgoll (Migrated from github.com) reviewed 2025-05-07 21:56:03 +08:00
app/views/rules/index.html.erb
zachgoll (Migrated from github.com) commented 2025-05-07 21:56:03 +08:00
Copy Link

I think the easiest way to handle this would be to keep the sort_by select field inside the form and move the direction toggle outside the form as a link.

You could still keep the form.hidden_field :direction inside the form, and then you could additionally include the sort_by in the link:

<%= render LinkComponent.new(
   href: rules_path(direction: @direction == "asc" ? "desc" : "asc", sort_by: @sort_by),
   variant: "icon",
   icon: "arrow-up-down"
) %>

That way, you let the URL deal with syncing the state between the two

I think the easiest way to handle this would be to keep the `sort_by` select field inside the form and move the direction toggle outside the form as a link. You could still keep the `form.hidden_field :direction` inside the form, and then you could additionally include the `sort_by` in the link: ```erb <%= render LinkComponent.new( href: rules_path(direction: @direction == "asc" ? "desc" : "asc", sort_by: @sort_by), variant: "icon", icon: "arrow-up-down" ) %> ``` That way, you let the URL deal with syncing the state between the two
zachgoll (Migrated from github.com) reviewed 2025-05-07 21:59:24 +08:00
db/schema.rb
zachgoll (Migrated from github.com) commented 2025-05-07 21:59:24 +08:00
Copy Link

This could be from some stale migrations, but I believe we need to delete this. You can run bin/rails db:migrate:reset to rerun everything from a clean slate, then rake demo_data:empty to generate the dev user again

This could be from some stale migrations, but I believe we need to delete this. You can run `bin/rails db:migrate:reset` to rerun everything from a clean slate, then `rake demo_data:empty` to generate the dev user again
javiermartingonzalez commented 2025-05-08 06:41:36 +08:00 (Migrated from github.com)
Copy Link

Hi @ahatzz11,

As we discussed earlier in #2198, I found another bug in the conditional group rule UI. I’ve created a new issue with a screen recording and explanation (#2221).

Since you're working on improving the UI as part of this task, could you take a look and see if the issue is resolved by your changes? If not, perhaps you can identify the root cause and address it?

Thanks!

Hi @ahatzz11, As we discussed earlier in #2198, I found another bug in the conditional group rule UI. I’ve created a new issue with a screen recording and explanation (#2221). Since you're working on improving the UI as part of this task, could you take a look and see if the issue is resolved by your changes? If not, perhaps you can identify the root cause and address it? Thanks!
ahatzz11 (Migrated from github.com) reviewed 2025-05-08 21:04:37 +08:00
db/schema.rb
ahatzz11 (Migrated from github.com) commented 2025-05-08 21:04:37 +08:00
Copy Link

Good catch - fixed this!

Good catch - fixed this!
ahatzz11 (Migrated from github.com) reviewed 2025-05-08 22:20:03 +08:00
app/views/rules/index.html.erb
ahatzz11 (Migrated from github.com) commented 2025-05-08 22:20:03 +08:00
Copy Link

Awesome - fixed this!

Awesome - fixed this!
ahatzz11 commented 2025-05-08 22:37:52 +08:00 (Migrated from github.com)
Copy Link

@justinfar

Would rather keep it matching the design (as it'll stand out a bit too much within this context if we use the same input style).

As for the selects not having cursor-pointer that's definitely something we should address and have across the board, as it's an affordance that lets people know they can interact.

I have updated the dropdown to match the figma design! I do like it blending in a bit more.

One final question on the design - The figma for rules currently has a full width border separating out each rule. I have an implementation for this with border instead of a shadowbox, but as I was clicking around the site I noticed it's a bit different than the other pages where there are a list of many items of the same type, e.g. tags and categories. Specifically - these use a shadowbox around the main card and then a not-quite-full-width divider (which alleviates the issue of two shadow borders overlapping and changing the colors a bit):

CleanShot-002931 2025-05-08 at 09 33 10@2x

I ended up changing the rules list to match tags/categories in my latest push, but wanted to run this change by you before saying this PR is 100% ready to go. I personally like the exact match to the other pages. Here is a comparison:

CleanShot-002936 2025-05-08 at 09 35 16@2x

Let me know what you think!

@justinfar > Would rather keep it matching the design (as it'll stand out a bit too much within this context if we use the same input style). > > As for the selects not having `cursor-pointer` that's definitely something we should address and have across the board, as it's an affordance that lets people know they can interact. I have updated the dropdown to match the figma design! I do like it blending in a bit more. One final question on the design - The figma for rules currently has a full width border separating out each rule. I have an implementation for this with border instead of a shadowbox, but as I was clicking around the site I noticed it's a bit different than the other pages where there are a list of many items of the same type, e.g. tags and categories. Specifically - these use a shadowbox around the main card and then a not-quite-full-width divider (which alleviates the issue of two shadow borders overlapping and changing the colors a bit): ![CleanShot-002931 2025-05-08 at 09 33 10@2x](https://github.com/user-attachments/assets/a4158cca-ae90-45d6-9cd5-c2aceabed22c) I ended up changing the rules list to match tags/categories in my latest push, but wanted to run this change by you before saying this PR is 100% ready to go. I personally like the exact match to the other pages. Here is a comparison: ![CleanShot-002936 2025-05-08 at 09 35 16@2x](https://github.com/user-attachments/assets/1cbf03e9-4b1e-46ec-913b-34a8723e1773) Let me know what you think!
❤️ 1
zachgoll (Migrated from github.com) approved these changes 2025-05-09 22:04:50 +08:00
zachgoll (Migrated from github.com) left a comment
Copy Link

I added a few code improvement ideas, but I the functionality looks good to me.

Once @justinfar approves, we'll be all set.

I added a few code improvement ideas, but I the functionality looks good to me. Once @justinfar approves, we'll be all set.
app/models/rule/condition.rb
@@ -1,5 +1,5 @@
class Rule::Condition < ApplicationRecord
belongs_to :rule, optional: -> { where.not(parent_id: nil) }
belongs_to :rule, touch: true, optional: -> { where.not(parent_id: nil) }
belongs_to :parent, class_name: "Rule::Condition", optional: true, inverse_of: :sub_conditions
zachgoll (Migrated from github.com) commented 2025-05-09 21:58:50 +08:00
Copy Link

For the touch: true on both Condition and Action, what specifically is this achieving? I'm wondering if this may be a symptom of a domain invariant that we've missed during the rule building

For the `touch: true` on both `Condition` and `Action`, what specifically is this achieving? I'm wondering if this may be a symptom of a domain invariant that we've missed during the rule building
app/views/rules/_rule.html.erb
@@ -7,0 +23,4 @@
</p>
</div>
<% end %>
<div class="flex items-center gap-2 mt-1">
zachgoll (Migrated from github.com) commented 2025-05-09 22:03:53 +08:00
Copy Link

I had thrown a lot of this checking in a bit hastily for a quick fix a few weeks back and I'm totally fine keeping it as is for now. But probably worth noting that we'll eventually need to capture this a bit more clearly. Just an idea, but something that looks more like:

<span class="px-2 py-1 border border-secondary rounded-full">
  <%= rule.primary_condition_title %>
</span>

<% if rule.conditions.count > 1 %>
  and <%= rule.conditions.count - 1 %> more <%= rule.conditions.count - 1 == 1 ? "condition" : "conditions" %>
<% end %>
I had thrown a lot of this checking in a bit hastily for a quick fix a few weeks back and I'm totally fine keeping it as is for now. But probably worth noting that we'll eventually need to capture this a bit more clearly. Just an idea, but something that looks more like: ```erb <span class="px-2 py-1 border border-secondary rounded-full"> <%= rule.primary_condition_title %> </span> <% if rule.conditions.count > 1 %> and <%= rule.conditions.count - 1 %> more <%= rule.conditions.count - 1 == 1 ? "condition" : "conditions" %> <% end %> ```
ahatzz11 (Migrated from github.com) reviewed 2025-05-09 23:09:17 +08:00
app/models/rule/condition.rb
@@ -1,5 +1,5 @@
class Rule::Condition < ApplicationRecord
belongs_to :rule, optional: -> { where.not(parent_id: nil) }
belongs_to :rule, touch: true, optional: -> { where.not(parent_id: nil) }
belongs_to :parent, class_name: "Rule::Condition", optional: true, inverse_of: :sub_conditions
ahatzz11 (Migrated from github.com) commented 2025-05-09 23:09:17 +08:00
Copy Link

The reason I added these was for showing a proper updated_at status when a condition or action changed. When adding that sorting I noticed an updated action on a rule wasn't making it appear as the latest updated. I looked in rails console and the updated_at timestamp wouldn't change when editing an action or a condition, but it would on a name change.

If there's a better way to solve this happy to work on that!

The reason I added these was for showing a proper `updated_at` status when a condition or action changed. When adding that sorting I noticed an updated action on a rule wasn't making it appear as the latest updated. I looked in rails console and the `updated_at` timestamp wouldn't change when editing an action or a condition, but it would on a name change. If there's a better way to solve this happy to work on that!
ahatzz11 (Migrated from github.com) reviewed 2025-05-09 23:34:54 +08:00
app/views/rules/_rule.html.erb
@@ -7,0 +23,4 @@
</p>
</div>
<% end %>
<div class="flex items-center gap-2 mt-1">
ahatzz11 (Migrated from github.com) commented 2025-05-09 23:34:54 +08:00
Copy Link

Agreed! I have seen some other weird stuff on this condition thing too so I threw this comment and my other experience into https://github.com/maybe-finance/maybe/issues/2230

Agreed! I have seen some other weird stuff on this condition thing too so I threw this comment and my other experience into https://github.com/maybe-finance/maybe/issues/2230
ahatzz11 (Migrated from github.com) reviewed 2025-05-09 23:56:45 +08:00
app/models/rule/condition.rb
@@ -1,5 +1,5 @@
class Rule::Condition < ApplicationRecord
belongs_to :rule, optional: -> { where.not(parent_id: nil) }
belongs_to :rule, touch: true, optional: -> { where.not(parent_id: nil) }
belongs_to :parent, class_name: "Rule::Condition", optional: true, inverse_of: :sub_conditions
ahatzz11 (Migrated from github.com) commented 2025-05-09 23:56:45 +08:00
Copy Link

To prove this out a bit I removed the touch: true on both the condition.rb and action.rb and made the following video . Notice that the name and timeframe both create a new updated timestamp, but changing an action and condition done. I threw in the rules updated_at field on the rule card just as a demonstration.

Event Time
start 11:52:26
condition change 11:52:26
action change 11:52:26
timeframe change 11:53:18
condition change 11:53:18
name change 11:53:34

https://github.com/user-attachments/assets/dce001e7-9cae-4ecb-8350-1e0122eb7eca

To prove this out a bit I removed the `touch: true` on both the `condition.rb` and `action.rb` and made the following video . Notice that the name and timeframe both create a new updated timestamp, but changing an action and condition done. I threw in the rules `updated_at` field on the rule card just as a demonstration. | Event | Time | |-------------------|----------| | start | 11:52:26 | | condition change | 11:52:26 | | action change | 11:52:26 | | timeframe change | 11:53:18 | | condition change | 11:53:18 | | name change | 11:53:34 | https://github.com/user-attachments/assets/dce001e7-9cae-4ecb-8350-1e0122eb7eca
zachgoll (Migrated from github.com) reviewed 2025-05-10 00:12:19 +08:00
app/models/rule/condition.rb
@@ -1,5 +1,5 @@
class Rule::Condition < ApplicationRecord
belongs_to :rule, optional: -> { where.not(parent_id: nil) }
belongs_to :rule, touch: true, optional: -> { where.not(parent_id: nil) }
belongs_to :parent, class_name: "Rule::Condition", optional: true, inverse_of: :sub_conditions
zachgoll (Migrated from github.com) commented 2025-05-10 00:12:19 +08:00
Copy Link

Ahh that makes sense! I had incorrectly assumed that since we're updating the Rule in rules_controller via the accepts_nested_attributes_for it would automatically inherit this touch behavior, but guess not!

Ahh that makes sense! I had incorrectly assumed that since we're updating the Rule in `rules_controller` via the `accepts_nested_attributes_for` it would automatically inherit this touch behavior, but guess not!
zachgoll commented 2025-05-13 22:35:47 +08:00 (Migrated from github.com)
Copy Link

@justinfar we all set on this one?

@justinfar we all set on this one?
ahatzz11 commented 2025-05-13 22:42:32 +08:00 (Migrated from github.com)
Copy Link

@zachgoll I'll fix this merge conflict here quick so it's ready once justin gives a 👍

@zachgoll I'll fix this merge conflict here quick so it's ready once justin gives a 👍
justinfar commented 2025-05-14 00:49:29 +08:00 (Migrated from github.com)
Copy Link

sorry just seeing this now!

I ended up changing the rules list to match tags/categories in my latest push, but wanted to run this change by you before saying this PR is 100% ready to go. I personally like the exact match to the other pages. Here is a comparison:

This works better than my solution, didn't even notice the shadowbox overlap so thank you for bringing it to my attention! Appreciate you looking out for consistency :)

all good to go on this one, excellent work @ahatzz11!

sorry just seeing this now! > I ended up changing the rules list to match tags/categories in my latest push, but wanted to run this change by you before saying this PR is 100% ready to go. I personally like the exact match to the other pages. Here is a comparison: This works better than my solution, didn't even notice the shadowbox overlap so thank you for bringing it to my attention! Appreciate you looking out for consistency :) all good to go on this one, excellent work @ahatzz11!
🚀 1
ahatzz11 commented 2025-05-14 02:19:12 +08:00 (Migrated from github.com)
Copy Link

@zachgoll Alright this one is ready to go from my side - I added a new bg-divider utility that we can use on dividers/rulers so dark mode is handled nicely as well. I started down the rabbit hole to clean up some of the other rulers/dividers in the app, but I'll keep that as a separate PR so this one doesn't hang out for too long. Let me know if you think this utility belongs elsewhere!

New bg-divier utility in dark mode (implemented on rules):
image

New bg-divider not implemented on categories (yet, I'll do this in a followup):
image

@zachgoll Alright this one is ready to go from my side - I added a new `bg-divider` utility that we can use on dividers/rulers so dark mode is handled nicely as well. I started down the rabbit hole to clean up some of the other rulers/dividers in the app, but I'll keep that as a separate PR so this one doesn't hang out for too long. Let me know if you think this utility belongs elsewhere! New `bg-divier` utility in dark mode (implemented on rules): <img width="808" alt="image" src="https://github.com/user-attachments/assets/efe1f8ca-38d2-4bc7-8044-a842fd910e79" /> New `bg-divider` not implemented on categories (yet, I'll do this in a followup): <img width="819" alt="image" src="https://github.com/user-attachments/assets/a261e490-409a-4a6d-afd4-a2f39db0e086" />
zachgoll (Migrated from github.com) approved these changes 2025-05-14 03:51:33 +08:00
zachgoll (Migrated from github.com) left a comment
Copy Link

Looks great! Thanks for tackling this one.

Looks great! Thanks for tackling this one.
zachgoll commented 2025-05-14 03:53:07 +08:00 (Migrated from github.com)
Copy Link

@ahatzz11 I haven't deeply thought through this, but just an FYI, I had started following this pattern for dividers in a few places:

<hr class="border-tertiary" />

050d5ebaad/app/components/menu_item_component.html.erb (L2)

Not a big deal either way, just thought I'd call it out.

@ahatzz11 I haven't deeply thought through this, but just an FYI, I had started following this pattern for dividers in a few places: ``` <hr class="border-tertiary" /> ``` https://github.com/maybe-finance/maybe/blob/050d5ebaadaaf35f7e51d0111197426ebc689899/app/components/menu_item_component.html.erb#L2 Not a big deal either way, just thought I'd call it out.
albertorizzi commented 2025-05-14 15:00:29 +08:00 (Migrated from github.com)
Copy Link

How can I apply the rule to categorize every transaction using AI without using an IF statement?

How can I apply the rule to categorize every transaction using AI without using an IF statement?
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#2177
Delete Branch "rule-name"

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?