Skip to content

feat(broadcasts): Add status and limit query params to org broadcasts endpoint#110286

Closed
JonasBa wants to merge 11 commits intomasterfrom
jb/feat/broadcasts-status-limit
Closed

feat(broadcasts): Add status and limit query params to org broadcasts endpoint#110286
JonasBa wants to merge 11 commits intomasterfrom
jb/feat/broadcasts-status-limit

Conversation

@JonasBa
Copy link
Member

@JonasBa JonasBa commented Mar 10, 2026

Add two optional query parameters to the organization broadcasts endpoint:

  • status=all|seen|unseen — filters broadcasts by the requesting user's seen state. Defaults to all.
  • limit=n — limits the number of broadcasts returned, sorted by date_added descending.

… endpoint

Add two new optional query parameters to the organization broadcasts endpoint:

- status=all|seen|unseen: filter broadcasts by the user's seen state.
  Defaults to 'all', which returns all active broadcasts regardless of
  seen status.
- limit=n: guarantee at least n results. If the status filter yields
  fewer than n broadcasts, backfills from other active broadcasts and
  re-sorts the combined result by date_added descending.

This replaces the hardcoded MIN_BROADCASTS=3 backfill that always fired
and was breaking getsentry CI tests where _secondary_filtering returns
0 results for users who have seen everything.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Mar 10, 2026
JonasBa and others added 2 commits March 9, 2026 19:07
limit=n now slices the first n results from the already-ordered queryset
instead of backfilling when results are below the threshold. This keeps
the endpoint fully backwards compatible: callers that pass no params get
identical behaviour to before.

status and limit remain independent — status filters first, then limit
slices the ordered result.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Replace the Python-set approach (load all seen IDs, then exclude/filter
in Python) with direct queryset filtering via the broadcastseen reverse
relation. This keeps the filtering in the database and removes the
duplicate BroadcastSeen query that both branches shared.

Also replaces try/except KeyError for the missing-limit case with an
explicit None check, which is more idiomatic for optional query params.

The is_authenticated guard on status filtering is no longer needed:
an anonymous user has user.id=None, so broadcastseen__user_id=None
matches nothing, giving correct behaviour naturally.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
JonasBa and others added 3 commits March 9, 2026 19:12
Previously invalid status values and non-integer or negative limit values
were silently ignored, causing callers with typos or bad inputs to receive
a full result set with no indication of the error. Return a 400 with a
descriptive message instead.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
When no status filter is provided, annotate the queryset with a has_seen
boolean and order by it (unseen first) before applying any limit. This
ensures that if a caller requests limit=3 and there are both seen and
unseen broadcasts, the unseen ones are returned first rather than purely
the most recent by date.

When status=seen or status=unseen is set explicitly, all results share
the same seen state so the annotation is not needed.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ter is set

Reverts the has_seen annotation and order_by change. Old unseen BroadcastSeen
entries in the DB would bubble stale broadcasts to the top, making the
behaviour worse than the original date-based sort.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
request.user.id is None for anonymous users, which mypy correctly flags
as incompatible with the str | int expected by the ORM lookup. Guard each
status branch behind is_authenticated. For status=unseen an unauthenticated
user sees everything (nothing has been seen). For status=seen they get an
empty queryset.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Add support for explicit status=all parameter in organization broadcasts endpoint
- Update error message to include 'all' as a valid status value
- Add test case for explicit status=all parameter
- Update invalid status test to verify error message includes all valid values

Co-authored-by: Jonas <JonasBa@users.noreply.github.com>
queryset = queryset.none()

if organization:
status = request.GET.get("status")
Copy link
Member

Choose a reason for hiding this comment

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

It's unclear to me when organization would be None but is there a reason why the status filter only applies when organization is not None?


data = self._secondary_filtering(request, organization, queryset)

limit_param = request.GET.get("limit")
Copy link
Member

Choose a reason for hiding this comment

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

Same comment here.

…dpoint paths

The status filter was only applied inside the 'if organization:' branch, making
it a no-op for the non-org admin endpoint. Move the filter before the branch so
it applies consistently to both paths.

Also accept 'all' as an explicit status value (equivalent to omitting the param)
instead of returning 400, matching the documented API interface.

Co-Authored-By: Claude Sonnet 4 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4 <noreply@anthropic.com>
Comment on lines +118 to +128
queryset = queryset.exclude(broadcastseen__user_id=request.user.id)
elif status == "seen":
if request.user.is_authenticated:
queryset = queryset.filter(broadcastseen__user_id=request.user.id)
else:
queryset = queryset.none()
elif status is not None and status != "all":
return self.respond(
{"detail": "Invalid status. Valid values are 'seen', 'unseen', and 'all'."},
status=400,
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Bug: The status query parameter filtering is incorrectly applied to the non-organization admin endpoint (/api/0/broadcasts/), not just the intended organization-specific endpoint.
Severity: MEDIUM

Suggested Fix

Move the status parameter handling logic, including validation and filtering, inside the if organization: block. This will ensure it only executes for organization-specific requests and is ignored for the non-organization admin path.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: src/sentry/api/endpoints/broadcast_index.py#L115-L128

Potential issue: The `status` query parameter filtering logic is placed before the `if
organization:` check in the `BroadcastIndexEndpoint`. This causes the filter to be
applied to both the organization-specific endpoint (`/organizations/{slug}/broadcasts/`)
and the non-organization admin endpoint (`/api/0/broadcasts/`). The feature was intended
only for the organization endpoint, as user-perspective filtering ('seen'/'unseen') is
not appropriate for the system-wide admin view. This also causes invalid status values
to return 400 errors on the admin endpoint, which is unintended behavior.

Did we get this right? 👍 / 👎 to inform future reviews.

Copy link
Contributor

@cursor cursor bot left a comment

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 2 potential issues.

Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

return self.respond({"detail": "limit must be an integer."}, status=400)
if limit <= 0:
return self.respond({"detail": "limit must be a positive integer."}, status=400)
data = data[:limit]
Copy link
Contributor

Choose a reason for hiding this comment

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

limit and status have inconsistent organization-scoping behavior

Medium Severity

The limit parameter is only processed inside the if organization: block, so it is silently ignored when organization is None. Meanwhile, the status filter is applied regardless of whether organization is set. Both parameters were introduced together for the same endpoint, so having limit silently ignored on the non-organization code path (while status still applies) creates an inconsistency. As the reviewer noted, it's unclear why limit is scoped differently than status.

Additional Locations (1)
Fix in Cursor Fix in Web

…iltering

The endpoint already requires authentication, so these guards are unnecessary.

Co-Authored-By: Claude Sonnet 4 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Scope: Backend Automatically applied to PRs that change backend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants