Skip to content

chore (8/8): CKAN 2.11 post-migration cleanup & regression fixes - #331

Merged
A-Souhei merged 13 commits into
developmentfrom
chore/ckan-211-post-migration-cleanup
May 26, 2026
Merged

A-Souhei merged 13 commits into
developmentfrom
chore/ckan-211-post-migration-cleanup

Conversation

@A-Souhei

@A-Souhei A-Souhei commented May 21, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Final PR in the CKAN 2.11 migration stack. Removes migration-orphaned dead code and fixes regressions surfaced during review/QA on the running deploy. Kept separate so the focused migration PRs stay clean.

Hook/route claims below were verified against CKAN 2.11 source: ckan/plugins/interfaces.py (interface method names), ckan/views/dashboard.py, ckanext/activity/views.py.

Dead code (migration leftovers — no behaviour change)

  • auth_logic.py — remove json_response_omitting_challenge_decider (its only caller, the repoze.who IChallengeDecider registration, was removed in chore (2/8): make plugin load and import under CKAN 2.11 #325 when repoze.who left 2.11) and its test; remove the write-only g.current_user = payload (nothing reads it — CKAN's current_user is the separate flask-login proxy). The token-decode path is unchanged.
  • svg_map_options.py / user_info_blueprint.py — drop the last six usages (six.iteritems() → .items(), ensure_str() → str()).
  • user/edit_user_form.html — remove the dead "Regenerate API Key" button: CKAN 2.11 removed the user.generate_apikey route and user_generate_apikey auth function (API keys → API tokens), so the button raised BuildError.

CKAN 2.11 regressions (renamed hooks/routes silently broke features)

CKAN 2.10/2.11 renamed the controller hooks (confirmed in interfaces.py): IResourceController → before/after_resource_create|update|delete (no after_create); IPackageController → after_dataset_create|update|delete (no after_update). Core never calls the old names, so the extension's old-named hooks were dead.

  • Routes (validate_user_affiliation.py, user/dashboard.html) — dashboard.index → activity.dashboard. The dashboard news-feed view moved to the activity plugin in 2.11 (the dashboard blueprint now only has /datasets, /groups, /organizations; activity registers /dashboard/). The stale route name raised BuildError, breaking the post-login affiliation redirect and the dashboard nav tab.
  • Dataset-transfer emails (plugin.py) — after_update → after_dataset_update, restoring the transfer-request notification email (silently dead since the rename). Reads org_to_allow_transfer_to top-level (scheming) with an extras fallback. Verified end-to-end (target org admins receive the email); covered by a new regression test.
  • Bulk-upload validation (plugin.py, logic.py) — after_create → after_resource_create, which now triggers resource_validation_run for a stored resource with a schema + uploaded file + supported format. The bulk uploader creates resources with only name+resource_type; schema/format are applied as scheming defaults after ckanext-validation's can_validate gate runs, so validation was skipped — triggering it once the resource is stored fixes it. Separately, validate_resource_upload_fields no longer treats the standard CKAN size field as a giftless upload field (only sha256+lfs_prefix must be present-together and valid); the old size coupling made ckanext-validation's validation_status patch fail with "Invalid blob storage upload fields", silently blocking validation on the create path.

Bootstrap 5 theme regressions

  • Restore the .hidden utility that BS5 dropped (un-hides autogenerated schema fields — title/url/tags/license/visibility — across all dataset forms); hide the validation "View Error Report" link for valid resources (d-none).
  • Match production masthead nav link colour + hover/active styling.
  • Remove the duplicate "Activity Stream" tab (the activity plugin already adds it via super()); stop listing every page in the top navbar (build_pages_nav_main).

Tests

  • Removed the test for the deleted json_response_omitting_challenge_decider (suite count −6 cases).
  • Added test_after_dataset_update_sends_transfer_email — regression guard for the transfer hook (fails on the old after_update, passes on the fix).
  • Fixed test_send_dataset_transfer_emails_errors — CKAN 2.11 factories auto-make the org creator an admin with an email, so the "no recipient" case was impossible; reworked to a genuine no-eligible-recipient case.
  • Aligned test_validate_resource_upload_fields parametrized cases with the size decoupling.

Verification

Full ckanext-unaids suite green (155 passed, 1 skipped); flake8 clean. Transfer-email and bulk-upload validation verified on the running deploy. Hook/route renames confirmed against CKAN 2.11 source.

Stacking

Targets chore/ckan-211-pr8-activity-routes (#330) — tip of the migration chain.

Copilot AI review requested due to automatic review settings May 21, 2026 09:10

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Removes dead code and Python 2/six leftovers that became unused as part of the CKAN 2.11 migration work, with the goal of reducing maintenance surface without changing runtime behavior.

Changes:

  • Removed the unused json_response_omitting_challenge_decider helper and its unit test (no remaining references found).
  • Removed a write-only g.current_user = payload assignment from token validation (no remaining reads of g.current_user found).
  • Dropped remaining six usage in blueprints (ensure_str, six.iteritems) in favor of Python 3 equivalents.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
ckanext/unaids/auth_logic.py Deletes orphaned challenge-decider helper and removes unused g.current_user assignment.
ckanext/unaids/tests/test_auth_logic.py Removes the unit test that only exercised the deleted helper.
ckanext/unaids/blueprints/user_info_blueprint.py Removes six.ensure_str import and uses str() for the DOM attribute value.
ckanext/unaids/blueprints/svg_map_options.py Removes six import and replaces six.iteritems() with dict.items().

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@A-Souhei A-Souhei changed the title chore (8/8): post-migration cleanup — remove dead code chore (8/8): post-migration cleanup + CKAN 2.11/BS5 regression fixes May 21, 2026
@A-Souhei
A-Souhei marked this pull request as draft May 21, 2026 17:54
@A-Souhei A-Souhei changed the title chore (8/8): post-migration cleanup + CKAN 2.11/BS5 regression fixes chore (8/8): CKAN 2.11 post-migration cleanup & regression fixes May 22, 2026
@A-Souhei
A-Souhei force-pushed the chore/ckan-211-post-migration-cleanup branch from 18df2cb to bdc0dc7 Compare May 22, 2026 08:45
@A-Souhei
A-Souhei requested a review from Copilot May 22, 2026 08:48

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated 4 comments.

Comments suppressed due to low confidence (1)

ckanext/unaids/helpers.py:20

  • log is assigned twice in a row (logging.getLogger() then immediately logging.getLogger(__name__)), so the first assignment is dead code. Remove the unused assignment to avoid confusion and ensure the module logger name is consistently used.
log = logging.getLogger()
BULK_FILE_UPLOADER_DEFAULT_FIELDS = 'ckanext.bulk_file_uploader_default_fields'

log = logging.getLogger(__name__)

Comment thread ckanext/unaids/theme/templates/package/read_base.html
Comment thread ckanext/unaids/theme/templates/organization/read_base.html
Comment thread ckanext/unaids/logic.py
Comment thread ckanext/unaids/assets/custom.css
@A-Souhei
A-Souhei marked this pull request as ready for review May 22, 2026 09:38
@A-Souhei
A-Souhei requested a review from ChasNelson1990 May 22, 2026 09:38

@ChasNelson1990 ChasNelson1990 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - go! Go! Go!

@A-Souhei
A-Souhei force-pushed the chore/ckan-211-pr8-activity-routes branch from be23add to 7f5261a Compare May 26, 2026 14:47
Base automatically changed from chore/ckan-211-pr8-activity-routes to development May 26, 2026 15:01
A-Souhei added 13 commits May 26, 2026 18:03
Small, safe dead-code removals noted during the CKAN 2.11 migration
review (no behaviour change):

- auth_logic.py: drop the orphaned `json_response_omitting_challenge_decider`
  function (its only caller, the repoze.who `directlyProvides` registration,
  was removed in the runtime-fixes PR when repoze.who left CKAN 2.11) and
  its now-pointless unit test.
- auth_logic.py: drop the write-only `g.current_user = payload` assignment
  (nothing reads `g.current_user`; the authenticated identity is set via
  `g.userobj` / `g.user`). Confirmed write-only by security review.
- user_info_blueprint.py: drop `from six import ensure_str`; use `str()`.
- svg_map_options.py: drop `import six`; use `dict.items()` instead of
  `six.iteritems()`. `six` is no longer used anywhere in the extension.

flake8 clean. Suite: 154 passed / 1 skipped (the -6 vs prior runs is the
removed test's 6 parametrized cases; no failures).
Bootstrap 5 dropped the .hidden utility that BS3 provided; our scheming schemas use classes:["hidden"] on autogenerated fields, so restore it. Also point the masthead nav-link colour rules at the BS5 markup (.main-navbar ul li a) so links aren't invisible (white-on-white) when not active.
…ages from top navbar

Use BS5 d-none (not the removed BS3 .hidden) so the 'View Error Report' link is hidden for successful validations. build_pages_nav_main no longer dumps every page into the top navbar (removes duplicate About and clutter); pages remain reachable via homepage, footer and direct URLs.
The activity plugin adds the Activity Stream tab via super() in its read_base templates, so the explicit build_nav_icon call here produced a duplicate.
CKAN 2.11 removed legacy API keys along with the user.generate_apikey route and user_generate_apikey auth function (replaced by API tokens). The button was carried over from the 2.9 default form and raised 'Authorization function not found' on the user edit page.
Bulk-uploaded resources are created with only name+resource_type; schema/format are applied as scheming defaults AFTER ckanext-validation's can_validate gate, so validation never enqueued (individual upload works because its form carries the fields). Add an after_resource_create hook (the CKAN 2.9 after_create, renamed for 2.11) that triggers validation once the resource is stored with its schema. Also stop validate_resource_upload_fields from coupling the standard 'size' field with the giftless sha256/lfs_prefix fields, which made validation-status patches fail.
CKAN 2.11 moved the dashboard news-feed view to the activity plugin; the dashboard blueprint no longer has an 'index' endpoint, so url_for/redirect_to/build_nav_icon with 'dashboard.index' raise BuildError (breaking the affiliation-redirect/login flow and the user dashboard nav). Point them at activity.dashboard.
CKAN 2.11 renamed IPackageController.after_update -> after_dataset_update, so the old after_update hook was never called and dataset-transfer-request emails silently stopped. Rename the hook and read org_to_allow_transfer_to from the top-level scheming field (with an extras fallback) so the email actually fires.
Asserts that setting org_to_allow_transfer_to on a dataset update fires send_dataset_transfer_emails (and that plain creation does not). Guards the CKAN 2.11 after_update -> after_dataset_update regression that silently disabled transfer-request emails.
test_send_dataset_transfer_emails_errors assumed the recipient org had no emailable admin, but in CKAN 2.11 the org-creating user is an admin and factories.User() auto-assigns an email, so an email was always sent and the assertion failed. Explicitly null the recipient admin's email to construct a genuine no-eligible-recipient case.
The earlier BS5 fix made nav links brand-red and left vanilla's dark-teal + white active state. Production (CKAN 2.9) uses dark-grey links with a light-grey background and red text when active/hovered. Update the .main-navbar rules to match.
…coupling

validate_resource_upload_fields no longer treats the standard 'size' field as a blob-storage upload field (only sha256+lfs_prefix matter). Update the parametrized cases that asserted size made the upload invalid; they now correctly validate. Also point the invalid-sha256 case at a valid lfs_prefix so it actually exercises sha256 validation.
html_escape was only used by the build_pages_nav_main page loop and the build_nav_icon overrides, both removed/reverted in this branch.
@A-Souhei
A-Souhei force-pushed the chore/ckan-211-post-migration-cleanup branch from bdc0dc7 to 5d383d0 Compare May 26, 2026 15:06
@A-Souhei
A-Souhei merged commit 69b89fc into development May 26, 2026
3 checks passed
@A-Souhei
A-Souhei deleted the chore/ckan-211-post-migration-cleanup branch May 26, 2026 15:20
cooper667 added a commit that referenced this pull request Jul 9, 2026
The IAuthenticator.identify() hook only set g.userobj/g.user directly,
but CKAN's API action dispatcher (ckan/views/api.py) builds its context
from Flask-Login's current_user, not g.user/g.userobj -- and
identify_user() unconditionally re-derives g.user/g.userobj from
current_user right after every IAuthenticator.identify() call, silently
overwriting what this plugin set. Net effect: Bearer-token requests
authenticated successfully (no error) but ran as the anonymous user,
so e.g. organization_list_for_user returned an empty list instead of
the caller's actual orgs.

This is a real regression from the CKAN 2.11 migration: the original
pre-2.11 code had a g.current_user = payload line, which the migration's
post-migration cleanup (#331) removed as 'confirmed write-only' -- true,
but the actual fix needed was calling Flask-Login's login_user(), the
same pattern ckanext-saml2auth already uses for CKAN 2.10+.
cooper667 added a commit that referenced this pull request Jul 9, 2026
* Turn unhandled OAuth2 JWKS-fetch errors into proper 401s

If ckanext.unaids.auth0_domain/oauth2_api_audience are unset, or the
JWKS fetch fails for any reason (network issue, Auth0 outage), this
previously raised an uncaught TypeError/URLError that fell through to
a generic 500 Internal Server Error, since only jwt.decode() itself
was wrapped in error handling. Bearer-token requests now get a clean
401 with a useful message in both cases instead.

* Authenticate Flask-Login's current_user for OAuth2 Bearer requests

The IAuthenticator.identify() hook only set g.userobj/g.user directly,
but CKAN's API action dispatcher (ckan/views/api.py) builds its context
from Flask-Login's current_user, not g.user/g.userobj -- and
identify_user() unconditionally re-derives g.user/g.userobj from
current_user right after every IAuthenticator.identify() call, silently
overwriting what this plugin set. Net effect: Bearer-token requests
authenticated successfully (no error) but ran as the anonymous user,
so e.g. organization_list_for_user returned an empty list instead of
the caller's actual orgs.

This is a real regression from the CKAN 2.11 migration: the original
pre-2.11 code had a g.current_user = payload line, which the migration's
post-migration cleanup (#331) removed as 'confirmed write-only' -- true,
but the actual fix needed was calling Flask-Login's login_user(), the
same pattern ckanext-saml2auth already uses for CKAN 2.10+.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants