[RHDHPAI-1027] Add dynamic feedback status update endpoint #458
Conversation
WalkthroughAdds a new admin-protected PUT /v1/feedback/status endpoint with request/response models, a module-level lock to update configuration.user_data_collection_configuration.feedback_enabled, OpenAPI schema updates (including two new schemas), and unit tests verifying update behavior. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant API as PUT /v1/feedback/status
participant Lock as feedback_status_lock
participant Config as Configuration
participant Resp as FeedbackStatusUpdateResponse
Client->>API: PUT { status: <bool> }
API->>Lock: acquire
API->>Config: read current feedback_enabled
API->>Config: set feedback_enabled = requested_status
API->>Lock: release
API->>Resp: build { status: { previous_status, updated_status, updated_by } }
API-->>Client: 200 OK with FeedbackStatusUpdateResponse
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ 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 (
|
manstis
left a comment
manstis
left a comment
There was a problem hiding this comment.
Small typo.
My main concern is that the new endpoint does not do as its name suggests, namely "toggle". It "sets" the feedback state to what is provided.
Perhaps I'm being too picky.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
src/models/requests.py (1)
399-407: Drop trivial getter; use the field directly.The method adds no value and encourages indirection elsewhere.
Apply:
- def get_value(self) -> bool: - """Return the value of the status attribute.""" - return self.status + # Access status directly; helper removed.tests/unit/app/endpoints/test_feedback.py (3)
210-221: Rename for clarity and prep for auth; keep tests resilient.Names are inverted vs. behavior; also pre-empt auth by mocking resolvers.
Apply:
-def test_feedback_toggle_enabled(): - """Test that feedback_toggle processes feedback toggle for disabled status payloads.""" +def test_feedback_toggle_to_disabled(mocker): + """Test that feedback_toggle processes disabled status payloads.""" + mock_authorization_resolvers(mocker) @@ - response = feedback_toggle(feedback_toggle_request=feedback_toggle_request) + response = feedback_toggle(feedback_toggle_request=feedback_toggle_request)
223-234: Mirror rename and auth mocking for the enable case.Apply:
-def test_feedback_toggle_disabled(): - """Test that feedback_toggle processes feedback toggle for enabled status payloads.""" +def test_feedback_toggle_to_enabled(mocker): + """Test that feedback_toggle processes enabled status payloads.""" + mock_authorization_resolvers(mocker) @@ - response = feedback_toggle(feedback_toggle_request=feedback_toggle_request) + response = feedback_toggle(feedback_toggle_request=feedback_toggle_request)
236-249: Add auth mocking to the no-op test as well.Keeps consistency if the endpoint becomes protected.
Apply:
-def test_feedback_toggle_equivalent(): +def test_feedback_toggle_equivalent(mocker): @@ - """ - Test that feedback_toggle processes feedback toggle for status payloads with the same value - as what is presently set. - """ + """Test that feedback_toggle processes status payloads equal to current value.""" + mock_authorization_resolvers(mocker)src/app/endpoints/feedback.py (2)
195-209: Doc accuracy and minor simplification.This function sets, it doesn’t toggle; also avoid the trivial getter.
Apply:
-def toggle_feedback_status(req: FeedbackToggleRequest) -> bool: +def toggle_feedback_status(req: FeedbackToggleRequest) -> bool: @@ - Toggles the feedback boolean stored in the configuration. + Set the feedback boolean stored in the configuration. @@ - configuration.user_data_collection_configuration.feedback_enabled = req.get_value() + configuration.user_data_collection_configuration.feedback_enabled = req.status return is_feedback_enabled()
195-209: Multi-worker caveat: state won’t sync across processes.If running multiple Uvicorn/Gunicorn workers, this in-memory flag diverges per process. Consider a shared store (DB/Redis) or single-worker for consistency.
📜 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(2 hunks)src/models/requests.py(1 hunks)tests/unit/app/endpoints/test_feedback.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/app/endpoints/feedback.py (3)
src/models/requests.py (2)
FeedbackToggleRequest(385-407)get_value(405-407)src/models/responses.py (1)
StatusResponse(264-293)src/configuration.py (2)
configuration(56-61)user_data_collection_configuration(80-85)
tests/unit/app/endpoints/test_feedback.py (3)
src/app/endpoints/feedback.py (1)
feedback_toggle(181-192)src/models/requests.py (1)
FeedbackToggleRequest(385-407)src/configuration.py (2)
configuration(56-61)user_data_collection_configuration(80-85)
⏰ 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: build-pr
- GitHub Check: e2e_tests
🔇 Additional comments (2)
tests/unit/app/endpoints/test_feedback.py (1)
13-16: LGTM: new imports align with the new endpoint and request model.src/app/endpoints/feedback.py (1)
23-23: LGTM: import of FeedbackToggleRequest is correct.
docs/openapi.json
Outdated
| "FeedbackToggleRequest": { | ||
| "properties": { | ||
| "status": { | ||
| "type": "boolean", | ||
| "title": "Status", | ||
| "description": "Desired state of feedback enablement, must be False or True", | ||
| "default": false, | ||
| "examples": [ | ||
| true, | ||
| false | ||
| ] | ||
| } | ||
| }, | ||
| "type": "object", | ||
| "title": "FeedbackToggleRequest", | ||
| "description": "Model representing a feedback toggle request.\n\nAttributes:\n status: Boolean controlling what the Feedback status should be.\n\nExample:\n ```python\n feedback_request = FeedbackRequest(\n status=false\n )\n ```" | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Require 'status', remove default, and fix example class/casing.
Prevents accidental disable on empty body and aligns with the Python example.
Apply:
"FeedbackToggleRequest": {
"properties": {
"status": {
"type": "boolean",
"title": "Status",
"description": "Desired state of feedback enablement, must be False or True",
- "default": false,
"examples": [
true,
false
]
}
},
"type": "object",
+ "required": ["status"],
"title": "FeedbackToggleRequest",
- "description": "Model representing a feedback toggle request.\n\nAttributes:\n status: Boolean controlling what the Feedback status should be.\n\nExample:\n ```python\n feedback_request = FeedbackRequest(\n status=false\n )\n ```"
+ "description": "Model representing a feedback toggle request.\n\nAttributes:\n status: Boolean controlling what the Feedback status should be.\n\nExample:\n ```python\n feedback_toggle_request = FeedbackToggleRequest(\n status=False\n )\n ```"
},🤖 Prompt for AI Agents
In docs/openapi.json around lines 1491 to 1507, the FeedbackToggleRequest schema
must require the status field, must not include a default value, and the example
in the description should use the correct class name and Python boolean casing;
remove the "default": false entry, add a "required": ["status"] array at the
schema level, and update the description/example text to reference
FeedbackToggleRequest and use status=False with a lowercase class name preserved
exactly as "FeedbackToggleRequest".
@manstis |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/app/endpoints/feedback.py (1)
179-181: Require ADMIN authorization and audit who flipped the flagRight now anyone can flip a global feature. Gate with ADMIN and log actor.
Apply:
-@router.post("/toggle") -def feedback_toggle() -> StatusResponse: +@router.post( + "/toggle", + responses={ + 401: {"description": "Missing or invalid credentials", "model": UnauthorizedResponse}, + 403: {"description": "Insufficient permissions", "model": ForbiddenResponse}, + }, +) +@authorize(Action.ADMIN) +def feedback_toggle( + auth: Annotated[AuthTuple, Depends(auth_dependency)], +) -> StatusResponse:
🧹 Nitpick comments (1)
src/app/endpoints/feedback.py (1)
184-189: Clarify non-idempotent semantics in docstringMake behavior explicit for API consumers.
Apply:
- Returns the current enabled status of the feedback functionality after it has been - updated to the opposite value from the previously set value. + Flips the in-memory feedback enabled flag and returns the new state. + Non-persistent and non-idempotent (calling twice reverts to the original state).
📜 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 (3)
docs/openapi.json(1 hunks)src/app/endpoints/feedback.py(1 hunks)tests/unit/app/endpoints/test_feedback.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/openapi.json
- tests/unit/app/endpoints/test_feedback.py
🧰 Additional context used
🧬 Code graph analysis (1)
src/app/endpoints/feedback.py (2)
src/models/responses.py (1)
StatusResponse(264-293)src/configuration.py (2)
configuration(56-61)user_data_collection_configuration(80-85)
⏰ 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: build-pr
- GitHub Check: e2e_tests
|
Converting to draft to address some concerns |
|
@Jdubrick Thank-you for the update. Personally, I wonder whether "set" would be better than "toggle". "set" is more deterministic whereas "toggle" requires you to know the current state. i.e. if you want to disable feedback you need to first read the current setting and then "toggle" it if enabled. Simply calling "toggle", if already disabled, would have negative consequences. However, since you have the use-case to "set"/"toggle" I'll leave the ultimate choice to you. CodeRabbit's comments about Uvicorn How configuration state is managed across |
|
@Jdubrick what's the use case there please? Is it something to be controlled by RBAC perhaps? (ATM we'd probably need to think about having an feature/epic/jira tickets, if it's something officially needed) |
@manstis agreed, I marked this as draft as I am currently making some changes to go to a more of a The point about state across workers/pods I agree is out of the scope, I will push the changes I am working on at some point today for re-review |
@tisnik our customers need some sort of way to be able to change certain settings on the fly without needing to tear down the container and update the config yaml, while we know it isn't really possible to persist these changes right now, an ephemeral change for the life of the container is important to us |
Jdubrick
force-pushed
the
add-dynamic-feedback-toggle
branch
from
a3e10a1 to
13c37a3
Compare
Signed-off-by: Jordan Dubrick <jdubrick@redhat.com>
Signed-off-by: Jordan Dubrick <jdubrick@redhat.com>
Signed-off-by: Jordan Dubrick <jdubrick@redhat.com>
Signed-off-by: Jordan Dubrick <jdubrick@redhat.com>
Jdubrick
force-pushed
the
add-dynamic-feedback-toggle
branch
from
13c37a3 to
ec5e7a7
Compare
Jdubrick
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/models/responses.py (1)
575-609: Type the response payload and fix the docstring example.Return shape is stable; avoid a loose dict and wrong class name in the example.
Apply:
class FeedbackStatusUpdateResponse(BaseModel): """Model representing a response to a feedback status update request. @@ - Example: + Example: ```python - status_response = StatusResponse( + status_response = FeedbackStatusUpdateResponse( status={ - "previous_status": true, - "updated_status": false, + "previous_status": True, + "updated_status": False, "updated_by": "user/test" }, ) ``` """ - status: dict + status: "StatusUpdateDetails"Add this model just above the response class:
class StatusUpdateDetails(BaseModel): previous_status: bool updated_status: bool updated_by: str model_config = {"extra": "forbid"}
♻️ Duplicate comments (2)
src/models/requests.py (1)
385-407: Make 'status' required, fix example, and forbid extra fields.Empty body would silently disable feedback. Also example uses wrong class and Python boolean casing. Forbid unknown fields.
Apply:
class FeedbackStatusUpdateRequest(BaseModel): @@ - Example: - ```python - feedback_request = FeedbackRequest( - status=false - ) - ``` + Example: + ```python + feedback_status_update_request = FeedbackStatusUpdateRequest( + status=False + ) + ``` @@ - status: bool = Field( - False, + status: bool = Field( + ..., description="Desired state of feedback enablement, must be False or True", examples=[True, False], ) + # examples for /docs; reject unknown fields + model_config = { + "extra": "forbid", + "json_schema_extra": {"examples": [{"status": True}, {"status": False}]}, + } def get_value(self) -> bool: """Return the value of the status attribute.""" return self.statusOptional: use StrictBool to avoid coercing 0/1, "true"/"false".
docs/openapi.json (1)
1490-1505: Require 'status', remove default, fix example and forbid extras (schema).Prevents accidental disable on empty body; aligns with Python example; reject unknown keys.
"FeedbackStatusUpdateRequest": { "properties": { "status": { "type": "boolean", "title": "Status", "description": "Desired state of feedback enablement, must be False or True", - "default": false, "examples": [ true, false ] } }, - "type": "object", + "type": "object", + "additionalProperties": false, + "required": ["status"], "title": "FeedbackStatusUpdateRequest", - "description": "Model representing a feedback status update request.\n\nAttributes:\n status: Value of the desired feedback enabled state.\n\nExample:\n ```python\n feedback_request = FeedbackRequest(\n status=false\n )\n ```" + "description": "Model representing a feedback status update request.\n\nAttributes:\n status: Value of the desired feedback enabled state.\n\nExample:\n ```python\n feedback_status_update_request = FeedbackStatusUpdateRequest(\n status=False\n )\n ```" },
🧹 Nitpick comments (2)
src/app/endpoints/feedback.py (2)
30-30: Thread-safety added is good; note it’s process-local.Lock prevents intra-process races; multi-worker/pod divergence remains (accepted per PR scope). Document explicitly in API docs and logs.
194-196: Fix return type in docstring.Function returns FeedbackStatusUpdateResponse, not StatusResponse.
- Returns: - StatusResponse: Indicates whether feedback is enabled. + Returns: + FeedbackStatusUpdateResponse: Previous and updated status with updater.
📜 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 (5)
docs/openapi.json(2 hunks)src/app/endpoints/feedback.py(4 hunks)src/models/requests.py(1 hunks)src/models/responses.py(1 hunks)tests/unit/app/endpoints/test_feedback.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/unit/app/endpoints/test_feedback.py
🔇 Additional comments (1)
src/app/endpoints/feedback.py (1)
200-210: Add audit log for feedback status changeswith feedback_status_lock: previous_status = ( configuration.user_data_collection_configuration.feedback_enabled ) configuration.user_data_collection_configuration.feedback_enabled = ( requested_status ) updated_status = ( configuration.user_data_collection_configuration.feedback_enabled ) + logger.info( + "Feedback status update by %s: %s -> %s", + user_id, + previous_status, + updated_status, + )
| "FeedbackStatusUpdateResponse": { | ||
| "properties": { | ||
| "status": { | ||
| "additionalProperties": true, | ||
| "type": "object", | ||
| "title": "Status" | ||
| } | ||
| }, | ||
| "type": "object", | ||
| "required": [ | ||
| "status" | ||
| ], | ||
| "title": "FeedbackStatusUpdateResponse", | ||
| "description": "Model representing a response to a feedback status update request.\n\nAttributes:\n status: The previous and current status of the service and who updated it.\n\nExample:\n ```python\n status_response = StatusResponse(\n status={\n \"previous_status\": true,\n \"updated_status\": false,\n \"updated_by\": \"user/test\"\n },\n )\n ```", | ||
| "examples": [ | ||
| { | ||
| "status": { | ||
| "previous_status": true, | ||
| "updated_by": "user/test", | ||
| "updated_status": false | ||
| } | ||
| } | ||
| ] | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Constrain response schema to explicit fields.
Publish stable keys for clients.
"FeedbackStatusUpdateResponse": {
"properties": {
"status": {
- "additionalProperties": true,
- "type": "object",
- "title": "Status"
+ "type": "object",
+ "title": "Status",
+ "additionalProperties": false,
+ "properties": {
+ "previous_status": { "type": "boolean" },
+ "updated_status": { "type": "boolean" },
+ "updated_by": { "type": "string" }
+ },
+ "required": ["previous_status", "updated_status", "updated_by"]
}
},
"type": "object",
"required": [
"status"
],
"title": "FeedbackStatusUpdateResponse",
- "description": "Model representing a response to a feedback status update request.\n\nAttributes:\n status: The previous and current status of the service and who updated it.\n\nExample:\n ```python\n status_response = StatusResponse(\n status={\n \"previous_status\": true,\n \"updated_status\": false,\n \"updated_by\": \"user/test\"\n },\n )\n ```",
+ "description": "Model representing a response to a feedback status update request.\n\nAttributes:\n status: The previous and current status of the service and who updated it.\n\nExample:\n ```python\n status_response = FeedbackStatusUpdateResponse(\n status={\n \"previous_status\": true,\n \"updated_status\": false,\n \"updated_by\": \"user/test\"\n }\n )\n ```",
"examples": [
{
"status": {
"previous_status": true,
"updated_by": "user/test",
"updated_status": false
}
}
]
},🤖 Prompt for AI Agents
In docs/openapi.json around lines 1506 to 1529, the FeedbackStatusUpdateResponse
currently allows arbitrary keys (additionalProperties: true) for the status
object and has an inconsistent example name; change the status schema to
enumerate explicit properties (previous_status: boolean, updated_status:
boolean, updated_by: string), mark them required as appropriate, remove
additionalProperties, and update the description/example to reference
FeedbackStatusUpdateResponse consistently so clients have a stable, validated
response shape.
Signed-off-by: Jordan Dubrick <jdubrick@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app/endpoints/feedback.py (1)
182-219: Return type/docs mismatch and missing audit log.
- Docstring says StatusResponse but function returns FeedbackStatusUpdateResponse.
- Add an INFO audit line logging who changed what.
@router.put("/status") @authorize(Action.ADMIN) async def update_feedback_status( @@ - Returns: - StatusResponse: Indicates whether feedback is enabled. + Returns: + FeedbackStatusUpdateResponse: Indicates whether feedback is enabled and who updated it. @@ - with feedback_status_lock: + with feedback_status_lock: previous_status = ( configuration.user_data_collection_configuration.feedback_enabled ) configuration.user_data_collection_configuration.feedback_enabled = ( requested_status ) updated_status = ( configuration.user_data_collection_configuration.feedback_enabled ) + logger.info( + "Feedback status updated by %s: %s -> %s (ephemeral, process-local)", + user_id, + previous_status, + updated_status, + )
♻️ Duplicate comments (3)
docs/openapi.json (3)
389-429: PUT /v1/feedback/status should advertise auth failures and correct return-type wording.Add 401/403, keep the non-persistent note, and fix the “Returns:” text.
"put": { @@ - "summary": "Update Feedback Status", - "description": "Handle feedback status update requests.\n\nTakes a request with the desired state of the feedback status.\nReturns the updated state of the feedback status based on the request's value.\nThese changes are for the life of the service and are on a per-worker basis.\n\nReturns:\n StatusResponse: Indicates whether feedback is enabled.", + "summary": "Update Feedback Status", + "description": "Handle feedback status update requests.\n\nTakes a request with the desired state of the feedback status. The update is in-memory only (non-persistent) and applies to the current worker process.\nReturns the updated state of the feedback status based on the request's value.\n\nReturns:\n FeedbackStatusUpdateResponse: Indicates whether feedback is enabled and who updated it.", @@ "responses": { "200": { @@ - }, + }, + "401": { + "description": "Missing or invalid credentials provided by client", + "content": { + "application/json": { + "schema": { "$ref": "#/components/schemas/UnauthorizedResponse" } + } + } + }, + "403": { + "description": "Client does not have permission to access resource", + "content": { + "application/json": { + "schema": { "$ref": "#/components/schemas/ForbiddenResponse" } + } + } + }, "422": {
1496-1512: Require ‘status’, remove default false, and fix example class/casing.Avoid accidental disables on empty body; example should reference the correct class.
"FeedbackStatusUpdateRequest": { "properties": { "status": { "type": "boolean", "title": "Status", "description": "Desired state of feedback enablement, must be False or True", - "default": false, "examples": [ true, false ] } }, "type": "object", + "required": ["status"], "title": "FeedbackStatusUpdateRequest", - "description": "Model representing a feedback status update request.\n\nAttributes:\n status: Value of the desired feedback enabled state.\n\nExample:\n ```python\n feedback_request = FeedbackRequest(\n status=false\n )\n ```" + "description": "Model representing a feedback status update request.\n\nAttributes:\n status: Value of the desired feedback enabled state.\n\nExample:\n ```python\n feedback_status_update_request = FeedbackStatusUpdateRequest(\n status=False\n )\n ```" }
1513-1536: Constrain FeedbackStatusUpdateResponse.status to explicit fields; fix example name.Publish a stable schema with required keys.
"FeedbackStatusUpdateResponse": { "properties": { "status": { - "additionalProperties": true, - "type": "object", - "title": "Status" + "type": "object", + "title": "Status", + "additionalProperties": false, + "properties": { + "previous_status": { "type": "boolean" }, + "updated_status": { "type": "boolean" }, + "updated_by": { "type": "string" } + }, + "required": ["previous_status", "updated_status", "updated_by"] } }, @@ - "description": "Model representing a response to a feedback status update request.\n\nAttributes:\n status: The previous and current status of the service and who updated it.\n\nExample:\n ```python\n status_response = StatusResponse(\n status={\n \"previous_status\": true,\n \"updated_status\": false,\n \"updated_by\": \"user/test\"\n },\n )\n ```", + "description": "Model representing a response to a feedback status update request.\n\nAttributes:\n status: The previous and current status of the service and who updated it.\n\nExample:\n ```python\n status_response = FeedbackStatusUpdateResponse(\n status={\n \"previous_status\": true,\n \"updated_status\": false,\n \"updated_by\": \"user/test\"\n }\n )\n ```",
🧹 Nitpick comments (2)
src/app/endpoints/feedback.py (1)
4-4: Consider asyncio.Lock to avoid blocking the event loop.The critical section is tiny, so threading.Lock is acceptable. If traffic spikes, prefer an async lock.
-import threading +import asyncio @@ -feedback_status_lock = threading.Lock() +feedback_status_lock = asyncio.Lock()And later:
- with feedback_status_lock: + async with feedback_status_lock:Also applies to: 30-30
docs/openapi.json (1)
755-755: Confirm intent: broad additionalProperties=false constraints added.This tightens many schemas and can break clients sending extra keys. If intentional, consider a minor version bump and a changelog entry; otherwise, revert or gate to a separate PR.
Also applies to: 887-887, 903-903, 979-979, 1047-1047, 1299-1299, 1594-1594, 1661-1661, 1690-1690, 1720-1720, 1796-1796, 1817-1817, 1925-1925, 2230-2230, 2291-2291, 2363-2363, 2422-2422
📜 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 (3)
docs/openapi.json(20 hunks)src/app/endpoints/feedback.py(4 hunks)src/models/responses.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/app/endpoints/feedback.py (5)
src/models/requests.py (3)
FeedbackRequest(240-382)FeedbackStatusUpdateRequest(385-407)get_value(405-407)src/models/responses.py (1)
FeedbackStatusUpdateResponse(575-609)src/authorization/middleware.py (1)
authorize(104-115)src/models/config.py (1)
Action(276-313)src/configuration.py (2)
configuration(60-64)user_data_collection_configuration(81-85)
⏰ 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: build-pr
- GitHub Check: e2e_tests
| class FeedbackStatusUpdateResponse(BaseModel): | ||
| """ | ||
| Model representing a response to a feedback status update request. | ||
| Attributes: | ||
| status: The previous and current status of the service and who updated it. | ||
| Example: | ||
| ```python | ||
| status_response = StatusResponse( | ||
| status={ | ||
| "previous_status": true, | ||
| "updated_status": false, | ||
| "updated_by": "user/test" | ||
| }, | ||
| ) | ||
| ``` | ||
| """ | ||
|
|
||
| status: dict | ||
|
|
||
| # provides examples for /docs endpoint | ||
| model_config = { | ||
| "json_schema_extra": { | ||
| "examples": [ | ||
| { | ||
| "status": { | ||
| "previous_status": True, | ||
| "updated_status": False, | ||
| "updated_by": "user/test", | ||
| }, | ||
| } | ||
| ] | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make response shape explicit; fix example class name and booleans.
- Status should be a typed object, not a free-form dict.
- Docstring example references StatusResponse and uses lowercase true/false; should reference FeedbackStatusUpdateResponse and use Python booleans.
Apply:
-class FeedbackStatusUpdateResponse(BaseModel):
+class FeedbackStatusUpdateResponse(BaseModel):
@@
- Example:
- ```python
- status_response = StatusResponse(
- status={
- "previous_status": true,
- "updated_status": false,
- "updated_by": "user/test"
- },
- )
- ```
+ Example:
+ ```python
+ status_response = FeedbackStatusUpdateResponse(
+ status={
+ "previous_status": True,
+ "updated_status": False,
+ "updated_by": "user/test",
+ }
+ )
+ ```
@@
- status: dict
+ status: "FeedbackStatusDelta"Add this model (top-level in this file):
class FeedbackStatusDelta(BaseModel):
previous_status: bool = Field(..., description="State before the update")
updated_status: bool = Field(..., description="State after the update")
updated_by: str = Field(..., description="User ID of the updater")🤖 Prompt for AI Agents
In src/models/responses.py around lines 575-609, the
FeedbackStatusUpdateResponse currently types status as dict and its
docstring/example incorrectly names StatusResponse and uses lowercase booleans;
replace the free-form dict with a concrete FeedbackStatusDelta model, update the
docstring example to use FeedbackStatusUpdateResponse and Python booleans, and
update the status annotation to FeedbackStatusDelta. Add a top-level
FeedbackStatusDelta BaseModel with previous_status: bool, updated_status: bool,
and updated_by: str (using Field with descriptions) and ensure Field (and
BaseModel) are imported where needed. Update the json_schema_extra example to
use True/False and the new class name.
|
fyi @yangcao77 |
|
@Jdubrick LGTMed, but could you pls add a jira ticket into the PR title? |
Jdubrick
changed the title
|
@tisnik all done, thanks! :) |
3 participants
Description
/v1/feedback/statusthat allows an admin of Lightspeed Core to set the feedback enabled state totrueorfalseas they wish. This change will be ephemeral for the life of the service and will not be extended across all workers (single at this point)Type of change
Related Tickets & Documents
Checklist before requesting a review
Testing
Summary by CodeRabbit
New Features
Documentation
Tests