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

Don't allow a subcategory to be assigned to another subcategory to ensure 1 level of nesting max #1730

Merged
elvisserrao merged 12 commits from fix/1677 into main 2025-01-31 01:35:30 +08:00
Conversation 10 Commits 12 Files Changed 3 +31 -8
3 changed files with 31 additions and 8 deletions
Download Patch File Download Diff File Expand all files Collapse all files

26
app/controllers/categories_controller.rb
View File

@@ -2,6 +2,7 @@ class CategoriesController < ApplicationController
layout :with_sidebar
before_action :set_category, only: %i[edit update destroy]
before_action :set_categories, only: %i[update edit]
before_action :set_transaction, only: :create
def index
@@ -10,7 +11,7 @@ class CategoriesController < ApplicationController
def new
@category = Current.family.categories.new color: Category::COLORS.sample
@categories = Current.family.categories.alphabetically.where(parent_id: nil).where.not(id: @category.id)
set_categories
end
def create
@@ -27,19 +28,26 @@ class CategoriesController < ApplicationController
format.turbo_stream { render turbo_stream: turbo_stream.action(:redirect, redirect_target_url) }
end
else
@categories = Current.family.categories.alphabetically.where(parent_id: nil).where.not(id: @category.id)
set_categories
render :new, status: :unprocessable_entity
end
end
def edit
@categories = Current.family.categories.alphabetically.where(parent_id: nil).where.not(id: @category.id)
end
def update
@category.update! category_params
if @category.update(category_params)
flash[:notice] = t(".success")
redirect_back_or_to categories_path, notice: t(".success")
redirect_target_url = request.referer || categories_path
respond_to do |format|
format.html { redirect_back_or_to categories_path, notice: t(".success") }
format.turbo_stream { render turbo_stream: turbo_stream.action(:redirect, redirect_target_url) }
end
else
render :edit, status: :unprocessable_entity
end
end
def destroy
@@ -59,6 +67,14 @@ class CategoriesController < ApplicationController
@category = Current.family.categories.find(params[:id])
zachgoll commented 2025-01-29 03:07:00 +08:00 (Migrated from github.com)
Review
Copy Link

So we will need the "redirect back or to" logic here since categories can be edited in the modal in many places across the app:

https://github.com/user-attachments/assets/89ac7141-8113-4eb5-ab9e-21689b582a10

Additionally, we should include the html response type here as a fallback and for when forms specify turbo: false

Finally, the else logic here should not need to manually replace the form. Turbo automatically handles the 422 and will simply render the instance variables in the existing view. So we should be able to do render :edit, status: :unprocessable_entity and still get those validations rendering in the modal without closing it.

So we will need the "redirect back or to" logic here since categories can be edited in the modal in many places across the app: https://github.com/user-attachments/assets/89ac7141-8113-4eb5-ab9e-21689b582a10 Additionally, we should include the `html` response type here as a fallback and for when forms specify `turbo: false` Finally, the `else` logic here should not need to manually replace the form. Turbo automatically handles the 422 and will simply render the instance variables in the existing view. So we should be able to do `render :edit, status: :unprocessable_entity` and still get those validations rendering in the modal without closing it.
elvisserrao commented 2025-01-30 02:53:08 +08:00 (Migrated from github.com)
Review
Copy Link

You're right, I made these changes in 2926b84.
I believe it's correct now.

You're right, I made these changes in [2926b84](https://github.com/maybe-finance/maybe/pull/1730/commits/2926b848b895253fd698a89c79db35713f4a2fe4). I believe it's correct now.
end
def set_categories
zachgoll commented 2025-01-29 00:48:34 +08:00 (Migrated from github.com)
Review
Copy Link

IIRC, we're submitting the category form to the :_top turbo frame, so I believe this :unprocessable_entity will cause the background of the modal to go blank on an error.

I think we either need to keep the update! or remove that :_top target that was set here so that both success/failure cases render correctly:

8256d116dd

IIRC, we're submitting the category form to the `:_top` turbo frame, so I believe this `:unprocessable_entity` will cause the background of the modal to go blank on an error. I think we either need to keep the `update!` or remove that `:_top` target that was set here so that both success/failure cases render correctly: https://github.com/maybe-finance/maybe/commit/8256d116ddd3b892d2a4256abdad86d1da25fd95
elvisserrao commented 2025-01-29 00:58:06 +08:00 (Migrated from github.com)
Review
Copy Link

The problem with keeping the update! is that the user never receives any feedback, which is why I changed it.
In the create action, we’re facing the same issue, causing the background to go blank when there is a validation error. That’s why I chose to ignore this issue for now.

The problem with keeping the ```update!``` is that the user never receives any feedback, which is why I changed it. In the create action, we’re facing the same issue, causing the background to go blank when there is a validation error. That’s why I chose to ignore this issue for now.
zachgoll commented 2025-01-29 01:41:49 +08:00 (Migrated from github.com)
Review
Copy Link

Ahh you're right, looks like we had a regression in 8256d116dd

You think we could throw in the correct handling for this endpoint (and remove the :_top target introduced in that commit)? Here's an example of how we're doing it for other endpoints:

    if @entry.save
      flash[:notice] = t(".success")

      respond_to do |format|
        format.html { redirect_back_or_to account_path(@entry.account) }

        redirect_target_url = request.referer || account_path(@entry.account)
        format.turbo_stream { render turbo_stream: turbo_stream.action(:redirect, redirect_target_url) }
      end
    else
      render :new, status: :unprocessable_entity
    end

We're essentially using a custom Turbo action to allow us to handle error states directly with streams, but do a full page refresh on success. It helps avoid that "blank screen" issue:

247d91b99d/app/javascript/application.js (L5-L7)

Ahh you're right, looks like we had a regression in https://github.com/maybe-finance/maybe/commit/8256d116ddd3b892d2a4256abdad86d1da25fd95 You think we could throw in the correct handling for this endpoint (and _remove_ the `:_top` target introduced in that commit)? Here's an example of how we're doing it for other endpoints: ```rb if @entry.save flash[:notice] = t(".success") respond_to do |format| format.html { redirect_back_or_to account_path(@entry.account) } redirect_target_url = request.referer || account_path(@entry.account) format.turbo_stream { render turbo_stream: turbo_stream.action(:redirect, redirect_target_url) } end else render :new, status: :unprocessable_entity end ``` We're essentially using a custom Turbo action to allow us to handle error states directly with streams, but do a full page refresh on success. It helps avoid that "blank screen" issue: https://github.com/maybe-finance/maybe/blob/247d91b99de256465001590b9b77c91df1942f9c/app/javascript/application.js#L5-L7
elvisserrao commented 2025-01-29 02:22:10 +08:00 (Migrated from github.com)
Review
Copy Link

I made some changes on 4087275 to handle the turbo_stream form requests. I removed the :_topturbo target and added a turbo_frame_tag to be replaced by the form partial and render the errors.

I made some changes on [4087275](https://github.com/maybe-finance/maybe/pull/1730/commits/408727575142cb4f3620f6f665e2a6a19374e585) to handle the turbo_stream form requests. I removed the ```:_top```turbo target and added a ```turbo_frame_tag``` to be replaced by the form partial and render the errors.
@categories = unless @category.parent?
Current.family.categories.alphabetically.roots.where.not(id: @category.id)
else
[]
end
end
def set_transaction
if params[:transaction_id].present?
@transaction = Current.family.transactions.find(params[:transaction_id])

7
app/models/category.rb
View File

@@ -15,6 +15,7 @@ class Category < ApplicationRecord
validate :nested_category_matches_parent_classification
scope :alphabetically, -> { order(:name) }
scope :roots, -> { where(parent_id: nil) }
scope :incomes, -> { where(classification: "income") }
scope :expenses, -> { where(classification: "expense") }
@@ -91,6 +92,10 @@ class Category < ApplicationRecord
end
end
def parent?
subcategories.any?
end
def subcategory?
parent.present?
end
@@ -121,7 +126,7 @@ class Category < ApplicationRecord
private
def category_level_limit
if subcategory? && parent.subcategory?
if (subcategory? && parent.subcategory?) || (parent? && subcategory?)
errors.add(:parent, "can't have more than 2 levels of subcategories")
end
end

6
app/views/categories/_form.html.erb
View File

@@ -1,7 +1,7 @@
<%# locals: (category:, categories:) %>
zachgoll commented 2025-01-30 23:22:34 +08:00 (Migrated from github.com)
Review
Copy Link

Do we need this? What was the purpose of adding?

Do we need this? What was the purpose of adding?
elvisserrao commented 2025-01-30 23:28:37 +08:00 (Migrated from github.com)
Review
Copy Link

ohhh, i forgot to remove this... it's done in 2c07cd6. ;-)

ohhh, i forgot to remove this... it's done in [2c07cd6](https://github.com/maybe-finance/maybe/pull/1730/commits/2c07cd6d19ee5d43e9e4fb1a36e4ed49b3702d89). ;-)
<div data-controller="color-avatar">
<%= styled_form_with model: category, class: "space-y-4", data: { turbo_frame: :_top } do |f| %>
<%= styled_form_with model: category, class: "space-y-4" do |f| %>
<section class="space-y-4">
<div class="w-fit m-auto">
<%= render partial: "shared/color_avatar", locals: { name: category.name, color: category.color } %>
@@ -34,7 +34,9 @@
<div class="space-y-2">
<%= f.select :classification, [["Income", "income"], ["Expense", "expense"]], { label: "Classification" }, required: true %>
<%= f.text_field :name, placeholder: t(".placeholder"), required: true, autofocus: true, label: "Name", data: { color_avatar_target: "name" } %>
<%= f.select :parent_id, categories.pluck(:name, :id), { include_blank: "(unassigned)", label: "Parent category (optional)" } %>
<% unless category.parent? %>
<%= f.select :parent_id, categories.pluck(:name, :id), { include_blank: "(unassigned)", label: "Parent category (optional)" }, disabled: category.parent? %>
<% end %>
</div>
</section>