Development - #4
Open
xsrust wants to merge 2493 commits into
Open
Conversation
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
Fix Flaky Tests / Optimize Checking of `plan.title` Within `spec/features/plans/exports_spec.rb`
…ar_chart_broken Fix for #3443 - bar chart click function broken in usage dashboard
…d-id-as-identifier Fix for bug in Plan duplication results in original plan identifier b…
Move the "read" scope authorization check to a shared `before_action` in `BaseApiController` - Removes redundant checks from individual actions in `PlansController` and `TemplatesController` - Allows for the safe removal of `@scopes` from `base_response_content`
Prior to this change, `LocaleService.default_locale` was always assigned to `json.language`. - Now we use `plan.owner&.language&.abbreviation` (or `LocaleService.default_locale` as a fallback)
- Updated `it 'includes the :language'` test to reflect changes made in b238c8128e0fefb9ea25cc7528e5be2c499d9318. - Updated `Api::V1` references within spec files with `Api::V2`
NOTE: The prior comment stated the following: # Attach the first data_curation role as the data_contact, otherwise # add the contributor to the contributors array However, the contributor was ALWAYS added to the contributors array
Moved complex query logic from policies to `for_api_v2` scopes in Template and Plan models.
Addresses the following Bullet warning: user: aaron GET /api/v2/templates USE eager loading detected Identifier => [:identifier_scheme] Add to your query: .includes([:identifier_scheme]) Call stack /home/aaron/Documents/GitHub/roadmap/app/presenters/api/v2/org_presenter.rb:9:in `block in affiliation_id' /home/aaron/Documents/GitHub/roadmap/app/presenters/api/v2/org_presenter.rb:9:in `affiliation_id' /home/aaron/Documents/GitHub/roadmap/app/views/api/v2/orgs/_show.json.jbuilder:11:in `block in _app_views_api_v__orgs__show_json_jbuilder__1237609272914133563_48080' /home/aaron/Documents/GitHub/roadmap/app/views/api/v2/orgs/_show.json.jbuilder:10:in `_app_views_api_v__orgs__show_json_jbuilder__1237609272914133563_48080' /home/aaron/Documents/GitHub/roadmap/app/views/api/v2/templates/index.json.jbuilder:16:in `block (3 levels) in _app_views_api_v__templates_index_json_jbuilder___4313931085157922975_47640' /home/aaron/Documents/GitHub/roadmap/app/views/api/v2/templates/index.json.jbuilder:15:in `block (2 levels) in _app_views_api_v__templates_index_json_jbuilder___4313931085157922975_47640' /home/aaron/Documents/GitHub/roadmap/app/views/api/v2/templates/index.json.jbuilder:8:in `block in _app_views_api_v__templates_index_json_jbuilder___4313931085157922975_47640' /home/aaron/Documents/GitHub/roadmap/app/views/api/v2/templates/index.json.jbuilder:5:in `_app_views_api_v__templates_index_json_jbuilder___4313931085157922975_47640' /home/aaron/Documents/GitHub/roadmap/app/controllers/api/v2/templates_controller.rb:13:in `index'
This change further addresses Bullet warnings that were earlier addressed in commit 6ed969b
This change updates `Api::V2::PlansController#show` to use the `.for_api_v2` scope, and applies eager loading of answers (`.includes(answers: { question: :section })`) based on the `complete` param--making the `show` action consistent with `index`.
A new `plans_scope` helper DRYs up the controller by centralizing the shared scope and eager loading logic. Since `.for_api_v2` already filters by `.where(roles: { user_id: user_id, active: true })`, only a presence check is now required in the policy.
Add API V2 from DMPonline
Update Devise error message rendering to account for the breaking change introduced in Devise 5. The deprecated `devise_error_messages!` helper was removed, so views must render the `devise/shared/error_messages` partial directly.
Revert the RuboCop autocorrection in ApplicationController that replaced Array#any? with Array#intersect?. The autocorrection was not equivalent because the original code checks whether request.fullpath contains any of the configured paths using String#include?, while Array#intersect? performs an array intersection against matching elements. Keep the original behavior of detecting matching URL substrings when deciding whether to store the previous location.
app/controllers/application_controller.rb: - Refactor the `store_location` path matching logic to use a regular expression instead of iterating over an array of paths. - This preserves the existing behavior of checking whether request.fullpath contains one of the Devise authentication paths while avoiding the Style/ArrayIntersect RuboCop autocorrection, which was not equivalent for String#include? usage. - No functional behavior changes are intended. lib/tasks/upgrade.rake: - Replace manual File.open/File.close handling with File.write in the `upgrade rake task`. - This removes the possibility of file descriptor leaks and satisfies the Style/FileOpen RuboCop cop while keeping the file generation logic unchanged. app/models/plan.rb - Disable Metrics/BlockLength offence within `:for_api_v2` scope
Update all GitHub Actions workflows to Node.js 20 and consolidate version management into a `.node-version` file for consistency across CI and local development. Previously, workflows ran with Node.js 18.20.8, which caused `yarn install` to fail after sass was upgraded to 1.101.1 due to an incompatible engine requirement. - Removed explicit `node-version` from all workflow files - Created .`node-version` file with version 20 - `actions/setup-node` automatically detects and uses `.node-version`
Pin jquery-ui to 1.13.3 to prevent an unintended upgrade to 1.14.2. The jquery-ui 1.14.2 upgrade was introduced as part of running `yarn upgrade`. This version adds a dependency on jQuery 4, resulting in compatibility issues with the application's existing jQuery 3 dependency. The upgrade caused feature specs to fail, including JSON parsing failures when submitting organisation links. Resolved failures: - Plans User creates a new Plan - SuperAdmins Orgs Super admin submits invalid data The latter failed with: JSON::ParserError: unexpected character: 'org' at line 1 column 1
chore(deps): `bundle update && yarn upgrade` and bump `node-version` in workflows
Prior to this change, `config/database.yml` was configured such that running tests would be executed against the dev db. This would result in the dev db being destroyed after tests. This change ensures that the tests are executed against a separate test db and that the dev db persists while testing.
…e-config Update `database.yml.sample`: add separate test db
- Function checks if resource owner is active and present before authorization, and returns error if not - Move code assigning resource_owner to this new function for better readability
- Remove payload[:message], copying v1 behaviour
Patch inactive user issue in V2 API
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
9 participants
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.
Fixes # .
Changes proposed in this PR: