Skip to content

fix: stop Project.save() from wiping ProjectComponent edges - #53

Merged
peterus merged 12 commits into
mainfrom
fix/project-save-corruptor
Sep 22, 2026
Merged

peterus merged 12 commits into
mainfrom
fix/project-save-corruptor

Conversation

@peterus

@peterus peterus commented Sep 22, 2026

Copy link
Copy Markdown
Owner

Critical bug (production data corruptor)

A leftover Phase-6 dual-write shim in Project.save() deleted a project's
composition edges on every save:

if self.parent_id is None:
    ProjectComponent.objects.filter(child_project=self).delete()
    return

The legacy parent FK is always None for edge-based projects, so this branch
always fired. Any plain GUI "edit project" or API PATCH (e.g. of
default_print_preset) silently wiped the assembly's parent ProjectComponent
edges. Observed in production: Truck-24 and Kipper-23 module edges were lost
(restored manually via ORM).

Fix

Project.save() no longer deletes edges. Edge reconciliation is now seed on
insert only
: when a project is inserted with the legacy parent FK set, one
matching ProjectComponent edge is created if missing (inside one
transaction.atomic() block with the row insert), so Project(parent=…) code
paths still appear under their assembly. A later save/PATCH never deletes an
edge, overwrites an edge quantity, or re-runs the mirror — the composition graph
is authoritative once the row exists, so detach/re-parent through the
ProjectComponent edge UI is durable.

Consistency across the other legacy write paths:

  • ProjectEditForm no longer exposes the parent/quantity re-parent control
    (a single FK can't represent a multi-parent module, and editing an edge-based
    module through it was the primary corruption trigger).
  • ProjectAdmin excludes parent/quantity.
  • SubProjectCreateView sets the legacy parent FK (so preset inheritance keeps
    working) and relies on the insert mirror to create the edge.
  • ProjectComponentDeleteView clears the child's stale legacy parent FK on
    detach, so the former parent stays deletable (on_delete=PROTECT).

No migration — behavioural change only; no fields or constraints change.

Test evidence (TDD)

Regression tests: saving an edge-based project keeps its incoming / outgoing /
shared-module edges; an API PATCH keeps the edges; a scalar save neither
clobbers an edge quantity nor recreates a detached edge; sub-project create makes
an edge; project edit leaves edges untouched; detach clears the stale legacy FK;
the admin form omits parent/quantity. Full affected set green; ruff +
makemigrations --check clean.

Deferred follow-up (separate PR)

Remove the dead Project.parent / Project.quantity fields + the
project_quantity_gte_1 constraint + the insert mirror, and move
effective_default_print_preset(_id) off the legacy parent chain onto the
composition edges (full multi-parent preset inheritance). That is a semantic
change (a shared module has multiple parents), so it needs its own design — kept
out of this urgent corruption fix by design.

A leftover Phase-6 dual-write shim in Project.save() deleted the
composition edges of any project whose legacy `parent` FK was None —
which is every edge-based project. So a plain GUI "edit project" or an
API PATCH (e.g. of default_print_preset) silently ran
`ProjectComponent.objects.filter(child_project=self).delete()` and wiped
the assembly's parent edges (observed: Truck-24 / Kipper-23 module edges
lost). The docstring even said "Removed in the contract phase" — it never
was.

Fix: Project.save() no longer touches ProjectComponent at all. It only
runs the legacy `parent`-FK acyclic guard and super().save(). The
composition graph is authoritative and edited solely through its own edge
models (ProjectComponent/ProjectPart). Drops the now-unused
transaction/ProjectComponent imports.

No migration: this is behavioural only; no fields or constraints change.

Regression tests (fail before, pass after):
- test_composition.ProjectSaveDoesNotTouchEdgesTests — saving a project
  keeps its incoming/outgoing/shared-module ProjectComponent edges.
- test_api_projects.test_patch_project_keeps_component_edges — an API
  PATCH keeps the edges.

