Development#4
Open
xsrust wants to merge 2474 commits into
Open
Conversation
getConstant('VALIDATION_MESSAGE_PASSWORDS_MATCH') still exists in passwordhelper.js
Was previously removed here: portagenetwork@33a6058
Fix "undefined" Tooltip Messages
- Prior to this commit, if current_user.org was also a @default_org, then @important_ggs would contain duplicate [org, ggs].
Prevent Duplicate Options in 'Select Guidance'
Changes:
- To deal with missing favicon.ico error ( ActionController::RoutingError:
No route matches [GET] "/favicon.ico") :
added config/routes_test.rb to add a route will return an empty response
with a 200 OK status code when the browser requests the favicon.ico file.
Then for test purposes we add the lines to config/encvironments/test.rb:
# Add config/routes_test.rb to routes
config.paths['config/routes.rb'] << Rails.root.join('config/routes_test.rb')
- The other outstanding errors arose because in the test environment
we sometimes encounter errors like (Selenium::WebDriver::Error::ElementClickInterceptedError: element click intercepted:
# Element <input type="submit" name="commit" value="Change affiliation" class="btn btn-secondary" data-disable-with="Change affiliation">
# is not clickable at point (101, 203). Other element would receive
the click: <div id="ui-id-2" tabindex="-1"
class="ui-menu-item-wrapper">...</div>).
To get round this we use JS to click on element using code like
this:
change_affiliation_input_button = find('input[value="Change affiliation"]')
execute_script('arguments[0].click();', change_affiliation_input_button)
:selenium_chrome_headless driver has a default window-size of (800x600). This small window-size seems to result in the `Selenium::WebDriver::Error::ElementClickInterceptedError:` This commit increases the window-size and also reverts the prior fix for these errors (commit 55321d0)
…p_upgrade Fix for failing Rubocop tests.
Roadmap V4.2.0
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.
Replaced raw SQL with ActiveRecord query methods + enum scopes - Improves readability and eliminates SQL injection risk
Move theme filtering from Ruby to SQL using joins for better performance and scalability.
Previously, `fetch_q_and_a` iterated over all plan questions and searched for matching answers in Ruby, performing theme filtering and Q&A formatting in memory. This caused unnecessary loops, potential N+1 lookups, and inefficient handling of multi-theme questions. Accessing questions directly via `Plan.questions` also triggered joins through sections, phases, and templates, along with ordering (due to default scope), which added query overhead. The refactored version: - Queries answers directly via a join to question themes, filtering at the DB level. - Accesses questions through the answers (`answer.question`) instead of `plan.questions`, avoiding heavy joins and unnecessary default-scoped sorting.
Previously, the /api/v2/plans endpoint triggered many N+1 queries when rendering plan associations, causing slow response times. This change adds .includes() for all associations that were previously triggering Bullet N+1 warnings. Benchmark comparison (local dev, 10 sequential requests): | Metric | Before | After | | ----------------- | ------- | ------ | | Mean request time | 1909 ms | 492 ms | | Requests/sec | 0.52 | 2.03 | | Max request time | 2628 ms | 641 ms | - This simple comparison shows that these changes make the `GET api/v2/plans` ~4x faster.
- Use warden.authenticate! (instead of manually storing session[:user_return_to] + explicitly redirecting to login page) - Simplify conditional logic by checking "native" path first - Replace hardcoded "/oauth/authorize/native path" string with native_oauth_authorization_path When warden.authenticate! is invoked, Warden both automatically stores the request URL in the session and reroutes the unauthenticated user to the login page. The existing session[:user_return_to] checks in ApplicationController continue to work. This reduces custom code and leverages Devise's built-in authentication handling for better maintainability. NOTE: For better alignment with Devise conventions and automatic session cleanup, we could refactor ApplicationController to use stored_location_for(resource) instead of session[:user_return_to].
- Use start_with?(oauth_authorization_path) instead of include?('/oauth/authorize') for stricter path matching
- Use stored_location_for(:user) instead of session[:user_return_to] for automatic cleanup and explicit Devise scope usage
- OAuth flow detection is tied to session[:user_return_to], so the :user scope is explicit and correct
- Add ? suffix to predicate method name
Improves security (prevents substring matches), maintainability (uses framework APIs with explicit scope), and follows Ruby conventions.
With the exception of specific 'disallowed paths' (i.e. `new_user_session_path` for `after_sign_in_path_for` vs `new_user_session_path` and `new_user_registration_path` for `after_sign_up_path_for`, these helpers are otherwise identical. This change refactors the code so that these helpers better adhere to DRY principles. We introduce `def after_auth_path(disallowed_paths:)`. `after_sign_in_path_for` and `after_sign_up_path_for` each call `after_auth_path` with an array containing their aforementioned disallowed paths. Some small additional refactoring is also performed. `return root_path if request.referer.nil? || from_external_domain?` allows for possible early return before assigning `referer_path`. Also, repeated `referer_path.eql?(path)` statements have been replaced with a single `disallowed_paths.include?(referer_path)` check.
Amongst its changes, this commit removes `warden.authenticate!(scope: :user)`. When warden.authenticate! is invoked, the OAuth flow is preserved while the user signs in. However, our current custom implementation of ApplicationController#after_sign_in_path_for only returns `stored_location_for(resource)` for the `oauth/authorize` path. - We could further modify after_sign_in_path_for to also return `stored_location_for(resource)` for the `oauth/applications` path. However, it is simple enough for a super admin to navigate to `oauth/applications` via the admin panel.
- Edit plan query to check for user role to prevent eager loading - Add set_complete_param function that sets complete flag according to its value if its set to true in the API call
- Edit initialize to include complete flag, which is set to false as default - If flag is true in call, call fetch_all_q_and_a - Add fetch_all_q_and_a function that fetches questions and answers Update plan_presenter
- Add complete flag data to extension in json
- Pre-fetch the answers, questions, and sections as part of the initial plans query (but only when we are in fact fetching all of the questions and answers). - This speeds up the mean request time by 25%
Add complete plan flag to V2 API
More information regarding these Doorkeeper rake tasks can be found at https://doorkeeper.gitbook.io/guides/internals/rake
Generate and apply migration to remove NOT NULL constraint from `oauth_applications.redirect_uri`. This works in conjunction with `allow_blank_redirect_uri true` in `config/initializers/doorkeeper.rb`, allowing us to save `OAuthApplications` with blank `redirect_uri` values for URI-less flows like client_credentials or password grant flows. NOTE: Applications using `authorization_code` (or other flows that require a redirect URI) must still have a non-blank `redirect_uri`. Otherwise, `/oauth/authorize` requests with `response_type=code` will fail. More info is available here: https://github.com/doorkeeper-gem/doorkeeper/wiki/Allow-blank-redirect-URI-for-Applications
- Enable `hash_application_secrets` and `hash_token_secrets` in Doorkeeper initializer for improved security. - This stores application secrets and access tokens as hashes in the database, reducing risk if the database is compromised. - Note: `hash_token_secrets` is incompatible with `reuse_access_token`, so token reuse is now disabled/removed (see warning in Doorkeeper docs). - For more details, see: https://doorkeeper.gitbook.io/guides/security/token-and-application-secrets
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
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: