⚙️ 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.
| ) | ||
| if fields_filter_kwargs: | ||
| qst = qst.filter(**fields_filter_kwargs) | ||
| return qst |
There was a problem hiding this comment.
Removed try/except makes kickoff lookup crash-prone
Medium Severity
get_kickoff_output_fields now calls self.kickoff.get() without a try/except for ObjectDoesNotExist. The previous code gracefully returned an empty queryset when no kickoff existed. Any caller (including get_fields, get_kickoff_fields_values, get_fields_markdown_values) will now crash with an unhandled exception if no kickoff is present.
Additional Locations (1)
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)
| def partial_update(self, **update_kwargs) -> Model: | ||
| self._validate(**update_kwargs) |
There was a problem hiding this comment.
🟡 Medium templates/field_template.py:35
In partial_update, _validate receives only the update kwargs, not the full instance state. When partial_update(is_required=False) is called on a type='user' field without passing type, _validate sees field_type=None and skips the FieldType.USER check, allowing an invalid update that violates the is_required=True requirement. Consider passing the merged instance state and update kwargs to _validate so existing field values are considered during validation.
def partial_update(self, **update_kwargs) -> Model:
- self._validate(**update_kwargs)
+ merged_state = {
+ 'type': self.instance.type,
+ 'is_required': self.instance.is_required,
+ }
+ merged_state.update(update_kwargs)
+ self._validate(**merged_state)🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file backend/src/processes/services/templates/field_template.py around lines 35-36:
In `partial_update`, `_validate` receives only the update kwargs, not the full instance state. When `partial_update(is_required=False)` is called on a `type='user'` field without passing `type`, `_validate` sees `field_type=None` and skips the `FieldType.USER` check, allowing an invalid update that violates the `is_required=True` requirement. Consider passing the merged instance state and update kwargs to `_validate` so existing field values are considered during validation.
Evidence trail:
backend/src/processes/services/templates/field_template.py lines 19-28 (_validate), lines 35-36 (partial_update calls _validate with only update_kwargs); backend/src/generics/base/service.py lines 28-33 (self.instance is set in __init__); backend/src/processes/services/templates/fieldsets/fieldset.py lines 86-92 (caller that invokes partial_update with field_data from external input)
| ) | ||
| service.partial_update( | ||
| value=fields_values.get(task_field.api_name), | ||
| value=fields_values[field.api_name], |
There was a problem hiding this comment.
Task completion skips required-field validation for unsubmitted fields
High Severity
The complete_task_for_user function now only validates task output fields explicitly provided in fields_values. Required fields omitted from fields_values are silently skipped, bypassing validation and allowing tasks to complete without all necessary information.
Reviewed by Cursor Bugbot for commit 4e44624. Configure here.
| """ 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
| skip_value: bool = False, | ||
| **kwargs, | ||
| ): | ||
| fields_data = fields_data or {} |
There was a problem hiding this comment.
Fieldset fields type annotation wrong, List[Dict] vs dict
Medium Severity
_create_fields declares fields_data as Optional[List[Dict]] but defaults it to {} (an empty dict) on line 51 with fields_data = fields_data or {}. The caller passes a dict (mapping api_name to value), which is correct for the .get() call on line 63, but the type annotation is misleading and the default should be {} not confusingly mixed with the List[Dict] annotation. More critically, if someone passes an empty list [], or {} converts it to a dict, which is fine, but passing an actual list would break .get().
Reviewed by Cursor Bugbot for commit e5d253f. Configure here.
… "start_task" actions
…ates/id/fields response
…ates/id/fields response
…to backend/fieldsets/45773__fieldsets
…mpleted_or_skipped" for backward compatibility
…mpleted_or_skipped" in the drafts
…to backend/fieldsets/45773__fieldsets
…to backend/fieldsets/45773__fieldsets
…to backend/fieldsets/45773__fieldsets
…to backend/fieldsets/45773__fieldsets
…' of github.com:pneumaticapp/pneumaticworkflow into backend/fieldsets/45773__fieldsets
| instance=instance.fieldsets.all(), | ||
| many=True, | ||
| ).data | ||
| return None |
There was a problem hiding this comment.
Empty fieldsets return null
Low Severity
For completed task events, get_fieldsets returns serialized data only when instance.fieldsets.exists(); otherwise it returns null. The feature spec calls for an empty array when there are no fieldsets on a complete event, which keeps event payloads consistently typed for webhooks and reports.
Reviewed by Cursor Bugbot for commit ed74295. Configure here.
…to backend/fieldsets/45773__fieldsets
| except FieldsetServiceException as ex: | ||
| self.raise_validation_error( | ||
| message=ex.message, | ||
| ) |
There was a problem hiding this comment.
Kickoff create not atomic
Medium Severity
Kickoff value creation inserts the KickoffValue row before materializing fieldsets and standalone fields, outside a single transaction. If fieldset rule validation or field creation fails afterward, the workflow keeps a kickoff shell with partial or no fields.
Reviewed by Cursor Bugbot for commit 3ed44be. Configure here.
| ('is_shared', models.BooleanField(default=True)), | ||
| ('account', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to='accounts.Account')), | ||
| ('kickoff', models.ForeignKey(blank=True, null=True, on_delete=django.db.models.deletion.CASCADE, related_name='fieldsets', to='processes.Kickoff')), | ||
| ('shared_fieldset', models.ForeignKey(blank=True, null=True, on_delete=django.db.models.deletion.CASCADE, related_name='child_fieldsets', to='processes.FieldsetTemplate')), |
There was a problem hiding this comment.
Shared FK delete mismatch
Medium Severity
Migration 0254 defines shared_fieldset with on_delete=CASCADE, while the model uses SET_NULL. Deleting a library fieldset can cascade-delete template copies instead of nulling the reference as the model declares.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 3ed44be. Configure here.
| class FieldsetTemplateSerializer( | ||
| ModelSerializer, | ||
| CustomValidationErrorMixin, | ||
| ): |
There was a problem hiding this comment.
🟡 Medium templates/fieldset.py:38
FieldsetTemplateSerializer lists ModelSerializer before CustomValidationErrorMixin, so run_validation, to_internal_value, and is_valid resolve to ModelSerializer's implementations. The custom error enrichment methods in CustomValidationErrorMixin are shadowed and never execute, breaking the enriched validation error format. Consider reversing the inheritance order to CustomValidationErrorMixin, ModelSerializer to match SharedFieldsetTemplateSerializer and FieldsetTemplateRuleSerializer.
| class FieldsetTemplateSerializer( | |
| ModelSerializer, | |
| CustomValidationErrorMixin, | |
| ): | |
| class FieldsetTemplateSerializer( | |
| CustomValidationErrorMixin, | |
| ModelSerializer, | |
| ): |
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @backend/src/processes/serializers/templates/fieldset.py around lines 38-41:
`FieldsetTemplateSerializer` lists `ModelSerializer` before `CustomValidationErrorMixin`, so `run_validation`, `to_internal_value`, and `is_valid` resolve to `ModelSerializer`'s implementations. The custom error enrichment methods in `CustomValidationErrorMixin` are shadowed and never execute, breaking the enriched validation error format. Consider reversing the inheritance order to `CustomValidationErrorMixin, ModelSerializer` to match `SharedFieldsetTemplateSerializer` and `FieldsetTemplateRuleSerializer`.
Evidence trail:
backend/src/processes/serializers/templates/fieldset.py lines 38-41: `class FieldsetTemplateSerializer(ModelSerializer, CustomValidationErrorMixin)` — ModelSerializer listed first. Lines 16-18: `class FieldsetTemplateRuleSerializer(CustomValidationErrorMixin, ModelSerializer)` — mixin listed first (correct). Lines 83-86: `class SharedFieldsetTemplateSerializer(CustomValidationErrorMixin, ModelSerializer)` — mixin listed first (correct). backend/src/generics/mixins/serializers.py lines 356, 381, 430: `CustomValidationErrorMixin` defines `run_validation`, `to_internal_value`, and `is_valid`. Python MRO (C3 linearization) places `ModelSerializer` → `Serializer` (DRF) before `CustomValidationErrorMixin`, so Serializer's default implementations shadow the mixin's overrides.
| template=template, | ||
| task=instance, | ||
| ) | ||
| if raw_due_date_created: |
There was a problem hiding this comment.
🟢 Low templates/task.py:549
In update(), AnalyticService.templates_task_due_date_created() fires at line 549-556 before create_or_update_related_one() creates the due date at lines 618-629. If due date creation fails, the analytics event is incorrectly recorded. Move the analytics call to after line 629, matching the correct order in create() where it fires after creation (lines 508-515 after 496-507).
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @backend/src/processes/serializers/templates/task.py around line 549:
In `update()`, `AnalyticService.templates_task_due_date_created()` fires at line 549-556 before `create_or_update_related_one()` creates the due date at lines 618-629. If due date creation fails, the analytics event is incorrectly recorded. Move the analytics call to after line 629, matching the correct order in `create()` where it fires after creation (lines 508-515 after 496-507).
Evidence trail:
backend/src/processes/serializers/templates/task.py lines 496-516 (create method: create_or_update_related_one at 496-507, then analytics at 508-515), lines 518-630 (update method: analytics at 549-556, create_or_update_related_one at 618-629)
| fieldset = existing_fieldsets[fieldset_api_name] | ||
| update_kwargs = {} | ||
| if fieldset.order != fieldset_data['order']: | ||
| update_kwargs['order'] = fieldset_data['order'] |
There was a problem hiding this comment.
Missing order key crashes update
Medium Severity
When updating an existing fieldset by api_name, create_or_update_fieldsets compares fieldset_data['order'] without a default. Payloads that omit order raise KeyError even though the serializer treats order as optional elsewhere.
Reviewed by Cursor Bugbot for commit b023b11. Configure here.
| task_id=task.id if task else None, | ||
| kickoff_id=kickoff.id if kickoff else None, | ||
| order=fieldset_data['order'], | ||
| api_name=fieldset_data.get('api_name'), |
There was a problem hiding this comment.
🟡 Medium templates/mixins.py:301
At line 301, api_name=fieldset_data.get('api_name') always returns None because api_name was already popped from fieldset_data at line 267. User-provided api_name values for new fieldsets are lost, causing new fieldsets to receive auto-generated api_names instead of the requested ones. Use the already-popped fieldset_api_name variable instead.
| api_name=fieldset_data.get('api_name'), | |
| api_name=fieldset_api_name, |
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @backend/src/processes/serializers/templates/mixins.py around line 301:
At line 301, `api_name=fieldset_data.get('api_name')` always returns `None` because `api_name` was already popped from `fieldset_data` at line 267. User-provided `api_name` values for new fieldsets are lost, causing new fieldsets to receive auto-generated api_names instead of the requested ones. Use the already-popped `fieldset_api_name` variable instead.
Evidence trail:
backend/src/processes/serializers/templates/mixins.py lines 267 and 301 at REVIEWED_COMMIT. Line 267: `fieldset_api_name = fieldset_data.pop('api_name', None)` removes the key. Line 301: `api_name=fieldset_data.get('api_name')` reads the now-missing key, always returning None. The else branch (line 289) is entered when `fieldset_api_name` is not in `existing_fieldsets`, which includes the case where it's truthy but refers to a new fieldset.
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').
| description=description, | ||
| ) | ||
|
|
||
| return self.create( |
There was a problem hiding this comment.
🟡 Medium fieldsets/fieldset.py:287
In create_from_shared, is_shared=True incorrectly marks the new fieldset as shared when it should create a template-specific (non-shared) copy. With is_shared=True, the validation that requires template_id and shared_fieldset_id for non-shared fieldsets (lines 50-54) is bypassed, allowing creation of invalid fieldsets that appear shared but are actually linked to a specific template.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @backend/src/processes/services/templates/fieldsets/fieldset.py around line 287:
In `create_from_shared`, `is_shared=True` incorrectly marks the new fieldset as shared when it should create a template-specific (non-shared) copy. With `is_shared=True`, the validation that requires `template_id` and `shared_fieldset_id` for non-shared fieldsets (lines 50-54) is bypassed, allowing creation of invalid fieldsets that appear shared but are actually linked to a specific template.
Evidence trail:
- backend/src/processes/services/templates/fieldsets/fieldset.py line 289: `is_shared=True` in `create_from_shared`
- backend/src/processes/services/templates/fieldsets/fieldset.py lines 86-87: `create_shared_fieldset` docstring defines shared as 'not linked to a template'
- backend/src/processes/services/templates/fieldsets/fieldset.py lines 50-54: validation for non-shared requiring template_id and shared_fieldset_id
- backend/src/processes/services/templates/fieldsets/fieldset.py lines 345-357: `to_json` uses `SharedFieldsetTemplateSerializer` when `is_shared=True`
- backend/src/processes/serializers/templates/fieldset.py lines 38-80 vs 83-113: `FieldsetTemplateSerializer` includes `shared_fieldset_id`, `SharedFieldsetTemplateSerializer` does not
- backend/src/processes/services/templates/fieldsets/fieldset.py lines 180, 213: update/delete guards check `is_shared`
- backend/src/processes/models/templates/fieldset.py lines 42-48: `shared_fieldset` FK with `related_name='child_fieldsets'`
- backend/src/processes/tests/test_services/test_fieldsets/test_fieldset_template_service.py lines 1836-1845, 1904-1913: tests assert `is_shared=True` (matching the buggy implementation)
- Commit REVIEWED_COMMIT: entire file is new in this PR
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 4 potential issues.
There are 13 total unresolved issues (including 9 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 48bd6c8. Configure here.
| kickoff_id=kickoff_id, | ||
| task_id=task_id, | ||
| template_id=template_id, | ||
| ) |
There was a problem hiding this comment.
Wrong shared flag on copy
High Severity
create_from_shared creates template kickoff/task fieldsets with is_shared=True, while tests and fixtures use is_shared=False for those rows. Shared list queries filter on is_shared=True, so copies can appear on /fieldsets and blur account library vs template instances.
Reviewed by Cursor Bugbot for commit 48bd6c8. Configure here.
| 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.
1 participant
Note
High Risk
Large schema and API surface change across template editing, workflow execution, and versioning; kickoff field helpers now assume a kickoff exists (
.get()), which can change error behavior vs. empty querysets.Overview
Adds fieldset groupings for template and workflow fields: reusable shared
FieldsetTemplatedefinitions, per-template/kickoff/task placements, and mirrored runtimeFieldSet/FieldSetRuleinstances withsum_equalvalidation on task complete and kickoff updates.Template and workflow APIs gain
fieldsetson kickoff and tasks (nested fields and rules); draft save and versioning paths sync fieldsets via newFieldSetTemplateService,FieldSetService, andFieldTemplateService. Field discovery on templates/workflows now unions fields attached directly and fields under fieldsets.BaseModelServiceis tightened (defaultdelete, optionalaccount, shared_create_relatedhook).Supporting changes include
RelatedApiNameField/ slug-based rule field wiring, a dedicated template-fields serializer module, workflow event payloads exposing fieldsets on task complete, and a large processes migration plus i18n POT refresh.Reviewed by Cursor Bugbot for commit 48bd6c8. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Add FieldSets to templates and workflow instances with full CRUD API
FieldsetTemplateandFieldsetTemplateRulemodels for templates andFieldSet/FieldSetRulemodels for runtime workflow instances, enabling grouped fields on kickoffs and tasks./fieldsetsREST API (SharedFieldsetTemplateViewSet) for managing shared fieldset templates with list, create, retrieve, partial update, and delete operations.fieldsetsarray;TASK_COMPLETEevents and webhook payloads now carry fieldset data.Template.kickoff_fields,Template.tasks_fields,Workflow.kickoff_output_fields, andWorkflow.tasks_fieldsto include fields attached via fieldsets in addition to direct fields.FieldSetTemplateService,FieldSetService,FieldTemplateService, andFieldSetRuleServiceto orchestrate creation, validation, and rule enforcement (e.g.SUM_EQUALrules on number fields).kickoff_fieldsandkickoff_output_fieldsnow propagateObjectDoesNotExistwhen no kickoff exists instead of returning an empty queryset, which is a breaking behavioral change for callers that relied on the silent fallback.Macroscope summarized 48bd6c8.