From a7c4bf7de7d4ac76f17af751a4e20971dfc0640d Mon Sep 17 00:00:00 2001
From: Csaba Tuncsik <csaba@n8n.io>
Date: Fri, 20 Sep 2024 16:58:06 +0200
Subject: [PATCH 1/3] fix(editor): Fix source control push modal checkboxes

---
 .../components/SourceControlPushModal.ee.vue  | 74 +++++++++----------
 1 file changed, 34 insertions(+), 40 deletions(-)

diff --git a/packages/editor-ui/src/components/SourceControlPushModal.ee.vue b/packages/editor-ui/src/components/SourceControlPushModal.ee.vue
index 2f15af68133b1..0f4a1f2502046 100644
--- a/packages/editor-ui/src/components/SourceControlPushModal.ee.vue
+++ b/packages/editor-ui/src/components/SourceControlPushModal.ee.vue
@@ -283,45 +283,41 @@ function getStatusText(file: SourceControlAggregatedFile): string {
 								</n8n-text>
 							</n8n-checkbox>
 						</div>
-						<n8n-card
+						<label
 							v-for="file in sortedFiles"
 							v-show="!defaultStagedFileTypes.includes(file.type)"
 							:key="file.file"
+							:for="file.name"
 							:class="$style.listItem"
-							@click="setStagedStatus(file, !staged[file.file])"
 						>
-							<div :class="$style.listItemBody">
-								<n8n-checkbox
-									:model-value="staged[file.file]"
-									:class="$style.listItemCheckbox"
-									@update:model-value="setStagedStatus(file, !staged[file.file])"
-								/>
-								<div>
-									<n8n-text v-if="file.status === 'deleted'" color="text-light">
-										<span v-if="file.type === 'workflow'"> Deleted Workflow: </span>
-										<span v-if="file.type === 'credential'"> Deleted Credential: </span>
-										<strong>{{ file.name || file.id }}</strong>
+							<n8n-checkbox
+								:id="file.name"
+								:model-value="staged[file.file]"
+								:class="$style.listItemCheckbox"
+								@update:model-value="setStagedStatus(file, !staged[file.file])"
+							/>
+							<div>
+								<n8n-text v-if="file.status === 'deleted'" color="text-light">
+									<span v-if="file.type === 'workflow'"> Deleted Workflow: </span>
+									<span v-if="file.type === 'credential'"> Deleted Credential: </span>
+									<strong>{{ file.name || file.id }}</strong>
+								</n8n-text>
+								<n8n-text v-else bold> {{ file.name }} </n8n-text>
+								<div v-if="file.updatedAt">
+									<n8n-text color="text-light" size="small">
+										{{ renderUpdatedAt(file) }}
 									</n8n-text>
-									<n8n-text v-else bold> {{ file.name }} </n8n-text>
-									<div v-if="file.updatedAt">
-										<n8n-text color="text-light" size="small">
-											{{ renderUpdatedAt(file) }}
-										</n8n-text>
-									</div>
-								</div>
-								<div :class="$style.listItemStatus">
-									<n8n-badge
-										v-if="workflowId === file.id && file.type === 'workflow'"
-										class="mr-2xs"
-									>
-										Current workflow
-									</n8n-badge>
-									<n8n-badge :theme="statusToBadgeThemeMap[file.status] || 'default'">
-										{{ getStatusText(file) }}
-									</n8n-badge>
 								</div>
 							</div>
-						</n8n-card>
+							<div :class="$style.listItemStatus">
+								<n8n-badge v-if="workflowId === file.id && file.type === 'workflow'" class="mr-2xs">
+									Current workflow
+								</n8n-badge>
+								<n8n-badge :theme="statusToBadgeThemeMap[file.status] || 'default'">
+									{{ getStatusText(file) }}
+								</n8n-badge>
+							</div>
+						</label>
 					</div>
 					<n8n-notice v-else class="mt-0">
 						<i18n-t keypath="settings.sourceControl.modals.push.noWorkflowChanges">
@@ -380,11 +376,15 @@ function getStatusText(file: SourceControlAggregatedFile): string {
 }
 
 .listItem {
-	margin-top: var(--spacing-2xs);
-	margin-bottom: var(--spacing-2xs);
+	display: flex;
+	width: 100%;
+	align-items: center;
+	margin: var(--spacing-2xs) 0 var(--spacing-2xs);
+	padding: var(--spacing-xs);
 	cursor: pointer;
 	transition: border 0.3s ease;
-	padding: var(--spacing-xs);
+	border-radius: var(--border-radius-large);
+	border: var(--border-base);
 
 	&:hover {
 		border-color: var(--color-foreground-dark);
@@ -399,12 +399,6 @@ function getStatusText(file: SourceControlAggregatedFile): string {
 	}
 }
 
-.listItemBody {
-	display: flex;
-	flex-direction: row;
-	align-items: center;
-}
-
 .listItemCheckbox {
 	display: inline-flex !important;
 	margin-bottom: 0 !important;

From 7d3620e934cb5c6846b6ed638bbf4e578e4ab025 Mon Sep 17 00:00:00 2001
From: Csaba Tuncsik <csaba@n8n.io>
Date: Fri, 20 Sep 2024 19:18:47 +0200
Subject: [PATCH 2/3] test(editor): Test source control push modal checkboxes

---
 .../SourceControlPushModal.ee.test.ts         | 111 ++++++++++++++++++
 .../components/SourceControlPushModal.ee.vue  |   1 +
 2 files changed, 112 insertions(+)
 create mode 100644 packages/editor-ui/src/components/SourceControlPushModal.ee.test.ts

diff --git a/packages/editor-ui/src/components/SourceControlPushModal.ee.test.ts b/packages/editor-ui/src/components/SourceControlPushModal.ee.test.ts
new file mode 100644
index 0000000000000..e9ce1f21753f3
--- /dev/null
+++ b/packages/editor-ui/src/components/SourceControlPushModal.ee.test.ts
@@ -0,0 +1,111 @@
+import { within } from '@testing-library/dom';
+import userEvent from '@testing-library/user-event';
+import { useRoute } from 'vue-router';
+import { createComponentRenderer } from '@/__tests__/render';
+import SourceControlPushModal from '@/components/SourceControlPushModal.ee.vue';
+import { createTestingPinia } from '@pinia/testing';
+import { createEventBus } from 'n8n-design-system';
+import type { SourceControlAggregatedFile } from '@/Interface';
+
+const eventBus = createEventBus();
+
+vi.mock('vue-router', () => ({
+	useRoute: vi.fn().mockReturnValue({
+		params: vi.fn(),
+		fullPath: vi.fn(),
+	}),
+	RouterLink: vi.fn(),
+}));
+
+let route: ReturnType<typeof useRoute>;
+
+const renderModal = createComponentRenderer(SourceControlPushModal, {
+	global: {
+		stubs: {
+			Modal: {
+				template: `
+					<div>
+						<slot name="header" />
+						<slot name="title" />
+						<slot name="content" />
+						<slot name="footer" />
+					</div>
+				`,
+			},
+		},
+	},
+});
+
+describe('SourceControlPushModal', () => {
+	beforeEach(() => {
+		route = useRoute();
+	});
+
+	it('mounts', () => {
+		vi.spyOn(route, 'fullPath', 'get').mockReturnValue('');
+
+		const { getByTitle } = renderModal({
+			pinia: createTestingPinia(),
+			props: {
+				data: {
+					eventBus,
+					status: [],
+				},
+			},
+		});
+		expect(getByTitle('Commit and push changes')).toBeInTheDocument();
+	});
+
+	it('should toggle checkboxes', async () => {
+		const status: SourceControlAggregatedFile[] = [
+			{
+				id: 'gTbbBkkYTnNyX1jD',
+				name: 'My workflow 1',
+				type: 'workflow',
+				status: 'created',
+				location: 'local',
+				conflict: false,
+				file: '/home/user/.n8n/git/workflows/gTbbBkkYTnNyX1jD.json',
+				updatedAt: '2024-09-20T10:31:40.000Z',
+			},
+			{
+				id: 'JIGKevgZagmJAnM6',
+				name: 'My workflow 2',
+				type: 'workflow',
+				status: 'created',
+				location: 'local',
+				conflict: false,
+				file: '/home/user/.n8n/git/workflows/JIGKevgZagmJAnM6.json',
+				updatedAt: '2024-09-20T14:42:51.968Z',
+			},
+		];
+
+		vi.spyOn(route, 'fullPath', 'get').mockReturnValue('/home/workflows');
+
+		const { getAllByTestId } = renderModal({
+			pinia: createTestingPinia(),
+			props: {
+				data: {
+					eventBus,
+					status,
+				},
+			},
+		});
+
+		const files = getAllByTestId('source-control-push-modal-file');
+		expect(files).toHaveLength(2);
+
+		await userEvent.click(files[0]);
+		expect(within(files[0]).getByRole('checkbox')).toBeChecked();
+		expect(within(files[1]).getByRole('checkbox')).not.toBeChecked();
+
+		await userEvent.click(within(files[0]).getByRole('checkbox'));
+		expect(within(files[0]).getByRole('checkbox')).not.toBeChecked();
+
+		await userEvent.click(within(files[1]).getByRole('checkbox'));
+		expect(within(files[1]).getByRole('checkbox')).toBeChecked();
+
+		await userEvent.click(files[1]);
+		expect(within(files[1]).getByRole('checkbox')).not.toBeChecked();
+	});
+});
diff --git a/packages/editor-ui/src/components/SourceControlPushModal.ee.vue b/packages/editor-ui/src/components/SourceControlPushModal.ee.vue
index 0f4a1f2502046..f0d262ba5554f 100644
--- a/packages/editor-ui/src/components/SourceControlPushModal.ee.vue
+++ b/packages/editor-ui/src/components/SourceControlPushModal.ee.vue
@@ -289,6 +289,7 @@ function getStatusText(file: SourceControlAggregatedFile): string {
 							:key="file.file"
 							:for="file.name"
 							:class="$style.listItem"
+							data-test-id="source-control-push-modal-file"
 						>
 							<n8n-checkbox
 								:id="file.name"

From 6318166a83079ad889102c9699b869e985b96be8 Mon Sep 17 00:00:00 2001
From: Csaba Tuncsik <csaba@n8n.io>
Date: Mon, 23 Sep 2024 14:27:42 +0200
Subject: [PATCH 3/3] fix(editor): Simplifying DOM by removing elements

---
 .../SourceControlPushModal.ee.test.ts         | 43 +++++++-
 .../components/SourceControlPushModal.ee.vue  | 98 +++++++++++--------
 2 files changed, 96 insertions(+), 45 deletions(-)

diff --git a/packages/editor-ui/src/components/SourceControlPushModal.ee.test.ts b/packages/editor-ui/src/components/SourceControlPushModal.ee.test.ts
index e9ce1f21753f3..c7691340fd2a7 100644
--- a/packages/editor-ui/src/components/SourceControlPushModal.ee.test.ts
+++ b/packages/editor-ui/src/components/SourceControlPushModal.ee.test.ts
@@ -82,7 +82,7 @@ describe('SourceControlPushModal', () => {
 
 		vi.spyOn(route, 'fullPath', 'get').mockReturnValue('/home/workflows');
 
-		const { getAllByTestId } = renderModal({
+		const { getByTestId, getAllByTestId } = renderModal({
 			pinia: createTestingPinia(),
 			props: {
 				data: {
@@ -92,7 +92,7 @@ describe('SourceControlPushModal', () => {
 			},
 		});
 
-		const files = getAllByTestId('source-control-push-modal-file');
+		const files = getAllByTestId('source-control-push-modal-file-checkbox');
 		expect(files).toHaveLength(2);
 
 		await userEvent.click(files[0]);
@@ -101,11 +101,50 @@ describe('SourceControlPushModal', () => {
 
 		await userEvent.click(within(files[0]).getByRole('checkbox'));
 		expect(within(files[0]).getByRole('checkbox')).not.toBeChecked();
+		expect(within(files[1]).getByRole('checkbox')).not.toBeChecked();
 
 		await userEvent.click(within(files[1]).getByRole('checkbox'));
+		expect(within(files[0]).getByRole('checkbox')).not.toBeChecked();
 		expect(within(files[1]).getByRole('checkbox')).toBeChecked();
 
 		await userEvent.click(files[1]);
+		expect(within(files[0]).getByRole('checkbox')).not.toBeChecked();
+		expect(within(files[1]).getByRole('checkbox')).not.toBeChecked();
+
+		await userEvent.click(within(files[0]).getByText('My workflow 2'));
+		expect(within(files[0]).getByRole('checkbox')).toBeChecked();
+		expect(within(files[1]).getByRole('checkbox')).not.toBeChecked();
+
+		await userEvent.click(within(files[1]).getByText('My workflow 1'));
+		expect(within(files[0]).getByRole('checkbox')).toBeChecked();
+		expect(within(files[1]).getByRole('checkbox')).toBeChecked();
+
+		await userEvent.click(within(files[1]).getByText('My workflow 1'));
+		expect(within(files[0]).getByRole('checkbox')).toBeChecked();
+		expect(within(files[1]).getByRole('checkbox')).not.toBeChecked();
+
+		await userEvent.click(getByTestId('source-control-push-modal-toggle-all'));
+		expect(within(files[0]).getByRole('checkbox')).toBeChecked();
+		expect(within(files[1]).getByRole('checkbox')).toBeChecked();
+
+		await userEvent.click(within(files[0]).getByText('My workflow 2'));
+		await userEvent.click(within(files[1]).getByText('My workflow 1'));
+		expect(within(files[0]).getByRole('checkbox')).not.toBeChecked();
+		expect(within(files[1]).getByRole('checkbox')).not.toBeChecked();
+		expect(
+			within(getByTestId('source-control-push-modal-toggle-all')).getByRole('checkbox'),
+		).not.toBeChecked();
+
+		await userEvent.click(within(files[0]).getByText('My workflow 2'));
+		await userEvent.click(within(files[1]).getByText('My workflow 1'));
+		expect(within(files[0]).getByRole('checkbox')).toBeChecked();
+		expect(within(files[1]).getByRole('checkbox')).toBeChecked();
+		expect(
+			within(getByTestId('source-control-push-modal-toggle-all')).getByRole('checkbox'),
+		).toBeChecked();
+
+		await userEvent.click(getByTestId('source-control-push-modal-toggle-all'));
+		expect(within(files[0]).getByRole('checkbox')).not.toBeChecked();
 		expect(within(files[1]).getByRole('checkbox')).not.toBeChecked();
 	});
 });
diff --git a/packages/editor-ui/src/components/SourceControlPushModal.ee.vue b/packages/editor-ui/src/components/SourceControlPushModal.ee.vue
index f0d262ba5554f..0a0d99389a330 100644
--- a/packages/editor-ui/src/components/SourceControlPushModal.ee.vue
+++ b/packages/editor-ui/src/components/SourceControlPushModal.ee.vue
@@ -262,63 +262,66 @@ function getStatusText(file: SourceControlAggregatedFile): string {
 			<div :class="$style.container">
 				<div v-if="files.length > 0">
 					<div v-if="workflowFiles.length > 0">
-						<n8n-text>
+						<n8n-text tag="div" class="mb-l">
 							{{ i18n.baseText('settings.sourceControl.modals.push.description') }}
 							<n8n-link :to="i18n.baseText('settings.sourceControl.docs.using.pushPull.url')">
 								{{ i18n.baseText('settings.sourceControl.modals.push.description.learnMore') }}
 							</n8n-link>
 						</n8n-text>
 
-						<div class="mt-l mb-2xs">
-							<n8n-checkbox
-								:indeterminate="selectAllIndeterminate"
-								:model-value="selectAll"
-								@update:model-value="onToggleSelectAll"
-							>
-								<n8n-text bold tag="strong">
-									{{ i18n.baseText('settings.sourceControl.modals.push.workflowsToCommit') }}
-								</n8n-text>
-								<n8n-text v-show="workflowFiles.length > 0" tag="strong">
-									({{ stagedWorkflowFiles.length }}/{{ workflowFiles.length }})
-								</n8n-text>
-							</n8n-checkbox>
-						</div>
-						<label
+						<n8n-checkbox
+							:class="$style.selectAll"
+							:indeterminate="selectAllIndeterminate"
+							:model-value="selectAll"
+							data-test-id="source-control-push-modal-toggle-all"
+							@update:model-value="onToggleSelectAll"
+						>
+							<n8n-text bold tag="strong">
+								{{ i18n.baseText('settings.sourceControl.modals.push.workflowsToCommit') }}
+							</n8n-text>
+							<n8n-text v-show="workflowFiles.length > 0" tag="strong">
+								({{ stagedWorkflowFiles.length }}/{{ workflowFiles.length }})
+							</n8n-text>
+						</n8n-checkbox>
+
+						<n8n-checkbox
 							v-for="file in sortedFiles"
-							v-show="!defaultStagedFileTypes.includes(file.type)"
 							:key="file.file"
-							:for="file.name"
-							:class="$style.listItem"
-							data-test-id="source-control-push-modal-file"
+							:class="[
+								'scopedListItem',
+								$style.listItem,
+								{ [$style.hiddenListItem]: defaultStagedFileTypes.includes(file.type) },
+							]"
+							data-test-id="source-control-push-modal-file-checkbox"
+							:model-value="staged[file.file]"
+							@update:model-value="setStagedStatus(file, !staged[file.file])"
 						>
-							<n8n-checkbox
-								:id="file.name"
-								:model-value="staged[file.file]"
-								:class="$style.listItemCheckbox"
-								@update:model-value="setStagedStatus(file, !staged[file.file])"
-							/>
-							<div>
+							<span>
 								<n8n-text v-if="file.status === 'deleted'" color="text-light">
 									<span v-if="file.type === 'workflow'"> Deleted Workflow: </span>
 									<span v-if="file.type === 'credential'"> Deleted Credential: </span>
 									<strong>{{ file.name || file.id }}</strong>
 								</n8n-text>
 								<n8n-text v-else bold> {{ file.name }} </n8n-text>
-								<div v-if="file.updatedAt">
-									<n8n-text color="text-light" size="small">
-										{{ renderUpdatedAt(file) }}
-									</n8n-text>
-								</div>
-							</div>
-							<div :class="$style.listItemStatus">
+								<n8n-text
+									v-if="file.updatedAt"
+									tag="p"
+									class="mt-0"
+									color="text-light"
+									size="small"
+								>
+									{{ renderUpdatedAt(file) }}
+								</n8n-text>
+							</span>
+							<span>
 								<n8n-badge v-if="workflowId === file.id && file.type === 'workflow'" class="mr-2xs">
 									Current workflow
 								</n8n-badge>
 								<n8n-badge :theme="statusToBadgeThemeMap[file.status] || 'default'">
 									{{ getStatusText(file) }}
 								</n8n-badge>
-							</div>
-						</label>
+							</span>
+						</n8n-checkbox>
 					</div>
 					<n8n-notice v-else class="mt-0">
 						<i18n-t keypath="settings.sourceControl.modals.push.noWorkflowChanges">
@@ -398,16 +401,16 @@ function getStatusText(file: SourceControlAggregatedFile): string {
 	&:last-child {
 		margin-bottom: 0;
 	}
-}
 
-.listItemCheckbox {
-	display: inline-flex !important;
-	margin-bottom: 0 !important;
-	margin-right: var(--spacing-2xs) !important;
+	&.hiddenListItem {
+		display: none !important;
+	}
 }
 
-.listItemStatus {
-	margin-left: auto;
+.selectAll {
+	float: left;
+	clear: both;
+	margin: 0 0 var(--spacing-2xs);
 }
 
 .footer {
@@ -416,3 +419,12 @@ function getStatusText(file: SourceControlAggregatedFile): string {
 	justify-content: flex-end;
 }
 </style>
+
+<style scoped lang="scss">
+.scopedListItem :deep(.el-checkbox__label) {
+	display: flex;
+	width: 100%;
+	justify-content: space-between;
+	align-items: center;
+}
+</style>