uncap additional expenses and allow delete #5908

shacon · 2024-07-12T15:59:46Z

What github issue is this PR for, if any?

Resolves #5773

What changed, and why?

Allows for adding more than 10 additional expenses and allows for deleting additional expenses from the form
Used this stimulus controller for nested form fields as suggested in the ticket: https://www.stimulus-components.com/docs/stimulus-rails-nested-form/

How is this tested? (please write tests!) 💖💪

Edited the capybara tests that existed with the changes to allow for more than 10 additional expenses.

Also added capybara test for the delete functionality.

This pr does add a javascript controller that resets the indexes on the nested fields generated by the stimulus controller. The capybara tests so end up testing this functionality but if it seems like we want specific javascript tests I can take a stab at those.

Screenshots please :)

A note on testing locally - this requires a feature flag to be set and for a config to be set on the related Casa Org. Here's what I ran in the rails console to do this:

org =CaseContact.last.casa_case.casa_org

 org.additional_expenses_enabled=true
org.save

Flipper.enable :show_additional_expenses

Feelings gif (optional)

elasticspoon · 2024-07-13T20:08:08Z

app/javascript/controllers/additional_expenses_form_controller.js

+  reindexFields () {
+    // Reindex fields so that nested parameters come in with consecutive
+    // indexes starting at 0
+    const containers = this.element.querySelectorAll('.expense-container')
+    containers.forEach((container, index) => {
+      container.querySelectorAll('input, select, textarea').forEach(field => {
+        const newName = field.name.replace(/\[additional_expenses_attributes\]\[\d+\]/, `[additional_expenses_attributes][${index}]`)
+        field.name = newName
+        field.id = newName.replace(/\[/g, '_').replace(/\]/g, '')
+      })
+
+      container.querySelectorAll('label').forEach(label => {
+        const forAttr = label.getAttribute('for')
+        if (forAttr) {
+          const newForAttr = forAttr.replace(/additional_expenses_attributes_\d+/, `additional_expenses_attributes_${index}`)
+          label.setAttribute('for', newForAttr)
+        }
+      })
+    })
+  }


What happens if we don't reindex the fields?

You shouldn't need a new controller at all for this. This should be doable 100% in the template and controller. Look at the documentation here.

We already have the component installed, so you can skip the installation step.

Essentially, persisted associations get their ID as their index and new records get the timestamp they were created (line 19). This ensures unique identifiers for every association in the form without needing a reindex.

@schoork @elasticspoon thanks! I'll make these changes and push something up by the end of this week

schoork

Overall this looks great, but we don't need the added controller.

schoork · 2024-07-14T14:41:00Z

app/javascript/controllers/additional_expenses_form_controller.js

+  reindexFields () {
+    // Reindex fields so that nested parameters come in with consecutive
+    // indexes starting at 0
+    const containers = this.element.querySelectorAll('.expense-container')
+    containers.forEach((container, index) => {
+      container.querySelectorAll('input, select, textarea').forEach(field => {
+        const newName = field.name.replace(/\[additional_expenses_attributes\]\[\d+\]/, `[additional_expenses_attributes][${index}]`)
+        field.name = newName
+        field.id = newName.replace(/\[/g, '_').replace(/\]/g, '')
+      })
+
+      container.querySelectorAll('label').forEach(label => {
+        const forAttr = label.getAttribute('for')
+        if (forAttr) {
+          const newForAttr = forAttr.replace(/additional_expenses_attributes_\d+/, `additional_expenses_attributes_${index}`)
+          label.setAttribute('for', newForAttr)
+        }
+      })
+    })
+  }


You shouldn't need a new controller at all for this. This should be doable 100% in the template and controller. Look at the documentation here.

We already have the component installed, so you can skip the installation step.

schoork · 2024-07-14T14:41:29Z

app/assets/stylesheets/pages/case_contacts.scss

@@ -113,6 +113,10 @@ legend {
    cursor: pointer;
 }

