Fix issues with Conditional question serialization (offered by @briri from DMPTool) in PR https://github.com/CDLUC3/dmptool/pull/667 by johnpinto1 · Pull Request #3497

Fix issues with Conditional question serialization (offered by @briri from DMPTool) in PR https://github.com/CDLUC3/dmptool/pull/667 by johnpinto1 · Pull Request #3497 · DMPRoadmap/roadmap

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.

Hash.
Removing type fixes issue.

sanitize_hash in QuestionsController to reflect the change structure. It
is now a hash of a  hash.

from
<% cond_lbl = (!conditions.nil? && conditions.any?) ? 'Edit Conditions' : 'Add Conditions' %>
to concise version
 <% cond_lbl = conditions&.any? ? 'Edit Conditions' : 'Add Conditions' %>.

John Pinto and others added 6 commits

April 4, 2025 10:29

    <% 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.

Adding a comment to help with maintainability of `app/views/org_admin/conditions/_form.html.erb`

…-condition

Refactor `Question.save_condition`

- #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`

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 deleted the johnpinto1-updated-port-of-dmptool-conditional-questions-fix-for-rails7 branch

April 15, 2025 16:01