Deferred follow-up (separate PR): remove the dead Project.parent /
Project.quantity fields + the project_quantity_gte_1 constraint, and
migrate effective_default_print_preset(_id) off the legacy parent chain
onto the edge graph (semantic change — a shared module has multiple
parents — so it needs its own design).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

GUI sub-project creation and re-parenting can leave legacy fields and composition edges inconsistent.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 High severity

Open (1)
What changed in this PR

This PR prevents Project.save() from deleting ProjectComponent edges and adds regression coverage for direct saves and API updates.

Changes:

  • Removes legacy edge reconciliation from Project.save().
  • Adds save and API PATCH edge-preservation tests.
  • Removes unused transaction and model imports.
File Summary
core/​tests/​test_composition.py Adds tests confirming composition edges survive project saves.
core/​tests/​test_api_projects.py Adds API PATCH edge-preservation coverage.
core/​models/​projects.py Removes destructive edge mutation; GUI sub-project workflows still rely on legacy fields without synchronizing edges.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread core/models/projects.py Outdated
…e-only mirror

Copilot flagged that dropping the save() reconciliation entirely broke the
legacy GUI composition flows (SubProjectCreateView set the parent FK and
ProjectEditForm exposed parent/quantity, both relying on the shim to mirror an
edge), leaving the FK and the composition graph divergent.

Resolution:
- Project.save() now reconciles edges ADDITIVELY only: when the legacy parent FK
  is set it upserts the one matching ProjectComponent edge; it NEVER deletes
  edges. This still stops the corruptor (the unconditional
  filter(child_project=self).delete() is gone) while keeping every
  Project(parent=…) code path — fixtures and SubProjectCreateView — producing a
  consistent edge.
- ProjectEditForm no longer exposes the legacy parent/quantity re-parent control.
  A single parent field cannot represent a module shared by several assemblies,
  and editing an edge-based module through it was the primary corruption trigger.
  Re-parenting/detaching is done through the ProjectComponent edge UI, which owns
  edge deletion.
- SubProjectCreateView creates the ProjectComponent edge explicitly (forward
  compatible with the eventual parent-FK removal).
- ProjectUpdateView drops the now-unused parent-queryset filtering.

Tests: edge-based assertions for the GUI flows (subproject-create makes an edge;
project-edit leaves edges untouched and has no parent/quantity fields); removed
obsolete ProjectEditForm cycle tests (cycle protection lives on the edge layer,
covered in test_composition). Full affected set (composition, api_projects,
views_projects, forms, aggregation): 142 tests OK; ruff + makemigrations --check
clean.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

A critical non-atomic save path and two moderate edge-based subproject regressions remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 High severity

Open (1)
Resolved since last review (1)

Comment thread core/models/projects.py Outdated
@github-actions

github-actions Bot commented Sep 22, 2026

Copy link
Copy Markdown

Preview image ready

By tag:

docker pull ghcr.io/peterus/layernexus:pr-53

By digest:

docker pull ghcr.io/peterus/layernexus@sha256:d5277ddf337777fb3089225ac2d86b1c95c011eecc1ff9f037dea7c4b68fcae9

Commit: 4a5552ccc1768078fbbe764c72ecd16e1fa380ff

@peterus
peterus requested a lite review from Copilot September 22, 2026 16:57
…oject.save()

Copilot follow-up: the additive edge mirror does two dependent writes
(super().save() persists the parent FK row, then ProjectComponent.update_or_create
upserts the mirrored edge). Without a transaction a failure of the edge upsert
would leave the FK row committed but the edge missing. Wrap both writes in one
transaction.atomic() block (the acyclic guard stays before it) so they commit or
roll back together.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Unresolved issues remain around quantity synchronization, atomic creation, Cancel navigation, and print-preset inheritance.

Review effort: Lite
Findings: None

Resolved since last review (1)
Previously missed (3)

In code that hasn't changed since last review

Medium severity Stale legacy quantity overwrites updated composition edge quantity

core/​models/​projects.py:125

