| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Adding a comment to help with maintainability of `app/views/org_admin/conditions/_form.html.erb`
It appears that the following previous commit removed the need for this variable: da4033b#diff-9b4ef24d6aebd8b59257b1df4b7bd77adc5873a46a803a69a7ff146266d9658fR3-L15
- #3497 introduces `<% condition ||= nil %>` on line 6. - This refactor replaces the usage of `condition_exists` with `condition.present?` - `app/views/org_admin/conditions/_container.html.erb` appears to be the only `app/views/org_admin/conditions/_form.html.erb` that passes the `locals : { condition` variable. The `<% condition ||= nil %>` code on line 6 should be sufficient for performing this check. - Additionally, if `condition == nil` and `condition_exists == true` ever occurred, then we would encounter `NoMethodError` exceptions on lines 13 and 27.
- `<%= select_tag(:action_type, options_for_select(action_type_arr, type_default)` was only being executed when `if condition.nil?` was true (now line 20). Additionally, that was the only line that was using the `type_default` variable. - Thus, no conditional is required for the assigning of `type_default` and we can just use `:remove` explicitly on line 30 now.
The modified code was and is only executed when `condition.nil?` is true (line 20). Thus, the ternary operator always evaluated to the else.
- Line 35 is the only that uses the `remove_question_group` variable. However, line 35 is only executed when `condition.nil? == true` (line 18).
This change refactors `app/views/org_admin/conditions/_form.html.erb`. The change moves the logic for new conditions to `app/views/org_admin/conditions/_new_condition_form.erb` and existing conditions to `app/views/org_admin/conditions/_existing_condition_display.erb`.
- ` action_type_arr` and `remove_question_group` are only needed in `conditions/_new_condition_form.erb`. The assignments have been moved directly to that partial. - - ` view_email_content_info` is only needed in `conditions/_new_condition_form.erb`. The assignment has been moved directly to that partial.
Generated by 🚫 Danger |
Sorry, something went wrong.
There was a problem hiding this comment.
0c2e2d2 Approved
97677d4 Approved
c7abc11 Approved
b2eaac4 Approved
e651186 Approved well spotted
dc04625 Approved
3a11067 Approved
@aaronskiba This is brilliant. I can't say I fully understood all the changes. You really have done a good job re-factoring this spaghetti code into smaller chunks. I learnt quite a few Ruby concepts in reviewing.
Further I tested it on n a template with conditional questions.
Sorry, something went wrong.
…s-fix-for-rails7' into aaron/refactor-conditions-form
| Back | FazBrowse Home | New Git URL |
Changes proposed in this PR: