⚙️ 45773 backend [ templates ] Add FieldSets#211
Conversation
…d/fieldsets endpoint
…off and FieldSetTemplate models
…ntermediate m2m models
…to backend/fieldsets/45773__fieldsets
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| if field.value not in self.NULL_VALUES: | ||
| total += float(field.value) | ||
| if total != float(self.instance.value): | ||
| raise FieldsetServiceException(MSG_FS_0002(self.instance.value)) |
There was a problem hiding this comment.
Float equality check causes spurious validation failures
Medium Severity
_validate_sum_equal sums field values as float and checks total != float(self.instance.value) using exact equality. Floating-point arithmetic accumulation (e.g., 0.1 + 0.2 ≠ 0.3) can cause valid inputs to fail validation. The template-side validator in fieldset_rule.py uses the same fragile pattern.
Reviewed by Cursor Bugbot for commit cd8ef70. Configure here.
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| def _link_rules( | ||
| self, | ||
| instance_template: FieldTemplate, | ||
| **kwargs, | ||
| ): | ||
|
|
||
| rule_api_names = set( | ||
| instance_template.rules.values_list('api_name', flat=True), | ||
| ) | ||
| rules = FieldSetRule.objects.filter( | ||
| account=self.account, | ||
| fieldset_id=kwargs['fieldset_id'], | ||
| api_name__in=rule_api_names, | ||
| ) |
There was a problem hiding this comment.
🟢 Low tasks/field.py:361
_link_rules accesses kwargs['fieldset_id'] unconditionally, which raises KeyError when fieldset_id is omitted from kwargs. This crashes when a template has rules but _create_instance was called without a fieldset (it uses kwargs.get('fieldset_id') at line 291, making it optional there). Consider using kwargs.get('fieldset_id') and handling the None case, or ensure fieldset_id is required consistently.
- rules = FieldSetRule.objects.filter(
- account=self.account,
- fieldset_id=kwargs['fieldset_id'],
- api_name__in=rule_api_names,
- )Also found in 2 other location(s)
backend/src/processes/serializers/workflows/kickoff_value.py:111
When creating
TaskFieldinstances for fields without fieldsets (lines 109-116),fieldset_idis not passed toTaskFieldService.create(). However,TaskFieldService._create_relatedchecksinstance_template.rules.all().exists()and calls_link_rules, which unconditionally accesseskwargs['fieldset_id'](visible in the references). If a field template without a fieldset has associated rules, this will raise aKeyError.
backend/src/processes/services/tasks/task.py:218
Calling
service.create()without passingfieldset_idwill cause aKeyErrorinTaskFieldService._link_rulesif the field template has rules. The_link_rulesmethod (added in this commit) accesseskwargs['fieldset_id']with direct dictionary access, butcreate_fields_from_templateonly passesworkflow_id,task_id, andskip_value. If any field template excluded from fieldsets has associated rules (field_template.rules.all().exists()is True), the code will crash.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file backend/src/processes/services/tasks/field.py around lines 361-374:
`_link_rules` accesses `kwargs['fieldset_id']` unconditionally, which raises `KeyError` when `fieldset_id` is omitted from kwargs. This crashes when a template has rules but `_create_instance` was called without a fieldset (it uses `kwargs.get('fieldset_id')` at line 291, making it optional there). Consider using `kwargs.get('fieldset_id')` and handling the None case, or ensure fieldset_id is required consistently.
Evidence trail:
backend/src/processes/services/tasks/field.py line 291: `fieldset_id=kwargs.get('fieldset_id')` (optional); line 373: `fieldset_id=kwargs['fieldset_id']` (required, raises KeyError if missing); line 325: conditional call to `_link_rules` when `instance_template.rules.all().exists()`; backend/src/processes/services/tasks/task.py lines 217-222: `service.create()` called without `fieldset_id`; backend/src/generics/base/service.py line 66-69: `create()` passes same kwargs to both `_create_instance` and `_create_related`; backend/src/processes/models/templates/fields.py line 68-71: `rules = models.ManyToManyField('processes.FieldsetTemplateRule', ...)` on FieldTemplate.
Also found in 2 other location(s):
- backend/src/processes/serializers/workflows/kickoff_value.py:111 -- When creating `TaskField` instances for fields without fieldsets (lines 109-116), `fieldset_id` is not passed to `TaskFieldService.create()`. However, `TaskFieldService._create_related` checks `instance_template.rules.all().exists()` and calls `_link_rules`, which unconditionally accesses `kwargs['fieldset_id']` (visible in the references). If a field template without a fieldset has associated rules, this will raise a `KeyError`.
- backend/src/processes/services/tasks/task.py:218 -- Calling `service.create()` without passing `fieldset_id` will cause a `KeyError` in `TaskFieldService._link_rules` if the field template has rules. The `_link_rules` method (added in this commit) accesses `kwargs['fieldset_id']` with direct dictionary access, but `create_fields_from_template` only passes `workflow_id`, `task_id`, and `skip_value`. If any field template excluded from fieldsets has associated rules (`field_template.rules.all().exists()` is True), the code will crash.
| class FieldSet( | ||
| BaseApiNameModel, | ||
| BaseFieldSetMixin, | ||
| AccountBaseMixin, | ||
| ): | ||
|
|
||
| class Meta: | ||
| ordering = ['-id'] | ||
|
|
||
| workflow = models.ForeignKey( |
There was a problem hiding this comment.
🟡 Medium workflows/fieldset.py:16
FieldSet inherits from BaseApiNameModel but does not implement the abstract api_name_prefix property. When save() is called without an existing api_name, _create_api_name() calls self.api_name_prefix, which returns None from the abstract method's pass statement. This causes create_api_name(None) to receive an invalid prefix. Consider adding an api_name_prefix property to FieldSet or make the field non-auto-generated if no prefix is needed.
+class FieldSet(
+ BaseApiNameModel,
+ BaseFieldSetMixin,
+ AccountBaseMixin,
+):
+
+ class Meta:
+ ordering = ['-id']
+
+ @property
+ def api_name_prefix(self) -> str:
+ return 'fieldset'
+
+ workflow = models.ForeignKey(🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file backend/src/processes/models/workflows/fieldset.py around lines 16-25:
`FieldSet` inherits from `BaseApiNameModel` but does not implement the abstract `api_name_prefix` property. When `save()` is called without an existing `api_name`, `_create_api_name()` calls `self.api_name_prefix`, which returns `None` from the abstract method's `pass` statement. This causes `create_api_name(None)` to receive an invalid prefix. Consider adding an `api_name_prefix` property to `FieldSet` or make the field non-auto-generated if no prefix is needed.
Evidence trail:
backend/src/processes/models/workflows/fieldset.py (new file, REVIEWED_COMMIT) - lines 16-47: FieldSet class without api_name_prefix; lines 50-64: FieldSetRule class without api_name_prefix.
backend/src/processes/models/base.py lines 8-27 (REVIEWED_COMMIT): BaseApiNameModel defines abstract property api_name_prefix with pass body, _create_api_name calls create_api_name(self.api_name_prefix), save() auto-generates api_name.
backend/src/processes/utils/common.py lines 160-162 (REVIEWED_COMMIT): create_api_name uses f'{prefix}-{salt}'.
backend/src/processes/models/templates/fieldset.py line 39 (REVIEWED_COMMIT): FieldsetTemplate correctly sets api_name_prefix = 'fieldset'; line 146: FieldsetTemplateRule sets api_name_prefix = 'fieldsetrule'.
git_grep for 'api_name_prefix' in fieldset.py returned no matches.
| selection_ids.add(selection.id) | ||
| field.selections.exclude(id__in=selection_ids).delete() | ||
| self._update_field_selections(field, field_data) | ||
| self.instance.output.exclude(id__in=field_ids).delete() |
There was a problem hiding this comment.
🟠 High tasks/task_version.py:94
In _update_fields, the delete query self.instance.output.exclude(id__in=field_ids).delete() removes all TaskField objects not in field_ids, including fields that belong to fieldsets. Since _update_fields runs before _update_fieldsets, this deletes existing fieldset fields before they can be updated. Consider filtering the delete to only fields where fieldset is None to preserve fieldset fields.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file backend/src/processes/services/tasks/task_version.py around line 94:
In `_update_fields`, the delete query `self.instance.output.exclude(id__in=field_ids).delete()` removes all `TaskField` objects not in `field_ids`, including fields that belong to fieldsets. Since `_update_fields` runs before `_update_fieldsets`, this deletes existing fieldset fields before they can be updated. Consider filtering the delete to only fields where `fieldset` is `None` to preserve fieldset fields.
Evidence trail:
backend/src/processes/models/workflows/fields.py:48-51 (task FK with related_name='output'), backend/src/processes/models/workflows/fields.py:61-67 (fieldset FK, nullable), backend/src/processes/models/workflows/fields.py:78 (objects = BaseSoftDeleteManager), backend/src/processes/services/tasks/task_version.py:84-94 (_update_fields method, delete at line 94), backend/src/processes/services/tasks/task_version.py:92 (_update_field called with fieldset=None), backend/src/processes/services/tasks/task_version.py:211-218 (_update_fieldset_fields has its own scoped cleanup), backend/src/processes/services/tasks/task_version.py:270-271 (_update_fields called before _update_fieldsets in update_from_version), backend/src/generics/querysets.py:67 (BaseQuerySet.delete does soft delete), backend/src/generics/managers.py:7-8 (BaseSoftDeleteManager filters is_deleted=False)
| """ Call after objects save """ | ||
|
|
||
| validator = getattr(self, f'_validate_{self.instance.type}', None) | ||
| validator(**kwargs) |
There was a problem hiding this comment.
Validator dispatch calls None for unknown rule types
Low Severity
FieldsetTemplateRuleService._validate uses getattr(self, ..., None) with a None default, then immediately calls validator(**kwargs). If the rule type doesn't match any _validate_* method (e.g., due to data corruption or future types), this calls None() producing a confusing TypeError: 'NoneType' object is not callable instead of a clear error.
Reviewed by Cursor Bugbot for commit 4e44624. Configure here.
…d not requred fields
There was a problem hiding this comment.
🟢 Low
When generating api_name for task fields that lack one, line 106 uses TaskTemplate.api_name_prefix (which is 'task') instead of FieldTemplate.api_name_prefix (which is 'field'). This causes task fields to receive api_names prefixed with task- instead of field-, inconsistent with kickoff fields (line 82-84) which correctly use FieldTemplate.api_name_prefix.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @backend/src/processes/services/templates/template.py around line 104:
When generating `api_name` for task fields that lack one, line 106 uses `TaskTemplate.api_name_prefix` (which is `'task'`) instead of `FieldTemplate.api_name_prefix` (which is `'field'`). This causes task fields to receive api_names prefixed with `task-` instead of `field-`, inconsistent with kickoff fields (line 82-84) which correctly use `FieldTemplate.api_name_prefix`.
Evidence trail:
backend/src/processes/services/templates/template.py lines 82-84 (kickoff fields use FieldTemplate.api_name_prefix) and lines 99-107 (task fields use TaskTemplate.api_name_prefix). backend/src/processes/models/templates/fields.py:33 (FieldTemplate.api_name_prefix = 'field'), lines 42-53 (FieldTemplate has FKs to both Kickoff and TaskTemplate). backend/src/processes/models/templates/task.py:48 (TaskTemplate.api_name_prefix = 'task').
| fs_api_names = set() | ||
| for fs_data in data or []: | ||
| order = fs_data['kickoff_links'][0]['order'] | ||
| fieldset, _ = FieldSet.objects.update_or_create( |
There was a problem hiding this comment.
Kickoff version fieldset order key
High Severity
Kickoff workflow versioning reads fieldset order from kickoff_links[0]['order'], but version snapshots from FieldSetSchemaV1 expose order on the fieldset object. Applying a template version with kickoff fieldsets can raise KeyError and abort sync.
Reviewed by Cursor Bugbot for commit 48bd6c8. Configure here.
| shared_fieldset = models.ForeignKey( | ||
| 'FieldsetTemplate', | ||
| on_delete=models.SET_NULL, | ||
| related_name='child_fieldsets', |
There was a problem hiding this comment.
Shared fieldset delete mismatch
Medium Severity
The model sets shared_fieldset to on_delete=SET_NULL, but migration 0254 creates the FK with CASCADE. Deleting a shared fieldset may cascade-delete template copies at the DB while the ORM expects nulling, causing inconsistent lifecycle behavior.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 48bd6c8. Configure here.
| shared_fieldset_id = AccountPrimaryKeyRelatedField( | ||
| queryset=FieldsetTemplate.objects.all(), | ||
| required=True, | ||
| ) |
There was a problem hiding this comment.
Shared fieldset ID unfiltered
Medium Severity
FieldsetTemplateSerializer resolves shared_fieldset_id against all account fieldsets, not only is_shared=True library rows. Attaching a template copy or non-shared row as the shared source can duplicate wrong definitions.
Reviewed by Cursor Bugbot for commit 48bd6c8. Configure here.
| rule_data['fields'] = [ | ||
| fields_map[old_api_name] | ||
| for old_api_name in rule_data.get('fields', []) | ||
| ] |
There was a problem hiding this comment.
Rule field remap KeyError
Medium Severity
_replace_api_names remaps rule fields through fields_map without guarding missing keys. If serialized rule data references a field api_name not present in fields, cloning a shared fieldset raises KeyError instead of validation error.
Reviewed by Cursor Bugbot for commit 314ba8a. Configure here.
| UniqueConstraint( | ||
| fields=['api_name', 'fieldset'], | ||
| condition=Q(is_deleted=False), | ||
| name='fieldsettemplate_api_name_template_unique', |
There was a problem hiding this comment.
🟠 High templates/fieldset.py:107
FieldsetTemplateRule uses the constraint name 'fieldsettemplate_api_name_template_unique', which duplicates the existing constraint on FieldsetTemplate and mismatches the migration name 'fieldsettemplate_rule_api_name_template_unique'. Django requires unique constraint names across tables, and the mismatch between model and migration state causes makemigrations to generate spurious auto-detected migrations. Rename the constraint to 'fieldsettemplate_rule_api_name_template_unique'.
| name='fieldsettemplate_api_name_template_unique', | |
| name='fieldsettemplate_rule_api_name_template_unique', |
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @backend/src/processes/models/templates/fieldset.py around line 107:
`FieldsetTemplateRule` uses the constraint name `'fieldsettemplate_api_name_template_unique'`, which duplicates the existing constraint on `FieldsetTemplate` and mismatches the migration name `'fieldsettemplate_rule_api_name_template_unique'`. Django requires unique constraint names across tables, and the mismatch between model and migration state causes `makemigrations` to generate spurious auto-detected migrations. Rename the constraint to `'fieldsettemplate_rule_api_name_template_unique'`.
Evidence trail:
backend/src/processes/models/templates/fieldset.py lines 30-35 (FieldsetTemplate constraint name 'fieldsettemplate_api_name_template_unique'), lines 103-108 (FieldsetTemplateRule uses same constraint name). backend/src/processes/migrations/0255_add_shared_fieldsets.py lines 267-273 (migration uses 'fieldsettemplate_rule_api_name_template_unique' for FieldsetTemplateRule). backend/src/processes/migrations/0256_auto_20260622_1939.py lines 120-122 and 147-149 (only touches FieldsetTemplate constraint, not FieldsetTemplateRule). git_grep for 'fieldsettemplate_rule_api_name_template_unique' shows it only exists in migration 0255 line 272, never in model code.
| fieldsets_api_names.add(fieldset.api_name) | ||
| instance.fieldsets.exclude(api_name__in=fieldsets_api_names).delete() | ||
|
|
||
| def get_draft_fieldsets(self, fieldsets_data: Any): |
There was a problem hiding this comment.
🟠 High templates/mixins.py:308
get_draft_fieldsets fetches the shared FieldsetTemplate with FieldsetTemplate.objects.get(id=shared_fieldset_id, is_shared=True) and uses its data without any account scoping. A caller can pass another account's shared fieldset ID, and the server will copy that fieldset's fields and rules into the draft, exposing private template structure across accounts. Scope the lookup to the current account, for example by filtering on account_id from the template or user.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @backend/src/processes/serializers/templates/mixins.py around line 308:
`get_draft_fieldsets` fetches the shared `FieldsetTemplate` with `FieldsetTemplate.objects.get(id=shared_fieldset_id, is_shared=True)` and uses its data without any account scoping. A caller can pass another account's shared fieldset ID, and the server will copy that fieldset's `fields` and `rules` into the draft, exposing private template structure across accounts. Scope the lookup to the current account, for example by filtering on `account_id` from the template or user.
| ) | ||
| service.validate_rules() | ||
| except FieldsetServiceException as ex: | ||
| self.raise_validation_error(message=ex.message) |
There was a problem hiding this comment.
Kickoff update not transactional
High Severity
Kickoff field updates apply each TaskField via partial_update before validate_rules runs, with no enclosing transaction. If a fieldset rule fails validation, earlier field values may already be saved while the API returns an error.
Reviewed by Cursor Bugbot for commit 16d69c8. Configure here.
…rate_fieldsets" command
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
There are 12 total unresolved issues (including 11 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want higher recall? High effort reviews run extra passes and find more bugs. A team admin can switch effort levels in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit be8dc4d. Configure here.
| fields_values=tasks_fields_values, | ||
| ) | ||
| self._update_fields(data=data.get('fields')) | ||
| self._update_fieldsets(data=data.get('fieldsets')) |
There was a problem hiding this comment.
Task sync wipes fieldsets
High Severity
During workflow task version sync, _update_fieldsets is always invoked with data.get('fieldsets'). When stored version JSON has no fieldsets key, None is passed; _update_fieldsets then deletes every FieldSet on the task. Kickoff sync only runs fieldset updates when the key is present.
Reviewed by Cursor Bugbot for commit be8dc4d. Configure here.
| self.create_or_update_fieldsets( | ||
| fieldsets_data=validated_data.pop('fieldsets', []), | ||
| template=template, | ||
| task=instance, | ||
| user=self.context['user'], | ||
| ) |
There was a problem hiding this comment.
🟠 High templates/task.py:544
In update(), calling create_or_update_fieldsets with validated_data.pop('fieldsets', []) deletes every existing fieldset whenever the update payload omits fieldsets. Because fieldsets is required=False, a partial task update (e.g. changing only the task name) passes an empty list, and create_or_update_fieldsets runs instance.fieldsets.exclude(api_name__in=fieldsets_api_names).delete() with an empty set, wiping all fieldsets for the task. The same issue exists in create(). Consider only invoking create_or_update_fieldsets when fieldsets is actually present in the validated data.
- self.create_or_update_fieldsets(
- fieldsets_data=validated_data.pop('fieldsets', []),
- template=template,
- task=instance,
- user=self.context['user'],
- )
+ if 'fieldsets' in validated_data:
+ self.create_or_update_fieldsets(
+ fieldsets_data=validated_data.pop('fieldsets'),
+ template=template,
+ task=instance,
+ user=self.context['user'],
+ )🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @backend/src/processes/serializers/templates/task.py around lines 544-549:
In `update()`, calling `create_or_update_fieldsets` with `validated_data.pop('fieldsets', [])` deletes every existing fieldset whenever the update payload omits `fieldsets`. Because `fieldsets` is `required=False`, a partial task update (e.g. changing only the task name) passes an empty list, and `create_or_update_fieldsets` runs `instance.fieldsets.exclude(api_name__in=fieldsets_api_names).delete()` with an empty set, wiping all fieldsets for the task. The same issue exists in `create()`. Consider only invoking `create_or_update_fieldsets` when `fieldsets` is actually present in the validated data.
1 participant
Note
High Risk
Large schema and workflow-path changes (task completion, kickoff submit, template save) plus field/account mixin adjustments; incorrect rule or field resolution could break existing templates and running workflows.
Overview
Adds fieldset support so kickoff and task forms can group fields under reusable shared templates, with layout/label metadata and validation rules.
New template/runtime models (
FieldsetTemplate,FieldSet, and matching rule types) land via migration0254_add_fieldsets, linking fields throughfieldsetFKs and M2M rule associations. A shared fieldset CRUD API (SharedFieldsetTemplateViewSet) manages account-scoped reusable definitions; template kickoff/task serializers acceptfieldsetsand persist them throughFieldsetMixin(clone fromshared_fieldset_id, refresh api_names, sync order/title/description).Runtime behavior changes: workflow/kickoff creation instantiates fieldsets and fields; completing tasks and updating kickoff values validates sum_equal rules (OR across multiple rules of the same type). Template and workflow “output fields” queries now include fields nested in fieldsets. Task-complete events can expose
fieldsetsin payloads; workflow detail prefetch splits top-level vs fieldset-nested kickoff fields.Supporting refactors:
BaseModelServicegains defaultdelete()and optionalaccount; newRelatedApiNameField/FieldTemplateService;SystemVariable.TASK_VARSaligned with workflow name vars;TemplateOnlyFieldsSerializermoved totemplate_fields.py. Removes the obsoletemigrate_presetsmanagement command and in-diffTemplateFilter(replaced byFieldSetFilterfor shared fieldsets).Reviewed by Cursor Bugbot for commit be8dc4d. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Add FieldSets to template and workflow models, APIs, and serializers
FieldsetTemplateandFieldsetTemplateRulemodels for templates, andFieldSetandFieldSetRulemodels for workflow instances, allowing fields on kickoffs and tasks to be grouped into named fieldsets with layout and rule metadata./fieldsetsREST API (SharedFieldsetTemplateViewSet) for managing shared fieldset templates, and extends template/workflow serializers to includefieldsetsarrays in kickoff, task, and event payloads.FieldSetrecords from templates, excluding grouped fields from standalone field creation; field updates validate fieldset rules (e.g.SUM_EQUAL) post-update.KickoffUpdateVersionService,TaskUpdateVersionService) are extended to sync fieldsets, their fields, and rules during template version propagation.FieldsetMixinon template serializers handles create/update/delete of fieldsets in a single payload;FieldSetServiceandFieldSetRuleServiceback the runtime side.WorkflowDetailsSerializerandTaskViewSetnow split prefetches between standalone fields (fieldsetis null) and fieldset-contained fields; queries returning taskoutputwill no longer include fields that belong to a fieldset.📊 Macroscope summarized be8dc4d. 2 files reviewed, 0 issues evaluated, 0 issues filtered, 0 comments posted
🗂️ Filtered Issues
No issues evaluated.