Once a legacy-parent project has a composition edge, the edge UI can change ProjectComponent.quantity without changing Project.quantity (ProjectComponentUpdateView only saves the edge). A later scalar Project.save() therefore enters this branch and update_or_create(... defaults=...) overwrites the authoritative edge quantity with the stale legacy value, so an ordinary edit silently reverts quantity changes. Seed only missing mirrors (for example, use get_or_create) or remove this legacy write path, and add a regression test for editing a legacy-parent project after its edge quantity changes.

Medium severity Null legacy parent breaks child edit-page Cancel URL

core/​views/​projects.py:250

This path deliberately leaves the new child's legacy parent_id unset, so is_subproject is true only through the new parent_links edge. However, core/templates/core/project_form.html:58-60 still builds the edit-page Cancel URL from form.instance.parent.pk; opening the edit page for a child created here therefore passes a null ID to the integer URL and can raise NoReverseMatch. Update the template/view to use an edge-based parent (or a safe project-list/used-in fallback) for this link.

Medium severity Project and composition edge creation are not atomic

core/​views/​projects.py:250

super().form_valid(form) saves the new Project before this edge insert runs, so the node and its authoritative composition edge are not atomic. If the edge insert fails (for example because the parent is concurrently removed or a database constraint rejects the edge), the request can leave an orphan project behind. Wrap the form save and ProjectComponent.objects.create() in one transaction.atomic() block.

@peterus

peterus commented Sep 22, 2026

Copy link
Copy Markdown
Owner Author

Addressed all three findings from the last review (commit 5691a26):

  1. Stale legacy quantity clobbering the edge quantityProject.save() now uses get_or_create instead of update_or_create, so the mirror only seeds a missing edge and never overwrites an edge quantity changed through the edge UI. Added a regression test (test_save_does_not_clobber_edge_quantity_of_legacy_parent).
  2. Null legacy parent breaking the child edit-page Cancel URLproject_form.html now builds the Cancel link from the edge-based used_in assemblies (fallback: project list), instead of form.instance.parent.pk (which is null for edge-created sub-projects).
  3. Non-atomic project + edge creation in SubProjectCreateView — both writes are now wrapped in transaction.atomic().

Affected test modules (composition, api_projects, views_projects): 65 tests OK; ruff clean.

@peterus
peterus requested a lite review from Copilot September 22, 2026 17:05
…omic subproject create

Three medium findings from Copilot's review body:

1. Stale legacy quantity clobbered the edge quantity. Project.save() used
   update_or_create(defaults={"quantity": self.quantity}), so after the edge UI
   changed ProjectComponent.quantity (without touching the legacy
   Project.quantity), any later scalar Project.save() reverted it. Switched to
   get_or_create — the mirror only seeds a missing edge and never overwrites an
   existing edge's quantity. Added a regression test.

2. Null legacy parent broke the child edit-page Cancel link. project_form.html
   built the URL from form.instance.parent.pk, which is None for edge-created
   sub-projects (is_subproject is true via the edge), risking NoReverseMatch. It
   now uses the edge-based used_in assemblies (falling back to the project list).

3. SubProjectCreateView created the project and its edge non-atomically. Wrapped
   both in transaction.atomic() so a failed edge create rolls back the project —
   no orphan node.

Affected modules (composition, api_projects, views_projects): 65 tests OK; ruff
clean.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

The legacy parent mirror can recreate detached edges, and edge-only sub-project creation breaks legacy preset inheritance.

Review effort: Lite
Findings: None

Previously missed (1)

In code that hasn't changed since last review

Medium severity Prevent detached legacy edges from being recreated on save

core/​models/​projects.py:128

Because the edge-delete view removes only the ProjectComponent row (core/views/projects.py:475-489), a legacy child whose parent_id is still set will have the detached edge recreated on its next scalar save or API PATCH by this get_or_create. That makes edge-authoritative detach/reparenting non-durable for legacy-backed projects; either clear the matching legacy FK when detaching or stop this mirror after migrating those rows.

@peterus

