From b4dcb68bea17d7029a0288afec818b38adf02017 Mon Sep 17 00:00:00 2001 From: Ian Roddis <31021769+iroddis@users.noreply.github.com> Date: Tue, 18 Aug 2026 16:57:28 -0300 Subject: [PATCH] Adding Manual connection point to allow undeclared fits --- app/api/attachment_points.py | 10 +- app/models/attachment_point.py | 5 +- app/services/fit_matching.py | 7 +- app/services/scope_import.py | 65 +++++++++-- .../_attachment_point_connection_select.html | 2 + .../admin/_attachment_point_row.html | 2 +- .../admin/_attachment_point_row_edit.html | 3 +- .../admin/_augmented_part_ap_row.html | 2 +- .../admin/_augmented_part_ap_row_edit.html | 3 +- .../_augmented_part_attachment_points.html | 1 + .../admin/_augmented_part_fit_row.html | 2 +- app/templates/admin/attachment_points.html | 1 + app/web/admin.py | 6 +- tests/test_attachment_points_api.py | 106 ++++++++++++++++++ tests/test_scope_import.py | 35 ++++++ 15 files changed, 227 insertions(+), 23 deletions(-) create mode 100644 tests/test_attachment_points_api.py diff --git a/app/api/attachment_points.py b/app/api/attachment_points.py index 2a44283..483a62c 100644 --- a/app/api/attachment_points.py +++ b/app/api/attachment_points.py @@ -27,7 +27,7 @@ def _connection_label(ap) -> str: return f"{cc.min_in}–{cc.max_in} in" if ap.static_connection: return f"{ap.static_connection.od_mm} mm OD" - return "—" + return "Manual" @router.get("", response_model=list[AttachmentPointRead]) @@ -54,10 +54,14 @@ async def create_attachment_point( payload.clamp_connection_id, payload.static_connection_id, ] - if sum(v is not None for v in conn_ids) != 1: + if sum(v is not None for v in conn_ids) > 1: raise HTTPException( status_code=400, - detail="Exactly one connection (iso_thread, unc_thread, clamp_connection, or static_connection) must be provided.", + detail=( + "At most one connection (iso_thread, unc_thread, clamp_connection, " + "or static_connection) may be provided. Leave all unset for a " + "manual attachment point." + ), ) ap = await attachment_point_crud.create(db, **payload.model_dump()) await audit_crud.log( diff --git a/app/models/attachment_point.py b/app/models/attachment_point.py index 9accc15..956791f 100644 --- a/app/models/attachment_point.py +++ b/app/models/attachment_point.py @@ -21,7 +21,10 @@ class AttachmentPoint(Base): ForeignKey("attachment_point_names.id", ondelete="RESTRICT"), nullable=False ) - # Exactly one of the four below must be non-null (enforced in application layer) + # At most one of the four below is non-null (enforced in application layer). + # All four null means a "Manual" attachment point: no connection details, + # used to let a user assert a Fit by hand without the fit-matching algorithm + # (app.services.fit_matching) ever auto-matching it. iso_thread_id: Mapped[int | None] = mapped_column( ForeignKey("iso_threads.id", ondelete="RESTRICT"), nullable=True, default=None ) diff --git a/app/services/fit_matching.py b/app/services/fit_matching.py index d1a1cdf..8f31e24 100644 --- a/app/services/fit_matching.py +++ b/app/services/fit_matching.py @@ -76,7 +76,12 @@ def _static_clamp_match(ap_a: AttachmentPoint, ap_b: AttachmentPoint) -> bool: def attachment_points_match(ap_a: AttachmentPoint, ap_b: AttachmentPoint) -> bool: - """True if these two attachment points satisfy the fit-matching algorithm.""" + """True if these two attachment points satisfy the fit-matching algorithm. + + A "Manual" attachment point (all four connections null) always returns + False here — it never participates in algorithmic matching, only in + fits a user asserts by hand. + """ # Disabled for now, the attachment_point_names should not be considered for the purposes of matching # if ap_a.attachment_point_name_id == ap_b.attachment_point_name_id: # return False diff --git a/app/services/scope_import.py b/app/services/scope_import.py index fe998ea..106fbcf 100644 --- a/app/services/scope_import.py +++ b/app/services/scope_import.py @@ -27,9 +27,13 @@ Fits are created with label POSSIBLE. The referenced part is looked up by ``product_number`` and linked to its first attachment point (chosen answer: "sole/first AP"). If the referenced part has no attachment points at all, one is created for it that mirrors the scope's attachment point's connection, so -the fit has something valid to attach to. Both directions are resolved in a -second pass so a fit between two scopes in the same spreadsheet resolves -regardless of row order. +the fit has something valid to attach to. If the scope itself has no Ocular/ +Objective attachment point (no ocular_outer_diameter_mm/objective_thread, or +an unparseable one), a Manual attachment point — no connection details, never +picked up by the fit-matching algorithm — is created on the scope under the +appropriate label so the fit still has somewhere to attach. Both directions +are resolved in a second pass so a fit between two scopes in the same +spreadsheet resolves regardless of row order. Bad rows (missing key, unknown fit target, unparseable thread) are skipped and reported per-row; good rows are committed as they go. @@ -207,6 +211,36 @@ async def _mirrored_ap(db: AsyncSession, *, augmented_part_id: int, scope_ap): ) +async def _ensure_manual_scope_ap( + db: AsyncSession, + *, + scope_ap, + augmented_part_id: int, + label: str, + messages: list[str], +): + """Return ``scope_ap``, creating a Manual attachment point named ``label`` + on the scope if it's None. + + Used so an ocular_fit/objective_fit reference still has something to link + from even when the scope has no ocular_outer_diameter_mm/objective_thread + of its own. + """ + if scope_ap is not None: + return scope_ap + apn = await apn_crud.get_by_name(db, label) + if apn is None: + apn = await apn_crud.create(db, name=label) + messages.append( + f"scope has no {label.lower()} attachment point; created a manual one to record the fit" + ) + return await attachment_point_crud.create( + db, + augmented_part_id=augmented_part_id, + attachment_point_name_id=apn.id, + ) + + async def _link_fit( db: AsyncSession, *, @@ -218,14 +252,11 @@ async def _link_fit( ) -> None: """Create a POSSIBLE fit from the scope's AP to the referenced part's first AP. - If the referenced part has no attachment points at all, one is created - mirroring the scope's attachment point's connection. + ``scope_ap`` must already exist — callers ensure this via + ``_ensure_manual_scope_ap`` before calling. If the referenced part has no + attachment points at all, one is created mirroring the scope's attachment + point's connection. """ - if scope_ap is None: - messages.append( - f"{scope_role}_fit '{target_sku}' skipped: scope has no {scope_role} attachment point" - ) - return target = await augmented_part_crud.get_by_sku(db, target_sku) if target is None: messages.append(f"{scope_role}_fit references unknown product '{target_sku}'") @@ -406,6 +437,13 @@ async def import_scopes(db: AsyncSession, rows: list[dict]) -> ScopeImportResult try: ocular_fit = _text(row.get("ocular_fit")) if ocular_fit: + ocular_ap = await _ensure_manual_scope_ap( + db, + scope_ap=ocular_ap, + augmented_part_id=part_id, + label=OCULAR_LABEL, + messages=result.messages, + ) fits = ocular_fit if "/" in fits: fits = [x.strip() for x in fits.split("/")] @@ -422,6 +460,13 @@ async def import_scopes(db: AsyncSession, rows: list[dict]) -> ScopeImportResult ) objective_fit = _text(row.get("objective_fit")) if objective_fit: + objective_ap = await _ensure_manual_scope_ap( + db, + scope_ap=objective_ap, + augmented_part_id=part_id, + label=OBJECTIVE_LABEL, + messages=result.messages, + ) fits = objective_fit if "/" in fits: fits = [x.strip() for x in fits.split("/")] diff --git a/app/templates/admin/_attachment_point_connection_select.html b/app/templates/admin/_attachment_point_connection_select.html index f4b5365..2237c03 100644 --- a/app/templates/admin/_attachment_point_connection_select.html +++ b/app/templates/admin/_attachment_point_connection_select.html @@ -48,4 +48,6 @@ {% endfor %} +{% elif connection_type == "manual" %} +

No connection details — fits for this attachment point must be asserted manually.

{% endif %} diff --git a/app/templates/admin/_attachment_point_row.html b/app/templates/admin/_attachment_point_row.html index c661fe8..e184087 100644 --- a/app/templates/admin/_attachment_point_row.html +++ b/app/templates/admin/_attachment_point_row.html @@ -15,7 +15,7 @@ {% set conn_type = "Static Connection" %} {% set conn_label = ap.static_connection.od_mm ~ " mm OD" %} {% else %} - {% set conn_type = "—" %} + {% set conn_type = "Manual" %} {% set conn_label = "—" %} {% endif %} diff --git a/app/templates/admin/_attachment_point_row_edit.html b/app/templates/admin/_attachment_point_row_edit.html index 2b4ce5d..984425d 100644 --- a/app/templates/admin/_attachment_point_row_edit.html +++ b/app/templates/admin/_attachment_point_row_edit.html @@ -2,7 +2,7 @@ {% elif ap.unc_thread_id %}{% set current_type = "unc_thread" %} {% elif ap.clamp_connection_id %}{% set current_type = "clamp_connection" %} {% elif ap.static_connection_id %}{% set current_type = "static_connection" %} -{% else %}{% set current_type = "" %}{% endif %} +{% else %}{% set current_type = "manual" %}{% endif %}
UNC Thread + {# Render the current connection select pre-filled #} diff --git a/app/templates/admin/_augmented_part_ap_row.html b/app/templates/admin/_augmented_part_ap_row.html index 733dffd..c037592 100644 --- a/app/templates/admin/_augmented_part_ap_row.html +++ b/app/templates/admin/_augmented_part_ap_row.html @@ -15,7 +15,7 @@ {% set conn_type = "Static" %} {% set conn_label = apoint.static_connection.od_mm ~ " mm OD" %} {% else %} - {% set conn_type = "—" %} + {% set conn_type = "Manual" %} {% set conn_label = "—" %} {% endif %}
diff --git a/app/templates/admin/_augmented_part_ap_row_edit.html b/app/templates/admin/_augmented_part_ap_row_edit.html index 526b2ca..3f15be0 100644 --- a/app/templates/admin/_augmented_part_ap_row_edit.html +++ b/app/templates/admin/_augmented_part_ap_row_edit.html @@ -2,7 +2,7 @@ {% elif apoint.unc_thread_id %}{% set current_type = "unc_thread" %} {% elif apoint.clamp_connection_id %}{% set current_type = "clamp_connection" %} {% elif apoint.static_connection_id %}{% set current_type = "static_connection" %} -{% else %}{% set current_type = "" %}{% endif %} +{% else %}{% set current_type = "manual" %}{% endif %} {# _attachment_point_connection_select.html expects `ap` for pre-selection #} {% set ap = apoint %} {% set connection_type = current_type %} @@ -29,6 +29,7 @@ +
diff --git a/app/templates/admin/_augmented_part_attachment_points.html b/app/templates/admin/_augmented_part_attachment_points.html index 44f0976..11f807e 100644 --- a/app/templates/admin/_augmented_part_attachment_points.html +++ b/app/templates/admin/_augmented_part_attachment_points.html @@ -31,6 +31,7 @@ +
diff --git a/app/templates/admin/_augmented_part_fit_row.html b/app/templates/admin/_augmented_part_fit_row.html index d1c79f2..75f551b 100644 --- a/app/templates/admin/_augmented_part_fit_row.html +++ b/app/templates/admin/_augmented_part_fit_row.html @@ -8,7 +8,7 @@ {%- elif ap.static_connection -%} Static {{ ap.static_connection.od_mm }} mm OD {%- else -%} - — + Manual {%- endif -%} {%- endmacro %} diff --git a/app/templates/admin/attachment_points.html b/app/templates/admin/attachment_points.html index c2df89b..da99458 100644 --- a/app/templates/admin/attachment_points.html +++ b/app/templates/admin/attachment_points.html @@ -38,6 +38,7 @@ +
diff --git a/app/web/admin.py b/app/web/admin.py index a1689b4..fc082b0 100644 --- a/app/web/admin.py +++ b/app/web/admin.py @@ -2387,7 +2387,7 @@ async def edit_augmented_part_attachment_point( elif apoint.static_connection_id: ctx["connection_type"] = "static_connection" else: - ctx["connection_type"] = "" + ctx["connection_type"] = "manual" return templates.TemplateResponse( request, "admin/_augmented_part_ap_row_edit.html", ctx ) @@ -3242,7 +3242,7 @@ def _ap_connection_label(ap) -> str: return f"{cc.min_in}–{cc.max_in} in" if ap.static_connection: return f"{ap.static_connection.od_mm} mm OD" - return "—" + return "Manual" @router.get("/attachment-points", response_class=HTMLResponse) @@ -3339,7 +3339,7 @@ async def edit_attachment_point_row( elif ap.static_connection_id: ctx["connection_type"] = "static_connection" else: - ctx["connection_type"] = "" + ctx["connection_type"] = "manual" return templates.TemplateResponse( request, "admin/_attachment_point_row_edit.html", ctx ) diff --git a/tests/test_attachment_points_api.py b/tests/test_attachment_points_api.py new file mode 100644 index 0000000..59b8525 --- /dev/null +++ b/tests/test_attachment_points_api.py @@ -0,0 +1,106 @@ +from app.models.attachment_point_name import AttachmentPointName +from app.models.augmented_part import AugmentedPart +from app.models.iso_thread import IsoThread +from app.models.manufacturer import Manufacturer +from app.models.part_colour import PartColour +from app.models.part_type import PartType + + +async def _seed_part(db): + manufacturer = Manufacturer(name="Acme", url="https://acme.test") + colour = PartColour(name="Black") + part_type = PartType(name="Flip Cover") + apn = AttachmentPointName(name="Mount") + iso = IsoThread(diameter_mm=52.0, pitch_mm=0.75) + db.add_all([manufacturer, colour, part_type, apn, iso]) + await db.flush() + + part = AugmentedPart( + description="Flip cover", + colour_id=colour.id, + manufacturer_id=manufacturer.id, + part_type_id=part_type.id, + ) + db.add(part) + await db.flush() + await db.commit() + return part.id, apn.id, iso.id + + +async def _token(client, email, password="secret123"): + resp = await client.post( + "/api/auth/login", data={"username": email, "password": password} + ) + assert resp.status_code == 200, resp.text + return resp.json()["access_token"] + + +async def test_create_manual_attachment_point_with_no_connection_succeeds( + client, seeded, make_user, db_sessionmaker +): + await make_user("root@example.com", is_superuser=True) + headers = {"Authorization": f"Bearer {await _token(client, 'root@example.com')}"} + async with db_sessionmaker() as db: + part_id, apn_id, _ = await _seed_part(db) + + resp = await client.post( + "/api/attachment-points", + json={ + "augmented_part_id": part_id, + "attachment_point_name_id": apn_id, + }, + headers=headers, + ) + assert resp.status_code == 201, resp.text + body = resp.json() + assert body["iso_thread_id"] is None + assert body["unc_thread_id"] is None + assert body["clamp_connection_id"] is None + assert body["static_connection_id"] is None + + +async def test_create_attachment_point_with_two_connections_rejected( + client, seeded, make_user, db_sessionmaker +): + await make_user("root2@example.com", is_superuser=True) + headers = {"Authorization": f"Bearer {await _token(client, 'root2@example.com')}"} + async with db_sessionmaker() as db: + part_id, apn_id, iso_id = await _seed_part(db) + from app.crud import static_connection as static_connection_crud + + static = await static_connection_crud.create(db, od_mm=40.0, od_in=1.57) + static_id = static.id + + resp = await client.post( + "/api/attachment-points", + json={ + "augmented_part_id": part_id, + "attachment_point_name_id": apn_id, + "iso_thread_id": iso_id, + "static_connection_id": static_id, + }, + headers=headers, + ) + assert resp.status_code == 400 + assert "at most one" in resp.json()["detail"].lower() + + +async def test_create_attachment_point_with_one_connection_still_works( + client, seeded, make_user, db_sessionmaker +): + await make_user("root3@example.com", is_superuser=True) + headers = {"Authorization": f"Bearer {await _token(client, 'root3@example.com')}"} + async with db_sessionmaker() as db: + part_id, apn_id, iso_id = await _seed_part(db) + + resp = await client.post( + "/api/attachment-points", + json={ + "augmented_part_id": part_id, + "attachment_point_name_id": apn_id, + "iso_thread_id": iso_id, + }, + headers=headers, + ) + assert resp.status_code == 201, resp.text + assert resp.json()["iso_thread_id"] == iso_id diff --git a/tests/test_scope_import.py b/tests/test_scope_import.py index 67b5a0a..c97adce 100644 --- a/tests/test_scope_import.py +++ b/tests/test_scope_import.py @@ -284,6 +284,41 @@ async def test_fit_reference_without_attachment_points_creates_matching_one( assert acc_aps[0].id in linked_ap_ids +async def test_scope_fit_without_scope_attachment_point_creates_manual_one( + db_sessionmaker, +): + async with db_sessionmaker() as db: + _, acc_ap = await _make_accessory(db, "ACC-1", od_mm=40) + + # No ocular_outer_diameter_mm, so the scope itself has no Ocular AP, + # but an ocular_fit reference is still given. + data = _workbook( + [_scope_row(ocular_outer_diameter_mm=None, ocular_fit="ACC-1")] + ) + result = await scope_import_service.import_from_bytes(db, data) + assert result.created == 1 + msgs = " ".join(result.rows[0].messages) + assert "created a manual one to record the fit" in msgs + + part = await augmented_part_crud.get_by_sku(db, "SCOPE-A") + scope_aps = await attachment_point_crud.list_for_augmented_part(db, part.id) + by_label = {ap.attachment_point_name.name: ap for ap in scope_aps} + assert "Ocular" in by_label + ocular_ap = by_label["Ocular"] + assert ocular_ap.iso_thread is None + assert ocular_ap.unc_thread is None + assert ocular_ap.clamp_connection is None + assert ocular_ap.static_connection is None + # Objective still gets its normal thread-based AP. + assert "Objective" in by_label + + fits = await fit_crud.list_for_part(db, part.id) + assert len(fits) == 1 + linked_ap_ids = {fits[0].attachment_point_a_id, fits[0].attachment_point_b_id} + assert ocular_ap.id in linked_ap_ids + assert acc_ap.id in linked_ap_ids + + async def test_unc_objective_thread_creates_unc_attachment_point(db_sessionmaker): async with db_sessionmaker() as db: data = _workbook([_scope_row(objective_thread="1.750-32 UNS")])