Skip to content

fix: fetch single event by uid directly (#137, #138) - #236

Open
Olen wants to merge 17 commits into
mainfrom
fix/get-event-direct-fetch
Open

fix: fetch single event by uid directly (#137, #138)#236
Olen wants to merge 17 commits into
mainfrom
fix/get-event-direct-fetch

Conversation

@Olen

@Olen Olen commented May 14, 2026

Copy link
Copy Markdown
Owner

Summary

Closes #137 and #138 with a single root-cause fix.

get_event(uid) previously routed through _get_entity(), which fetched a list via get_events() (with no args) and searched it. That list inherits get_events()'s defaults:

Switch _get_entity() for events to fetch from the singular sponds/{uid}?includeComments=true endpoint directly. Neither limit applies. Group lookups stay on the cache+search path — they don't have these defaults.

Live verification against the Spond API

Probed the endpoint before coding (and re-verified the fixed code) using credentials from ../spond-kalender:

[real id]                     status=200  ok=True   → event JSON dict
[real id + includeComments]   status=200  ok=True   → adds `comments` field
[bogus id]                    status=404  ok=False  → {"message":"sponds/...","errorCode":404}

OK: get_event returned id=455D0EEA..., heading='Fotballtrening 9-er'
OK: bogus id raises KeyError: "No event with id='BOGUS...'."

After adding ?includeComments=true, the singular response shape matches the list-endpoint element exactly: 29 keys vs 29 keys, set difference is empty. get_event() output is a drop-in replacement for [e for e in get_events() if e["id"]==uid][0].

Why ?includeComments=true instead of a second API call

Initially the plan was two calls (singular + /sponds/{uid}/comments) and merging. Probed three options first:

  • ?includeComments=true ✅ — returns comments inline, one call, exact list-endpoint shape
  • /sponds/{uid}/comments ✅ — works as a separate sub-resource, but requires merging
  • /sponds/{uid}/posts ❌ — 404

One call with a query param is strictly cleaner.

Incidental fix

update_event() also calls _get_entity(EVENT, ...), so it had the same latent bug for events outside the cached window. Fixed automatically by this refactor.

Stacked behind #235

Branched off fix/get-entity-handles-empty-results. Merge #235 first, then this. (PR base retargeted to main so CI fires; once #235 merges, the diff cleans up automatically.)

Test plan

  • Live probe: real id returns event, ?includeComments=true adds the field, bogus id returns 404
  • Live test of fixed code: get_event(real_id) works, returns 29-key dict matching list-endpoint shape, get_event(bogus_id) raises KeyError
  • Local pytest — 22 tests pass
  • Local ruff check — clean
  • Local ruff format --check — clean
  • CI: full Python matrix green
  • Suggested: rerun examples/manual_test_functions.py end-to-end before merge

🤖 Generated with Claude Code

…rs (#137, #138)

`get_event()` previously routed through `_get_entity()`, which called
`get_events()` (with no args) to populate a cache and then searched
that cache. This had two unrelated-looking-but-shared-root-cause bugs:

- Events past the `max_events=100` default were unreachable (#137).
- Scheduled events (where `include_scheduled=False` is the default)
  were unreachable (#138).

Switch `_get_entity()` for events to fetch directly from the singular
`sponds/{uid}` endpoint, which has neither limit. Verified live
against the Spond API: returns 200 + event JSON for real ids, 404
for unknown ids. Map 404→KeyError to preserve the existing
exception contract.

Group lookups are unchanged — `get_groups()` has no max/filter
defaults, and the cache+search behaviour is still useful there.

Notes:
- The singular endpoint omits the `comments` field that the list
  endpoint returns; this field is not in `_event_template` and
  nothing internal reads it. Callers that need comments should
  fetch them separately.
- `update_event()` also calls `_get_entity(EVENT, ...)`; this
  change incidentally fixes a latent same-class bug there too.
@Olen
Olen changed the base branch from fix/get-entity-handles-empty-results to main May 14, 2026 12:59
@Olen Olen closed this May 14, 2026
@Olen Olen reopened this May 14, 2026
Pass `?includeComments=true` so the singular endpoint returns the
`comments` field that the list endpoint includes by default.

Verified live: response shape now matches the list endpoint exactly
(29 keys vs 29 keys, set difference is empty). Keeps `get_event()`
output a drop-in replacement for `[e for e in get_events() if ...][0]`.
@Olen
Olen requested a review from Copilot May 14, 2026 13:02
@Olen Olen added the updateme Automatically update PR from main label May 14, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Fixes Spond.get_event(uid) so it can return events that are beyond the default max_events=100 window or marked as scheduled. Previously _get_entity() for events routed through get_events() (with default filters), making such events unreachable. The fix replaces that path with a direct call to the singular sponds/{uid} endpoint and surfaces 404s as KeyError to preserve existing semantics. update_event() benefits from the same fix transparently.

Changes:

  • In _get_entity(), route event lookups to a new _fetch_event_by_uid() helper that calls GET sponds/{uid} (with includeComments=true to keep response shape close to list elements); group lookups remain on the cached path.
  • Add a KeyError guard for the empty-cache group case (the dependency from stacked PR #235).
  • Replace the cache-based event tests with HTTP-mocked tests covering 200/404/500 paths and add a group-empty KeyError test; update the manual example to exercise the new path.

Reviewed changes

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

File Description
spond/spond.py Routes _get_entity for events through new _fetch_event_by_uid (singular endpoint, 404→KeyError, other errors→ValueError); keeps group cache path with empty-cache KeyError guard.
tests/test_spond.py Replaces cached-list event tests with HTTP-mocked happy-path/404/500 tests; adds empty-groups KeyError test.
examples/manual_test_functions.py Adds a get_event(events[0]["id"]) step and reuses e for the existing attendance export.

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

@Olen
Olen requested a review from elliot-100 May 14, 2026 13:11
Olen added a commit that referenced this pull request May 14, 2026
Four fixes raised in Copilot's review of #241:

- Spond class example: guard `await s.get_groups()` with `or []` so
  iterating the result is safe when the account has no groups (the
  return type is documented as `list[JSONDict] | None`).
- `get_events()` docstring: removed the forward-looking claim that
  `get_event(uid)` "queries the singular endpoint directly" — that's
  only true after #236 lands. Replaced with current behavior plus
  reference to #236 as the planned change.
- `get_events()` `subgroup_id` parameter: docstring said `subgroupId`
  API parameter, but the code sends `subGroupId` (capital G, line
  387). Aligned the docstring with the actual API param.
- `update_event()` Returns section: was declared `JSONDict` but the
  method (due to known bug #239) currently returns `self.events`, a
  `list[JSONDict] | None`. Updated the Returns type to match the
  current contract, with the bug and workaround called out in Notes.
This test was written against a `_get_entity()` that resolved events from
the in-memory cache, so seeding `s.events` was sufficient isolation and no
HTTP request occurred. This branch moves that lookup onto the singular
endpoint, so the seeding no longer intercepts anything and the request
escaped to the live API, failing CI with a 401 from api.spond.com.

Neither change conflicts textually, which is why the branch reported
mergeable while its tests failed: the test arrived on main after this
branch's last green run, and the break only surfaced when main was merged
back in.

Mock `ClientSession.get` alongside the existing `post` mock. The seeded
`s.events` is kept — it is no longer plumbing for `_get_entity()`, but it
remains the decoy that the `result is not s.events` assertion checks
against, so the #239 regression guard still means something.

Verified with sockets disabled (`--disable-socket --allow-unix-socket`):
29 passed, confirming the request is genuinely mocked rather than merely
succeeding against the network.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@Olen

Olen commented Aug 15, 2026

Copy link
Copy Markdown
Owner Author

CI is green again as of 60f27a8. Recording why it went red, since nothing in this PR changed.

test_update_event__returns_api_response arrived on main in e33ddcb, after this branch's last green run on 14 May. It patched ClientSession.post and seeded s.events — and that seeding was the isolation, because _get_entity() resolved events from the in-memory cache without any HTTP request.

This branch moves that lookup to the singular sponds/{uid} endpoint. Composed with the new test, the seeded cache no longer intercepts anything, the GET went unmocked, and the request escaped to the live API:

ValueError: Request failed with status 401: {"message":"User is not logged in.","errorKey":"notLoggedIn"}

A semantic merge conflict: neither change conflicts textually, which is why the PR reported MERGEABLE while its tests failed. It only surfaced today because merging main back in ran CI for the first time in three months.

The fix mocks ClientSession.get alongside the existing post mock. The seeded s.events is kept deliberately — it is no longer plumbing for _get_entity(), but it remains the decoy that assert result is not s.events checks against, so the #239 regression guard still means something. The comment above it was updated to say so.

Verified with sockets disabled (--disable-socket --allow-unix-socket): 29 passed, confirming the request is genuinely mocked rather than merely succeeding against the network. #256 proposes making that guard the default for the suite.

Note the checklist item "rerun examples/manual_test_functions.py end-to-end" is still unticked, and this PR's behaviour is exercised against the live API — worth doing before merge. #246 also touches this area, so the two may want sequencing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

updateme Automatically update PR from main

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Spond.get_event() may not return an event if more than 100 events are available

2 participants