peterus commented Sep 22, 2026

Copy link
Copy Markdown
Owner Author

Addressed the R4 finding (commit 98a8ee4):

Detached legacy edge recreated on save — the mirror now runs on insert only (guarded by self._state.adding). It still seeds an edge for a project created via Project(parent=…), but a later scalar save / API PATCH never re-runs it, so detaching through the edge UI is durable for legacy-parent_id projects too. Added a regression test (a detached legacy edge is not recreated on a subsequent save). 144 tests OK across the affected modules.

Re the review-summary note about preset inheritance for edge-only sub-projects: effective_default_print_preset(_id) still walks the legacy parent chain, so an edge-only sub-project won't inherit its assembly's preset until that traversal is moved onto the composition edges. That migration (plus removing the legacy parent/quantity fields) is the explicit deferred follow-up called out in the PR description — kept out of this urgent corruption fix by design.

@peterus
peterus requested a lite review from Copilot September 22, 2026 17:15
Copilot: the edge-delete view removes only the ProjectComponent row, so a legacy
child whose parent_id is still set had its detached edge resurrected by the
get_or_create mirror on the next scalar save / API PATCH — making
edge-authoritative detach/re-parent non-durable.

Fix: the mirror now runs on INSERT only (guarded by self._state.adding). It still
seeds an edge for a project created via Project(parent=…), but a later save never
re-runs it, so detaching through the edge UI is durable. Added a regression test
(a detached legacy edge is not recreated on a subsequent save).

Affected modules (composition, aggregation, api_projects, views_projects, forms):
144 tests OK; ruff clean.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Moderate issues remain around admin writes, legacy FK deletion behavior, and preset inheritance.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 Medium severity

Open (1)
Previously missed (1)

In code that hasn't changed since last review

Medium severity New sub-projects lose preset inheritance without legacy parent FK

core/​views/​projects.py:256

This create path no longer sets the legacy parent FK, but Project.effective_default_print_preset and _id still walk only that FK (core/models/projects.py:287-326). A new sub-project with no own preset therefore stops inheriting the parent assembly's preset, despite the follow-up explicitly deferring that graph migration. Preserve the legacy FK for this transitional create path and let the seed-on-create bridge create the edge, or migrate the preset lookup in this change.

Comment thread core/models/projects.py
@peterus
peterus requested a lite review from Copilot September 22, 2026 17:26
…Admin

With the save() mirror now seeding on insert only, the default ProjectAdmin (no
fields/exclude declared) was the last remaining write path that could change the
legacy parent/quantity on an existing project without touching the authoritative
ProjectComponent edge — diverging the legacy row from the composition graph.

Exclude ("parent", "quantity") from ProjectAdmin, consistent with removing them
from ProjectEditForm; composition is edited through the edge models. Added a test
asserting the admin change form omits both fields.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Unresolved model lifecycle, preset inheritance, and navigation issues remain before approval.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 High severity

Open (1)
Resolved since last review (1)
Previously missed (2)

In code that hasn't changed since last review

Medium severity Removing legacy parent breaks preset inheritance

core/​views/​projects.py:255

Removing the legacy parent assignment here leaves every GUI-created sub-project with parent_id=NULL. Project.effective_default_print_preset still walks only self.parent (core/models/projects.py:298-306), so a child without its own preset no longer inherits the parent assembly's preset; this is an observable regression before the deferred edge-based preset migration. Preserve the legacy bridge for this create path without duplicating the edge, or include the preset-inheritance migration in this change.

Low severity Implementation contradicts documented Project.save behavior

core/​models/​projects.py:127

The PR description says Project.save() no longer touches ProjectComponent and removes the transaction/ProjectComponent imports, but this implementation still opens transaction.atomic() and creates a ProjectComponent on insert. That seed-on-create bridge is relied on by the new tests and existing parent-based creation paths, so the implementation and description specify different contracts; update the description or change the code/tests as a deliberate choice.

Comment thread core/models/projects.py
… on detach

