Fix issues with Conditional question serialization (offered by @briri from DMPTool) in PR https://github.com/CDLUC3/dmptool/pull/667 #3497
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
from DMPTool).nBased on DMPtool commit CDLUC3#667. Changes proposed in this PR (text from cited PR): - There is a migration file with code for MySQL and Postgres to update the Conditions table to convert JSON Arrays in string format records in the conditions table so that they are JSON Arrays. - Updated the form partials in app/views/org_admin/conditions/_form.erb.rb to properly send condition data to the controller. - Removed all JSON.parse calls in the app/helpers/conditions.rb helper - The user canno longer edit a condition. They need to remove it and create a new condition. This applies to the email for 'add notifications' too. Made the following changes to simplify this patch and to make it a little more user friendly: - The "Add Conditions" button will now say "Edit Conditions" if there are any conditions for a given question. - Updated the column heading over the "thing that happens when the condition is met" from "Remove" to "Target" since the content of the column can either be questions being removed or an email notification being sent. - Conditions of any action type can be added or removed (not updated anymore). - Hovering over the email of an existing condition displays a tooltip that shows the email message, subject, etc. The email content can no longer be edited once saved. It will need to be removed and re-created. - We allow only one question option to be selected when adding a Condition unlike inthe DMPTool version because experience with multiple options chosen has been problematic and buggy when used by users in DMPOnline. - To add a condition you must have selected an Option and Action together with: o if Action is 'remove', you need to select one or more choices in Target. o if Action is 'add notification', you need to fill in all the fields in the 'Send email' popup. Otherwise, the condition will not be saved.
Generated by 🚫 Danger |
|
This was added as Rails 7 update was breaking for editing template and adding conditional question |
aaronskiba
reviewed
Apr 1, 2025
db/migrate/20250115102816_update_conditions_json_columns_data.rb
Outdated
Show resolved
Hide resolved
|
(UPDATE: I have verified that this same behaviour currently exists on main. Rather than requesting changes in this PR, I will create an issue describing what I've found here.) I will have to look at code behaviour prior to this PR, but I'm noticing the following:
|
aaronskiba
reviewed
Apr 1, 2025
Hash. Removing type fixes issue.
aaronskiba
reviewed
Apr 3, 2025
sanitize_hash in QuestionsController to reflect the change structure. It is now a hash of a hash.
6f543cc to
dba3999
Compare
aaronskiba
reviewed
Apr 3, 2025
from <% cond_lbl = (!conditions.nil? && conditions.any?) ? 'Edit Conditions' : 'Add Conditions' %> to concise version <% cond_lbl = conditions&.any? ? 'Edit Conditions' : 'Add Conditions' %>.
aaronskiba
reviewed
Apr 3, 2025
<% condition = condition ||= nil %>
with concise expression as in
<% condition ||= nil %>.
This change uses `.map` to refactor/simplify the mapping of `option_list` and `remove_data`. It also removes the needed for temporary arrays.
Refactored `save_condition` method via new `handle_webhook_data` helper method. - Helper method returns nil if any of the required webhook_data fields are absent. Else, it constructs and returns the webhook_data hash - Returning nil when any fields are absent enables us to simplify the if check that immediately follows (now line 240) to `if c.option_list.empty? || c.webhook_data.nil?` - Overall, these changes simplify the `save_condition` method and even remove a previous rubocop offense.
- Moved `c.option_list.empty?` immediately after `c.option_list` assignment to streamline logic. - This change reduces unnecessary processing of `c.remove_data` and `c.webhook_data`. - Isolating the `c.option_list.empty?` check also simplifies subsequent checks for `c.remove_data.empty?` and c.webhook_data.nil?` later in the code.
- Moved logic for handling option_list and remove_data into separate helper methods (handle_option_list and handle_remove_data). - This simplifies our `save_condition` method and resolves some previously disabled rubocop offenses.
aaronskiba
reviewed
Apr 4, 2025
aaronskiba
reviewed
Apr 4, 2025
aaronskiba
reviewed
Apr 4, 2025
This is
Adding a comment to help with maintainability of `app/views/org_admin/conditions/_form.html.erb`
…-condition Refactor `Question.save_condition`
It appears that the following previous commit removed the need for this variable: da4033b#diff-9b4ef24d6aebd8b59257b1df4b7bd77adc5873a46a803a69a7ff146266d9658fR3-L15
aaronskiba
reviewed
Apr 7, 2025
- #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.
…s-fix-for-rails7' into aaron/refactor-conditions-form
Refactor `org_admin/conditions/_form.html.erb`
aaronskiba
reviewed
Apr 9, 2025
aaronskiba
reviewed
Apr 9, 2025
aaronskiba
reviewed
Apr 9, 2025
NOTE: For app/views/org_admin/conditions/_existing_condition_display.erb, `hook_tip` refers to the pop up message that is rendered when hovering over the email address in the "Target" section of an "Email" type action. We are only enabling for the titles here ('Name', 'Email', 'Subject', and 'Message'), not the actual corresponding values.
aaronskiba
approved these changes
Apr 9, 2025
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.
Previous PR closed as it was easier to add code to a new pull of development. Old PR #3476 closed
Fix issues with Conditional question serialization (offered by @briri from DMPTool) in PR CDLUC3#667
Changes proposed in this PR (text from cited PR):
Made the following changes to simplify this patch and to make it a little more user friendly:
-- If Action is 'remove', you need to select one or more choices in Target.
-- If Action is 'add notification', you need to fill in all the fields in the 'Send email' popup.
Otherwise, the condition will not be saved.
Some screenshots:
