Skip to content

Feature/import export asset attribute configuration - #3380

Draft
Ekhorn wants to merge 2 commits into
mainfrom
feature/import-export-asset-attribute-configuration
Draft

Ekhorn wants to merge 2 commits into
mainfrom
feature/import-export-asset-attribute-configuration

Conversation

@Ekhorn

@Ekhorn Ekhorn commented Oct 4, 2026

Copy link
Copy Markdown
Contributor

Description

Checklist

  • 1. Acceptance criteria of the linked issue(s) are met
  • 2. Tests are written and all tests pass
  • 3. Changes are manually tested by you and the reviewer
  • 4. Documentation is written or updated

@Ekhorn Ekhorn self-assigned this Oct 4, 2026
@Ekhorn
Ekhorn force-pushed the feature/import-export-asset-attribute-configuration branch from 4c60293 to 0f5c80c Compare October 5, 2026 07:02
@Ekhorn

Ekhorn commented Oct 5, 2026

Copy link
Copy Markdown
Contributor Author

This dialog after the import flow is not defined in the designs, it could look better if the attribute names were nicely listed. Or removed as it is not in the design.

image

@MartinaeyNL MartinaeyNL left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

AI Review of #3380 — Feature/import export asset attribute configuration

Reviewed the full diff (2 commits, closes #3378).

Verdict

Solid, well-structured implementation that meets most acceptance criteria, with tests at both util and container level. A few contract inconsistencies, one misleading UI string, and some duplication to clean up before marking ready.

Issues

Medium

  1. Misleading overwrite warning — attributeConfiguration.overwriteConfirm says "overwrite existing attribute values", but import only replaces meta (the test explicitly asserts values are untouched). Should say metadata, or users will fear data loss.
  2. Broken contract javadoc on AttributeConfigurationImportPreview — doc says "A failure means ... the remaining fields are empty", but the NO_IMPORTABLE_ATTRIBUTES path in AttributeConfigurationUtil.preview returns populated missingAttributes/typeMismatches/asset types alongside the failure. Fix the doc (or the behavior) so API consumers know what to expect.
  3. Missing-body reported as 409 DRAFT_MISMATCH — in AssetResourceImpl.previewAttributeConfigurationImport, request == null (or draft == null) falls into the DRAFT_MISMATCH branch and returns CONFLICT. A missing/empty body is a 400-class problem, not "the draft changed".
  4. No test for DRAFT_MISMATCH — the whole draft identity check (id/realm/type vs stored asset) and its 409 mapping are untested, unlike every other failure mode.
  5. Duplicated authorization logic — export smartly reuses get(), but import re-implements realm + restricted-user checks that update() already has. Extract a shared helper or the two paths will drift.

Low

  1. Export "disabled while dirty" state — modified is bound at render time in index.ts (.modified="${!!this.isModified()}"). Verify the panel re-renders on OrEditAssetModifiedEvent so the button actually disables mid-edit.
  2. Generic error for 403 on import — a user without write:assets gets errorOccurred instead of a permission message (getFailurePreview only understands the preview body).
  3. Possible stray character in getSkippedTemplate: (${mismatch.importedType} ${mismatch.targetType}) looks like it should be an arrow (→); check it's a real glyph and not a control char (same check for geëxporteerd/geïmporteerd in the nl locale).
  4. selectFile translation key is referenced but not added in this PR — confirm it exists in or.json.
  5. Docs — checklist item 4 claims docs are written/updated; nothing in the diff touches documentation.
  6. Commit hygiene — second commit is literally wip; squash/reword before merge.

Acceptance criteria coverage

AC Status
1. Import/Export in Attributes section header ✅ via getPanel actions slot — issue says "before the expand control", confirm placement matches
2. Export v1 doc, selection of non-empty meta attrs, <asset-name>-attribute-config.json, disabled while dirty ✅
3. Distinguishable rejections (5 failure modes) ✅ with tests
4. Exact name/type matching, skip + report, never change type ✅
5. Confirm dialog: importable/skipped lists, overwrite + asset-type-mismatch warnings, cancellable ✅
6. Wholesale meta replacement, dirty state, result reported ✅ (result dialog is the one flagged as off-design in the PR comment)
7. read:assets / write:assets enforced in resource ✅ with tests
8. Tests: rejected docs, missing attrs, type mismatch, asset type mismatch, meta replacement ✅ except DRAFT_MISMATCH (see #4)

Other notes

  • Sonar reports 5 new issues and 0% coverage on new code (likely Groovy tests not counted).
  • PR is still Draft with an open design question from the author about the post-import dialog.

@Ekhorn
Ekhorn force-pushed the feature/import-export-asset-attribute-configuration branch from 0f5c80c to 046f32a Compare October 14, 2026 15:01
@Ekhorn
Ekhorn force-pushed the feature/import-export-asset-attribute-configuration branch from 046f32a to cbdf8e1 Compare October 14, 2026 15:10
@sonarqubecloud

Copy link
Copy Markdown

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.

Import and export asset attribute configuration

2 participants