Three findings around the legacy parent-FK seam:

1. (medium) Removing the legacy parent assignment in SubProjectCreateView left
   every GUI-created sub-project with parent_id=NULL, so
   Project.effective_default_print_preset (which still walks self.parent) no
   longer inherited the assembly's preset — an observable regression ahead of the
   deferred edge-based preset migration. Reverted the view to set the legacy
   parent FK again; Project.save() mirrors it into the ProjectComponent edge on
   insert (inside its own atomic block), so the node and edge are still created
   together and the form quantity becomes the edge quantity.

2. (high) Detaching a module through the edge UI left the child's legacy parent_id
   stale. Because Project.parent is on_delete=PROTECT, deleting the former parent
   then raised ProtectedError even though the composition graph said the child was
   detached. ProjectComponentDeleteView now clears the child's parent FK (via a
   queryset update, so Project.save() does not re-seed the edge) when the removed
   edge was that FK's mirror. Added a regression test.

3. (low) PR description was stale (an earlier revision claimed save() no longer
   touches ProjectComponent). Updated separately on the PR.

The full multi-parent preset inheritance + legacy parent/quantity removal remains
the documented deferred follow-up. Affected modules (views_projects, composition,
forms): 109 tests OK; ruff + makemigrations --check clean.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@peterus

peterus commented Sep 22, 2026

Copy link
Copy Markdown
Owner Author

Also addressed the two body findings in 7c62575:

  • Removing legacy parent breaks preset inheritance (medium) — reverted SubProjectCreateView to set the legacy parent FK again, so effective_default_print_preset (which still walks self.parent) keeps inheriting the assembly's preset. Project.save() mirrors that FK into the ProjectComponent edge on insert, so the node + edge are still created together. Full multi-parent, edge-based preset inheritance remains the documented deferred migration.
  • Implementation contradicts documented save() behavior (low) — updated the PR description to match the current seed-on-insert implementation (an earlier revision claimed save() no longer touches ProjectComponent).

@peterus
peterus requested a lite review from Copilot September 22, 2026 17:41

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

API edge deletion leaves a stale legacy parent, and web detach cleanup is not atomic.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 Medium severity

Open (1)
Resolved since last review (1)

Comment thread core/views/projects.py Outdated
ProjectComponentDeleteView deleted the edge and cleared the child's stale legacy
parent FK as two separate autocommit writes; if the FK update failed after the
edge delete committed, the child kept a stale parent_id (the exact inconsistency
the clear is meant to prevent). Wrap both in transaction.atomic() so they commit
or roll back together.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

API edge deletion or reassignment can leave stale legacy parent references, so the shared/API mutation path needs cleanup and regression coverage.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 2 High severity

Open (2)
Resolved since last review (1)

Comment thread core/models/projects.py
Comment thread core/views/projects.py Outdated
@peterus
peterus requested a lite review from Copilot September 22, 2026 17:58
…path

The detach-time legacy-FK cleanup lived only in the HTML ProjectComponentDeleteView,
so the DRF ProjectComponentDetail (generic RetrieveUpdateDestroy) bypassed it — an
API edge delete left the child's parent_id stale, again blocking deletion of the
former parent (on_delete=PROTECT).

Move the logic into ProjectComponent.delete(): it deletes the edge and, in the same
transaction.atomic() block, clears the child's legacy parent FK when it mirrored this
edge (queryset update, so Project.save does not re-seed). Every delete path — HTML
view, DRF API, shell — now detaches durably. Removed the now-redundant HTML-view
override. Added an API-path regression test.

Affected modules (api_edges, views_projects, composition): 71 tests OK; ruff clean.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Unresolved critical bulk-delete cleanup and moderate edge re-parenting issues block approval.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 High severity

Open (1)
Resolved since last review (2)

Comment thread core/models/composition.py Outdated
@peterus
peterus requested a lite review from Copilot September 22, 2026 18:09
…s via post_delete

ProjectComponent.delete() (instance-level) was bypassed by Django's bulk
QuerySet.delete() and cascade deletes, so a bulk detach (e.g.
child.parent_links.all().delete()) removed the edge but left the child's legacy
parent_id set — again blocking deletion of the former parent (on_delete=PROTECT)
and leaking preset inheritance.

Replace the instance-level override with a post_delete signal on ProjectComponent.
Django emits post_delete for every deleted object — instance.delete(), bulk
QuerySet.delete(), and cascades — inside the delete's own transaction, so the FK
cleanup now runs on all detach paths uniformly (queryset update, so Project.save
does not re-seed). Added a bulk-delete regression test.

Affected modules (composition, api_edges, views_projects): 72 tests OK; ruff +
makemigrations --check clean.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Unresolved critical stale-FK behavior and moderate edge re-parenting and insert-detection issues block approval.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 High severity · 1 Low severity

Open (2)
Resolved since last review (1)

Comment thread core/models/composition.py
Comment thread core/views/projects.py Outdated
… reload

- (low) ProjectComponentDeleteView.get_success_url docstring referenced a
  ProjectComponent.delete() override that no longer exists; point maintainers at
  the post_delete receiver (_clear_legacy_parent_on_component_delete) instead.
- (medium) test_save_does_not_recreate_a_detached_legacy_edge saved a stale
  in-memory instance whose parent_id was still set (the post_delete signal cleared
  it in the DB, not on the loaded object), so a full save() would write the stale
  FK back. Reload the child after detach (as a separate request does) and assert
  parent_id stays null. The residual "hold a pre-detach instance and re-save it"
  divergence is inherent to the legacy parent FK and goes away with its removal
  (the documented deferred migration); no app flow re-saves such an instance.

core.tests.test_composition: 18 tests OK; ruff clean.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

A critical API edge-reassignment path can leave stale legacy parent linkage and must be fixed atomically.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 High severity · 1 Low severity

Open (2)
Resolved since last review (2)

Comment thread core/models/composition.py
Comment thread core/tests/test_api_edges.py Outdated
(high) ProjectComponentSerializer exposed child_project as writable, and
ProjectComponentDetail uses DRF's generic update, so a PATCH could reassign an
edge from one child to another. That update emits no post_delete, so the old
child kept a stale legacy parent_id (blocking deletion of the former parent and
leaking preset inheritance) — and reassigning an edge's endpoint isn't a
meaningful operation anyway. Make child_project read-only on update (only quantity
is mutable); change composition by deleting and recreating edges. Added a test
that a PATCH cannot reassign the child.

(low) Corrected a stale test docstring that still referenced a
ProjectComponent.delete() override (the cleanup is the post_delete receiver).

core.tests.test_api_edges / test_api_projects: 15 tests OK; ruff clean.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Moderate API schema consistency and bulk-detach performance issues remain unresolved.

Review effort: Lite
Findings: None

Resolved since last review (2)

@peterus
peterus merged commit 770a1e0 into main Sep 22, 2026
6 checks passed
@peterus
peterus deleted the fix/project-save-corruptor branch September 22, 2026 18:45
peterus added a commit that referenced this pull request Sep 22, 2026
… preset (Copilot #54 r5 + #53 merge)

- _estimate_part_in_background consumes estimation_requested_preset conditionally
  on the value it read, so a newer project re-estimate racing the consume is not lost.
- test fixtures updated for the now-mandatory preset: AddPartToJobTargetAssembly part
  gets an override; CreateJobsFromProjectView project gets a default; subproject_create
  POST carries a preset. Resolves the #53 merge (ProjectEditForm no longer exposes
  parent/quantity) against the preset-required forms.
