fix(editor): Improve commit modal user facing messaging (#12161)
Some checks are pending
Test Master / install-and-build (push) Waiting to run
Test Master / Unit tests (18.x) (push) Blocked by required conditions
Test Master / Unit tests (20.x) (push) Blocked by required conditions
Test Master / Unit tests (22.4) (push) Blocked by required conditions
Test Master / Lint (push) Blocked by required conditions
Test Master / Notify Slack on failure (push) Blocked by required conditions

This commit is contained in:
Raúl Gómez Morales 2024-12-17 16:24:50 +01:00 committed by GitHub
parent 9180b46b52
commit ad39243982
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
8 changed files with 207 additions and 50 deletions

View file

@ -498,7 +498,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()
@ -506,7 +506,7 @@ describe('Projects', { disableAutoLogin: true }, () => {
.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
@ -524,7 +524,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()
@ -532,7 +532,7 @@ describe('Projects', { disableAutoLogin: true }, () => {
.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();
@ -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()
@ -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
@ -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()
@ -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()
@ -596,7 +596,7 @@ 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()
@ -604,7 +604,7 @@ describe('Projects', { disableAutoLogin: true }, () => {
.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');
@ -619,7 +619,7 @@ 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()
@ -627,7 +627,7 @@ describe('Projects', { disableAutoLogin: true }, () => {
.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
@ -641,7 +641,7 @@ 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()
@ -649,7 +649,7 @@ describe('Projects', { disableAutoLogin: true }, () => {
.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();
@ -666,7 +666,7 @@ 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()
@ -674,7 +674,7 @@ describe('Projects', { disableAutoLogin: true }, () => {
.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();
@ -721,7 +721,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()
@ -729,7 +729,7 @@ describe('Projects', { disableAutoLogin: true }, () => {
.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()

View file

@ -10,6 +10,7 @@ interface NoticeProps {
theme?: 'success' | 'warning' | 'danger' | 'info';
content?: string;
fullContent?: string;
compact?: boolean;
}
const props = withDefaults(defineProps<NoticeProps>(), {
@ -17,6 +18,7 @@ const props = withDefaults(defineProps<NoticeProps>(), {
theme: 'warning',
content: '',
fullContent: '',
compact: true,
});
const emit = defineEmits<{
@ -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`"

View file

@ -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]: {
@ -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 () => {
@ -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' }),
),
);
});
});
});

View file

@ -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 },
@ -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'),

View file

@ -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);
@ -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);
});
});
});

View file

@ -28,6 +28,7 @@ import {
N8nSelect,
N8nOption,
N8nInputLabel,
N8nInfoTip,
} from 'n8n-design-system';
import {
SOURCE_CONTROL_FILE_STATUS,
@ -36,7 +37,7 @@ import {
type SourceControlledFileStatus,
type SourceControlAggregatedFile,
} from '@/types/sourceControl.types';
import { orderBy } from 'lodash-es';
import { orderBy, groupBy } from 'lodash-es';
const props = defineProps<{
data: { eventBus: EventBus; status: SourceControlAggregatedFile[] };
@ -101,6 +102,27 @@ const classifyFilesByType = (
{ tags: [], variables: [], credentials: [], workflows: [], currentWorkflow: undefined },
);
const userNotices = computed(() => {
const messages: string[] = [];
if (changes.value.credentials.length) {
const { created, deleted, modified } = groupBy(changes.value.credentials, 'status');
messages.push(
`${created?.length ?? 0} new credentials added, ${deleted?.length ?? 0} deleted and ${modified?.length ?? 0} changed`,
);
}
if (changes.value.variables.length) {
messages.push('At least one new variable has been added or modified');
}
if (changes.value.tags.length) {
messages.push('At least one new tag has been added or modified');
}
return messages;
});
const workflowId = computed(
() =>
([VIEWS.WORKFLOW].includes(route.name as VIEWS) && route.params.name?.toString()) || undefined,
@ -121,9 +143,12 @@ const maybeSelectCurrentWorkflow = (workflow?: SourceControlAggregatedFile) =>
workflow && selectedChanges.value.add(workflow.id);
onMounted(() => maybeSelectCurrentWorkflow(changes.value.currentWorkflow));
const filters = ref<{ status?: SourceControlledFileStatus }>({
status: undefined,
});
const filters = ref<{ status?: SourceControlledFileStatus }>({});
const filtersApplied = computed(() => Boolean(Object.keys(filters.value).length));
const resetFilters = () => {
filters.value = {};
};
const statusFilterOptions: Array<{ label: string; value: SourceControlledFileStatus }> = [
{
label: 'New',
@ -245,12 +270,46 @@ async function onCommitKeyDownEnter() {
}
}
const successNotificationMessage = () => {
const messages: string[] = [];
if (selectedChanges.value.size) {
messages.push(
i18n.baseText('generic.workflow', {
adjustToNumber: selectedChanges.value.size,
interpolate: { count: selectedChanges.value.size },
}),
);
}
if (changes.value.credentials.length) {
messages.push(
i18n.baseText('generic.credential', {
adjustToNumber: changes.value.credentials.length,
interpolate: { count: changes.value.credentials.length },
}),
);
}
if (changes.value.variables.length) {
messages.push(i18n.baseText('generic.variable_plural'));
}
if (changes.value.tags.length) {
messages.push(i18n.baseText('generic.tag_plural'));
}
return [
new Intl.ListFormat(i18n.locale, { style: 'long', type: 'conjunction' }).format(messages),
i18n.baseText('settings.sourceControl.modals.push.success.description'),
].join(' ');
};
async function commitAndPush() {
const files = changes.value.tags
.concat(changes.value.variables)
.concat(changes.value.credentials)
.concat(changes.value.workflows.filter((file) => selectedChanges.value.has(file.id)));
loadingService.startLoading(i18n.baseText('settings.sourceControl.loading.push'));
close();
@ -263,7 +322,7 @@ async function commitAndPush() {
toast.showToast({
title: i18n.baseText('settings.sourceControl.modals.push.success.title'),
message: i18n.baseText('settings.sourceControl.modals.push.success.description'),
message: successNotificationMessage(),
type: 'success',
});
} catch (error) {
@ -299,7 +358,7 @@ const getStatusTheme = (status: SourceControlledFileStatus) => {
<N8nHeading tag="h1" size="xlarge">
{{ i18n.baseText('settings.sourceControl.modals.push.title') }}
</N8nHeading>
<div class="mb-l mt-l">
<div class="mt-l">
<N8nText tag="div">
{{ i18n.baseText('settings.sourceControl.modals.push.description') }}
<N8nLink :to="i18n.baseText('settings.sourceControl.docs.using.pushPull.url')">
@ -307,18 +366,14 @@ const getStatusTheme = (status: SourceControlledFileStatus) => {
</N8nLink>
</N8nText>
<N8nNotice v-if="!changes.workflows.length" class="mt-xs">
<i18n-t keypath="settings.sourceControl.modals.push.noWorkflowChanges">
<template #link>
<N8nLink size="small" :to="i18n.baseText('settings.sourceControl.docs.using.url')">
{{ i18n.baseText('settings.sourceControl.modals.push.noWorkflowChanges.moreInfo') }}
</N8nLink>
</template>
</i18n-t>
<N8nNotice v-if="userNotices.length" class="mt-xs" :compact="false">
<ul class="ml-m">
<li v-for="notice in userNotices" :key="notice">{{ notice }}</li>
</ul>
</N8nNotice>
</div>
<div :class="[$style.filers]">
<div v-if="changes.workflows.length" :class="[$style.filers]" class="mt-l">
<N8nCheckbox
:class="$style.selectAll"
:indeterminate="selectAllIndeterminate"
@ -378,7 +433,14 @@ const getStatusTheme = (status: SourceControlledFileStatus) => {
</div>
</template>
<template #content>
<N8nInfoTip v-if="filtersApplied && !sortedWorkflows.length" :bold="false">
{{ i18n.baseText('workflows.filters.active') }}
<N8nLink size="small" data-test-id="source-control-filters-reset" @click="resetFilters">
{{ i18n.baseText('workflows.filters.active.reset') }}
</N8nLink>
</N8nInfoTip>
<RecycleScroller
v-if="sortedWorkflows.length"
:class="[$style.scroller]"
:items="sortedWorkflows"
:item-size="69"
@ -463,8 +525,7 @@ const getStatusTheme = (status: SourceControlledFileStatus) => {
}
.scroller {
height: 380px;
max-height: 100%;
max-height: 380px;
}
.listItem {

View file

@ -22,7 +22,10 @@ export const i18nInstance = createI18n({
warnHtmlInMessage: 'off',
});
type BaseTextOptions = { adjustToNumber?: number; interpolate?: Record<string, string | number> };
type BaseTextOptions = {
adjustToNumber?: number;
interpolate?: Record<string, string | number>;
};
export class I18nClass {
private baseTextCache = new Map<string, string>();
@ -43,6 +46,10 @@ export class I18nClass {
return longNodeType.replace('n8n-nodes-base.', '');
}
get locale() {
return i18nInstance.global.locale;
}
// ----------------------------------
// render methods
// ----------------------------------

View file

@ -48,6 +48,8 @@
"generic.dontShowAgain": "Don't show again",
"generic.enterprise": "Enterprise",
"generic.executions": "Executions",
"generic.tag_plural": "Tags",
"generic.tag": "Tag | {count} Tags",
"generic.tests": "Tests",
"generic.or": "or",
"generic.clickToCopy": "Click to copy",
@ -68,8 +70,8 @@
"generic.unsavedWork.confirmMessage.cancelButtonText": "Leave without saving",
"generic.upgrade": "Upgrade",
"generic.upgradeNow": "Upgrade now",
"generic.credential": "Credential",
"generic.workflow": "Workflow",
"generic.credential": "Credential | {count} Credential | {count} Credentials",
"generic.workflow": "Workflow | {count} Workflow | {count} Workflows",
"generic.workflowSaved": "Workflow changes saved",
"generic.editor": "Editor",
"generic.seePlans": "See plans",
@ -79,6 +81,8 @@
"generic.moreInfo": "More info",
"generic.next": "Next",
"generic.pro": "Pro",
"generic.variable_plural": "Variables",
"generic.variable": "Variable | {count} Variables",
"generic.viewDocs": "View docs",
"about.aboutN8n": "About n8n",
"about.close": "Close",
@ -1925,19 +1929,19 @@
"settings.sourceControl.button.push": "Push",
"settings.sourceControl.button.pull": "Pull",
"settings.sourceControl.modals.push.title": "Commit and push changes",
"settings.sourceControl.modals.push.description": "Workflows you push will overwrite any existing versions in the repository. ",
"settings.sourceControl.modals.push.description": "The following will be committed: ",
"settings.sourceControl.modals.push.description.learnMore": "More info",
"settings.sourceControl.modals.push.filesToCommit": "Files to commit",
"settings.sourceControl.modals.push.workflowsToCommit": "Select workflows",
"settings.sourceControl.modals.push.everythingIsUpToDate": "Everything is up to date",
"settings.sourceControl.modals.push.noWorkflowChanges": "No workflow changes to push. Only modified credentials, variables, and tags will be pushed. {link}",
"settings.sourceControl.modals.push.noWorkflowChanges": "There are no workflow changes but the following will be committed: {link}",
"settings.sourceControl.modals.push.noWorkflowChanges.moreInfo": "More info",
"settings.sourceControl.modals.push.commitMessage": "Commit message",
"settings.sourceControl.modals.push.commitMessage.placeholder": "e.g. My commit",
"settings.sourceControl.modals.push.buttons.cancel": "Cancel",
"settings.sourceControl.modals.push.buttons.save": "Commit and push",
"settings.sourceControl.modals.push.success.title": "Pushed successfully",
"settings.sourceControl.modals.push.success.description": "The files you selected were committed and pushed to the remote repository",
"settings.sourceControl.modals.push.success.description": "were committed and pushed to your remote repository",
"settings.sourceControl.status.modified": "Modified",
"settings.sourceControl.status.deleted": "Deleted",
"settings.sourceControl.status.created": "New",