Refactor org_admin/conditions/_form.html.erb
#3502
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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 |
johnpinto1
approved these changes
Apr 9, 2025
Contributor
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.
…s-fix-for-rails7' into aaron/refactor-conditions-form
aaronskiba
merged commit 478fdad
into
johnpinto1-updated-port-of-dmptool-conditional-questions-fix-for-rails7
8 checks passed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changes proposed in this PR:
app/views/org_admin/conditions/_form.html.erb.app/views/org_admin/conditions/_new_condition_form.erbandapp/views/org_admin/conditions/_existing_condition_display.erbwhich now handle the logic for new conditions vs existing conditions respectively. This change was largely performed via a simple copy/paste. Commit 4724bc7 further refines this change.