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
elvisserrao commented 2025-01-29 00:34:17 +08:00 (Migrated from github.com)
Copy Link

Fixes issue #1677

Fixes issue #1677
zachgoll (Migrated from github.com) reviewed 2025-01-29 00:49:18 +08:00
zachgoll (Migrated from github.com) left a comment
Copy Link

Overall, agree with the strategy here!

Overall, agree with the strategy here!
app/controllers/categories_controller.rb
@@ -59,6 +67,14 @@ class CategoriesController < ApplicationController
@category = Current.family.categories.find(params[:id])
end
def set_categories
zachgoll (Migrated from github.com) commented 2025-01-29 00:48:34 +08:00
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
app/views/categories/_form.html.erb
zachgoll (Migrated from github.com) commented 2025-01-29 00:45:31 +08:00
Copy Link

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.

I think here, we can just have a conditional: ```erb <% 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.
elvisserrao (Migrated from github.com) reviewed 2025-01-29 00:58:06 +08:00
app/controllers/categories_controller.rb
@@ -59,6 +67,14 @@ class CategoriesController < ApplicationController
@category = Current.family.categories.find(params[:id])
end
def set_categories
elvisserrao (Migrated from github.com) commented 2025-01-29 00:58:06 +08:00
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.
elvisserrao (Migrated from github.com) reviewed 2025-01-29 01:04:29 +08:00
app/views/categories/_form.html.erb
elvisserrao (Migrated from github.com) commented 2025-01-29 01:04:29 +08:00
Copy Link

Nice, i agree.

Nice, i agree.
zachgoll (Migrated from github.com) reviewed 2025-01-29 01:41:49 +08:00
app/controllers/categories_controller.rb
@@ -59,6 +67,14 @@ class CategoriesController < ApplicationController
@category = Current.family.categories.find(params[:id])
end
def set_categories
zachgoll (Migrated from github.com) commented 2025-01-29 01:41:49 +08:00
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 (Migrated from github.com) reviewed 2025-01-29 02:22:10 +08:00
app/controllers/categories_controller.rb
@@ -59,6 +67,14 @@ class CategoriesController < ApplicationController
@category = Current.family.categories.find(params[:id])
end
def set_categories
elvisserrao (Migrated from github.com) commented 2025-01-29 02:22:10 +08:00
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.
zachgoll (Migrated from github.com) reviewed 2025-01-30 01:45:51 +08:00
app/controllers/categories_controller.rb
@@ -59,6 +67,14 @@ class CategoriesController < ApplicationController
@category = Current.family.categories.find(params[:id])
zachgoll (Migrated from github.com) commented 2025-01-29 03:07:00 +08:00
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 (Migrated from github.com) reviewed 2025-01-30 02:53:08 +08:00
app/controllers/categories_controller.rb
@@ -59,6 +67,14 @@ class CategoriesController < ApplicationController
@category = Current.family.categories.find(params[:id])
elvisserrao (Migrated from github.com) commented 2025-01-30 02:53:08 +08:00
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.
zachgoll (Migrated from github.com) reviewed 2025-01-30 23:23:16 +08:00
zachgoll (Migrated from github.com) left a comment
Copy Link

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

Looks good now. Left one more comment to address, then I think we're good to merge!
app/views/categories/_form.html.erb
@@ -1,7 +1,7 @@
<%# locals: (category:, categories:) %>
zachgoll (Migrated from github.com) commented 2025-01-30 23:22:34 +08:00
Copy Link

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

Do we need this? What was the purpose of adding?
elvisserrao (Migrated from github.com) reviewed 2025-01-30 23:28:37 +08:00
app/views/categories/_form.html.erb
@@ -1,7 +1,7 @@
<%# locals: (category:, categories:) %>
elvisserrao (Migrated from github.com) commented 2025-01-30 23:28:37 +08:00
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). ;-)
zachgoll (Migrated from github.com) approved these changes 2025-01-31 01:34:49 +08:00
zachgoll (Migrated from github.com) left a comment
Copy Link

Looking good! Nice work, thanks for the fix.

Looking good! Nice work, thanks for the fix.
Sign in to join this conversation.
Reviewers
No Reviewers
elvisserrao
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#1730
Delete Branch "fix/1677"

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?