Fix AND prefix on rule form #2234

2025-05-12T01:35:18+08:00

ahatzz11 commented

2025-05-12 01:35:18 +08:00

(Migrated from github.com)

Currently, there are a handful of weird things when creating/editing rules around the prefix for a condition:

When creating a new rule, there is no AND prefix on any condition until the form is submitted
When creating a new rule, there is always the prefix on a condition group, even if its the first condition on the rule
When editing a rule with multiple conditions, the saved conditions have the prefix, but new conditions do not
When editing a rule with multiple conditions, if the first condition is deleted, the new first condition has the prefix

I'm sure there are some more odd cases for different circumstances, but ultimately it seems like the issues are:

The prefix isn't updated on any change
The index for an item that isn't yet saved is a very large number, and not 0, which breaks the current logic.

To solve for all of this weird behavior, I've done a few things:

Add a new method to our stimulus controller to dynamically add/remove the prefix, filtering out hidden items (those marked for deletion)
Run the stimulus method when form is rendered, adding a condition or group, and removing a condition or group. Removing a condition is done by the condition controller, so instead of duplicating the prefix logic, I look for the closet rule controller to the condition controller and use the prefix update method from there
I fell down a deep and dark rabbit hole trying to dynamically show AND vs OR on the 2+ subcondition in a condition group based on the ANY or ALL operators. This added a lot of complexity for a small payoff in my opinion, since the any/all operator already clearly show what the condition will be. Maybe this could be a future improvement, but it didn't seem worth the complexity at the moment.

Here's a video showing proper prefixes for all the actions (adding condition, group, subconditions, removals, edits, etc):

https://github.com/user-attachments/assets/b121eeb3-115e-4a2f-9de3-ffc4f5ae097c

Fixes #2230

This also implements the primary_condition_title field on a rule that Zach recommended here.

Currently, there are a handful of weird things when creating/editing rules around the prefix for a condition: - When creating a new rule, there is no AND prefix on any condition until the form is submitted - When creating a new rule, there is always the prefix on a condition group, even if its the first condition on the rule - When editing a rule with multiple conditions, the saved conditions have the prefix, but new conditions do not - When editing a rule with multiple conditions, if the first condition is deleted, the new first condition has the prefix I'm sure there are some more odd cases for different circumstances, but ultimately it seems like the issues are: - The prefix isn't updated on any change - The index for an item that isn't yet saved is a very large number, and not `0`, which breaks the current logic. To solve for all of this weird behavior, I've done a few things: - Add a new method to our stimulus controller to dynamically add/remove the prefix, filtering out hidden items (those marked for deletion) - Run the stimulus method when form is rendered, adding a condition or group, and removing a condition or group. Removing a condition is done by the condition controller, so instead of duplicating the prefix logic, I look for the closet rule controller to the condition controller and use the prefix update method from there - I fell down a deep and dark rabbit hole trying to dynamically show `AND` vs `OR` on the 2+ subcondition in a condition group based on the `ANY` or `ALL` operators. This added a lot of complexity for a small payoff in my opinion, since the any/all operator already clearly show what the condition will be. Maybe this could be a future improvement, but it didn't seem worth the complexity at the moment. Here's a video showing proper prefixes for all the actions (adding condition, group, subconditions, removals, edits, etc): https://github.com/user-attachments/assets/b121eeb3-115e-4a2f-9de3-ffc4f5ae097c Fixes #2230 This also implements the `primary_condition_title` field on a rule that [Zach recommended here](https://github.com/maybe-finance/maybe/pull/2177#discussion_r2081762981).

zachgoll (Migrated from github.com) approved these changes 2025-05-13 22:34:31 +08:00

zachgoll (Migrated from github.com) left a comment

Nice, this looks good! Good call on the complexity/value tradeoff, I 100% agree that it's not worth overloading this with complexity at this stage.

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: gavin/maybe#2234