Skip to content

feat(template): implement work item template CRUD and instantiation#9391

Open
gouthamx67 wants to merge 3 commits into
makeplane:previewfrom
gouthamx67:feat/template-architectural-review
Open

feat(template): implement work item template CRUD and instantiation#9391
gouthamx67 wants to merge 3 commits into
makeplane:previewfrom
gouthamx67:feat/template-architectural-review

Conversation

@gouthamx67

@gouthamx67 gouthamx67 commented Jul 10, 2026

Copy link
Copy Markdown

Description

Adds the work item template feature, enabling users to create, update, delete,
and instantiate reusable templates for work items within a project. Templates
support nested items with dependency graphs, including cycle detection.

Type of Change

  • Bug fix
  • Feature (non-breaking change which adds functionality)
  • Improvement
  • Code refactoring
  • Performance improvements
  • Documentation update

Test Scenarios

  • Create, update, and delete template endpoints with items and dependencies
  • Instantiate a template into real work items
  • Dependency validation: self-dependency, invalid relation type, cross-template refs, duplicate deps, cycle detection
  • Nested item creation and resolution through single-request serializer flow
  • Deletion cascade from template to items and dependencies

References

No related issues — original feature submission.

Summary by CodeRabbit

  • New Features

    • Added work item templates with configurable items, priorities, issue types, descriptions, ordering, and dependencies.
    • Added API endpoints to create, view, update, delete, and instantiate templates.
    • Instantiating a template creates a parent issue, child issues, and their dependency relationships.
    • Added validation for invalid, duplicate, self-referencing, and circular dependencies.
  • Bug Fixes

    • Added transactional safeguards to prevent partial template instantiation when errors occur.

gouthamx67 and others added 3 commits July 10, 2026 04:25
Complete implementation of work item template support with:

- Three models: WorkItemTemplate, WorkItemTemplateItem, WorkItemTemplateDependency
- Full CRUD operations through WorkItemTemplateViewSet
- Instantiate action creating parent/child Issue hierarchy with IssueRelation dependencies
- Proper validation, transaction wrapping, and error handling
- Serialized endpoints with pagination and filtering
- Comprehensive test coverage

Integrates seamlessly with existing Plane backend patterns:
- Consistent permission classes (ProjectEntityPermission)
- Same authentication and workspace validation as other issues
- Follows IssueRelation semantics for dependencies
- Uses transaction.atomic for rollback on failure

Co-authored-by: openhands <openhands@all-hands.dev>
- Fix URL configuration: replace circular import with correct path(),
  add workspace/project URL prefix matching existing Plane patterns
- Add dependency validation: reject self-dependencies, duplicates,
  cross-template refs, invalid relation types via serializer validate()
- Add cycle detection: DFS-based circular dependency detection
- Secure get_queryset: add project membership filter matching other viewsets
- Map template fields: description_html, type_id, sort_order now copied
  during instantiation instead of being dropped
- Optimize queries: remove redundant Project query, avoid defeating
  prefetch_related with unnecessary order_by()
- Model cleanup: add IssueRelationChoices to relation_type, add ordering
- Expand tests: 7 new tests covering validation, cascade, URLs, field mapping
Replace PrimaryKeyRelatedField (auto-generated for FK fields) with a custom TemplateItemIdField that accepts arbitrary strings on write (for temp client-provided IDs) while outputting PK UUIDs on read. Pop temp IDs before model create to avoid UUIDField validation errors.
@CLAassistant

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


gouthamx67 seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@coderabbitai

coderabbitai Bot commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

Adds work item template persistence, nested item and dependency serializers, project-scoped CRUD routes, and atomic instantiation of parent and child issues with relations and activity events. Tests cover validation, updates, constraints, routing, permissions, rollback, and response mappings.

Changes

Work item templates

