Merge development Into main For Upcoming Release
#3495
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
Addresses the following error that was encountered when attempting to execute the GitHub Actions that correspond with the edited workflows files: https://github.com/DMPRoadmap/roadmap/actions/runs/9513143079/job/26222602325 3s ``` Run bundle exec rails db:create RAILS_ENV=test To use retry middleware with Faraday v2.0+, install `faraday-retry` gem To use multipart middleware with Faraday v2.0+, install `faraday-multipart` gem; note: this is used by the ManageGHES client for uploading licenses Copying Bootstrap glyphicons to the public directory ... Copying TinyMCE skins to the public directory ... /home/runner/work/roadmap/roadmap/config/initializers/recaptcha.rb:8:in `block in <main>': undefined method `[]' for nil:NilClass (NoMethodError) from /home/runner/work/roadmap/roadmap/vendor/bundle/ruby/3.0.0/gems/recaptcha-5.17.0/lib/recaptcha.rb:37:in `configure' from /home/runner/work/roadmap/roadmap/config/initializers/recaptcha.rb:7:in `<main>' ```
Prior to this commit, the Rails credentials were not being updated during this setup process.
Here are links to the errors being raised prior to this commit: https://github.com/DMPRoadmap/roadmap/actions/runs/9822436610/job/27119190298?pr=3435 https://github.com/DMPRoadmap/roadmap/actions/runs/9822436613/job/27119190303?pr=3435 https://github.com/DMPRoadmap/roadmap/actions/runs/9844975903/job/27179509035?pr=3435
This commit undoes some Rubocop fixes made from a prior commit ( bda5b6e ). However, it also resolves the following error that was being raised: https://github.com/DMPRoadmap/roadmap/actions/runs/9845084297/job/27179836711
Omitting the arguments results in lambda implicitly using self, which appears to be the desired behaviour here. It also resolves the Rubocop offences.
`template.visibilty` now returns a string rather than an integer. The Rails 7 upgrade actually fixes a couple of bugs within `app/views/org_admin/templates/_form.html.erb` and `app/views/org_admin/templates/_show.html.erb`. Prior to this upgrade, template.visibility would return an integer. Now that it is returning a string, the `f.object.visibility == 'organisationally_visible'` and `template.visibility == 'organisationally_visible'` checks within the aforementioned files are behaving as desired.
Prior to this commit, the default checked/unchecked values were used (i.e. "1" would be returned when checked, and "0" would be returned when unchecked). However, the box is meant to be checked when selecting 'organisationally_visible' ('for internal %{org_name} use only'), which makes the default checked/unchecked values opposite to the mapping of our enums (i.e. `{"organisationally_visible"=>0, "publicly_visible"=>1}`).
Rails 7 appears to apply stricter parsing rules. If the Content-Type is not JSON, then the body will not be parsed as JSON.
Prior to this code change, any value assigned to the `'data-method':` attribute of the `link_to` method was not being read (and instead defaulting to `GET`). This was resulting in the breaking of several `spec/features/` tests (https://github.com/DMPRoadmap/roadmap/actions/runs/9946998559/job/27478725801). The `@rails/ujs` library is meant to handle this `'data-method':` attribute.
app/views/org_admin/plans/index.html.erb: - .length loads the records into memory. Because we are eager-loading other tables (see code changes in app/controllers/org_admin/plans_controller.rb), this was triggering a "AVOID eager loading detected" Bullet warning. Because we are in fact making use of these related tables further in the code, the warnings themselves appear to be false-positives. However, the change to this file manages to suppresses the Bullet warnings, and is still very fast.
…eing copied.
Change in class method Plan.deep_copy:
- Firstly, on duplicating the Plan we set the plan identifier to nil and save.
- Then we fill the identifier variable with the Plan id that was
regenerated when the duplicate copy was save in the previous.
- We then persist this change by saving the Plan again.
…eing copied. Changes (suggested by @aaronskiba): - removed an unnecessary line plan_copy.identifier = nil - cast to string the integer-valued plan id plan_copy.identifier = plan_copy.id.to_s
Fix PostgreSQL GitHub Action and Tests For Rails 7 Upgrade
Added `coder:` and `type:` keywords in various places to address deprecation warnings. Example warning (before adding `type: ` keyword in `app/models/user.rb`: ```` Please pass the class as a keyword argument: serialize :prefs, type: Hash (called from <class:User> at /path/to/app/models/user.rb:73) DEPRECATION WARNING: Passing the class as positional argument is deprecated and will be removed in Rails 7.2. ```
This change addresses the following deprecation warnings: ``` DEPRECATION WARNING: Setting action_dispatch.show_exceptions to false is deprecated. Set to :none instead. (called from rescue in call at /usr/share/rvm/gems/ruby-3.0.5@upstream/gems/actionpack-7.1.3.4/lib/action_dispatch/middleware/debug_exceptions.rb:43) DEPRECATION WARNING: Setting action_dispatch.show_exceptions to false is deprecated. Set to :none instead. (called from rescue in call at /usr/share/rvm/gems/ruby-3.0.5@upstream/gems/actionpack-7.1.3.4/lib/action_dispatch/middleware/show_exceptions.rb:36) ```
This commit replaces `page.source` with (the hopefully more efficient) `page.title` for verifying the page title. Checking the entire page source was potentially causing slowdowns and leading to the intermittent failing of tests within this file.
Address Deprecation Warnings
Updated app to rails 7
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' %>.
The `serialize :prefs, type: Hash` line in the `User` model was redundant since the `prefs` column does not exist in the `users` table. User preferences are handled via the `Pref` model, which serializes its `settings` column as a JSON hash. This change aligns with the update to store preferences in a separate model, as introduced in the following commit: 0405973
<% 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.
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
- #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.
…mptool-conditional-questions-fix-for-rails7 Fix issues with Conditional question serialization (offered by @briri from DMPTool) in PR CDLUC3#667
Match `config.load_defaults` Version to Rails Version
|
#3497 is now successfully merged and the conditional questions issues have been resolved. |
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:
plan.titleWithinspec/features/plans/exports_spec.rb#3451hidden_field_tagNested Attributes Format For Rails 7 Upgrade and Add Test Coverage #3479runs-on: ubuntu-24.04and Consolidate Capybara Config #3487render_respond_to_format_with_error_message#3482/featuresTests #3491bundle update && yarn upgrade#3483org_admin/conditions/_form.html.erb#3502Question.save_condition#3501