Skip to content

feat(LAB-4574): remove notification methods from the SDK - #2063

Draft
RuellePaul wants to merge 1 commit into
mainfrom
feature/lab-4574-aakd-i-dont-see-notifications-methods-on-the-sdk
Draft

feat(LAB-4574): remove notification methods from the SDK#2063
RuellePaul wants to merge 1 commit into
mainfrom
feature/lab-4574-aakd-i-dont-see-notifications-methods-on-the-sdk

Conversation

@RuellePaul

Copy link
Copy Markdown
Contributor

Purpose: Remove all four notification methods from the SDK's public surface. Clean break in 26.3.0 — no deprecation window.

Paired MR: kili !14393must merge AFTER this PR (see merge order below).

Plan document: https://linear.app/kili-technology/document/lab-4574-aakd-i-dont-see-notifications-methods-on-the-sdk-e5786e4e3ed4


⚠️ The ticket's premise is wrong, and it changes the backend half

The ticket assumes all four operations are SDK-only cruft "not attached to any meaningful use case". A trace across the monorepo and every sibling service repo shows otherwise:

Operation Callers Backend verdict
notifications the TopBar notification bell plus this SDK's own gateway KEEP — permanently
countNotifications the same bell query, plus one issue per SDK poll attempt KEEP — permanently
updatePropertiesInNotification the bell, plus the export and import services via raw GraphQL, not SDK calls KEEP
createNotification none anywhere except this SDK REMOVE

Removing a Python wrapper never breaks the backend, so all four wrappers go here regardless. But only createNotification corresponds to a backend field disappearing — which is why merge order matters.

What this removes

Deleted: entrypoints/mutations/notification/ (whole package), presentation/client/notification.py, use_cases/notification/, tests/integration/presentation/test_notification.py, docs/sdk/notification.md.

Modified: client.py (both base classes dropped), services/asset_import/base.py (rewired), the gateway's count_notification method, NotificationFilter trimmed to id, core/enums.py (NotificationStatus removed), mkdocs.yml nav entry.

The docs page and nav entry are what actually satisfy the ticket title, which is about discoverability.

THE count_notification QUESTION — verified, not assumed, and the answer splits in two

  • The gateway method count_notification had no caller once the public methods went → deleted.
  • The query constant GQL_COUNT_NOTIFICATIONS is required and keptexecute_query_from_paginated_call invokes get_number_of_elements_to_query unconditionally whenever a count query exists.

Proven by running the poll against a real KiliAPIGateway with only the transport faked, recording every operation sent:

countNotifications  variables={'where': {'id': 'notif-123'}}
notifications       variables={'where': {'id': 'notif-123'}, 'skip': 0, 'first': 1}

Both fire per poll attempt. Guessing either way would have been wrong.

GraphQL operations this SDK still issues — for the backend's deletion decision

STILL ISSUED (must NOT be retired): notifications (where: {id}, selecting status only) · countNotifications (where: {id}, once per poll attempt)

NO LONGER ISSUED: createNotification · updatePropertiesInNotification · the hasBeenSeen and user fields of NotificationWhere

⚠️ Merge order

This PR ships BEFORE the backend deletes createNotification. If the backend field goes first, any SDK version still calling it gets a client-side GraphQL validation error instead of today's accessDenied. Both are failures so nothing working regresses, but SDK-first keeps the window clean.

Post-review fixes (agent self-review)

  • [REQUIRED] No test exercised the body of verify_batch_imported — the existing tests patch the method out or mock the notification call wholesale, so a wrong filter, field set or options would still have passed. With no deprecation window that gap was unacceptable → added test_verify_batch_imported.py, which drives the real gateway and asserts the exact operations and variables.
  • The new tests were then mutation-tested rather than trusted: fields=("status","id") → caught; NotificationFilter(id=None) → caught; first=Nonenot caught, and correctly so, because with a single-id lookup the count is 1 so pagination clamps the page size either way and the wire payload is identical. first=1 is explicitness, not behaviour. Coverage is not claimed where it does not exist.
  • [checked] IndexError semantics preserved exactly — notifications(...)[0] on a materialised list becomes list(gen)[0]; an empty result raises identically, and IndexError is not in the retry predicate in either version.
  • [SUGGESTION, not acted on] The retry loop has no stop condition — a permanently PENDING notification polls forever with a 16s cap. Pre-existing and unchanged by this diff; noted because it was found while writing tests, and it is why no PENDING-path test was added (it would hang).

Verification

Baseline captured from a pristine tree before the first edit, compared as normalised sets:

Gate Baseline (pristine origin/main) After
pytest 749 passed, 1 skipped 751 passed, 1 skipped
pylint src/kili 10.00/10 10.00/10
pyright src/kili 0 errors 0 errors, 0 new diagnostics
pre-commit (changed files) all hooks pass
mkdocs build --strict clean, no dangling links

Collected-test set diff is exactly the deliberate deletion plus the 3 new tests — nothing silently vanished. Runtime surface confirmed by hasattr checks: all four methods absent, NotificationId/NotificationFilter still importable, gateway list_notifications present.

⚠️ Pre-existing CI failure, not caused by this PR

The coverage gate (--cov-fail-under=75) already fails on pristine origin/main at 74.83% — verified by stashing the work, running CI's exact scope on the untouched tree, then confirming the restored diff was byte-identical to a backup patch. This change moves it to 74.86%, still under the gate. The coverage job will be red here, and it was already red on main.

⚠️ Acceptance criterion 1 (Datadog) is unmet and human-owned

It is meaningful for createNotification — no first-party caller exists in any repo, so any production hit is a real external caller and a genuine blocker.

It is NOT answerable for notifications/countNotifications, and whoever runs it must be briefed or they will read the dashboard backwards. Three confounds stack: the bell polls both; this SDK's own import verification polls notifications from inside append_many_to_dataset; and the kili-client-method-name header comes from a singleton dict mutated on every decorated call, so the internal poll overwrites it. A non-zero count there is not evidence against removal — and both operations are staying regardless.

🤖 Planned and implemented by Claude Code.

The SDK exposed four notification methods that no customer workflow needs:
`notifications` and `count_notifications` (documented, list/count a user's
notifications) and `create_notification` /
`update_properties_in_notification` (undocumented, Kili-admin only).

All four are removed. This is a breaking change to the public client and
lands in 26.3.0 with no deprecation window, taken knowingly: the backend
team confirmed `createNotification` has no caller outside this repo, and
the domain API (`kili.client_domain.Kili`) never exposed notifications.

`notifications` was not dead code — `BaseBatchImporter.verify_batch_imported`
polled it to confirm asynchronous imports (video, tiled and geospatial
imagery). That poll now reads through `kili_api_gateway.list_notifications`
directly, matching the three other direct gateway calls in the same file. It
requests only `status` instead of six fields and disables the progress bar
that was redrawn on every retry.

The SDK still issues the `notifications` and `countNotifications` GraphQL
queries for that poll, so neither may be retired backend-side. It no longer
issues `createNotification` or `updatePropertiesInNotification`.

Merge order: this repo ships before the backend deletes `createNotification`.
@RuellePaul RuellePaul changed the title [Claude] LAB-4574: AAKD, I don't see notifications methods on the SDK feat(LAB-4574): remove notification methods from the SDK Aug 20, 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.

2 participants