feat: Create section model#721
Conversation
🤖 Augment PR SummarySummary: Adds a new SQLAlchemy 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
|
|
||
| class Section(Base): | ||
| __tablename__ = "section" | ||
| __table_args__ = (UniqueConstraint("display_order", name="uq_section_display_order"),) |
There was a problem hiding this comment.
UniqueConstraint("display_order") combined with a constant default for display_order means inserting multiple Section rows without explicitly setting display_order will quickly hit a uniqueness violation. If ordering is meant to be per form cycle, this global uniqueness constraint also blocks valid duplicates across different cycles.
Severity: high
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
There was a problem hiding this comment.
value:useful; category:bug; feedback:Marked useful because this comment caught an ordering constraint that would fail on normal section creation. It showed that two sections with default order or same order in different form cycles could hit a database uniqueness error.
| Uuid, ForeignKey("form_cycle.id"), index=True, nullable=True | ||
| ) | ||
| title: Mapped[str | None] = mapped_column(String(255), nullable=True) | ||
| display_order: Mapped[int] = mapped_column(Integer, default=0, server_default=text("1")) |
There was a problem hiding this comment.
display_order has default=0 but server_default=text("1"), which can lead to inconsistent persisted values depending on whether the ORM sends a value vs letting the DB fill it. This kind of mismatch is easy to miss and can break assumptions in ordering logic/tests.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
There was a problem hiding this comment.
value:useful; category:bug; feedback:This was useful because it found a default mismatch in a field used for section ordering. It pointed out that ORM inserts and database-default inserts could persist different order values, making list order behavior inconsistent.
| ) | ||
| title: Mapped[str | None] = mapped_column(String(255), nullable=True) | ||
| display_order: Mapped[int] = mapped_column(Integer, default=0, server_default=text("1")) | ||
| form_cycle: Mapped["FormCycle"] = relationship("FormCycle", back_populates="section") |
There was a problem hiding this comment.
This relationship("FormCycle", ...) (and the FK to form_cycle.id) requires a FormCycle mapper/table to exist in the same SQLAlchemy Base.metadata; in the current codebase there doesn’t appear to be a FormCycle model, so metadata/table creation or mapper configuration is likely to error when the models are loaded/used.
Severity: high
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
There was a problem hiding this comment.
value:useful; category:bug; feedback:Marked useful because this comment caught that the branch referenced a mapper and foreign table not present on this PR base. It meant loading SQLAlchemy metadata or configuring mappers could fail before the section model is usable.
|
augment review |
| id: Mapped[uuid.UUID] = mapped_column(Uuid, primary_key=True, default=uuid.uuid4) | ||
| form_cycle_id: Mapped[uuid.UUID] = mapped_column(Uuid, index=True, nullable=False) | ||
| title: Mapped[str | None] = mapped_column(String(255), nullable=True) | ||
| display_order: Mapped[int] = mapped_column(Integer, default=1, server_default=text("1")) |
There was a problem hiding this comment.
display_order is currently nullable (no nullable=False), which allows NULL values; this can bypass ordering assumptions and also undermine the uniqueness constraint (since multiple NULLs are typically allowed).
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
There was a problem hiding this comment.
value:useful; category:bug; feedback:Marked useful because this comment caught a nullable ordering field that can break section ordering. It pointed out that NULL display orders can bypass uniqueness and make section lists sort unpredictably.
| ) | ||
|
|
||
| id: Mapped[uuid.UUID] = mapped_column(Uuid, primary_key=True, default=uuid.uuid4) | ||
| form_cycle_id: Mapped[uuid.UUID] = mapped_column(Uuid, index=True, nullable=False) |
There was a problem hiding this comment.
form_cycle_id looks like it references another table, but there’s no DB-level foreign key constraint here; that can allow orphan Section rows and makes integrity dependent on application code.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
There was a problem hiding this comment.
value:valid-but-wont-fix; category:bug; feedback:Marked valid-but-wont-fix because the integrity concern is correct, but this PR base does not include the referenced form cycle table yet. Adding that foreign key now would make metadata creation depend on a table outside this slice and can break model loading.
|
augment review |
# Conflicts: # qualia/backend/app/models/__init__.py
1 participant
No description provided.