+.remove-expense-button {


Is this doable through Bootstrap code?

@schoork I can look into that, haven't worked with bootstrap in a long time but I'll check it out

schoork · 2024-07-14T14:43:39Z

app/javascript/controllers/additional_expenses_form_controller.js

+  reindexFields () {
+    // Reindex fields so that nested parameters come in with consecutive
+    // indexes starting at 0
+    const containers = this.element.querySelectorAll('.expense-container')
+    containers.forEach((container, index) => {
+      container.querySelectorAll('input, select, textarea').forEach(field => {
+        const newName = field.name.replace(/\[additional_expenses_attributes\]\[\d+\]/, `[additional_expenses_attributes][${index}]`)
+        field.name = newName
+        field.id = newName.replace(/\[/g, '_').replace(/\]/g, '')
+      })
+
+      container.querySelectorAll('label').forEach(label => {
+        const forAttr = label.getAttribute('for')
+        if (forAttr) {
+          const newForAttr = forAttr.replace(/additional_expenses_attributes_\d+/, `additional_expenses_attributes_${index}`)
+          label.setAttribute('for', newForAttr)
+        }
+      })
+    })
+  }


Essentially, persisted associations get their ID as their index and new records get the timestamp they were created (line 19). This ensures unique identifiers for every association in the form without needing a reindex.

schoork · 2024-07-14T14:45:36Z

app/javascript/controllers/additional_expenses_form_controller.js

You will be able to remove this file.

schoork · 2024-07-14T14:45:47Z

app/javascript/controllers/index.js

@@ -14,6 +14,8 @@ import DismissController from './dismiss_controller'

 import CourtOrderFormController from './court_order_form_controller'

+import AdditionalExpensesFormController from './additional_expenses_form_controller'


This you can remove.

schoork · 2024-07-14T14:45:54Z

app/javascript/controllers/index.js

@@ -34,6 +36,7 @@ application.register('autosave', AutosaveController)
 application.register('disable-form', DisableFormController)
 application.register('dismiss', DismissController)
 application.register('court-order-form', CourtOrderFormController)
+application.register('additional-expenses-form', AdditionalExpensesFormController)


Remove as well.

shacon · 2024-07-21T17:28:29Z

app/javascript/controllers/index.js

@@ -42,3 +45,4 @@ application.register('select-all', SelectAllController)
 application.register('sidebar', SidebarController)
 application.register('sidebar-group', SidebarGroupController)
 application.register('truncated-text', TruncatedTextController)
+application.register('nested-form', NestedForm)


I wasn't able to access the nested form controller until I added this - not sure if there is a more standard way to do this. I realize the rest of the imports in here are all local

The court_order_form_controller.js uses it, too. But it loads it directly. This is the right thing to do for your use case.

schoork · 2024-07-23T11:00:29Z

app/views/shared/_additional_expense_form.html.erb

It's not a huge deal, but I'd say get rid of the a and b. With multiple expenses it becomes confusing.

ah, just saw this, will take those out

schoork · 2024-07-23T11:02:11Z

app/views/shared/_additional_expense_form.html.erb

+              <%= form.number_field :other_expense_amount,
+                placeholder: "0",
+                class: "form-control other-expense-amount",
+                min: "0", max: 1000, step: 0.01 %>


After saving and coming back to edit, I get an expense in the form with only one cent place.

@schoork added number_with_precision to the field, let me know if this display is ok:

schoork

This looks great @shacon. One tweak to the way the cost is displayed when editing and good to go!

shacon · 2024-07-30T16:52:49Z

This looks great @shacon. One tweak to the way the cost is displayed when editing and good to go!

thanks @schoork! I made the requested changes and updated the screenshots - let me know if there is anything else I should do on my end.

schoork

Looks great @shacon! Thanks for the reminder.

uncap additional expenses and allow delete

02ac44e

github-actions bot added javascript for use by Github Labeler to mark pull requests that update Javascript code ruby Pull requests that update Ruby code Tests! 🎉💖👏 erb labels Jul 12, 2024

shacon marked this pull request as ready for review July 13, 2024 19:34

shacon requested review from schoork, FireLemons, elasticspoon, compwron and littleforest as code owners July 13, 2024 19:34

elasticspoon reviewed Jul 13, 2024

View reviewed changes

schoork requested changes Jul 14, 2024

View reviewed changes

5773 uncap additional expense pr cleanup

3005b58

shacon commented Jul 21, 2024

View reviewed changes

schoork reviewed Jul 23, 2024

View reviewed changes

schoork requested changes Jul 23, 2024

View reviewed changes

shacon added 2 commits July 23, 2024 13:11

5773 uncap additional expense field display adjustment

3815b27

5773 uncap additional expenses-remove a and b labels

731fdba

schoork approved these changes Jul 30, 2024

View reviewed changes

schoork merged commit 0d49402 into rubyforgood:main Jul 30, 2024
19 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

uncap additional expenses and allow delete #5908

uncap additional expenses and allow delete #5908

shacon commented Jul 12, 2024 •

edited

Loading

elasticspoon Jul 13, 2024

schoork Jul 14, 2024

schoork Jul 14, 2024

shacon Jul 17, 2024

schoork left a comment

schoork Jul 14, 2024

schoork Jul 14, 2024

shacon Jul 17, 2024

schoork Jul 14, 2024

schoork Jul 14, 2024

schoork Jul 14, 2024

schoork Jul 14, 2024

shacon Jul 21, 2024

schoork Jul 23, 2024

schoork Jul 23, 2024

shacon Jul 23, 2024

schoork Jul 23, 2024

shacon Jul 23, 2024

schoork left a comment

shacon commented Jul 30, 2024

schoork left a comment

		@@ -14,6 +14,8 @@ import DismissController from './dismiss_controller'

		import CourtOrderFormController from './court_order_form_controller'

		import AdditionalExpensesFormController from './additional_expenses_form_controller'

uncap additional expenses and allow delete #5908

uncap additional expenses and allow delete #5908

Conversation

shacon commented Jul 12, 2024 • edited Loading

What github issue is this PR for, if any?

What changed, and why?

How is this tested? (please write tests!) 💖💪

Screenshots please :)

Feelings gif (optional)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schoork left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schoork left a comment

Choose a reason for hiding this comment

shacon commented Jul 30, 2024

schoork left a comment

Choose a reason for hiding this comment

shacon commented Jul 12, 2024 •

edited

Loading