#18876: fix the wrong suggestion count being displayed on description (#19483)

* fix the wrong suggestion count being displayed on description

* added button to generate all the suggestions

* minor placement of button change

* added some unit test

* added playwright test for it

* minor changes

* change the button design
This commit is contained in:
Ashish Gupta 2025-03-03 11:56:15 +05:30 committed by GitHub
parent 4dbbc294bf
commit fe7ce3d11a
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
14 changed files with 441 additions and 45 deletions

View File

@ -0,0 +1,251 @@
/*
* Copyright 2025 Collate.
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
* http://www.apache.org/licenses/LICENSE-2.0
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
import test, { expect } from '@playwright/test';
import { TableClass } from '../../support/entity/TableClass';
import { UserClass } from '../../support/user/UserClass';
import { performAdminLogin } from '../../utils/admin';
import { redirectToHomePage } from '../../utils/common';
import { createTableSuggestions } from '../../utils/suggestions';
import { performUserLogin } from '../../utils/user';
const table = new TableClass();
const user1 = new UserClass();
const user2 = new UserClass();
let entityLinkList: string[];
test.describe('Description Suggestions Table Entity', () => {
test.slow(true);
test.beforeAll('Setup pre-requests', async ({ browser }) => {
const { afterAction, apiContext } = await performAdminLogin(browser);
await table.create(apiContext);
entityLinkList = table.entityLinkColumnsName.map(
(entityLinkName) =>
`<#E::table::${table.entityResponseData.fullyQualifiedName}::columns::${entityLinkName}>`
);
await user1.create(apiContext);
await user2.create(apiContext);
// Create suggestions for both users
for (const entityLink of entityLinkList) {
await createTableSuggestions(apiContext, entityLink);
}
await afterAction();
});
test.afterAll('Cleanup', async ({ browser }) => {
const { afterAction, apiContext } = await performAdminLogin(browser);
await table.delete(apiContext);
await user1.delete(apiContext);
await user2.delete(apiContext);
await afterAction();
});
test('View, Close, Reject and Accept the Suggestions', async ({
browser,
}) => {
const { page, afterAction } = await performAdminLogin(browser);
await test.step('View and Open the Suggestions', async () => {
await redirectToHomePage(page);
await table.visitEntityPage(page);
await expect(page.getByText('Suggested Descriptions')).toBeVisible();
const allAvatarSuggestion = page
.getByTestId('asset-description-container')
.getByTestId('profile-avatar');
// Two users profile will be visible, 3rd one will come after AllFetch is clicked
await expect(allAvatarSuggestion).toHaveCount(1);
// Click the first avatar
await allAvatarSuggestion.nth(0).click();
// Actions Buttons should be visible
await expect(page.getByTestId('accept-all-suggestions')).toBeVisible();
await expect(page.getByTestId('reject-all-suggestions')).toBeVisible();
await expect(page.getByTestId('close-suggestion')).toBeVisible();
// All Column Suggestions Card should be visible
await expect(page.locator('.suggested-description-card')).toHaveCount(6);
// Close the suggestions
await page.getByTestId('close-suggestion').click();
await expect(allAvatarSuggestion).toHaveCount(1); // suggestion should not reject or disappear
});
await test.step('Accept Single Suggestion', async () => {
const allAvatarSuggestion = page
.getByTestId('asset-description-container')
.getByTestId('profile-avatar');
// Click the first avatar
await allAvatarSuggestion.nth(0).click();
const singleResolveResponse = page.waitForResponse(
'/api/v1/suggestions/*/accept'
);
await page
.locator(
`[data-row-key*=${table.columnsName[0]}] [data-testid="accept-suggestion"]`
)
.click();
await singleResolveResponse;
await expect(
page.locator(
`[data-row-key*=${table.columnsName[0]}] [data-testid="description"]`
)
).toContainText('this is suggested data description');
});
await test.step('Reject Single Suggestion', async () => {
const allAvatarSuggestion = page
.getByTestId('asset-description-container')
.getByTestId('profile-avatar');
// Click the first avatar
await allAvatarSuggestion.nth(0).click();
const singleResolveResponse = page.waitForResponse(
'/api/v1/suggestions/*/reject'
);
await page
.locator(
`[data-row-key*=${table.columnsName[1]}] [data-testid="reject-suggestion"]`
)
.click();
await singleResolveResponse;
await expect(
page.locator(
`[data-row-key*=${table.columnsName[1]}] [data-testid="description"]`
)
).not.toContainText('this is suggested data description');
});
await test.step('Accept all Suggestion', async () => {
const allAvatarSuggestion = page
.getByTestId('asset-description-container')
.getByTestId('profile-avatar');
// Click the first avatar
await allAvatarSuggestion.nth(0).click();
const acceptResponse = page.waitForResponse(
'/api/v1/suggestions/accept-all?userId=*&entityFQN=*&suggestionType=SuggestDescription'
);
await page.click(`[data-testid="accept-all-suggestions"]`);
await acceptResponse;
// check the third column description, since other two are already checked
await expect(
page.locator(
`[data-row-key*=${table.columnsName[5]}] [data-testid="description"]`
)
).toContainText('this is suggested data description');
// Actions Buttons should not be visible
await expect(
page.getByTestId('accept-all-suggestions')
).not.toBeVisible();
await expect(
page.getByTestId('reject-all-suggestions')
).not.toBeVisible();
await expect(page.getByTestId('close-suggestion')).not.toBeVisible();
});
await afterAction();
});
test('Reject All Suggestions', async ({ browser }) => {
const { page, afterAction } = await performAdminLogin(browser);
await redirectToHomePage(page);
await table.visitEntityPage(page);
const allAvatarSuggestion = page
.getByTestId('asset-description-container')
.getByTestId('profile-avatar');
// Click the first avatar
await allAvatarSuggestion.nth(0).click();
const acceptResponse = page.waitForResponse(
'/api/v1/suggestions/reject-all?userId=*&entityFQN=*&suggestionType=SuggestDescription'
);
await page.click(`[data-testid="reject-all-suggestions"]`);
await acceptResponse;
// check the last column description
await expect(
page.locator(
`[data-row-key*=${table.columnsName[1]}] [data-testid="description"]`
)
).not.toContainText('this is suggested data description');
// Actions Buttons should not be visible
await expect(page.getByTestId('accept-all-suggestions')).not.toBeVisible();
await expect(page.getByTestId('reject-all-suggestions')).not.toBeVisible();
await expect(page.getByTestId('close-suggestion')).not.toBeVisible();
await afterAction();
});
test('Fetch All Pending Suggestions', async ({ browser }) => {
const { page, afterAction } = await performAdminLogin(browser);
const { afterAction: afterAction2, apiContext: apiContext2 } =
await performUserLogin(browser, user1);
const { afterAction: afterAction3, apiContext: apiContext3 } =
await performUserLogin(browser, user2);
for (const entityLink of entityLinkList) {
await createTableSuggestions(apiContext2, entityLink);
await createTableSuggestions(apiContext3, entityLink);
}
await redirectToHomePage(page);
await table.visitEntityPage(page);
await expect(page.getByTestId('more-suggestion-button')).toBeVisible();
const fetchMoreSuggestionResponse = page.waitForResponse(
'/api/v1/suggestions?entityFQN=*&limit=*'
);
await page.getByTestId('more-suggestion-button').click();
await fetchMoreSuggestionResponse;
const allAvatarSuggestion = page
.getByTestId('asset-description-container')
.getByTestId('profile-avatar');
// Click the first avatar
await expect(allAvatarSuggestion).toHaveCount(3);
await afterAction();
await afterAction2();
await afterAction3();
});
});

