add timestamp to feedback status updates #488
Conversation
Signed-off-by: Jordan Dubrick <[email protected]>
WalkthroughAdds "match" to JsonPathOperator in OpenAPI. Extends feedback status update response to include a timestamp across implementation, docs, and tests. The endpoint computes the timestamp at update time. Examples and tests are updated to expect the new field. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant API as Feedback API
participant Store as Storage
Client->>API: PATCH /feedback/{id}/status {updated_status}
API->>Store: Read current status
Store-->>API: previous_status
rect rgba(200,240,255,0.3)
note right of API: Compute current UTC timestamp
API->>Store: Persist updated_status (+ timestamp)
Store-->>API: Ack
end
API-->>Client: 200 OK {status: {previous_status, updated_status, updated_by, timestamp}}
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
src/app/endpoints/feedback.py (1)
211-219: Emit RFC3339/ISO-8601 UTC timestamp (with Z) instead of str(datetime).str(datetime.now(UTC)) yields a space separator and microseconds; examples elsewhere tend to use ISO. Recommend normalized, second-precision RFC3339 to ease client parsing and keep docs/tests consistent.
- current_time = str(datetime.now(UTC)) + # RFC3339 UTC, second precision, Z-suffixed + current_time = ( + datetime.now(UTC) + .replace(microsecond=0) + .isoformat() + .replace("+00:00", "Z") + )Follow-up: if accepted, mirror the same formatting in store_feedback() for consistency.
src/models/responses.py (3)
591-599: Fix example booleans and timestamp format in docstring.Use Python booleans and ISO 8601 UTC to match the intended payload.
- status_response = StatusResponse( + status_response = StatusResponse( status={ - "previous_status": true, - "updated_status": false, + "previous_status": True, + "updated_status": False, "updated_by": "user/test", - "timestamp": "2023-03-15 12:34:56" + "timestamp": "2023-03-15T12:34:56Z" }, )
610-615: Keep example timestamp consistent with RFC3339 (Zulu).- "timestamp": "2023-03-15 12:34:56", + "timestamp": "2023-03-15T12:34:56Z",
603-603: Consider strongly typing the status payload.Defining a dedicated model improves validation and OpenAPI fidelity (lets you mark timestamp as date-time). Optional, since this is additive.
+class FeedbackStatusPayload(BaseModel): + previous_status: bool + updated_status: bool + updated_by: str + # Use string to match emitted RFC3339 "Z" format + timestamp: str = Field( + description="RFC3339 UTC timestamp", + examples=["2023-03-15T12:34:56Z"], + ) + class FeedbackStatusUpdateResponse(BaseModel): @@ - status: dict + status: FeedbackStatusPayloaddocs/openapi.json (1)
1537-1547: Schema for FeedbackStatusUpdateResponse.status is too permissive; define fields and timestamp format.Current schema allows any shape. Consider declaring explicit properties and format for better contract clarity.
- "FeedbackStatusUpdateResponse": { - "properties": { - "status": { - "additionalProperties": true, - "type": "object", - "title": "Status" - } - }, + "FeedbackStatusUpdateResponse": { + "properties": { + "status": { + "type": "object", + "title": "Status", + "additionalProperties": false, + "required": [ + "previous_status", + "updated_status", + "updated_by", + "timestamp" + ], + "properties": { + "previous_status": { "type": "boolean" }, + "updated_status": { "type": "boolean" }, + "updated_by": { "type": "string" }, + "timestamp": { "type": "string", "format": "date-time" } + } + } + }, @@ - "examples": [ + "examples": [ { "status": { "previous_status": true, - "timestamp": "2023-03-15 12:34:56", + "timestamp": "2023-03-15T12:34:56Z", "updated_by": "user/test", "updated_status": false } } ]tests/unit/app/endpoints/test_feedback.py (2)
200-214: LGTM; optionally assert timestamp shape (ISO 8601 UTC).Current ANY check is fine. If you adopt RFC3339/Z formatting, add a light regex assert.
assert resp.status == { "previous_status": True, "updated_status": False, "updated_by": "test_user_id", "timestamp": mocker.ANY, } + # Optional: enforce format + import re + assert re.match(r"^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}Z$", resp.status["timestamp"])
217-231: LGTM; mirror the optional timestamp format assertion here.assert resp.status == { "previous_status": True, "updated_status": True, "updated_by": "test_user_id", "timestamp": mocker.ANY, } + import re + assert re.match(r"^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}Z$", resp.status["timestamp"])
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
docs/openapi.json(2 hunks)src/app/endpoints/feedback.py(1 hunks)src/models/responses.py(2 hunks)tests/unit/app/endpoints/test_feedback.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/app/endpoints/feedback.py (1)
src/models/responses.py (1)
FeedbackStatusUpdateResponse(583-619)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: e2e_tests
- GitHub Check: build-pr
🔇 Additional comments (1)
docs/openapi.json (1)
1663-1665: Verify backend supports the new "match" operator.OpenAPI now advertises "match", but if server-side JWT role evaluation doesn’t implement it, clients will get misleading docs.
Would you like me to scan the codebase for JsonPathOperator handling and confirm support paths?
2 participants
Description
timestampto the response when you update the feedback status via thePUTendpoint.Type of change
Related Tickets & Documents
Checklist before requesting a review
Testing
Summary by CodeRabbit