Skip to content
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.

Sign up for GitHub

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

fix(editor): Improve commit modal user facing messaging #12161

Merged
merged 15 commits into from
Dec 17, 2024
Merged

fix(editor): Improve commit modal user facing messaging #12161

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
ff03174
propagate prop
r00gm Dec 11, 2024
87ad800
show notification when no changes
r00gm Dec 11, 2024
efebdd7
added context for users
r00gm Dec 11, 2024
db02fe0
Merge branch 'master' into pay-2348-improve-commit-modal-when-there-a…
r00gm Dec 16, 2024
f19a448
expose locale
r00gm Dec 16, 2024
64acef9
feat: message granularity
r00gm Dec 16, 2024
1934c4c
fix test
r00gm Dec 16, 2024
462cb3f
test: filters reset
r00gm Dec 16, 2024
6268fbc
chore: remove unused variable
r00gm Dec 16, 2024
3c0b0c6
test: user notification
r00gm Dec 16, 2024
e3a1e7c
chore: improve messaging
r00gm Dec 16, 2024
a371a94
Merge branch 'master' into pay-2348-improve-commit-modal-when-there-a…
r00gm Dec 17, 2024
4b312df
fix e2e element query
r00gm Dec 17, 2024
2aed1f5
Merge branch 'master' into pay-2348-improve-commit-modal-when-there-a…
r00gm Dec 17, 2024
9321bdf
Merge branch 'master' into pay-2348-improve-commit-modal-when-there-a…
r00gm Dec 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 18 additions & 18 deletions cypress/e2e/39-projects.cy.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -498,15 +498,15 @@ describe('Projects', { disableAutoLogin: true }, () => {
projects
.getResourceMoveModal()
.should('be.visible')
.find('button:contains("Move workflow")')
.contains('button', 'Move workflow')
.should('be.disabled');
projects.getProjectMoveSelect().click();
getVisibleSelect()
.find('li')
.should('have.length', 5)
.filter(':contains("Project 1")')
.click();
projects.getResourceMoveModal().find('button:contains("Move workflow")').click();
projects.getResourceMoveModal().contains('button', 'Move workflow').click();
clearNotifications();

workflowsPage.getters
Expand All @@ -524,15 +524,15 @@ describe('Projects', { disableAutoLogin: true }, () => {
projects
.getResourceMoveModal()
.should('be.visible')
.find('button:contains("Move workflow")')
.contains('button', 'Move workflow')
.should('be.disabled');
projects.getProjectMoveSelect().click();
getVisibleSelect()
.find('li')
.should('have.length', 5)
.filter(':contains("Project 2")')
.click();
projects.getResourceMoveModal().find('button:contains("Move workflow")').click();
projects.getResourceMoveModal().contains('button', 'Move workflow').click();

// Move the workflow from Project 2 to a member user
projects.getMenuItems().last().click();
Expand All @@ -544,7 +544,7 @@ describe('Projects', { disableAutoLogin: true }, () => {
projects
.getResourceMoveModal()
.should('be.visible')
.find('button:contains("Move workflow")')
.contains('button', 'Move workflow')
.should('be.disabled');
projects.getProjectMoveSelect().click();
getVisibleSelect()
Expand All @@ -553,7 +553,7 @@ describe('Projects', { disableAutoLogin: true }, () => {
.filter(`:contains("${INSTANCE_MEMBERS[0].email}")`)
.click();

projects.getResourceMoveModal().find('button:contains("Move workflow")').click();
projects.getResourceMoveModal().contains('button', 'Move workflow').click();
workflowsPage.getters.workflowCards().should('have.length', 1);

// Move the workflow from member user back to Home
Expand All @@ -569,7 +569,7 @@ describe('Projects', { disableAutoLogin: true }, () => {
projects
.getResourceMoveModal()
.should('be.visible')
.find('button:contains("Move workflow")')
.contains('button', 'Move workflow')
.should('be.disabled');
projects.getProjectMoveSelect().click();
getVisibleSelect()
Expand All @@ -578,7 +578,7 @@ describe('Projects', { disableAutoLogin: true }, () => {
.filter(`:contains("${INSTANCE_OWNER.email}")`)
.click();

projects.getResourceMoveModal().find('button:contains("Move workflow")').click();
projects.getResourceMoveModal().contains('button', 'Move workflow').click();
clearNotifications();
workflowsPage.getters
.workflowCards()
Expand All @@ -596,15 +596,15 @@ describe('Projects', { disableAutoLogin: true }, () => {
projects
.getResourceMoveModal()
.should('be.visible')
.find('button:contains("Move credential")')
.contains('button', 'Move credential')
.should('be.disabled');
projects.getProjectMoveSelect().click();
getVisibleSelect()
.find('li')
.should('have.length', 5)
.filter(':contains("Project 2")')
.click();
projects.getResourceMoveModal().find('button:contains("Move credential")').click();
projects.getResourceMoveModal().contains('button', 'Move credential').click();
clearNotifications();
credentialsPage.getters.credentialCards().should('not.have.length');

Expand All @@ -619,15 +619,15 @@ describe('Projects', { disableAutoLogin: true }, () => {
projects
.getResourceMoveModal()
.should('be.visible')
.find('button:contains("Move credential")')
.contains('button', 'Move credential')
.should('be.disabled');
projects.getProjectMoveSelect().click();
getVisibleSelect()
.find('li')
.should('have.length', 5)
.filter(`:contains("${INSTANCE_ADMIN.email}")`)
.click();
projects.getResourceMoveModal().find('button:contains("Move credential")').click();
projects.getResourceMoveModal().contains('button', 'Move credential').click();
credentialsPage.getters.credentialCards().should('have.length', 1);

// Move the credential from admin user back to instance owner
Expand All @@ -641,15 +641,15 @@ describe('Projects', { disableAutoLogin: true }, () => {
projects
.getResourceMoveModal()
.should('be.visible')
.find('button:contains("Move credential")')
.contains('button', 'Move credential')
.should('be.disabled');
projects.getProjectMoveSelect().click();
getVisibleSelect()
.find('li')
.should('have.length', 5)
.filter(`:contains("${INSTANCE_OWNER.email}")`)
.click();
projects.getResourceMoveModal().find('button:contains("Move credential")').click();
projects.getResourceMoveModal().contains('button', 'Move credential').click();

clearNotifications();

Expand All @@ -666,15 +666,15 @@ describe('Projects', { disableAutoLogin: true }, () => {
projects
.getResourceMoveModal()
.should('be.visible')
.find('button:contains("Move credential")')
.contains('button', 'Move credential')
.should('be.disabled');
projects.getProjectMoveSelect().click();
getVisibleSelect()
.find('li')
.should('have.length', 5)
.filter(':contains("Project 1")')
.click();
projects.getResourceMoveModal().find('button:contains("Move credential")').click();
projects.getResourceMoveModal().contains('button', 'Move credential').click();

projects.getMenuItems().first().click();
projects.getProjectTabCredentials().click();
Expand Down Expand Up @@ -721,15 +721,15 @@ describe('Projects', { disableAutoLogin: true }, () => {
projects
.getResourceMoveModal()
.should('be.visible')
.find('button:contains("Move workflow")')
.contains('button', 'Move workflow')
.should('be.disabled');
projects.getProjectMoveSelect().click();
getVisibleSelect()
.find('li')
.should('have.length', 4)
.filter(':contains("Project 1")')
.click();
projects.getResourceMoveModal().find('button:contains("Move workflow")').click();
projects.getResourceMoveModal().contains('button', 'Move workflow').click();

workflowsPage.getters
.workflowCards()
Expand Down
4 changes: 3 additions & 1 deletion packages/design-system/src/components/N8nNotice/Notice.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,15 @@ interface NoticeProps {
theme?: 'success' | 'warning' | 'danger' | 'info';
content?: string;
fullContent?: string;
compact?: boolean;
}

const props = withDefaults(defineProps<NoticeProps>(), {
id: () => uid('notice'),
theme: 'warning',
content: '',
fullContent: '',
compact: true,
});

const emit = defineEmits<{
Expand Down Expand Up @@ -68,7 +70,7 @@ const onClick = (event: MouseEvent) => {
<template>
<div :id="id" :class="classes" role="alert" @click="onClick">
<div class="notice-content">
<N8nText size="small" :compact="true">
<N8nText size="small" :compact="compact">
<slot>
<span
:id="`${id}-content`"
Expand Down
27 changes: 25 additions & 2 deletions packages/editor-ui/src/components/MainSidebarSourceControl.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,17 @@ let sourceControlStore: ReturnType<typeof useSourceControlStore>;
let uiStore: ReturnType<typeof useUIStore>;
let rbacStore: ReturnType<typeof useRBACStore>;

const showMessage = vi.fn();
const showError = vi.fn();
vi.mock('@/composables/useToast', () => ({
useToast: () => ({ showMessage, showError }),
}));

const renderComponent = createComponentRenderer(MainSidebarSourceControl);

describe('MainSidebarSourceControl', () => {
beforeEach(() => {
vi.resetAllMocks();
pinia = createTestingPinia({
initialState: {
[STORES.SETTINGS]: {
Expand Down Expand Up @@ -75,13 +82,13 @@ describe('MainSidebarSourceControl', () => {
vi.spyOn(sourceControlStore, 'pullWorkfolder').mockRejectedValueOnce({
response: { status: 400 },
});
const { getAllByRole, getByRole } = renderComponent({
const { getAllByRole } = renderComponent({
pinia,
props: { isCollapsed: false },
});

await userEvent.click(getAllByRole('button')[0]);
await waitFor(() => expect(getByRole('alert')).toBeInTheDocument());
await waitFor(() => expect(showError).toHaveBeenCalled());
});

it('should show confirm if pull response http status code is 409', async () => {
Expand All @@ -108,5 +115,21 @@ describe('MainSidebarSourceControl', () => {
),
);
});

it('should show toast when there are no changes', async () => {
vi.spyOn(sourceControlStore, 'getAggregatedStatus').mockResolvedValueOnce([]);

const { getAllByRole } = renderComponent({
pinia,
props: { isCollapsed: false },
});

await userEvent.click(getAllByRole('button')[1]);
await waitFor(() =>
expect(showMessage).toHaveBeenCalledWith(
expect.objectContaining({ title: 'No changes to commit' }),
),
);
});
});
});
10 changes: 10 additions & 0 deletions packages/editor-ui/src/components/MainSidebarSourceControl.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,15 @@ async function pushWorkfolder() {
try {
const status = await sourceControlStore.getAggregatedStatus();

if (!status.length) {
toast.showMessage({
title: 'No changes to commit',
message: 'Everything is up to date',
type: 'info',
});
return;
}

uiStore.openModalWithData({
name: SOURCE_CONTROL_PUSH_MODAL_KEY,
data: { eventBus, status },
Expand All @@ -68,6 +77,7 @@ async function pullWorkfolder() {
const statusWithoutLocallyCreatedWorkflows = status.filter((file) => {
return !(file.type === 'workflow' && file.status === 'created' && file.location === 'local');
});

if (statusWithoutLocallyCreatedWorkflows.length === 0) {
toast.showMessage({
title: i18n.baseText('settings.sourceControl.pull.upToDate.title'),
Expand Down
60 changes: 55 additions & 5 deletions packages/editor-ui/src/components/SourceControlPushModal.ee.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,11 +207,9 @@ describe('SourceControlPushModal', () => {
const submitButton = getByTestId('source-control-push-modal-submit');
const commitMessage = 'commit message';
expect(submitButton).toBeDisabled();
expect(
getByText(
'No workflow changes to push. Only modified credentials, variables, and tags will be pushed.',
),
).toBeInTheDocument();
expect(getByText('1 new credentials added, 0 deleted and 0 changed')).toBeInTheDocument();
expect(getByText('At least one new variable has been added or modified')).toBeInTheDocument();
expect(getByText('At least one new tag has been added or modified')).toBeInTheDocument();

await userEvent.type(getByTestId('source-control-push-modal-commit'), commitMessage);

Expand Down Expand Up @@ -375,5 +373,57 @@ describe('SourceControlPushModal', () => {
expect(items[0]).toHaveTextContent('Created Workflow');
});
});

it('should reset', async () => {
const status: SourceControlAggregatedFile[] = [
{
id: 'JIGKevgZagmJAnM6',
name: 'Modified workflow',
type: 'workflow',
status: 'modified',
location: 'local',
conflict: false,
file: '/home/user/.n8n/git/workflows/JIGKevgZagmJAnM6.json',
updatedAt: '2024-09-20T14:42:51.968Z',
},
];

const { getByTestId, getAllByTestId, queryAllByTestId } = renderModal({
props: {
data: {
eventBus,
status,
},
},
});

expect(getAllByTestId('source-control-push-modal-file-checkbox')).toHaveLength(1);

await userEvent.click(getByTestId('source-control-filter-dropdown'));

expect(getByTestId('source-control-status-filter')).toBeVisible();

await userEvent.click(
within(getByTestId('source-control-status-filter')).getByRole('combobox'),
);

await waitFor(() =>
expect(getAllByTestId('source-control-status-filter-option')[0]).toBeVisible(),
);

const menu = getAllByTestId('source-control-status-filter-option')[0]
.parentElement as HTMLElement;

await userEvent.click(within(menu).getByText('New'));
await waitFor(() => {
expect(queryAllByTestId('source-control-push-modal-file-checkbox')).toHaveLength(0);
expect(getByTestId('source-control-filters-reset')).toBeInTheDocument();
});

await userEvent.click(getByTestId('source-control-filters-reset'));

const items = getAllByTestId('source-control-push-modal-file-checkbox');
expect(items).toHaveLength(1);
});
});
});
Loading
Loading