-
Notifications
You must be signed in to change notification settings - Fork 89
Release hardening: five catalog fixes from the master-vs-26.7.4 review #5217
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Closed
Closed
Changes from all commits
Commits
Show all changes
18 commits
Select commit
Hold shift + click to select a range
7c6a2d5
fix(athena): honor ui.athena.defaultWorkgroup for ?bucket=-scoped con…
smkohnstamm cff8067
fix(search): stable ordering-control count, accessible Order-by name
smkohnstamm 22e7742
fix(queries): name the QuerySelect field, blank it on load error
smkohnstamm c051822
fix(admin): explain the disabled Enabled switch, gate roles on isServ…
smkohnstamm 9dfc25d
fix(data-products): load the fixture adapter lazily
smkohnstamm 5c8babe
docs(admin): say what the isService gate actually does
sir-sigurd 2beda22
fix(admin): explain the disabled Admin switch too
sir-sigurd bc73eb9
review: apply the /code-review findings
smkohnstamm bb8d5d5
Merge remote-tracking branch 'origin/master' into fix/release-hardeni…
smkohnstamm b0b4351
fix(athena): keep the ?bucket= scope on in-console links
smkohnstamm e4e37a2
fix(data-products): degrade instead of latching a failed adapter load
smkohnstamm 2d951b3
fix(admin): reach the disabled-switch reason by keyboard, gate Role o…
smkohnstamm b36dfec
fix(search): stop the facet sort control flickering as the list narrows
smkohnstamm 4b36a13
fix(athena): pass the default workgroup as a string, not the prefs ob…
smkohnstamm 4b5efb2
test(athena): build the prefs fixture through extendDefaults
smkohnstamm 53cd485
fix(queries): carry ?bucket= on the legacy workgroup and execution re…
smkohnstamm 537b778
docs(data-products): drop the useAdapter tombstone
smkohnstamm 491639d
docs(changelog): match the #5217 entries to what the fixes now cover
smkohnstamm File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
216 changes: 216 additions & 0 deletions
216
catalog/app/containers/Admin/UsersAndRoles/Users.spec.tsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,216 @@ | ||
| import * as React from 'react' | ||
| import { render, cleanup, fireEvent, screen } from '@testing-library/react' | ||
| import { describe, it, expect, vi, afterEach } from 'vitest' | ||
|
|
||
| import { EditableSwitch, columns } from './Users' | ||
|
|
||
| vi.mock('constants/config', () => ({ default: {} })) | ||
|
|
||
| describe('containers/Admin/UsersAndRoles/Users', () => { | ||
| describe('EditableSwitch', () => { | ||
| afterEach(cleanup) | ||
|
|
||
| it('explains why it is disabled', async () => { | ||
| // A dead control with no cause is indistinguishable from a rendering | ||
| // bug -- and two causes (self, service user) share this column. | ||
| const { container } = render( | ||
| <EditableSwitch | ||
| hint="Deactivated users can't sign in" | ||
| disabled | ||
| disabledReason="This service user is managed by the stack" | ||
| checked | ||
| onChange={vi.fn()} | ||
| />, | ||
| ) | ||
| expect(container.querySelector('input')?.disabled).toBe(true) | ||
| fireEvent.mouseOver(container.querySelector('span')!) | ||
| expect( | ||
| await screen.findByText('This service user is managed by the stack'), | ||
| ).toBeDefined() | ||
| }) | ||
|
|
||
| it('carries the reason to keyboard and screen-reader users', async () => { | ||
| // The switch is disabled, so it is out of the tab order and fires no | ||
| // events: without a focusable, named wrapper the reason is mouse-only. | ||
| render( | ||
| <EditableSwitch | ||
| hint="Deactivated users can't sign in" | ||
| disabled | ||
| disabledReason="This service user is managed by the stack" | ||
| checked | ||
| onChange={vi.fn()} | ||
| />, | ||
| ) | ||
| const wrapper = screen.getByLabelText('This service user is managed by the stack') | ||
| expect(wrapper.getAttribute('tabindex')).toBe('0') | ||
|
|
||
| fireEvent.focus(wrapper) | ||
| expect( | ||
| await screen.findByText('This service user is managed by the stack'), | ||
| ).toBeDefined() | ||
| }) | ||
|
|
||
| it('keeps the hint on the enabled control', async () => { | ||
| const { container } = render( | ||
| <EditableSwitch | ||
| hint="Deactivated users can't sign in" | ||
| checked | ||
| onChange={vi.fn()} | ||
| />, | ||
| ) | ||
| // MUI seeds the tooltip as a `title` before opening the popper; hover the | ||
| // node that actually carries it. | ||
| const titled = container.querySelector('[title]')! | ||
| expect(titled.getAttribute('title')).toBe("Deactivated users can't sign in") | ||
| fireEvent.mouseOver(titled) | ||
| expect(await screen.findByText("Deactivated users can't sign in")).toBeDefined() | ||
| }) | ||
|
|
||
| it('renders a plain disabled switch when no reason is given', () => { | ||
| const { container } = render( | ||
| <EditableSwitch | ||
| hint="Deactivated users can't sign in" | ||
| disabled | ||
| checked | ||
| onChange={vi.fn()} | ||
| />, | ||
| ) | ||
| expect(container.querySelector('input')?.disabled).toBe(true) | ||
| }) | ||
| }) | ||
|
|
||
| describe('the Admin column switch', () => { | ||
| afterEach(cleanup) | ||
|
|
||
| const column = columns.find((c) => c.id === 'isAdmin')! | ||
|
|
||
| function renderSwitch(user: object, isSelf = false) { | ||
| return render( | ||
| <> | ||
| {column.getDisplay!(undefined, user as never, { isSelf, openDialog: vi.fn() })} | ||
| </>, | ||
| ) | ||
| } | ||
|
|
||
| it.each([ | ||
| [ | ||
| 'a service user', | ||
| { isAdminAssignmentDisabled: true, isService: true }, | ||
| false, | ||
| 'This service user is managed by the stack', | ||
| ], | ||
| [ | ||
| 'an SSO-managed user', | ||
| { isAdminAssignmentDisabled: true, isService: false }, | ||
| false, | ||
| 'Admin capabilities for this user are managed by the SSO configuration', | ||
| ], | ||
| [ | ||
| 'yourself', | ||
| { isAdminAssignmentDisabled: false, isService: false }, | ||
| true, | ||
| 'You cannot change your own admin status', | ||
| ], | ||
| ])('says why it is disabled for %s', async (_label, user, isSelf, reason) => { | ||
| const { container } = renderSwitch({ name: 'u', isAdmin: false, ...user }, isSelf) | ||
| expect(container.querySelector('input')?.disabled).toBe(true) | ||
| fireEvent.mouseOver(container.querySelector('span')!) | ||
| expect(await screen.findByText(reason)).toBeDefined() | ||
| }) | ||
|
|
||
| it('stays editable for an ordinary user', () => { | ||
| const { container } = renderSwitch({ | ||
| name: 'u', | ||
| isAdmin: false, | ||
| isAdminAssignmentDisabled: false, | ||
| isService: false, | ||
| }) | ||
| expect(container.querySelector('input')?.disabled).toBe(false) | ||
| }) | ||
| }) | ||
|
|
||
| describe('the Enabled column switch', () => { | ||
| afterEach(cleanup) | ||
|
|
||
| const column = columns.find((c) => c.id === 'isActive')! | ||
|
|
||
| function renderSwitch(user: object, isSelf = false) { | ||
| return render( | ||
| <> | ||
| {column.getDisplay!( | ||
| undefined, | ||
| user as never, | ||
| { | ||
| isSelf, | ||
| setActive: vi.fn(), | ||
| } as never, | ||
| )} | ||
| </>, | ||
| ) | ||
| } | ||
|
|
||
| it.each([ | ||
| [ | ||
| 'a service user', | ||
| { isService: true }, | ||
| false, | ||
| 'This service user is managed by the stack', | ||
| ], | ||
| ['yourself', { isService: false }, true, 'You cannot deactivate your own account'], | ||
| ])('says why it is disabled for %s', async (_label, user, isSelf, reason) => { | ||
| const { container } = renderSwitch({ name: 'u', isActive: true, ...user }, isSelf) | ||
| expect(container.querySelector('input')?.disabled).toBe(true) | ||
| fireEvent.mouseOver(container.querySelector('span')!) | ||
| expect(await screen.findByText(reason)).toBeDefined() | ||
| }) | ||
|
|
||
| it('stays editable for an ordinary user', () => { | ||
| const { container } = renderSwitch({ name: 'u', isActive: true, isService: false }) | ||
| expect(container.querySelector('input')?.disabled).toBe(false) | ||
| }) | ||
| }) | ||
|
|
||
| describe('the Role column', () => { | ||
| afterEach(cleanup) | ||
|
|
||
| const column = columns.find((c) => c.id === 'role')! | ||
|
|
||
| function renderRole(user: object) { | ||
| return render( | ||
| <> | ||
| {column.getDisplay!( | ||
| undefined, | ||
| { extraRoles: [], ...user } as never, | ||
| { | ||
| roles: [], | ||
| defaultRole: null, | ||
| openDialog: vi.fn(), | ||
| } as never, | ||
| )} | ||
| </>, | ||
| ) | ||
| } | ||
|
|
||
| // The dialog this opens is read-only for both flags, so the invitation to | ||
| // edit must be gated on the same pair. | ||
| it.each([ | ||
| ['an SSO-managed user', { isRoleAssignmentDisabled: true, isService: false }], | ||
| ['a service user', { isRoleAssignmentDisabled: false, isService: true }], | ||
| ])('offers only viewing for %s', (_label, user) => { | ||
| const { container } = renderRole(user) | ||
| expect(container.querySelector('[title]')?.getAttribute('title')).toBe( | ||
| 'Click to view', | ||
| ) | ||
| }) | ||
|
|
||
| it('offers editing for an ordinary user', () => { | ||
| const { container } = renderRole({ | ||
| isRoleAssignmentDisabled: false, | ||
| isService: false, | ||
| }) | ||
| expect(container.querySelector('[title]')?.getAttribute('title')).toBe( | ||
| 'Click to edit', | ||
| ) | ||
| }) | ||
| }) | ||
| }) |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
f27 · substantive ·
scopeFive subsystems sharing no code and no call path: A queryRedirects.jsx+spec plus Athena/{Athena,History,model/state,model/requests}; B Search/{model,Layout/PackageFilters}; C Queries/QuerySelect+snapshot; D Admin/UsersAndRoles/Users; E model/DataProducts/{hooks,index}. The DataProducts extraction is the one worth making first: the only [Changed] in the set, the only app/model layer change, and the only fix whose stated verification (a production build's chunk contents) leaves no artifact in the repo a reviewer or a later bisect can re-check. FAILURE SCENARIO: one regression post-release makes git revert of #5217 take the other four fixes with it, and all five CHANGELOG entries cite #5217, so any partial revert requires hand-editing catalog/CHANGELOG.md and cherry-picking hunks out of a 22-file commit under release pressure.
Subject: six independent fixes gathered as one unit — split candidate · anchored at 491639d · a reply here is read and dispositioned at the next pass.