peterus added a commit that referenced this pull request Sep 22, 2026
… default) (#54)

* docs: spec + implementation plan for project preset resolution

* feat: per-build-path print preset resolver (Variant B)

* feat: add PrintJob.print_preset field (schema only)

* feat: add Part.estimated_with_preset field (schema only)

* feat: slice jobs with pinned PrintJob.print_preset

* feat: project-path job creation resolves + pins preset (Variant B)

* feat: estimation resolves preset per Variant B, refuses to guess when ambiguous

* feat: re-estimate entry points (GUI + API) resolve preset in context

* feat: require default_print_preset on project forms

* feat: require default_print_preset on ProjectSerializer

* feat: part-path job creation resolves preset with dropdown fallback

* feat: job detail shows pinned print preset

* docs: document mandatory project preset + Variant B resolution

* test: add mandatory default_print_preset to RBAC project-create tests

* fix: project-context estimation + preset provenance clearing (Copilot #54)

- Project.resolve_estimation_preset_map resolves each part's preset in the
  project's build context; whole-project re-estimate (GUI + API) pins it so a
  legacy/shared part is estimated with the project-context preset instead of
  being flagged context-free-ambiguous.
- _estimate_part_in_background honors the pinned requested preset and clears
  estimated_with_preset provenance on ambiguous/none results.
- Single-part re-estimate paths clear estimated_with_preset (no build context).

* fix: thread resolved/pinned preset through job compatibility + creation (Copilot #54)

- CreateJobsFromProjectView groups per (part, resolved-preset) with greedy
  printed allocation, so a shared part reached via two modules with different
  nearest presets splits into two bundles instead of collapsing to the last path.
- AddPartToJobView validates the posted dropdown preset against the offered
  candidates, compares the resolved incoming preset against the job's pinned
  preset, and pins an unpinned draft on first add.
- PartDetailView draft-job filter offers only jobs whose pinned preset the part
  can resolve to.

* fix: dedicated queue-time preset channel + printed-by-preset allocation (Copilot #54 r2)

- Add Part.estimation_requested_preset (transient, consumed by the worker on
  claim) so project-context estimation no longer overloads estimated_with_preset;
  a part edit that clears estimates can no longer leave a stale preset the worker
  would reuse (Copilot A/C).
- CreateJobsFromProjectView subtracts completed prints grouped by the producing
  job's pinned preset (Part.printed_quantity_for_by_preset), with legacy unpinned
  prints allocated greedily, so prints under one preset no longer deplete another
  preset's bundle (Copilot B).

* fix: legacy-job preset fallback + reject unresolved-preset add + clear stale request on edit (Copilot #54 r3)

- _slice_job_in_background falls back to first_part.effective_print_preset when
  job.print_preset is NULL, so legacy jobs (pre-pinning) don't slice with None.
- AddPartToJobView rejects adding a part with no resolvable preset (no override,
  no project default) instead of creating an unsliceable None-preset job.
- PartUpdateView and the API STL-upload path clear estimation_requested_preset
  (and provenance) when inputs change, so a project-context request queued before
  the edit can't fire with a stale preset.

* fix: conditional estimate completion + atomic empty-draft pin + skip unresolved project rows (Copilot #54 r4)

- _estimate_part_in_background writes the result only while the part is still
  ESTIMATING (conditional update), so an in-flight result can't clobber a newer
  re-estimation queued while slicing was running.
- AddPartToJobView does the compat-check, empty-draft pin and part insert in one
  transaction; the pin is a conditional (print_preset__isnull) update so two
  concurrent adds can't mix presets into one job.
- CreateJobsFromProjectView skips part paths with no resolvable preset instead of
  creating a None-preset job that would fail slicing (warns how many).

* test: mirror worker ESTIMATING-claim + mandatory project preset in CreateJobs/estimation tests

* fix: conditional request-preset consume + test fixtures for mandatory preset (Copilot #54 r5 + #53 merge)

- _estimate_part_in_background consumes estimation_requested_preset conditionally
  on the value it read, so a newer project re-estimate racing the consume is not lost.
- test fixtures updated for the now-mandatory preset: AddPartToJobTargetAssembly part
  gets an override; CreateJobsFromProjectView project gets a default; subproject_create
  POST carries a preset. Resolves the #53 merge (ProjectEditForm no longer exposes
  parent/quantity) against the preset-required forms.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants