Fixed permission issue for Data Asset Header component#27967
Fixed permission issue for Data Asset Header component#27967
Conversation
Rohit0301
added
safe to test
Jest test CoverageUI tests summary
|
Code Review 👍 Approved with suggestions 0 resolved / 1 findingsUpdates permission logic for the Data Asset Header to prioritize EditTier evaluation over EditAll. Ensure the existing E2E test suite correctly reflects the intended permission precedence for combined allow/deny rules. 💡 Edge Case: E2E test expects specific-allow + EditAll-deny to hide buttonsThe test This test passing depends on the backend's policy resolution cascading the EditAll deny to specific operations (i.e., deny always wins). If that's the intended backend behavior, this is fine — but it means this test is actually testing backend policy resolution rather than the frontend logic change. Please confirm the test passes in CI. 🤖 Prompt for agentsOptionsDisplay: compact → Showing less information. Comment with these commands to change:
Was this helpful? React with 👍 / 👎 | Gitar |
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
| headerPermTest( | ||
| 'EditTier, EditOwners, EditCertification allowed but EditAll denied – edit buttons not visible', | ||
| async ({ specificEditsPage }) => { | ||
| await headerPermTable.visitEntityPage(specificEditsPage); | ||
|
|
||
| await expect( | ||
| specificEditsPage.getByTestId('edit-tier') | ||
| ).not.toBeVisible(); | ||
| await expect( | ||
| specificEditsPage.getByTestId('edit-owner') | ||
| ).not.toBeVisible(); | ||
| await expect( | ||
| specificEditsPage.getByTestId('edit-certification') | ||
| ).not.toBeVisible(); | ||
| } |
There was a problem hiding this comment.
💡 Edge Case: E2E test expects specific-allow + EditAll-deny to hide buttons
The test 'EditTier, EditOwners, EditCertification allowed but EditAll denied – edit buttons not visible' (line 229-244) uses SPECIFIC_ALLOW_EDIT_ALL_DENY_RULES which sets EditTier/EditOwners/EditCertification to allow and EditAll to deny. Since getPrioritizedEditPermission now returns permissions[Operation.EditTier] directly (the key always exists), if the backend resolves EditTier as true despite EditAll being denied, the button would be visible and the test would fail.
This test passing depends on the backend's policy resolution cascading the EditAll deny to specific operations (i.e., deny always wins). If that's the intended backend behavior, this is fine — but it means this test is actually testing backend policy resolution rather than the frontend logic change. Please confirm the test passes in CI.
Was this helpful? React with 👍 / 👎 | Reply gitar fix to apply this suggestion
|
🔴 Playwright Results — 3 failure(s), 11 flaky✅ 4016 passed · ❌ 3 failed · 🟡 11 flaky · ⏭️ 86 skipped
Genuine Failures (failed on all attempts)❌
|
2 participants
Describe your changes:
There was a case where EditALL permission is allowed and EditTier permission is deny, but it still the edit icon is visible for tier on UI, so i have first check the EditTier condition then check the EditAll permission.
Fixes #26866
Screen.Recording.2026-05-07.at.7.19.15.PM.mov
Screen.Recording.2026-05-07.at.7.17.45.PM.mov
I worked on ... because ...
Type of change:
High-level design:
N/A — small change.
Tests:
Use cases covered
Unit tests
Backend integration tests
Ingestion integration tests
Playwright (UI) tests
Manual testing performed
UI screen recording / screenshots:
Not applicable.
Checklist:
Fixes <issue-number>: <short explanation>Fixes #<issue-number>above.Summary by Gitar
DataAssetsHeaderto prioritizeEditTierpermissions overEditAllpermissions.EntityPermissions.spec.tsto verify the corrected permission handling.This will update automatically on new commits.