Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a JSON API endpoint for accessing account events, allowing API clients to consume activity feeds with filtering and pagination support. The implementation includes event scoping, particulars normalization, comprehensive tests, and API documentation.
Changes:
- Add JSON response format to EventsController#index with filtering by creator and board
- Implement scopes for event filtering and preloading required associations
- Create JSON serializers for events with top-level board/creator and normalized action-specific metadata
- Add comprehensive controller tests covering response shape, filtering, pagination, and access scoping
- Document the new API endpoint with query parameters, supported actions, and response examples
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| app/controllers/events_controller.rb | Added JSON response format, API_ACTIONS whitelist, and api_events filtering method |
| app/models/event.rb | Added reverse_chronologically, for_creators, for_boards scopes for API filtering |
| app/models/event/particulars.rb | Added api_particulars method to normalize action-specific metadata |
| app/views/events/index.json.jbuilder | JSON array renderer for paginated events |
| app/views/events/_event.json.jbuilder | Event JSON partial with board, creator, and eventable serialization |
| docs/API.md | Complete API documentation with query parameters, supported actions, and examples |
| test/controllers/events_controller_test.rb | Comprehensive tests for response format, filtering, pagination, and access control |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
app/models/event.rb:24
- The Event.preloaded scope doesn't explicitly include the creator for Comment eventables. When the event partial renders a Comment, it accesses
comment.creator(in the comment.json.jbuilder partial), which will trigger an N+1 query issue. Consider updating the preload to include the comment's creator and card associations.
scope :preloaded, -> {
includes(:creator, :board, {
eventable: [
:goldness, :closure, :image_attachment,
{ rich_text_body: :embeds_attachments },
{ rich_text_description: :embeds_attachments },
{ card: [ :goldness, :closure, :image_attachment ] }
]
})
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| creator: users(:david), | ||
| eventable: card, | ||
| account: accounts("37s"), | ||
| particulars: { particulars: { old_board: "Backlog", new_board: "Mobile" } } |
There was a problem hiding this comment.
Just an aside -- I was not aware we have been doing this nested particulars thing, and now my OCD is kicking in. 😮💨
There was a problem hiding this comment.
Yeah thats not elegant. I'll see how big of an issue it is and fix either in this or subsequent PR.
There was a problem hiding this comment.
@flavorjones this would be pretty big push to fix (would need a migration to over all the events recorded so far). Not in scope for this PR.
flavorjones
left a comment
There was a problem hiding this comment.
This is good and we can probably ship it. But I left some comments, particularly around whether we should make a new controller for this. Would love your thoughts?
app/controllers/events_controller.rb
Outdated
|
|
||
| def index | ||
| fresh_when @day_timeline | ||
| respond_to do |format| |
There was a problem hiding this comment.
The respond_to makes it explicit that HTML and JSON paths use different data sources ... but that feels like kind of a smell, too (like the DayTimelinesScoped module) which is making me wonder if this should be a separate controller.
There was a problem hiding this comment.
@flavorjones I think your instinct is right and I should have seen it - the behaviour and data were different and I was trying to squeeze them into the one controller. I have moved the JSON Api to its own Activities controller. It's sole purpose is to provide the json activity stream. The Events controller drives the HTML board activity view. Thoughts?
The events endpoint served both the HTML day timeline and the JSON API feed, but the two paths shared no data or behavior — the HTML side uses DayTimelinesScoped while the JSON side built its own query. Splitting into ActivitiesController gives the API its own home at GET /:account/activities.json without dragging in the timeline before_actions. Also preloads comment creator in Event.preloaded to avoid an N+1 when rendering comment eventables in the JSON feed.
Summary
This adds a JSON activities feed at
GET /:account/activities.jsonso API clients can consume account activity directly, filter by creator and board, and receive normalized event metadata.What changed
ActivitiesControllerwith a dedicated JSON index endpoint, separate from the HTMLEventsControllerEvent.preloadedto avoid N+1 queriesboard,creator, andeventablepayloadsparticularsfor assignment, board changes, title changes, and triage eventsGET /:account_slug/activitiesindocs/api/sections/activities.md