feature(pending-slot-assignment): Auto assign spool to next ams slot - #15
feature(pending-slot-assignment): Auto assign spool to next ams slot#15miguelangellv wants to merge 3 commits into
Conversation
…ol-to-slot assignments
…ol-to-slot assignments
…t-ams-slot' into feature/auto-assign-spool-to-next-ams-slot
vmhomelab
left a comment
There was a problem hiding this comment.
Review Summary
Thanks for the contribution — this is a useful feature and the overall direction looks good.
The PR implements the requested assign-on-next-AMS-slot workflow from issue #3:
- Adds pending slot assignment API endpoints
- Persists pending/completed/timed-out assignments
- Hooks into AMS slot change handling
- Matches inserted spool identifiers before assignment
- Emits WebSocket/MQTT events
- Restores timeout tasks after app restart
- Adds integration coverage for the main flows
I checked the PR locally and the targeted tests pass:
23 passed in 31.30s
ruff check: passed
ruff format --check: passed
So the core behavior appears to work.
Requested changes before merge
There are a few API-boundary issues I’d like cleaned up before merging.
- Request validation is too loose
The active route imports PendingSlotAssignmentCreateRequest from backend/app/schemas/spool.py. That schema currently accepts invalid or unsafe API input, including:
- no usable spool identifier
- arbitrary source values
- timeout=0
- very large timeout values
For a public API endpoint, this should be validated before the service layer creates a DB row or schedules an asyncio timeout task.
Suggested direction:
from typing import Literal
class PendingSlotAssignmentCreateRequest(BaseModel):
spool_id: int | None = Field(default=None, gt=0)
tray_uuid: str | None = None
tag_uid: str | None = None
printer_id: int | None = Field(default=None, gt=0)
source: Literal["nfc", "qr", "spoolbuddy"]
timeout: int = Field(default=300, ge=10, le=3600)
@model_validator(mode="after")
def require_identifier(self):
if not self.spool_id and not self.tray_uuid and not self.tag_uid:
raise ValueError("At least one of spool_id, tray_uuid, or tag_uid must be provided.")
return self
The tests currently document the loose behavior, so those should be updated to expect validation errors instead.
-
Duplicate pending-assignment schemas
The PR adds:
backend/app/schemas/pending_slot_assignment.py
But the route uses the classes from:
backend/app/schemas/spool.py
This makes the new schema file look unused or partially integrated. Please consolidate this so there is one canonical request/response schema location. -
Create endpoint should probably return
202 Accepted
Issue #3 proposed that creating a pending assignment should return202 Acceptedbecause the actual assignment is asynchronous and completed later by AMS events.
Current behavior returns the default 200 OK.
Not a functional blocker by itself, but since this is a new API contract, I’d prefer matching the issue unless there is a reason not to.
- Confirm permission model
The PR uses:
- create/delete:
INVENTORY_UPDATE - get status:
INVENTORY_READ
That may be fine, but the issue mentions manage-inventory scope for assignment management. Please confirm this is intentional.
Looks good
The AMS change hook checks pending assignments before falling back to normal RFID auto-assignment.
Identifier matching prevents assigning the wrong RFID spool when the inserted spool does not match the pending request.
Timeout restore on startup is a good operational detail.
MQTT and WebSocket event coverage is useful for mobile apps/integrations.
The integration tests cover the important happy paths and several edge cases.
Verdict
Good feature and the core implementation works, but I’d like the request validation and schema cleanup addressed before merge.
|
I will fixes soon ;) |
Description
This feature allows you to automatically load a spool into the next available AMS slot.
An allocation request is sent to the backend, specifying the reel (with its ID, tag_uid, or tray_uuid), the timing, and optionally the printer.
The backend will return an allocation ID if the spool is successfully found and will begin listening for changes in the AMS slots.
When a change occurs, it will allocate the reel to that slot and send an MQTT event.
An endpoint is available to check the allocation status and another to delete it.
Related Issue
Fixes #3
Documentation
Pick one:
Type of Change
Changes Made
Screenshots
Testing
Several tests using differents combinations and integration tests
Checklist