Skip to content

Commit ad39243

Browse files
r00gmr00gm
authored
fix(editor): Improve commit modal user facing messaging (#12161)
1 parent 9180b46 commit ad39243

File tree

8 files changed

+207
-50
lines changed

8 files changed

+207
-50
lines changed

‎cypress/e2e/39-projects.cy.ts

+18-18
Original file line numberDiff line numberDiff line change
@@ -498,15 +498,15 @@ describe('Projects', { disableAutoLogin: true }, () => {
498498
projects
499499
.getResourceMoveModal()
500500
.should('be.visible')
501-
.find('button:contains("Move workflow")')
501+
.contains('button', 'Move workflow')
502502
.should('be.disabled');
503503
projects.getProjectMoveSelect().click();
504504
getVisibleSelect()
505505
.find('li')
506506
.should('have.length', 5)
507507
.filter(':contains("Project 1")')
508508
.click();
509-
projects.getResourceMoveModal().find('button:contains("Move workflow")').click();
509+
projects.getResourceMoveModal().contains('button', 'Move workflow').click();
510510
clearNotifications();
511511

512512
workflowsPage.getters
@@ -524,15 +524,15 @@ describe('Projects', { disableAutoLogin: true }, () => {
524524
projects
525525
.getResourceMoveModal()
526526
.should('be.visible')
527-
.find('button:contains("Move workflow")')
527+
.contains('button', 'Move workflow')
528528
.should('be.disabled');
529529
projects.getProjectMoveSelect().click();
530530
getVisibleSelect()
531531
.find('li')
532532
.should('have.length', 5)
533533
.filter(':contains("Project 2")')
534534
.click();
535-
projects.getResourceMoveModal().find('button:contains("Move workflow")').click();
535+
projects.getResourceMoveModal().contains('button', 'Move workflow').click();
536536

537537
// Move the workflow from Project 2 to a member user
538538
projects.getMenuItems().last().click();
@@ -544,7 +544,7 @@ describe('Projects', { disableAutoLogin: true }, () => {
544544
projects
545545
.getResourceMoveModal()
546546
.should('be.visible')
547-
.find('button:contains("Move workflow")')
547+
.contains('button', 'Move workflow')
548548
.should('be.disabled');
549549
projects.getProjectMoveSelect().click();
550550
getVisibleSelect()
@@ -553,7 +553,7 @@ describe('Projects', { disableAutoLogin: true }, () => {
553553
.filter(`:contains("${INSTANCE_MEMBERS[0].email}")`)
554554
.click();
555555

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

559559
// Move the workflow from member user back to Home
@@ -569,7 +569,7 @@ describe('Projects', { disableAutoLogin: true }, () => {
569569
projects
570570
.getResourceMoveModal()
571571
.should('be.visible')
572-
.find('button:contains("Move workflow")')
572+
.contains('button', 'Move workflow')
573573
.should('be.disabled');
574574
projects.getProjectMoveSelect().click();
575575
getVisibleSelect()
@@ -578,7 +578,7 @@ describe('Projects', { disableAutoLogin: true }, () => {
578578
.filter(`:contains("${INSTANCE_OWNER.email}")`)
579579
.click();
580580

581-
projects.getResourceMoveModal().find('button:contains("Move workflow")').click();
581+
projects.getResourceMoveModal().contains('button', 'Move workflow').click();
582582
clearNotifications();
583583
workflowsPage.getters
584584
.workflowCards()
@@ -596,15 +596,15 @@ describe('Projects', { disableAutoLogin: true }, () => {
596596
projects
597597
.getResourceMoveModal()
598598
.should('be.visible')
599-
.find('button:contains("Move credential")')
599+
.contains('button', 'Move credential')
600600
.should('be.disabled');
601601
projects.getProjectMoveSelect().click();
602602
getVisibleSelect()
603603
.find('li')
604604
.should('have.length', 5)
605605
.filter(':contains("Project 2")')
606606
.click();
607-
projects.getResourceMoveModal().find('button:contains("Move credential")').click();
607+
projects.getResourceMoveModal().contains('button', 'Move credential').click();
608608
clearNotifications();
609609
credentialsPage.getters.credentialCards().should('not.have.length');
610610

@@ -619,15 +619,15 @@ describe('Projects', { disableAutoLogin: true }, () => {
619619
projects
620620
.getResourceMoveModal()
621621
.should('be.visible')
622-
.find('button:contains("Move credential")')
622+
.contains('button', 'Move credential')
623623
.should('be.disabled');
624624
projects.getProjectMoveSelect().click();
625625
getVisibleSelect()
626626
.find('li')
627627
.should('have.length', 5)
628628
.filter(`:contains("${INSTANCE_ADMIN.email}")`)
629629
.click();
630-
projects.getResourceMoveModal().find('button:contains("Move credential")').click();
630+
projects.getResourceMoveModal().contains('button', 'Move credential').click();
631631
credentialsPage.getters.credentialCards().should('have.length', 1);
632632

633633
// Move the credential from admin user back to instance owner
@@ -641,15 +641,15 @@ describe('Projects', { disableAutoLogin: true }, () => {
641641
projects
642642
.getResourceMoveModal()
643643
.should('be.visible')
644-
.find('button:contains("Move credential")')
644+
.contains('button', 'Move credential')
645645
.should('be.disabled');
646646
projects.getProjectMoveSelect().click();
647647
getVisibleSelect()
648648
.find('li')
649649
.should('have.length', 5)
650650
.filter(`:contains("${INSTANCE_OWNER.email}")`)
651651
.click();
652-
projects.getResourceMoveModal().find('button:contains("Move credential")').click();
652+
projects.getResourceMoveModal().contains('button', 'Move credential').click();
653653

654654
clearNotifications();
655655

@@ -666,15 +666,15 @@ describe('Projects', { disableAutoLogin: true }, () => {
666666
projects
667667
.getResourceMoveModal()
668668
.should('be.visible')
669-
.find('button:contains("Move credential")')
669+
.contains('button', 'Move credential')
670670
.should('be.disabled');
671671
projects.getProjectMoveSelect().click();
672672
getVisibleSelect()
673673
.find('li')
674674
.should('have.length', 5)
675675
.filter(':contains("Project 1")')
676676
.click();
677-
projects.getResourceMoveModal().find('button:contains("Move credential")').click();
677+
projects.getResourceMoveModal().contains('button', 'Move credential').click();
678678

679679
projects.getMenuItems().first().click();
680680
projects.getProjectTabCredentials().click();
@@ -721,15 +721,15 @@ describe('Projects', { disableAutoLogin: true }, () => {
721721
projects
722722
.getResourceMoveModal()
723723
.should('be.visible')
724-
.find('button:contains("Move workflow")')
724+
.contains('button', 'Move workflow')
725725
.should('be.disabled');
726726
projects.getProjectMoveSelect().click();
727727
getVisibleSelect()
728728
.find('li')
729729
.should('have.length', 4)
730730
.filter(':contains("Project 1")')
731731
.click();
732-
projects.getResourceMoveModal().find('button:contains("Move workflow")').click();
732+
projects.getResourceMoveModal().contains('button', 'Move workflow').click();
733733

734734
workflowsPage.getters
735735
.workflowCards()

‎packages/design-system/src/components/N8nNotice/Notice.vue

+3-1
Original file line numberDiff line numberDiff line change
@@ -10,13 +10,15 @@ interface NoticeProps {
1010
theme?: 'success' | 'warning' | 'danger' | 'info';
1111
content?: string;
1212
fullContent?: string;
13+
compact?: boolean;
1314
}
1415
1516
const props = withDefaults(defineProps<NoticeProps>(), {
1617
id: () => uid('notice'),
1718
theme: 'warning',
1819
content: '',
1920
fullContent: '',
21+
compact: true,
2022
});
2123
2224
const emit = defineEmits<{
@@ -68,7 +70,7 @@ const onClick = (event: MouseEvent) => {
6870
<template>
6971
<div :id="id" :class="classes" role="alert" @click="onClick">
7072
<div class="notice-content">
71-
<N8nText size="small" :compact="true">
73+
<N8nText size="small" :compact="compact">
7274
<slot>
7375
<span
7476
:id="`${id}-content`"

‎packages/editor-ui/src/components/MainSidebarSourceControl.test.ts

+25-2
Original file line numberDiff line numberDiff line change
@@ -16,10 +16,17 @@ let sourceControlStore: ReturnType<typeof useSourceControlStore>;
1616
let uiStore: ReturnType<typeof useUIStore>;
1717
let rbacStore: ReturnType<typeof useRBACStore>;
1818

19+
const showMessage = vi.fn();
20+
const showError = vi.fn();
21+
vi.mock('@/composables/useToast', () => ({
22+
useToast: () => ({ showMessage, showError }),
23+
}));
24+
1925
const renderComponent = createComponentRenderer(MainSidebarSourceControl);
2026

2127
describe('MainSidebarSourceControl', () => {
2228
beforeEach(() => {
29+
vi.resetAllMocks();
2330
pinia = createTestingPinia({
2431
initialState: {
2532
[STORES.SETTINGS]: {
@@ -75,13 +82,13 @@ describe('MainSidebarSourceControl', () => {
7582
vi.spyOn(sourceControlStore, 'pullWorkfolder').mockRejectedValueOnce({
7683
response: { status: 400 },
7784
});
78-
const { getAllByRole, getByRole } = renderComponent({
85+
const { getAllByRole } = renderComponent({
7986
pinia,
8087
props: { isCollapsed: false },
8188
});
8289

8390
await userEvent.click(getAllByRole('button')[0]);
84-
await waitFor(() => expect(getByRole('alert')).toBeInTheDocument());
91+
await waitFor(() => expect(showError).toHaveBeenCalled());
8592
});
8693

8794
it('should show confirm if pull response http status code is 409', async () => {
@@ -108,5 +115,21 @@ describe('MainSidebarSourceControl', () => {
108115
),
109116
);
110117
});
118+
119+
it('should show toast when there are no changes', async () => {
120+
vi.spyOn(sourceControlStore, 'getAggregatedStatus').mockResolvedValueOnce([]);
121+
122+
const { getAllByRole } = renderComponent({
123+
pinia,
124+
props: { isCollapsed: false },
125+
});
126+
127+
await userEvent.click(getAllByRole('button')[1]);
128+
await waitFor(() =>
129+
expect(showMessage).toHaveBeenCalledWith(
130+
expect.objectContaining({ title: 'No changes to commit' }),
131+
),
132+
);
133+
});
111134
});
112135
});

‎packages/editor-ui/src/components/MainSidebarSourceControl.vue

+10
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,15 @@ async function pushWorkfolder() {
4343
try {
4444
const status = await sourceControlStore.getAggregatedStatus();
4545
46+
if (!status.length) {
47+
toast.showMessage({
48+
title: 'No changes to commit',
49+
message: 'Everything is up to date',
50+
type: 'info',
51+
});
52+
return;
53+
}
54+
4655
uiStore.openModalWithData({
4756
name: SOURCE_CONTROL_PUSH_MODAL_KEY,
4857
data: { eventBus, status },
@@ -68,6 +77,7 @@ async function pullWorkfolder() {
6877
const statusWithoutLocallyCreatedWorkflows = status.filter((file) => {
6978
return !(file.type === 'workflow' && file.status === 'created' && file.location === 'local');
7079
});
80+
7181
if (statusWithoutLocallyCreatedWorkflows.length === 0) {
7282
toast.showMessage({
7383
title: i18n.baseText('settings.sourceControl.pull.upToDate.title'),

‎packages/editor-ui/src/components/SourceControlPushModal.ee.test.ts

+55-5
Original file line numberDiff line numberDiff line change
@@ -207,11 +207,9 @@ describe('SourceControlPushModal', () => {
207207
const submitButton = getByTestId('source-control-push-modal-submit');
208208
const commitMessage = 'commit message';
209209
expect(submitButton).toBeDisabled();
210-
expect(
211-
getByText(
212-
'No workflow changes to push. Only modified credentials, variables, and tags will be pushed.',
213-
),
214-
).toBeInTheDocument();
210+
expect(getByText('1 new credentials added, 0 deleted and 0 changed')).toBeInTheDocument();
211+
expect(getByText('At least one new variable has been added or modified')).toBeInTheDocument();
212+
expect(getByText('At least one new tag has been added or modified')).toBeInTheDocument();
215213

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

@@ -375,5 +373,57 @@ describe('SourceControlPushModal', () => {
375373
expect(items[0]).toHaveTextContent('Created Workflow');
376374
});
377375
});
376+
377+
it('should reset', async () => {
378+
const status: SourceControlAggregatedFile[] = [
379+
{
380+
id: 'JIGKevgZagmJAnM6',
381+
name: 'Modified workflow',
382+
type: 'workflow',
383+
status: 'modified',
384+
location: 'local',
385+
conflict: false,
386+
file: '/home/user/.n8n/git/workflows/JIGKevgZagmJAnM6.json',
387+
updatedAt: '2024-09-20T14:42:51.968Z',
388+
},
389+
];
390+
391+
const { getByTestId, getAllByTestId, queryAllByTestId } = renderModal({
392+
props: {
393+
data: {
394+
eventBus,
395+
status,
396+
},
397+
},
398+
});
399+
400+
expect(getAllByTestId('source-control-push-modal-file-checkbox')).toHaveLength(1);
401+
402+
await userEvent.click(getByTestId('source-control-filter-dropdown'));
403+
404+
expect(getByTestId('source-control-status-filter')).toBeVisible();
405+
406+
await userEvent.click(
407+
within(getByTestId('source-control-status-filter')).getByRole('combobox'),
408+
);
409+
410+
await waitFor(() =>
411+
expect(getAllByTestId('source-control-status-filter-option')[0]).toBeVisible(),
412+
);
413+
414+
const menu = getAllByTestId('source-control-status-filter-option')[0]
415+
.parentElement as HTMLElement;
416+
417+
await userEvent.click(within(menu).getByText('New'));
418+
await waitFor(() => {
419+
expect(queryAllByTestId('source-control-push-modal-file-checkbox')).toHaveLength(0);
420+
expect(getByTestId('source-control-filters-reset')).toBeInTheDocument();
421+
});
422+
423+
await userEvent.click(getByTestId('source-control-filters-reset'));
424+
425+
const items = getAllByTestId('source-control-push-modal-file-checkbox');
426+
expect(items).toHaveLength(1);
427+
});
378428
});
379429
});

0 commit comments

Comments
 (0)