Skip to content

test(e2e): pin the classification Users.spec asserts Data Consumer permissions on - #33042

Closed
harsh-vador wants to merge 3 commits into
mainfrom
fix/users-spec-data-consumer-classification-manage-button
Closed

harsh-vador wants to merge 3 commits into
mainfrom
fix/users-spec-data-consumer-classification-manage-button

Conversation

@harsh-vador

Copy link
Copy Markdown
Contributor

Describe your changes

Fixes the intermittent failure of Users.spec.ts › User should have only view permission for glossary and tags for Data Consumer at the Tags-page manage-button assertion (seen on the Collate AUT nightly, e.g. https://github.com/open-metadata/openmetadata-nightly/actions/runs/34326261170/job/102384516454).

Root cause

  1. Assertion went stale in Add export support for classification and tag level #31938 (Add export support for classification and tag level). ClassificationDetails now shows the Manage menu when showExportOption is true, which needs only ViewAll on a non-system classification. DataConsumerPolicy grants ViewAll on all, so a Data Consumer legitimately sees Manage → Export on every user-created classification. The test still asserted the button is never visible.
  2. Masked by list order. TagsPage opens response.data[0]; the API lists classifications ORDER BY name. On a clean DB Certification (system, no Export) is first, so the stale assertion kept passing.
  3. Test-data leak flips it. ContextCenterArticles.spec.ts creates cc_classification_<uuid> in beforeAll with no afterAll. cc_ sorts before Ce, so once that spec has run in the same server the leaked classification is the Tags-page default for everyone. Whether it runs before the Users.spec test depends on worker scheduling → pass/fail alternates run to run.

Fix

  • Users.spec.ts: navigate to Certification explicitly (visitClassificationPage) and assert no Manage button. Then create a user classification via API and assert the Data Consumer sees Manage with only export-button (no import-button, edit-classification, delete-button). Deletes it in finally.
  • ContextCenterArticles.spec.ts: afterAll deletes articleTagClassification (recursive → tags too).

Follows PLAYWRIGHT_DEVELOPER_HANDBOOK.md: API setup/cleanup, getByTestId, reuses existing visitClassificationPage util, no positional locators.

Verification

  • yarn lint:base on both files: 0 errors (3 pre-existing no-restricted-syntax warnings in Users.spec, untouched lines).
  • tsc -p playwright/tsconfig.json: no new errors (2 pre-existing in Users.spec lines 181/438, present on main).
  • Not run against a live server locally; relying on CI Playwright.

Type of change

  • Bug fix
  • Improvement
  • New feature
  • Breaking change
  • Documentation

Checklist

  • I have read the CONTRIBUTING document.
  • My PR title is Fixes <issue-number>: <short explanation> or a conventional-commit prefix.
  • I have commented on my code, particularly in hard-to-understand areas.
  • For JSON Schema changes: N/A.
  • I have added tests around the new logic.

🤖 Generated with Claude Code

…rmissions on

Users.spec asserted that a Data Consumer sees no Manage button on the Tags
page, but the page lands on whichever classification the API lists first.
Since #31938 the Manage menu also carries Export, gated only on ViewAll, so
on any user-created classification a viewer now legitimately sees it. The
assertion kept passing only while a system classification (Certification)
sorted first.

ContextCenterArticles.spec creates `cc_classification_<uuid>` in beforeAll
and never deletes it. `cc_` sorts ahead of `Certification`, so once that
spec ran in the same server the leaked classification became the default and
Users.spec failed — a worker-ordering race, hence the intermittent nightly.

- Users.spec: visit Certification explicitly and assert no Manage button;
  then create a user classification and assert Manage shows Export only
  (no Import / Edit / Delete). Covers the actual permission rule instead of
  depending on list order.
- ContextCenterArticles.spec: add afterAll that deletes the classification
  (recursive delete removes its tags).

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@github-actions

github-actions Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

❌ PR checklist incomplete

This PR cannot be merged until the following are addressed on its linked issue:

  • No GitHub issue is linked. Link an issue in the Development section of the PR (or add Fixes #12345 to the description). For a same-org cross-repo issue, add Fixes open-metadata/<repo>#123 to the description.

The fields live on the linked issue in the Shipping project (open the issue → right sidebar → Projects). After you set them, re-run this check (or push a commit) — issue/project changes do not re-trigger it automatically.

Maintainers can bypass this check by adding the skip-pr-checks label.

@github-actions github-actions Bot added the UI UI specific issues label Sep 9, 2026
@github-actions

github-actions Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Hi there 👋 Thanks for your contribution!

The OpenMetadata team will review the PR shortly! Once it has been labeled as safe to test, the CI workflows
will start executing and we'll be able to make sure everything is working as expected.

Let us know if you need any help!

@github-actions

github-actions Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

🔄 Playwright impact map auto-refreshed

This PR touched specs or UI source that changed the source→spec routing map. I regenerated .github/playwright/impact-map.generated.json and pushed the diff to this branch.

- source entries: 553 → 553
- 0 added, 0 removed, 5 changed spec-list

Entries whose spec list changed:
  openmetadata-ui/src/main/resources/ui/src/components/DataContract/ODCSImportModal/ODCSImportModal.component.tsx
  openmetadata-ui/src/main/resources/ui/src/components/Entity/EntityLineage/CustomControls.component.tsx
  openmetadata-ui/src/main/resources/ui/src/components/common/EntityImport/EntityImport.component.tsx
  openmetadata-ui/src/main/resources/ui/src/generated/entity/data/topic.ts
  openmetadata-ui/src/main/resources/ui/src/pages/PlatformLineage/PlatformLineage.tsx

What is this file? It is the auto-generated half of Playwright's PR planner. It routes "if source X changes, run specs Y" by walking spec imports and cross-referencing getByTestId strings. Hand-authored routing in impact-map.json always wins on conflict.

What if I want to regenerate locally instead? Run this before pushing your next change to skip the bot commit:

python3 .github/scripts/generate_playwright_impact_map.py
git add .github/playwright/impact-map.generated.json
git commit --amend --no-edit  # or a separate commit

@harsh-vador harsh-vador added the safe to test Add this label to run secure Github workflows on PRs label Sep 9, 2026
@gitar-bot

gitar-bot Bot commented Sep 9, 2026

Copy link
Copy Markdown
Code Review ✅ Approved

Pins the Data Consumer classification permission assertion in Users.spec to Certification and adds explicit test coverage for user-created classifications, resolving intermittent test failures caused by test-data leak and stale assertion logic from #31938. No issues found.

Options

Display: compact → Showing less information.

Comment with these commands to change the behavior for this request:

Compact
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Powered by Gitar — free for open source

@harsh-vador

Copy link
Copy Markdown
Contributor Author

closing this pr as test needs to be changed according to new changes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

safe to test Add this label to run secure Github workflows on PRs UI UI specific issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant