Skip to content

feat(datacolumns/datatemplates/propertydata): composite DAC support (Phase 1) - #546

Open
prasad-albert wants to merge 6 commits into
mainfrom
worktree-EX-2118
Open

prasad-albert wants to merge 6 commits into
mainfrom
worktree-EX-2118

Conversation

@prasad-albert

Copy link
Copy Markdown
Collaborator

Summary

  • DataColumns: add DataColumnType enum, SubDataColumnRef and CompositeDataColumn models; extend DataColumn with type, isSystemDefined, subDataColumns, parentId, validation; new create_composite() method and raw param on get_all()
  • DataTemplates: surface type, isSystemDefined, subDataColumns on DataColumnValue from GET responses; new reorder_columns() method for PATCH sequence reordering
  • PropertyData: add CompositePropertyValue model; widen PropertyData.value, PropertyValue.value, TaskPropertyCreate.value to accept composite arrays; add type field to TaskDataColumn and PropertyValue
  • Parameter groups: add DataType.COLOR; widen ValueValidation.min/max/value to accept float (API returns numbers in DAC validation context)

Closes EX-2118 (Phase 1 — DataColumn / DataTemplate / PropertyData)

…umeric fields

Add DataType.COLOR for composite DAC color validation. Widen ValueValidation
min, max, and value to accept float in addition to str, matching the updated
API spec where these fields can be returned as numbers.
Add DataColumnType enum (normal/composite/sub), SubDataColumnRef model,
and CompositeDataColumn model. Extend DataColumn with type, isSystemDefined,
subDataColumns, parentId, and validation fields. Add create_composite() method
for POST /datacolumns/composite and raw param on get_all() for nested responses.
…umns

Add type, isSystemDefined, and subDataColumns to DataColumnValue so composite
DAC metadata is available on GET responses. Add reorder_columns() method for
PATCH sequence reordering, fetching current state to derive oldValue.
Add CompositePropertyValue model (key, id, value) matching the API's
CompositeValueItem schema. Widen PropertyData.value, PropertyValue.value,
TaskPropertyValue.value, and TaskPropertyCreate.value to accept composite
arrays. Add DataColumnType field to PropertyValue and TaskDataColumn.
@claude

claude Bot commented Jun 12, 2026

Copy link
Copy Markdown

Code Review

Issues Found

  • [src/albert/collections/data_columns.py:178] create_composite is missing the @validate_call decorator. Every other public method in this collection (get_by_name, get_by_id, get_all, delete) has it, and AGENTS.md requires it on all public collection methods.

  • [src/albert/resources/data_columns.py:20] SubDataColumnRef has a one-liner docstring but no Numpy-style Parameters section for its fields (id, name, key, required, validation, parent_id). The sibling class CompositeDataColumn (line 43) has a proper Parameters section — SubDataColumnRef should match that standard.

  • [src/albert/resources/property_data.py:40] CompositePropertyValue has a one-liner docstring but no Numpy-style Parameters section for its fields (key, id, value). AGENTS.md requires Numpy-style docstrings on all new public classes.

Summary

3 issue(s) found. The missing @validate_call is a runtime correctness gap; the two docstring gaps violate the required Numpy-style standard.

@claude

claude Bot commented Jul 29, 2026

Copy link
Copy Markdown

Code Review

Issues Found

  • src/albert/resources/data_columns.py:66-70 New DataColumn fields (type, is_system_defined, sub_data_columns, parent_id, validation) have no attribute docstrings, breaking the standard this file otherwise follows (every other field is documented). BaseAlbertModel sets use_attribute_docstrings=True, so these fields emit empty JSON Schema descriptions (consumed by the ReAct worker tool factory / expand_schema). Same gap for the new SubDataColumnRef fields and the new type/is_system_defined/sub_data_columns fields on DataColumnValue in data_templates.py.
  • src/albert/collections/data_columns.py:173-175 The raw param docstring says "Defaults to True (flat list)" but the actual parameter default is None; that default-value claim describes backend behavior, not the signature. Clarify to avoid confusion.

Summary

2 issue(s) found. No correctness bugs; serialization (by_alias/mode=json), pagination, type hints, and union parsing are sound. Both items are documentation/standards gaps, with the missing attribute docstrings being the more meaningful one since they affect generated schema descriptions.

@claude

claude Bot commented Jul 29, 2026

Copy link
Copy Markdown

Code Review

Issues Found

  • [src/albert/collections/data_templates.py:98-124] New public method reorder_columns is missing from the DataTemplateCollection class docstring Methods section. AGENTS.md requires every collection class docstring to list all public methods. (create_composite was correctly added to DataColumnCollection's Methods list, so this is an inconsistency.)
  • [src/albert/collections/data_columns.py:254] create_composite deserializes the response as a single object (CompositeDataColumn(**response.json())), while the sibling create reads response.json()[0] (a list). If the POST /composite endpoint returns a list like the plain create endpoint, this will raise. Please verify the response shape.
  • [tests] No integration tests were added for create_composite or reorder_columns.

Summary

2 issues found plus a test gap. The docstring omission is a standards violation; the response-shape assumption in create_composite is a potential runtime error worth confirming against the API. Serialization, pagination mode, type hints, and PR title all look correct.

@lkubie
lkubie requested review from sbernasek and removed request for sbernasek September 28, 2026 15:59

lkubie commented Sep 28, 2026

Copy link
Copy Markdown
Collaborator

Bump: composite DAC Phase 1 has been waiting on review for a while. Re-requested @sbernasek. @prasad-albert if the approach has changed since, say the word and we can re-scope.

@prasad-albert

Copy link
Copy Markdown
Collaborator Author

Bump: composite DAC Phase 1 has been waiting on review for a while. Re-requested @sbernasek. @prasad-albert if the approach has changed since, say the word and we can re-scope.

I think we need to reverify if this PR is complete with the feature, there might be a few things we're missing here

This branch has not been deployed

No deployments
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.

3 participants