Layer / File(s) Summary
Template persistence model
apps/api/plane/db/models/template.py, apps/api/plane/db/models/__init__.py, apps/api/plane/db/migrations/0122_workitemtemplate_workitemtemplateitem_and_more.py
Adds template, item, and dependency models with UUIDs, audit fields, ordering, priorities, relationships, and active dependency uniqueness constraints.
Nested template serialization
apps/api/plane/app/serializers/template.py, apps/api/plane/app/serializers/__init__.py
Adds nested serializers, temporary item ID handling, dependency validation including cycle detection, creation, and item/dependency synchronization during updates.
Template API and instantiation
apps/api/plane/app/views/template.py, apps/api/plane/app/views/__init__.py, apps/api/plane/app/urls/template.py, apps/api/plane/app/urls/__init__.py
Adds project-scoped CRUD and instantiate routes, membership filtering, atomic issue creation, dependency relation creation, activity events, and response totals.
Template lifecycle coverage
apps/api/plane/tests/unit/template/test_instantiation.py
Tests serialization, validation, constraints, routes, permissions, updates, rollback, issue field mapping, relations, and instantiation responses.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant WorkItemTemplateViewSet
  participant WorkItemTemplate
  participant Issue
  participant IssueRelation
  participant issue_activity
  Client->>WorkItemTemplateViewSet: POST instantiate
  WorkItemTemplateViewSet->>WorkItemTemplate: Load template data
  WorkItemTemplateViewSet->>Issue: Create parent and child issues
  WorkItemTemplateViewSet->>IssueRelation: Create child dependency links
  WorkItemTemplateViewSet->>issue_activity: Enqueue activity events
  WorkItemTemplateViewSet-->>Client: Return issue IDs and totals
Loading
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title is concise and accurately summarizes the main change: work item template CRUD and instantiation.
Description check ✅ Passed The description covers the required sections and clearly explains the feature, type of change, tests, and references.
Docstring Coverage ✅ Passed Docstring coverage is 80.49% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with 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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@chatgpt-codex-connector chatgpt-codex-connector Bot 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.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 448a303811

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

project_id=project_id,
workspace_id=template.workspace_id,
name=template.name,
description_html=template.description or "<p></p>",

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Sanitize descriptions before creating issues

When a project member instantiates a template containing unsafe HTML in the template or item description, this path writes it directly to Issue.description_html, bypassing the HTML validation that normal issue creation performs. That leaves the created parent and child issues storing/rendering unvalidated markup from templates; validate or sanitize these descriptions before calling Issue.objects.create.

Useful? React with 👍 / 👎.

Comment on lines +182 to +183
dep_key = (source_id, target_id, relation_type)
if dep_key in seen:

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Reject duplicate item pairs regardless of relation

For payloads that include the same source/target pair twice with different relation_type values, this check treats them as distinct even though WorkItemTemplateDependency is unique only on template/source/target. The serializer then starts creating the template/items/dependencies and hits an IntegrityError partway through, so the API can return an error after leaving partial template data behind; the duplicate key should match the database uniqueness or the whole save should be atomic.

Useful? React with 👍 / 👎.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 5

🧹 Nitpick comments (3)
apps/api/plane/db/models/template.py (1)

23-34: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Extract duplicated priority choices into a shared definition.

The priority choices tuple is duplicated verbatim between WorkItemTemplate (lines 23-34) and WorkItemTemplateItem (lines 54-65). If priorities ever change, both must be updated in sync. Consider extracting to a TextChoices class or module-level constant.

Also applies to: 54-65

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@apps/api/plane/db/models/template.py` around lines 23 - 34, Extract the
duplicated priority choices from WorkItemTemplate and WorkItemTemplateItem into
a shared TextChoices class or module-level constant, then reference that
definition in both priority fields while preserving the existing values, labels,
and default.
apps/api/plane/app/serializers/template.py (1)

189-220: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

Avoid clearing visited/in_stack between DFS iterations.

Lines 215-216 clear visited and in_stack at the start of each outer loop iteration. This is unnecessary and makes cycle detection O(V × (V + E)) instead of O(V + E). The in_stack set is already correctly maintained by backtracking (line 211 removes nodes after exploring all neighbors), so it will be empty at the start of each iteration. The visited set should persist across iterations to skip fully-explored subtrees.

♻️ Proposed fix: remove redundant clearing
         for node_id in list(item_id_set):
-            visited.clear()
-            in_stack.clear()
             if dfs(node_id):
                 raise serializers.ValidationError(
                     {"dependencies": "Circular dependency detected. Dependencies must form a directed acyclic graph."}
                 )
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@apps/api/plane/app/serializers/template.py` around lines 189 - 220, Remove
the visited.clear() and in_stack.clear() calls from the outer loop in
_detect_cycles. Initialize both sets once before iterating through item_id_set,
allowing visited to persist across DFS traversals while in_stack is naturally
emptied by dfs backtracking, reducing cycle detection to linear time.
apps/api/plane/app/views/template.py (1)

