Sync: BioPortal v6.8.0 release #18
Draft
alexskr wants to merge 255 commits into
Draft
Conversation
…gies_api into feature/user-admin-endpoints
Feature: Ontology Admin Endpoints for Logs and Submission Management
…ests Replace non-idiomatic assertions with Minitest equivalents: - assert(x == y) -> assert_equal - .eql?() -> assert_equal - assert !x.empty? -> assert_operator x.length, :>, 0 - submissions.all? -> submissions.each for better error messages Extract repeated contact validation into helper method to reduce duplication. Fix intermittent test failures by sorting array keys before comparison to handle non-deterministic ordering. refs #197
- Replace class variables (@@) with class instance variables using accessors - Add attr_accessor definitions for shared test state - Modernize assertions to Minitest style (assert_equal, assert_includes, assert_nil) - Improve code formatting and readability
…dition - Replace random .sample(3) with deterministic .first(3) - Use dynamic assertion for keep_ids count instead of hardcoded value - Capture process_id before use for clarity - Simplify polling loop condition to check only for 'done' or 'errors' - Clean up assertion message formatting for consistency
…n/deletion - Replace `assert 201, ...` with `assert_equal 201, ...` to properly compare response status. - Add GET checks after slice creation to confirm the new slice exists. - Remove redundant manual reset of LinkedData.settings.enable_security. - Update DELETE test to expect 204 (No Content) instead of 201. - Add GET checks after deletion to verify the slice was actually removed.
- Add explicit flunk when status_payload contains 'errors' to prevent false positives from incomplete or invalid responses - Ensure test fails immediately on backend error instead of silently skipping assertions
Restore LinkedData.settings.enable_slices to its original value after tests.
…com:ncbo/ontologies_api into refactor/tests-stability-and-minitest-style
… user helpers This change refactors `TestOntologySubmissionsController` to eliminate suite-level globals and class-variable state in favor of per-test setup and shared helpers: - Replace `before_suite` + `@@acronym`/`@@name`/`@@file_params` with per-test `setup` that generates a unique ontology acronym and creates an ontology administered by a shared test user. - Introduce a suite-local `USERNAME = "test_user"` and use `ensure_user(USERNAME)` to guarantee the user exists without races. - Normalize assertion style: - Use `assert_equal(expected, actual, message)` with parentheses - Remove `msg=` pseudo-named args - Add `.sort` where appropriate when comparing unordered keys - Replace direct references to `@@acronym`/`@@name` with instance vars (`@acronym`, `@name`). New helper methods (in the base test class) used by this suite: - `create_user(username, email:, password:)` — always creates a user - `ensure_user(username, email:, password:)` — idempotently finds or creates - `delete_user(username)` — removes a user if present Notes: - `delete_ontologies_and_submissions` now runs in `setup` to guarantee a clean slate per test. `after_all` also cleans up the shared user via `delete_user(USERNAME)`. If the extra cleanup proves redundant, we can drop `before_all`/`after_all` and keep only the per-test cleanup.
…st-style Refactor/tests stability and minitest style
Feature/user admin endpoints - fix unit tests and stability after merging feature branch
Develop -> Master merge, release v6.8.0
The prefLabelExact/synonymExact fields now use the string_ci_exact field type (case-insensitive exact match + binary scoring via omitTermFreqAndPositions). The schema assertions still expected the old string_ci literal, failing CI on develop after the type change merged. Case-insensitivity is preserved, so the functional check is unchanged.
Fix: malformed Solr query in Annotator (#233)
Fix: restore production access log output
alexskr
marked this pull request as draft
June 18, 2026 16:58
Trailing-slash canonicalization was implemented in init.rb by registering
a companion "#{pattern}/" 301-redirect route for every route. That route
ran in the route-dispatch phase, so it sat behind every before filter and
doubled the route table, and it hardcoded 301 for all verbs (which can
downgrade the method and drop POST/PUT/PATCH bodies).
Replace it with Rack::TrailingSlashRedirect, used as the innermost
middleware so it runs before Sinatra routing and filters. It strips the
trailing slash and redirects (301 for GET/HEAD, 308 for other verbs to
preserve method and body), preserving the forwarded scheme
(X-Forwarded-Proto), the query string, and percent-encoded path segments.
The guard `error 403, "Access denied" unless current_user && current_user.admin?` was copy-pasted across slices (4x) and annotator (2x), with a nil-guard-less variant in users. Extract a single admin_only! helper next to current_user and call it from all seven sites.
Data-driven test asserting every reachable admin_only!-guarded endpoint (slices create/update/delete, user delete, annotator dictionary/cache) returns 403 for an authenticated non-admin.
Adds test/middleware/test_trailing_slash_redirect.rb driven directly via Rack::MockRequest (no app boot / backend) covering: the verb split (301 for GET/HEAD, 308 for POST/PUT/PATCH/DELETE), HEAD redirect with an empty body, root and non-trailing-slash pass-through, and Location building (query string, percent-encoded segments, X-Forwarded-Proto scheme selection). Also fixes the middleware to return an empty body for HEAD redirects (a HEAD response must not carry a body).
The redirect Location is built from X-Forwarded-Proto, but an outer Rack::Cache keys entries on path + query only. Without no-store a 301/308 generated for an https client could be cached and replayed to a plain-http client (or vice versa), reintroducing the scheme-downgrade from issue #217. Redirects are cheap, so keeping them out of the shared cache costs nothing.
Settings like enable_security live on global singletons, so a suite that flips one (or errors after flipping it) leaked the value into later tests. Add a with_settings block helper that restores prior values even when the block raises, plus per-test (before_setup/after_teardown) and per-suite (before_all/after_all) snapshot/restore nets in the test base class. Suites can now set what they need and never restore. Drop the ad-hoc enable_security/disable_security/reset_security class helpers and convert callers.
after_all ran after_suite before restoring the per-suite settings snapshot. Suites that enable a setting in before_suite (e.g. test_rack_attack sets enable_security) and do privileged cleanup in after_suite (deleting users) then ran that teardown with the setting still active -> WriteAccessDeniedError. Because after_suite raised, restore_settings never ran, so enable_security leaked into every later suite (data-setup WriteAccessDenied, 401s on public routes). Restore first so teardown runs with settings rolled back and a raising after_suite cannot leak.
…leware The Rack SPEC requires both env keys to always be present (possibly empty, never nil), so the nil checks were unreachable. Add tests pinning the boundary cases: empty PATH_INFO passes through untouched, and an empty query string appends no '?' to the Location.
Controller/routing cleanup: trailing-slash middleware, admin_only! helper, test settings isolation
Fixes staging term search failing with RSolr 400 'undefined field:
ontologyRank'. The deployed ontologies_linked_data (24c5a6d0) still
queried the legacy :term_search_core1 collection, which has no
ontologyRank field, while ontologies_api boosts on sum(ontologyRank,1).
Advances locked git revisions to current branch tips:
ontologies_linked_data develop -> a8a8dfbd (uses :term_search alias +
defines/populates ontologyRank)
goo development -> c0c1ec7
ncbo_cron develop -> 5e3768b9
goo/sparql-client track 'development'; the rest track 'develop'.
Guards the staging incident where a stale ontologies_linked_data (no ontologyRank field) was bundled with an api that boosts term search on sum(ontologyRank,1), making every /search fail with Solr 400 'undefined field: ontologyRank'. Each repo's own suite stays green on this (ld tests its schema generator; api's boost test bundles a matching ld), so the cross-repo version skew is invisible to both. This infra-free contract test binds the query-layer boost field to the *bundled* linked-data schema, so api CI fails whenever the resolved ontologies_linked_data omits the field.
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
3 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.
Sync BioPortal v6.8.0 release