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
15 changed files with 176 additions and 79 deletions
Download Patch File Download Diff File Expand all files Collapse all files

10
app/assets/tailwind/maybe-design-system/background-utils.css
View File

@@ -78,10 +78,18 @@
}
}
@utility bg-divider {
@apply bg-alpha-black-100;
@variant theme-dark {
@apply bg-alpha-white-100;
}
}
@utility bg-overlay {
background-color: --alpha(var(--color-gray-100) / 50%);
@variant theme-dark {
background-color: var(--color-alpha-black-900);
}
}
}

11
app/controllers/rules_controller.rb
View File

@@ -4,7 +4,14 @@ class RulesController < ApplicationController
before_action :set_rule, only: [ :edit, :update, :destroy, :apply, :confirm ]
def index
@rules = Current.family.rules.order(created_at: :desc)
@sort_by = params[:sort_by] || "name"
@direction = params[:direction] || "asc"
allowed_columns = [ "name", "updated_at" ]
@sort_by = "name" unless allowed_columns.include?(@sort_by)
@direction = "asc" unless [ "asc", "desc" ].include?(@direction)
@rules = Current.family.rules.order(@sort_by => @direction)
render layout: "settings"
end
@@ -64,7 +71,7 @@ class RulesController < ApplicationController
def rule_params
params.require(:rule).permit(
:resource_type, :effective_date, :active,
:resource_type, :effective_date, :active, :name,
conditions_attributes: [
:id, :condition_type, :operator, :value, :_destroy,
sub_conditions_attributes: [ :id, :condition_type, :operator, :value, :_destroy ]

7
app/models/rule.rb
View File

@@ -8,7 +8,10 @@ class Rule < ApplicationRecord
accepts_nested_attributes_for :conditions, allow_destroy: true
accepts_nested_attributes_for :actions, allow_destroy: true
before_validation :normalize_name
validates :resource_type, presence: true
validates :name, length: { minimum: 1 }, allow_nil: true
validate :no_nested_compound_conditions
# Every rule must have at least 1 action
@@ -99,4 +102,8 @@ class Rule < ApplicationRecord
end
end
end
def normalize_name
self.name = nil if name.is_a?(String) && name.strip.empty?
end
end

2
app/models/rule/action.rb
View File

@@ -1,5 +1,5 @@
class Rule::Action < ApplicationRecord
belongs_to :rule
belongs_to :rule, touch: true
validates :action_type, presence: true

2
app/models/rule/condition.rb
View File

@@ -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 commented 2025-05-09 21:58:50 +08:00 (Migrated from github.com)
Review
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
ahatzz11 commented 2025-05-09 23:09:17 +08:00 (Migrated from github.com)
Review
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 commented 2025-05-09 23:56:45 +08:00 (Migrated from github.com)
Review
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 commented 2025-05-10 00:12:19 +08:00 (Migrated from github.com)
Review
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!
has_many :sub_conditions, class_name: "Rule::Condition", foreign_key: :parent_id, dependent: :destroy, inverse_of: :parent

6
app/views/rule/actions/_action.html.erb
View File

@@ -16,7 +16,7 @@
<%# Initial rendering based on rule.action_executors.first from the rule form. %>
<%# This is currently always SetTransactionCategory from transaction_resource.rb, which is a select type. %>
<%# Subsequent renders are injected by the Stimulus controller, which uses the templates from below. %>
<span class="font-medium uppercase text-xs">to</span>
<span class="font-medium text-primary uppercase text-xs">to</span>
<%= form.select :value, action.options || [], {} %>
<% end %>
</div>
@@ -29,12 +29,12 @@
<%# Templates for different input types - these will be cloned and used by the Stimulus controller %>
<template data-rule--actions-target="selectTemplate">
<span class="font-medium uppercase text-xs">to</span>
<span class="font-medium text-primary uppercase text-xs">to</span>
<%= form.select :value, [], {} %>
</template>
<template data-rule--actions-target="textTemplate">
<span class="font-medium uppercase text-xs">to</span>
<span class="font-medium text-primary uppercase text-xs">to</span>
<%= form.text_field :value, placeholder: "Enter a value" %>
</template>

58
app/views/rules/_form.html.erb
View File

@@ -1,6 +1,6 @@
<%# locals: (rule:) %>
<%= styled_form_with model: rule, class: "space-y-4",
<%= styled_form_with model: rule, class: "space-y-6",
data: { controller: "rules", rule_registry_value: rule.registry.to_json } do |f| %>
<%= f.hidden_field :resource_type, value: rule.resource_type %>
@@ -10,7 +10,19 @@
<% end %>
<section class="space-y-4">
<h3 class="text-sm font-medium text-primary">If <%= rule.resource_type %></h3>
<div class="flex items-center gap-1 text-secondary">
<%= icon "tag", size: "sm" %>
<h3 class="text-sm font-medium text-primary">Rule name (optional)</h3>
</div>
<div class="ml-6">
<%= f.text_field :name, placeholder: "Enter a name for this rule", class: "form-field__input" %>
</div>
</section>
<section class="space-y-4">
<div class="flex items-center gap-1 text-secondary">
<h3 class="text-sm font-medium text-primary">IF</h3>
</div>
<%# Condition Group template, used by Stimulus controller to add new conditions dynamically %>
<template data-rules-target="conditionGroupTemplate">
@@ -26,24 +38,27 @@
<% end %>
</template>
<ul data-rules-target="conditionsList" class="space-y-3 mb-4">
<%= f.fields_for :conditions do |cf| %>
<% if cf.object.compound? %>
<%= render "rule/conditions/condition_group", form: cf %>
<% else %>
<%= render "rule/conditions/condition", form: cf %>
<div class="ml-6 space-y-4">
<ul data-rules-target="conditionsList" class="space-y-3">
<%= f.fields_for :conditions do |cf| %>
<% if cf.object.compound? %>
<%= render "rule/conditions/condition_group", form: cf %>
<% else %>
<%= render "rule/conditions/condition", form: cf %>
<% end %>
<% end %>
<% end %>
</ul>
</ul>
<div class="flex items-center gap-2">
<%= render ButtonComponent.new(text: "Add condition", icon: "plus", variant: "ghost", type: "button", data: { action: "rules#addCondition" }) %>
<%= render ButtonComponent.new(text: "Add condition group", icon: "boxes", variant: "ghost", type: "button", data: { action: "rules#addConditionGroup" }) %>
<%= render ButtonComponent.new(text: "Add condition group", icon: "copy-plus", variant: "ghost", type: "button", data: { action: "rules#addConditionGroup" }) %>
</div>
</section>
<section class="space-y-4">
<h3 class="text-sm font-medium text-primary">Then</h3>
<div class="flex items-center gap-1 text-secondary">
<h3 class="text-sm font-medium text-primary">THEN</h3>
</div>
<%# Action template, used by Stimulus controller to add new actions dynamically %>
<template data-rules-target="actionTemplate">
@@ -52,22 +67,25 @@
<% end %>
</template>
<ul data-rules-target="actionsList" class="space-y-3">
<%= f.fields_for :actions do |af| %>
<%= render "rule/actions/action", form: af %>
<% end %>
</ul>
<div class="ml-6 space-y-4">
<ul data-rules-target="actionsList" class="space-y-3">
<%= f.fields_for :actions do |af| %>
<%= render "rule/actions/action", form: af %>
<% end %>
</ul>
<%= render ButtonComponent.new(text: "Add action", icon: "plus", variant: "ghost", type: "button", data: { action: "rules#addAction" }) %>
</section>
<section class="space-y-4">
<h3 class="text-sm font-medium text-primary">Apply this</h3>
<div class="flex items-center gap-1 text-secondary">
<h3 class="text-sm font-medium text-primary">FOR</h3>
</div>
<div class="space-y-2">
<div class="ml-6 space-y-3">
<div class="flex items-center gap-2">
<%= f.radio_button :effective_date_enabled, false, checked: rule.effective_date.nil?, data: { action: "rules#clearEffectiveDate" } %>
<%= f.label :effective_date_enabled_false, "To all past and future #{rule.resource_type}s", class: "text-sm text-primary" %>
<%= f.label :effective_date_enabled_false, "All past and future #{rule.resource_type}s", class: "text-sm text-primary" %>
</div>
<div class="flex items-center gap-2">

81
app/views/rules/_rule.html.erb
View File

@@ -1,47 +1,64 @@
<%# locals: (rule:) %>
<div class="flex justify-between items-center gap-4 bg-white shadow-border-xs rounded-md p-4">
<div class="flex justify-between items-center p-4 <%= rule.active? ? 'text-primary' : 'text-secondary' %>">
<div class="text-sm space-y-1.5">
<% if rule.name.present? %>
<h3 class="font-medium text-md"><%= rule.name %></h3>
<% end %>
<% if rule.conditions.any? %>
<p class="flex items-center flex-wrap gap-1.5">
<div class="flex items-center gap-2 mt-1">
<div class="flex items-center gap-1 text-secondary w-16 shrink-0">
<span class="font-mono text-xs">IF</span>
</div>
<p class="flex items-center flex-wrap gap-1.5 m-0">
<span class="px-2 py-1 border border-secondary rounded-full">
<% if rule.conditions.first.compound? %>
<%= rule.conditions.first.sub_conditions.first.filter.label %> <%= rule.conditions.first.sub_conditions.first.operator %> <%= rule.conditions.first.sub_conditions.first.value_display %>
<% else %>
<%= rule.conditions.first.filter.label %> <%= rule.conditions.first.operator %> <%= rule.conditions.first.value_display %>
<% end %>
</span>
<% if rule.conditions.count > 1 %>
and <%= rule.conditions.count - 1 %> more <%= rule.conditions.count - 1 == 1 ? "condition" : "conditions" %>
<% end %>
</p>
</div>
<% end %>
<div class="flex items-center gap-2 mt-1">
zachgoll commented 2025-05-09 22:03:53 +08:00 (Migrated from github.com)
Review
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 commented 2025-05-09 23:34:54 +08:00 (Migrated from github.com)
Review
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
<div class="flex items-center gap-1 text-secondary w-16 shrink-0">
<span class="font-mono text-xs">THEN</span>
</div>
<p class="flex items-center flex-wrap gap-1.5 m-0">
<span class="px-2 py-1 border border-secondary rounded-full">
<%= rule.primary_condition_title %>
<% if rule.actions.first.value && rule.actions.first.options %>
<%= rule.actions.first.executor.label %> to <%= rule.actions.first.value_display %>
<% else %>
<%= rule.actions.first.executor.label %>
<% end %>
</span>
<% if rule.conditions.count > 1 %>
and <%= rule.conditions.count - 1 %> more <%= rule.conditions.count - 1 == 1 ? "condition" : "conditions" %>
<% if rule.actions.count > 1 %>
and <%= rule.actions.count - 1 %> more <%= rule.actions.count - 1 == 1 ? "action" : "actions" %>
<% end %>
</p>
<% end %>
<p class="flex items-center flex-wrap gap-1.5">
<span class="px-2 py-1 border border-alpha-black-200 rounded-full">
<% if rule.actions.first.value && rule.actions.first.options %>
<%= rule.actions.first.executor.label %> to <%= rule.actions.first.value_display %>
<% else %>
<%= rule.actions.first.executor.label %>
<% end %>
</span>
<% if rule.actions.count > 1 %>
and <%= rule.actions.count - 1 %> more <%= rule.actions.count - 1 == 1 ? "action" : "actions" %>
<% end %>
</p>
<p class="flex items-center flex-wrap gap-1.5">
<% if rule.effective_date.nil? %>
To all past and future <%= rule.resource_type.pluralize %>
<% else %>
To all <%= rule.resource_type.pluralize %> on or after <%= rule.effective_date %>
<% end %>
</p>
</div>
<div class="flex items-center gap-2 mt-1">
<div class="flex items-center gap-1 text-secondary w-16 shrink-0">
<span class="font-mono text-xs">FOR</span>
</div>
<p class="flex items-center flex-wrap gap-1.5 m-0">
<span class="px-2 py-1 border border-secondary rounded-full">
<% if rule.effective_date.nil? %>
All past and future <%= rule.resource_type.pluralize %>
<% else %>
<%= rule.resource_type.pluralize %> on or after <%= rule.effective_date.strftime('%b %-d, %Y') %>
<% end %>
</span>
</p>
</div>
</div>
<div class="flex items-center gap-4">
<%= styled_form_with model: rule, data: { controller: "auto-submit-form" } do |f| %>
<%= f.toggle :active, { data: { auto_submit_form_target: "auto" } } %>
<% end %>
<%= render MenuComponent.new do |menu| %>
<% menu.with_item(variant: "link", text: "Edit", href: edit_rule_path(rule), icon: "pencil", data: { turbo_frame: "modal" }) %>
<% menu.with_item(variant: "link", text: "Re-apply rule", href: confirm_rule_path(rule), icon: "refresh-cw", data: { turbo_frame: "modal" }) %>

11
app/views/rules/confirm.html.erb
View File

@@ -1,6 +1,13 @@
<%= render DialogComponent.new(reload_on_close: true) do |dialog| %>
<% dialog.with_header(title: "Confirm changes") %>
<%
title = if @rule.name.present?
"Confirm changes to \"#{@rule.name}\""
else
"Confirm changes"
end
%>
<% dialog.with_header(title: title) %>
<% dialog.with_body do %>
<p class="text-secondary text-sm mb-4">
You are about to apply this rule to

10
app/views/rules/edit.html.erb
View File

@@ -1,7 +1,15 @@
<%= link_to "Back to rules", rules_path %>
<%= render DialogComponent.new do |dialog| %>
<% dialog.with_header(title: "Edit #{@rule.resource_type} rule") %>
<%
title = if @rule.name.present?
"Edit #{@rule.resource_type} rule \"#{@rule.name}\""
else
"Edit #{@rule.resource_type} rule"
end
%>
<% dialog.with_header(title: title) %>
<% dialog.with_body do %>
<%= render "rules/form", rule: @rule %>
<% end %>

47
app/views/rules/index.html.erb
View File

@@ -1,6 +1,5 @@
<header class="flex items-center justify-between">
<h1 class="text-primary text-xl font-medium">Rules</h1>
<div class="flex items-center gap-2">
<% if @rules.any? %>
<%= render MenuComponent.new do |menu| %>
@@ -13,7 +12,6 @@
confirm: CustomConfirm.for_resource_deletion("all rules", high_severity: true)) %>
<% end %>
<% end %>
<%= render LinkComponent.new(
text: "New rule",
variant: "primary",
@@ -23,7 +21,6 @@
) %>
</div>
</header>
<% if self_hosted? %>
<div class="flex items-center gap-2 mb-2 py-4">
<%= icon("circle-alert", size: "sm") %>
@@ -32,19 +29,43 @@
</p>
</div>
<% end %>
<div class="bg-container shadow-border-xs rounded-xl p-4">
<% if @rules.any? %>
ahatzz11 commented 2025-04-30 08:52:35 +08:00 (Migrated from github.com)
Review
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 ```
<div class="rounded-xl bg-gray-25 space-y-1">
<div class="flex items-center gap-1.5 px-4 py-2 text-xs font-medium text-secondary uppercase">
<p>Rules</p>
<span class="text-subdued">&middot;</span>
<p><%= @rules.count %></p>
<div class="bg-container-inset rounded-xl">
<div class="flex justify-between px-4 py-2 text-xs uppercase">
<div class="flex items-center gap-1.5 font-medium text-secondary">
<p>Rules</p>
<span class="text-subdued">&middot;</span>
<p><%= @rules.count %></p>
</div>
<div class="flex items-center gap-1">
<span class="text-secondary">Sort by:</span>
<%= form_with url: rules_path, method: :get, local: true, class: "flex items-center", data: { controller: "auto-submit-form" } do |form| %>
<%= form.select :sort_by,
options_for_select([["Name", "name"], ["Updated At", "updated_at"]], @sort_by),
{},
class: "min-w-[120px] bg-transparent rounded border-none cursor-pointer text-primary uppercase text-xs w-auto",
data: { auto_submit_form_target: "auto", autosubmit_trigger_event: "change" } %>
<%= form.hidden_field :direction, value: @direction %>
<% end %>
<%= render LinkComponent.new(
href: rules_path(direction: @direction == "asc" ? "desc" : "asc", sort_by: @sort_by),
variant: "icon",
icon: "arrow-up-down",
size: :sm,
title: "Toggle sort direction"
) %>
</div>
</div>
<div class="space-y-1 p-1">
<%= render @rules %>
<div class="p-1">
<div class="flex flex-col bg-container rounded-xl shadow-border-xs first_child:rounded-t-xl last_child:rounded-b-xl">
<% @rules.each_with_index do |rule, idx| %>
<%= render "rule", rule: rule%>
<% unless idx == @rules.size - 1 %>
<div class="h-px bg-divider ml-4 mr-6"></div>
<% end %>
<% end %>
</div>
</div>
</div>
<% else %>

5
db/migrate/20250429021255_add_name_to_rules.rb Normal file
View File

@@ -0,0 +1,5 @@
class AddNameToRules < ActiveRecord::Migration[7.2]
def change
add_column :rules, :name, :string
end
end

1
db/schema.rb generated
View File

@@ -498,6 +498,7 @@ ActiveRecord::Schema[7.2].define(version: 2025_05_13_122703) do
t.boolean "active", default: false, null: false
t.datetime "created_at", null: false
t.datetime "updated_at", null: false
t.string "name"
t.index ["family_id"], name: "index_rules_on_family_id"
end

2
test/controllers/subscriptions_controller_test.rb
View File

@@ -32,8 +32,6 @@ class SubscriptionsControllerTest < ActionDispatch::IntegrationTest
end
test "users who have already trialed cannot create a new subscription" do
@family.start_trial_subscription!
assert_no_difference "Subscription.count" do
post subscription_path
end

2
test/models/subscription_test.rb
View File

@@ -2,7 +2,7 @@ require "test_helper"
class SubscriptionTest < ActiveSupport::TestCase
setup do
@family = families(:empty)
@family = Family.create!(name: "Test Family")
end
test "can create subscription without stripe details if trial" do