131-163: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Defer celery task dispatch to post-commit with transaction.on_commit().

issue_activity.delay() is called inside transaction.atomic(). Celery workers may pick up tasks before the transaction commits, causing them to query for issues that don't yet exist. If a delay() call fails mid-loop, earlier queued tasks also reference uncommitted rows. Wrap each call in transaction.on_commit() to ensure tasks are only dispatched after a successful commit.

♻️ Proposed fix
             # 4. Fire activity events for child issues
             for item in template_items:
                 child_issue = item_to_issue_map[str(item.id)]
-                issue_activity.delay(
-                    type="issue.activity.created",
-                    requested_data=json.dumps(
-                        {"name": item.name, "priority": item.priority},
-                        cls=DjangoJSONEncoder,
-                    ),
-                    actor_id=str(request.user.id),
-                    issue_id=str(child_issue.id),
-                    project_id=str(project_id),
-                    current_instance=None,
-                    epoch=int(timezone.now().timestamp()),
-                    notification=True,
-                    origin=base_host(request=request, is_app=True),
+                transaction.on_commit(
+                    lambda item=item, child_issue=child_issue: issue_activity.delay(
+                        type="issue.activity.created",
+                        requested_data=json.dumps(
+                            {"name": item.name, "priority": item.priority},
+                            cls=DjangoJSONEncoder,
+                        ),
+                        actor_id=str(request.user.id),
+                        issue_id=str(child_issue.id),
+                        project_id=str(project_id),
+                        current_instance=None,
+                        epoch=int(timezone.now().timestamp()),
+                        notification=True,
+                        origin=base_host(request=request, is_app=True),
+                    )
                 )
 
             # 5. Fire activity event for parent issue
-            issue_activity.delay(
-                type="issue.activity.created",
-                requested_data=json.dumps(
-                    {"name": template.name, "priority": template.priority},
-                    cls=DjangoJSONEncoder,
-                ),
-                actor_id=str(request.user.id),
-                issue_id=str(parent_issue.id),
-                project_id=str(project_id),
-                current_instance=None,
-                epoch=int(timezone.now().timestamp()),
-                notification=True,
-                origin=base_host(request=request, is_app=True),
+            transaction.on_commit(
+                lambda: issue_activity.delay(
+                    type="issue.activity.created",
+                    requested_data=json.dumps(
+                        {"name": template.name, "priority": template.priority},
+                        cls=DjangoJSONEncoder,
+                    ),
+                    actor_id=str(request.user.id),
+                    issue_id=str(parent_issue.id),
+                    project_id=str(project_id),
+                    current_instance=None,
+                    epoch=int(timezone.now().timestamp()),
+                    notification=True,
+                    origin=base_host(request=request, is_app=True),
+                )
             )
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@apps/api/plane/app/views/template.py` around lines 131 - 163, issue_activity
tasks are dispatched inside the atomic transaction before created issues are
committed. In the child-issue loop and parent-issue dispatch within the relevant
view, wrap each issue_activity.delay invocation in transaction.on_commit(lambda:
...) so Celery tasks are queued only after a successful commit, preserving each
iteration’s item and issue values via correctly bound callback arguments.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In
`@apps/api/plane/db/migrations/0122_workitemtemplate_workitemtemplateitem_and_more.py`:
- Around line 80-84: Update the WorkItemTemplateDependency migration options to
include 'ordering': ('-created_at',), matching the model’s Meta ordering and the
corresponding options for the other models in this migration.

In `@apps/api/plane/db/models/template.py`:
- Around line 107-119: Remove the redundant unique_together declaration from the
model’s Meta class, and remove the corresponding unique_together constraint from
the associated migration. Keep the conditional UniqueConstraint named
unique_active_template_dependency to enforce uniqueness only for active
dependencies.

In `@apps/api/plane/tests/unit/template/test_instantiation.py`:
- Around line 290-328: Update test_transaction_rollback_on_failure to exercise
the instantiate view/action instead of manually creating issues inside
transaction.atomic(). Mock a failure during instantiation, such as
IssueRelation.objects.create raising an exception, invoke the template
instantiation request, and assert the request fails appropriately and
Issue.objects.count() remains equal to the initial count.
- Around line 242-257: Update test_instantiate_empty_template to call the
template instantiation endpoint or helper, such as _instantiate_template, after
creating the empty template, and assert it returns HTTP 400 with the expected
empty-template error. Remove the duplicate items.count() assertion while
preserving the setup verifying the template has no items.
- Around line 730-737: Replace the no-op assertion in test_deletion_cascade with
an exact expected count for WorkItemTemplateItem records after
template.delete(), based on the intended soft-delete cascade behavior; assert
either zero or the original item count so the test verifies whether related
items are deleted or retained.

---

Nitpick comments:
In `@apps/api/plane/app/serializers/template.py`:
- Around line 189-220: Remove the visited.clear() and in_stack.clear() calls
from the outer loop in _detect_cycles. Initialize both sets once before
iterating through item_id_set, allowing visited to persist across DFS traversals
while in_stack is naturally emptied by dfs backtracking, reducing cycle
detection to linear time.

In `@apps/api/plane/app/views/template.py`:
- Around line 131-163: issue_activity tasks are dispatched inside the atomic
transaction before created issues are committed. In the child-issue loop and
parent-issue dispatch within the relevant view, wrap each issue_activity.delay
invocation in transaction.on_commit(lambda: ...) so Celery tasks are queued only
after a successful commit, preserving each iteration’s item and issue values via
correctly bound callback arguments.

In `@apps/api/plane/db/models/template.py`:
- Around line 23-34: Extract the duplicated priority choices from
WorkItemTemplate and WorkItemTemplateItem into a shared TextChoices class or
module-level constant, then reference that definition in both priority fields
while preserving the existing values, labels, and default.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 6d5fbfef-5143-44a7-9c98-40d78be74dbc

📥 Commits

Reviewing files that changed from the base of the PR and between dc9d80b and 448a303.

📒 Files selected for processing (10)
  • apps/api/plane/app/serializers/__init__.py
  • apps/api/plane/app/serializers/template.py
  • apps/api/plane/app/urls/__init__.py
  • apps/api/plane/app/urls/template.py
  • apps/api/plane/app/views/__init__.py
  • apps/api/plane/app/views/template.py
  • apps/api/plane/db/migrations/0122_workitemtemplate_workitemtemplateitem_and_more.py
  • apps/api/plane/db/models/__init__.py
  • apps/api/plane/db/models/template.py
  • apps/api/plane/tests/unit/template/test_instantiation.py

Comment on lines +80 to +84
options={
'verbose_name': 'Work Item Template Dependency',
'verbose_name_plural': 'Work Item Template Dependencies',
'db_table': 'work_item_template_dependencies',
},

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win

Migration options missing ordering for WorkItemTemplateDependency.

The model defines ordering = ("-created_at",) (template.py line 111), but the migration's options dict for WorkItemTemplateDependency (lines 80-84) omits ordering. The other two models in this same migration correctly include their ordering (lines 36 and 61). This discrepancy will cause makemigrations to detect a pending change and generate a follow-up migration. Add 'ordering': ('-created_at',) to the options.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@apps/api/plane/db/migrations/0122_workitemtemplate_workitemtemplateitem_and_more.py`
around lines 80 - 84, Update the WorkItemTemplateDependency migration options to
include 'ordering': ('-created_at',), matching the model’s Meta ordering and the
corresponding options for the other models in this migration.

Comment on lines +107 to +119
class Meta:
verbose_name = "Work Item Template Dependency"
verbose_name_plural = "Work Item Template Dependencies"
db_table = "work_item_template_dependencies"
ordering = ("-created_at",)
unique_together = ["template", "source_template_item", "target_template_item", "deleted_at"]
constraints = [
models.UniqueConstraint(
fields=["template", "source_template_item", "target_template_item"],
condition=models.Q(deleted_at__isnull=True),
name="unique_active_template_dependency",
)
]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Locate the model and nearby migrations
git ls-files apps/api/plane/db/models/template.py apps/api/plane/db/migrations | sed 's#^`#FILE` #'

echo
echo "== template.py excerpt =="
nl -ba apps/api/plane/db/models/template.py | sed -n '90,140p'

echo
echo "== migrations touching work_item_template_dependencies or template dependency =="
rg -n "work_item_template_dependencies|unique_active_template_dependency|unique_together|AlterUniqueTogether|RemoveConstraint|template dependency" apps/api/plane/db/migrations -S

Repository: makeplane/plane

Length of output: 9401


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect the relevant migration file(s) if found
for f in $(rg -l "unique_active_template_dependency|work_item_template_dependencies|AlterUniqueTogether|unique_together" apps/api/plane/db/migrations -S); do
  echo
  echo "== $f =="
  nl -ba "$f" | sed -n '1,260p'
done

Repository: makeplane/plane

Length of output: 276


🏁 Script executed:

#!/bin/bash
set -euo pipefail

python3 - <<'PY'
from pathlib import Path
p = Path("apps/api/plane/db/models/template.py")
text = p.read_text()
start = text.index("class Meta:")
print(text[start:text.index("\n\n", start)+2])
PY

Repository: makeplane/plane

Length of output: 340


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Quick model-level search for any runtime code referencing this exact uniqueness tuple.
rg -n "source_template_item.*target_template_item.*deleted_at|unique_active_template_dependency|work_item_template_dependencies" apps/api -S

Repository: makeplane/plane

Length of output: 1127


Remove unique_together here and in the migration The conditional UniqueConstraint already enforces active-row uniqueness; keeping deleted_at in unique_together is redundant and can still reject two soft-deleted rows with the same timestamp.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@apps/api/plane/db/models/template.py` around lines 107 - 119, Remove the
redundant unique_together declaration from the model’s Meta class, and remove
the corresponding unique_together constraint from the associated migration. Keep
the conditional UniqueConstraint named unique_active_template_dependency to
enforce uniqueness only for active dependencies.

Comment on lines +242 to +257
def test_instantiate_empty_template(self, db, project, user):
"""
Test instantiating an empty template (no items)
"""
template = WorkItemTemplate.objects.create(
name="Empty Template",
project=project,
workspace=project.workspace,
created_by=user,
updated_by=user,
)

assert template.items.count() == 0

# Cannot instantiate empty template
assert template.items.count() == 0

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

test_instantiate_empty_template doesn't actually test instantiation.

The test creates an empty template but never calls _instantiate_template to verify the 400 response. The view's empty-template guard (lines 78-82 in template.py) is untested.

💚 Proposed fix
     def test_instantiate_empty_template(self, db, project, user):
         """
         Test instantiating an empty template (no items)
         """
         template = WorkItemTemplate.objects.create(
             name="Empty Template",
             project=project,
             workspace=project.workspace,
             created_by=user,
             updated_by=user,
         )
 
         assert template.items.count() == 0
 
-        # Cannot instantiate empty template
-        assert template.items.count() == 0
+        # Verify instantiation returns 400 for empty template
+        response = self._instantiate_template(template, user, project)
+        assert response.status_code == 400
+        assert "error" in response.data
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
def test_instantiate_empty_template(self, db, project, user):
"""
Test instantiating an empty template (no items)
"""
template = WorkItemTemplate.objects.create(
name="Empty Template",
project=project,
workspace=project.workspace,
created_by=user,
updated_by=user,
)
assert template.items.count() == 0
# Cannot instantiate empty template
assert template.items.count() == 0
def test_instantiate_empty_template(self, db, project, user):
"""
Test instantiating an empty template (no items)
"""
template = WorkItemTemplate.objects.create(
name="Empty Template",
project=project,
workspace=project.workspace,
created_by=user,
updated_by=user,
)
assert template.items.count() == 0
# Verify instantiation returns 400 for empty template
response = self._instantiate_template(template, user, project)
assert response.status_code == 400
assert "error" in response.data
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@apps/api/plane/tests/unit/template/test_instantiation.py` around lines 242 -
257, Update test_instantiate_empty_template to call the template instantiation
endpoint or helper, such as _instantiate_template, after creating the empty
template, and assert it returns HTTP 400 with the expected empty-template error.
Remove the duplicate items.count() assertion while preserving the setup
verifying the template has no items.

Comment on lines +290 to +328
def test_transaction_rollback_on_failure(
self, db, project, user
):
"""
Test database transaction rollback on failure
"""
template = self._create_minimal_template(db, project, user)

initial_count = Issue.objects.count()
item = template.items.first()

# Simulate a failure during instantiation by using a failing transaction
try:
with transaction.atomic():
parent_issue = Issue.objects.create(
project=project,
workspace=project.workspace,
name=template.name,
description_html="<p></p>",
priority=template.priority,
created_by=user,
updated_by=user,
)
Issue.objects.create(
project=project,
workspace=project.workspace,
parent=parent_issue,
name=item.name,
description_html="<p></p>",
priority=item.priority,
created_by=user,
updated_by=user,
)
raise Exception("Simulated error to test rollback")
except Exception:
pass

# Both issues should not exist due to rollback (count unchanged)
assert Issue.objects.count() == initial_count

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

test_transaction_rollback_on_failure doesn't test the view's rollback behavior.

The test manually creates issues in a raw transaction.atomic() block and raises an exception. It verifies Django's transaction semantics, not the instantiate action's rollback. To test the view, mock a failure during instantiation (e.g., patch IssueRelation.objects.create to raise) and assert no issues remain after the call.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@apps/api/plane/tests/unit/template/test_instantiation.py` around lines 290 -
328, Update test_transaction_rollback_on_failure to exercise the instantiate
view/action instead of manually creating issues inside transaction.atomic().
Mock a failure during instantiation, such as IssueRelation.objects.create
raising an exception, invoke the template instantiation request, and assert the
request fails appropriately and Issue.objects.count() remains equal to the
initial count.

Comment on lines +730 to +737
# Delete the template
template_id = template.id
template.delete()

# Verify template is soft-deleted (excluded from default queryset)
assert WorkItemTemplate.objects.filter(id=template_id).count() == 0
# Soft-delete does not cascade to related items by default
assert WorkItemTemplateItem.objects.filter(template_id=template_id).count() >= 0

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

test_deletion_cascade has a no-op assertion.

Line 737 asserts count() >= 0, which is always true. The test doesn't verify whether items are cascade-deleted or retained after template soft-delete. Replace with a specific count assertion to validate the actual cascade behavior.

💚 Proposed fix
         # Verify template is soft-deleted (excluded from default queryset)
         assert WorkItemTemplate.objects.filter(id=template_id).count() == 0
-        # Soft-delete does not cascade to related items by default
-        assert WorkItemTemplateItem.objects.filter(template_id=template_id).count() >= 0
+        # Verify cascade behavior: items should be deleted with the template
+        assert WorkItemTemplateItem.objects.filter(template_id=template_id).count() == 0
+        assert WorkItemTemplateDependency.objects.filter(template_id=template_id).count() == 0
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
# Delete the template
template_id = template.id
template.delete()
# Verify template is soft-deleted (excluded from default queryset)
assert WorkItemTemplate.objects.filter(id=template_id).count() == 0
# Soft-delete does not cascade to related items by default
assert WorkItemTemplateItem.objects.filter(template_id=template_id).count() >= 0
# Delete the template
template_id = template.id
template.delete()
# Verify template is soft-deleted (excluded from default queryset)
assert WorkItemTemplate.objects.filter(id=template_id).count() == 0
# Verify cascade behavior: items should be deleted with the template
assert WorkItemTemplateItem.objects.filter(template_id=template_id).count() == 0
assert WorkItemTemplateDependency.objects.filter(template_id=template_id).count() == 0
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@apps/api/plane/tests/unit/template/test_instantiation.py` around lines 730 -
737, Replace the no-op assertion in test_deletion_cascade with an exact expected
count for WorkItemTemplateItem records after template.delete(), based on the
intended soft-delete cascade behavior; assert either zero or the original item
count so the test verifies whether related items are deleted or retained.

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