Skip to content
This repository was archived by the owner on Jul 27, 2025. It is now read-only.

Conversation

elvisserrao
Copy link
Contributor

Fixes issue #1677

Copy link
Collaborator

@zachgoll zachgoll left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, agree with the strategy here!

<%= f.select :parent_id, categories.pluck(:name, :id), { include_blank: "(unassigned)", label: "Parent category (optional)" }, disabled: category.parent? %>
<% if category.parent? %>
<span class="text-xs italic pl-2 text-gray-500">This category couldn’t be assigned as a subcategory because it already has subcategories.</span>
<% end %>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think here, we can just have a conditional:

<% unless category.parent? %>
  <%= f.select :parent_id ...
<% end %>

That way, if the user is not allowed to assign a parent, we don't ever give them the option.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, i agree.

@zachgoll zachgoll changed the title Fix/1677 Don't allow a subcategory to be assigned to another subcategory to ensure 1 level of nesting max Jan 28, 2025
@elvisserrao elvisserrao requested a review from zachgoll January 29, 2025 21:01
Copy link
Collaborator

@zachgoll zachgoll left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good now. Left one more comment to address, then I think we're good to merge!

@@ -1,46 +1,49 @@
<%# locals: (category:, categories:) %>
<%= turbo_frame_tag 'form' do %>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@elvisserrao elvisserrao requested a review from zachgoll January 30, 2025 15:29
Copy link
Collaborator

@zachgoll zachgoll left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good! Nice work, thanks for the fix.

@zachgoll zachgoll merged commit 0b17976 into maybe-finance:main Jan 30, 2025
5 checks passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants