Skip to content

Review fixes for the Affiliation Catalogs plugin#1

Closed
moliholy wants to merge 8 commits into
affiliation-schemasfrom
affiliation-schemas-review-fixes
Closed

Review fixes for the Affiliation Catalogs plugin#1
moliholy wants to merge 8 commits into
affiliation-schemasfrom
affiliation-schemas-review-fixes

Conversation

@moliholy

@moliholy moliholy commented Jun 9, 2026

Copy link
Copy Markdown
Owner

This PR applies the fixes from a review of indico#6 (Affiliation Catalogs). It targets the PR branch (affiliation-schemas) so the diff is only the fixes.

What this fixes

Correctness

  • Editing a registration after the catalog config changed (list disabled or removed, default catalog switched) no longer fails. The representation field skips re-validation when its stored value is unchanged, so unrelated edits to a registration keep working.
  • Duplicate affiliation list names within a catalog are now rejected (server and client), matching the contact-list behavior.

Performance

  • Inherited default-catalog resolution no longer runs get_all_catalogs once per ancestor category; it resolves the whole chain in a single query. This path runs on every registration form render.

Client

  • The "See affiliations" modal no longer crashes when the resolve request fails or returns nothing.
  • Unsaved catalog list rows now use stable keys, so reordering or deleting rows before saving no longer attaches input state to the wrong row.
  • Fixed the "invalid contact emails" warning, which was shown whenever any affiliation existed.
  • Removed a dead dashboard/types.ts module whose only import pointed at a nonexistent path.

Robustness and polish

  • models/__init__.py now imports every model, so importing the package registers all SQLAlchemy mappers regardless of import order.
  • User-facing validation messages in the representation field and catalog code are wrapped in _() for translation.
  • Added a conftest.py that mocks the plugin webpack manifest so management pages render in tests. This is the fix for the failing test-plugin (affiliation_extras) CI job (assets are not built in CI).

Verification

Note: the base here is affiliation-schemas, not master, so the repo's CI (which only runs on PRs targeting master/*.x) will not trigger automatically. Everything above was verified locally.

Deliberately not changed (need a decision)

  • 403-vs-404 on cross-scope catalog ids: returning 403 is locked in by an existing test; treating out-of-scope as 404 is a design call.
  • Regform managers can resolve global user emails by affiliation, while the sibling invite endpoints are admin-only. Confirm the intended trust level.
  • Cloning an inherited catalog copies parent-defined group/tag/affiliation membership into the child. Confirm intent.
  • The contact-list functional unique-index name mismatch is pre-existing (from Add Extended Affiliations plugin indico/indico-plugins-contrib#2's migration chain), not introduced here.

@moliholy moliholy self-assigned this Jun 10, 2026
@moliholy moliholy closed this Jun 10, 2026
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.

1 participant