View File

@ -54,30 +54,47 @@ export class TableClass extends EntityClass {
name: `pw-database-schema-${uuid()}`,
database: `${this.service.name}.${this.database.name}`,
};
columnsName = [
`user_id${uuid()}`,
`shop_id${uuid()}`,
`name${uuid()}`,
`first_name${uuid()}`,
`last_name${uuid()}`,
`email${uuid()}`,
];
entityLinkColumnsName = [
this.columnsName[0],
this.columnsName[1],
this.columnsName[2],
`"${this.columnsName[2]}.${this.columnsName[3]}"`,
`"${this.columnsName[2]}.${this.columnsName[4]}"`,
this.columnsName[5],
];
children = [
{
name: `user_id${uuid()}`,
name: this.columnsName[0],
dataType: 'NUMERIC',
dataTypeDisplay: 'numeric',
description:
'Unique identifier for the user of your Shopify POS or your Shopify admin.',
},
{
name: `shop_id${uuid()}`,
name: this.columnsName[1],
dataType: 'NUMERIC',
dataTypeDisplay: 'numeric',
description:
'The ID of the store. This column is a foreign key reference to the shop_id column in the dim.shop table.',
},
{
name: `name${uuid()}`,
name: this.columnsName[2],
dataType: 'VARCHAR',
dataLength: 100,
dataTypeDisplay: 'varchar',
description: 'Name of the staff member.',
children: [
{
name: `first_name${uuid()}`,
name: this.columnsName[3],
dataType: 'STRUCT',
dataLength: 100,
dataTypeDisplay:
@ -85,7 +102,7 @@ export class TableClass extends EntityClass {
description: 'First name of the staff member.',
},
{
name: `last_name${uuid()}`,
name: this.columnsName[4],
dataType: 'ARRAY',
dataLength: 100,
dataTypeDisplay: 'array<struct<type:string,provider:array<int>>>',
@ -93,7 +110,7 @@ export class TableClass extends EntityClass {
],
},
{
name: `email${uuid()}`,
name: this.columnsName[5],
dataType: 'VARCHAR',
dataLength: 100,
dataTypeDisplay: 'varchar',

View File

@ -0,0 +1,49 @@
/*
* Copyright 2025 Collate.
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
* http://www.apache.org/licenses/LICENSE-2.0
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
import { APIRequestContext } from '@playwright/test';
const SUGGESTION_DESCRIPTION_DATA = {
description: 'this is suggested data description',
tagLabels: [
{
description:
'Data that can be used to directly or indirectly identify a person.',
displayName: 'Personal15',
href: 'http://localhost:8585/api/v1/tags/d90759e3-f94e-4e17-8624-9cbe31d347b7',
labelType: 'Manual',
name: 'Personal',
source: 'Classification',
state: 'Suggested',
style: {
color: 'string',
iconURL: 'string',
},
tagFQN: 'PersonalData.Personal15',
},
],
type: 'SuggestDescription',
};
export const createTableSuggestions = async (
apiContext: APIRequestContext,
entityLink: string
) => {
const suggestionResponse = await apiContext.post('/api/v1/suggestions', {
data: {
...SUGGESTION_DESCRIPTION_DATA,
entityLink,
},
});
return suggestionResponse;
};

View File

@ -45,7 +45,7 @@ const TableDescription = ({
entityType === EntityType.TABLE
? EntityLink.getTableEntityLink(
entityFqn,
columnData.record?.name ?? ''
EntityLink.getTableColumnNameFromColumnFqn(columnData.fqn)
)
: getEntityFeedLink(entityType, columnData.fqn),
[entityType, entityFqn]

View File

@ -18,11 +18,11 @@
overflow: hidden;
}
.suggestion-pending-btn {
background: #ffbe0e0d !important;
border: 1px solid @yellow-2 !important;
color: @yellow-2 !important;
border-radius: 8px !important;
.ant-btn.suggestion-pending-btn {
margin-left: 4px;
border-radius: 20px;
padding: 2px 8px;
font-size: 12px;
}
.suggested-alert-content {

View File

@ -17,6 +17,7 @@ import {
import { EntityReference } from '../../../generated/entity/type';
export interface SuggestionsContextType {
suggestionLimit: number;
selectedUserSuggestions: Suggestion[];
suggestions: Suggestion[];
suggestionsByUser: Map<string, Suggestion[]>;
@ -25,7 +26,7 @@ export interface SuggestionsContextType {
loadingReject: boolean;
allSuggestionsUsers: EntityReference[];
onUpdateActiveUser: (user?: EntityReference) => void;
fetchSuggestions: (entityFqn: string) => void;
fetchSuggestions: () => void;
acceptRejectSuggestion: (
suggestion: Suggestion,
action: SuggestionAction

View File

@ -15,7 +15,7 @@ import React from 'react';
import { SuggestionType } from '../../../generated/entity/feed/suggestion';
import { mockEntityPermissions } from '../../../pages/DatabaseSchemaPage/mocks/DatabaseSchemaPage.mock';
import {
aproveRejectAllSuggestions,
approveRejectAllSuggestions,
getSuggestionsList,
updateSuggestionStatus,
} from '../../../rest/suggestionsAPI';
@ -45,7 +45,7 @@ jest.mock('../../../hooks/useFqn', () => ({
jest.mock('../../../rest/suggestionsAPI', () => ({
getSuggestionsList: jest.fn().mockImplementation(() => Promise.resolve()),
aproveRejectAllSuggestions: jest.fn(),
approveRejectAllSuggestions: jest.fn(),
updateSuggestionStatus: jest.fn(),
}));
@ -65,7 +65,10 @@ describe('SuggestionsProvider', () => {
);
});
expect(getSuggestionsList).toHaveBeenCalled();
expect(getSuggestionsList).toHaveBeenCalledWith({
entityFQN: 'mockFQN',
limit: 10,
});
});
it('calls approveRejectAllSuggestions when button is clicked', () => {
@ -81,7 +84,7 @@ describe('SuggestionsProvider', () => {
const acceptAllBtn = screen.getByText('Accept All');
fireEvent.click(acceptAllBtn);
expect(aproveRejectAllSuggestions).toHaveBeenCalledWith(
expect(approveRejectAllSuggestions).toHaveBeenCalledWith(
'1',
'mockFQN',
SuggestionType.SuggestDescription,
@ -102,7 +105,7 @@ describe('SuggestionsProvider', () => {
const rejectAll = screen.getByText('Reject All');
fireEvent.click(rejectAll);
expect(aproveRejectAllSuggestions).toHaveBeenCalledWith(
expect(approveRejectAllSuggestions).toHaveBeenCalledWith(
'1',
'mockFQN',
SuggestionType.SuggestDescription,

View File

@ -33,7 +33,7 @@ import { EntityReference } from '../../../generated/entity/type';
import { useFqn } from '../../../hooks/useFqn';
import { usePub } from '../../../hooks/usePubSub';
import {
aproveRejectAllSuggestions,
approveRejectAllSuggestions,
getSuggestionsList,
updateSuggestionStatus,
} from '../../../rest/suggestionsAPI';
@ -62,16 +62,19 @@ const SuggestionsProvider = ({ children }: { children?: ReactNode }) => {
const publish = usePub();
const [loading, setLoading] = useState(false);
const [suggestionLimit, setSuggestionLimit] = useState<number>(10);
const refreshEntity = useRef<(suggestion: Suggestion) => void>();
const { permissions } = usePermissionProvider();
const fetchSuggestions = useCallback(async (entityFQN: string) => {
const fetchSuggestions = useCallback(async () => {
setLoading(true);
try {
const { data } = await getSuggestionsList({
entityFQN,
const { data, paging } = await getSuggestionsList({
entityFQN: entityFqn,
limit: suggestionLimit,
});
setSuggestions(data);
setSuggestionLimit(paging.total);
const allUsersData = data.map(
(suggestion) => suggestion.createdBy as EntityReference
@ -100,13 +103,13 @@ const SuggestionsProvider = ({ children }: { children?: ReactNode }) => {
} finally {
setLoading(false);
}
}, []);
}, [entityFqn, suggestionLimit]);
const acceptRejectSuggestion = useCallback(
async (suggestion: Suggestion, status: SuggestionAction) => {
try {
await updateSuggestionStatus(suggestion, status);
await fetchSuggestions(entityFqn);
await fetchSuggestions();
if (status === SuggestionAction.Accept) {
// call component refresh function
publish('updateDetails', suggestion);
@ -115,7 +118,7 @@ const SuggestionsProvider = ({ children }: { children?: ReactNode }) => {
showErrorToast(err as AxiosError);
}
},
[entityFqn, refreshEntity]
[fetchSuggestions, refreshEntity]
);
const onUpdateActiveUser = useCallback(
@ -137,14 +140,14 @@ const SuggestionsProvider = ({ children }: { children?: ReactNode }) => {
setLoadingReject(true);
}
try {
await aproveRejectAllSuggestions(
await approveRejectAllSuggestions(
activeUser?.id ?? '',
entityFqn,
suggestionType,
status
);
await fetchSuggestions(entityFqn);
await fetchSuggestions();
if (status === SuggestionAction.Accept) {
selectedUserSuggestions.forEach((suggestion) => {
publish('updateDetails', suggestion);
@ -158,18 +161,19 @@ const SuggestionsProvider = ({ children }: { children?: ReactNode }) => {
setLoadingReject(false);
}
},
[activeUser, entityFqn, selectedUserSuggestions]
[activeUser, entityFqn, selectedUserSuggestions, fetchSuggestions]
);
useEffect(() => {
if (!isEmpty(permissions) && !isEmpty(entityFqn)) {
fetchSuggestions(entityFqn);
fetchSuggestions();
}
}, [entityFqn, permissions]);
const suggestionsContextObj = useMemo(() => {
return {
suggestions,
suggestionLimit,
suggestionsByUser,
selectedUserSuggestions,
entityFqn,
@ -184,6 +188,7 @@ const SuggestionsProvider = ({ children }: { children?: ReactNode }) => {
};
}, [
suggestions,
suggestionLimit,
suggestionsByUser,
selectedUserSuggestions,
entityFqn,

View File

@ -15,6 +15,9 @@ import React from 'react';
import { useSuggestionsContext } from '../SuggestionsProvider/SuggestionsProvider';
import SuggestionsSlider from './SuggestionsSlider';
const acceptRejectAllSuggestions = jest.fn();
const fetchSuggestions = jest.fn();
jest.mock('../SuggestionsProvider/SuggestionsProvider', () => ({
useSuggestionsContext: jest.fn(),
}));
@ -23,15 +26,22 @@ jest.mock('../../common/AvatarCarousel/AvatarCarousel', () => {
return jest.fn(() => <p>Avatar Carousel</p>);
});
jest.mock('../SuggestionsProvider/SuggestionsProvider', () => ({
useSuggestionsContext: jest.fn().mockImplementation(() => ({
suggestions: [{ id: '1' }, { id: '2' }],
selectedUserSuggestions: [{ id: '1' }, { id: '2' }],
suggestionLimit: 2,
acceptRejectAllSuggestions,
fetchSuggestions,
loadingAccept: false,
loadingReject: false,
})),
__esModule: true,
default: 'SuggestionsProvider',
}));
describe('SuggestionsSlider', () => {
it('renders buttons when there are selected user suggestions', () => {
(useSuggestionsContext as jest.Mock).mockReturnValue({
selectedUserSuggestions: [{ id: '1' }, { id: '2' }],
acceptRejectAllSuggestions: jest.fn(),
loadingAccept: false,
loadingReject: false,
});
render(<SuggestionsSlider />);
expect(screen.getByTestId('accept-all-suggestions')).toBeInTheDocument();
@ -39,17 +49,37 @@ describe('SuggestionsSlider', () => {
});
it('calls acceptRejectAllSuggestions on button click', () => {
const acceptRejectAllSuggestions = jest.fn();
(useSuggestionsContext as jest.Mock).mockReturnValue({
selectedUserSuggestions: [{ id: '1' }, { id: '2' }],
acceptRejectAllSuggestions,
loadingAccept: false,
loadingReject: false,
});
render(<SuggestionsSlider />);
fireEvent.click(screen.getByTestId('accept-all-suggestions'));
expect(acceptRejectAllSuggestions).toHaveBeenCalled();
});
it('should not renders more suggestion button if limit is match', () => {
render(<SuggestionsSlider />);
expect(
screen.queryByTestId('more-suggestion-button')
).not.toBeInTheDocument();
});
it("should show the more suggestion if limit and suggestion doesn't match", () => {
(useSuggestionsContext as jest.Mock).mockReturnValueOnce({
suggestions: [{ id: '1' }, { id: '2' }],
selectedUserSuggestions: [{ id: '1' }, { id: '2' }],
suggestionLimit: 20,
acceptRejectAllSuggestions,
fetchSuggestions,
loadingAccept: false,
loadingReject: false,
});
render(<SuggestionsSlider />);
expect(screen.getByTestId('more-suggestion-button')).toBeInTheDocument();
fireEvent.click(screen.getByTestId('more-suggestion-button'));
expect(fetchSuggestions).toHaveBeenCalled();
});
});

View File

@ -22,6 +22,10 @@ import { SuggestionAction } from '../SuggestionsProvider/SuggestionsProvider.int
const SuggestionsSlider = () => {
const {
suggestions,
loading,
fetchSuggestions,
suggestionLimit,
selectedUserSuggestions,
acceptRejectAllSuggestions,
loadingAccept,
@ -35,6 +39,18 @@ const SuggestionsSlider = () => {
{t('label.suggested-description-plural')}
</Typography.Text>
<AvatarCarousel />
{suggestions.length !== 0 && suggestions.length !== suggestionLimit && (
<Button
className="suggestion-pending-btn"
data-testid="more-suggestion-button"
loading={loading}
type="primary"
onClick={fetchSuggestions}>
{t('label.plus-count-more', {
count: suggestionLimit - 10, // 10 is the default limit, and only show count of pending suggestions
})}
</Button>
)}
{selectedUserSuggestions.length > 0 && (
<Space className="slider-btn-container m-l-xs">
<Button

View File

@ -33,6 +33,10 @@
}
.slick-list {
overflow: visible !important;
.slick-track {
width: auto !important;
}
}
.ant-badge-count {
right: 4px;

View File

@ -24,6 +24,7 @@ const BASE_URL = '/suggestions';
export type ListSuggestionsParams = ListParams & {
entityFQN?: string;
limit?: number;
};
export const getSuggestionsList = async (params?: ListSuggestionsParams) => {
@ -43,7 +44,7 @@ export const updateSuggestionStatus = (
return APIClient.put(url, {});
};
export const aproveRejectAllSuggestions = (
export const approveRejectAllSuggestions = (
userId: string,
entityFQN: string,
suggestionType: SuggestionType,

View File

@ -36,6 +36,7 @@
@yellow-6: #c18100;
@yellow-7: #e7b85d;
@yellow-8: #ffbe0e08;
@yellow-9: #ffbe0e0d;
@error-light-color: #ff9a8e40;
@failed-color: #cb2431;
@red-1: #cb2531;

View File

@ -13,8 +13,11 @@
import antlr4 from 'antlr4';
import { ParseTreeWalker } from 'antlr4/src/antlr4/tree';
import EntityLinkSplitListener from '../antlr/EntityLinkSplitListener';
import { FQN_SEPARATOR_CHAR } from '../constants/char.constants';
import { FqnPart } from '../enums/entity.enum';
import EntityLinkLexer from '../generated/antlr/EntityLinkLexer';
import EntityLinkParser from '../generated/antlr/EntityLinkParser';
import { getPartialNameFromTableFQN } from './CommonUtils';
import { ENTITY_LINK_SEPARATOR } from './EntityUtils';
export default class EntityLink {
@ -86,6 +89,21 @@ export default class EntityLink {
return this.split(entityLink)[3];
}
/**
*
* @param string columnFqn
* @returns column name for table entity
*/
static getTableColumnNameFromColumnFqn(columnFqn: string) {
const columnName = getPartialNameFromTableFQN(columnFqn, [
FqnPart.NestedColumn,
]);
return columnName.includes(FQN_SEPARATOR_CHAR)
? `"${columnName}"`
: columnName;
}
/**
*
* @param string entityLink