Merge pull request #379 from fractal-analytics-platform/performance-improvements

Improved performance and some minor issues

Author: zonia3000

CHANGELOG.md

@@ -1,5 +1,18 @@
 *Note: Numbers like (\#123) point to closed Pull Requests on the fractal-web repository.*
 
+# Unreleased 
+
+* Added search project field on projects list page (\#379).
+* Used modal to create new project (\#379).
+* Sorted users in admin-area page (\#379).
+* Exposed stop-job and download-logs in admin-area jobs page (\#379).
+* Displayed number of rows in admin jobs page (\#379).
+* Fixed semver circular dependency issue (\#379).
+* Fixed issue with version sorting (\#379).
+* Sorted datasets by name (\#379).
+* Fixed accessibility issue (\#379).
+* Improved performance reducing the number of API calls on project, workflow and dataset pages (\#379).
+
 # 0.7.2
 
 Note: with this release, `PUBLIC_OAUTH_CLIENT_NAME` becomes a required

__tests__/semver.test.js

@@ -1,5 +1,5 @@
 import { it, expect } from 'vitest';
-import semver from 'semver';
+import valid from 'semver/functions/valid';
 import { greatestVersionAsc } from '../src/lib/common/component_utilities';
 
 it('should be able to loosely validate versions', () => {
@@ -20,14 +20,14 @@ it('should be able to loosely validate versions', () => {
 	};
 
 	cases.forEach(([input, expected]) => {
-		const validatedVersion = semver.valid(input, validationOptions);
+		const validatedVersion = valid(input, validationOptions);
 		expect(validatedVersion).toBe(expected);
 	});
 });
 
 it('should sort versions correctly', () => {
-
 	const versions = [
+		{ version: '2' },
 		{ version: '0.10.0c0' },
 		{ version: '0.10.0b4' },
 		{ version: '0.10.0' },
@@ -37,12 +37,18 @@ it('should sort versions correctly', () => {
 		{ version: '0.10.0a0' },
 		{ version: '1.0.0rc4.dev7' },
 		{ version: '0.10.0beta5' },
-		{ version: '0.10.0alpha0' }
+		{ version: '0.10.0alpha0' },
+		{ version: '0.1.2' },
+		{ version: '0.1.dev27+g1458b59' },
+		{ version: '0.2.0a0' }
 	];
 
-	const sortedVersions = versions.sort(greatestVersionAsc);
+	const sortedVersions = [...versions].sort(greatestVersionAsc);
 
-	expect(sortedVersions).toEqual([
+	const expectedSortedVersions = [
+		{ version: '0.1.dev27+g1458b59' },
+		{ version: '0.1.2' },
+		{ version: '0.2.0a0' },
 		{ version: '0.10.0a0' },
 		{ version: '0.10.0a2' },
 		{ version: '0.10.0alpha0' },
@@ -52,6 +58,19 @@ it('should sort versions correctly', () => {
 		{ version: '0.10.0c0' },
 		{ version: '0.10.0' },
 		{ version: '1.0.0rc4.dev7' },
-		{ version: '1.0.0' }
-	]);
-});
+		{ version: '1.0.0' },
+		{ version: '2' }
+	];
+
+	expect(sortedVersions).toEqual(expectedSortedVersions);
+
+	// Test that the sorting works also with 'v' prefix
+	const v_versions = versions.map((v) => ({ version: `v${v.version}` }));
+	const v_expectedSortedVersions = expectedSortedVersions.map((v) => ({
+		version: `v${v.version}`
+	}));
+
+	const v_sortedVersions = [...v_versions].sort(greatestVersionAsc);
+
+	expect(v_sortedVersions).toEqual(v_expectedSortedVersions);
+});

__tests__/user_utilities.test.js

@@ -0,0 +1,32 @@
+import { it, expect } from 'vitest';
+import { sortUsers } from '$lib/components/admin/user_utilities';
+
+it('should sort user by current superuser, then by superuser, then by email', () => {
+	let users = [
+		{ id: 5, is_superuser: false, email: '[email protected]' },
+		{ id: 4, is_superuser: false, email: '[email protected]' },
+		{ id: 3, is_superuser: true, email: '[email protected]' },
+		{ id: 2, is_superuser: true, email: '[email protected]' },
+		{ id: 1, is_superuser: true, email: '[email protected]' }
+	];
+
+	sortUsers(users, 1);
+
+	expect(users[0].id).eq(1);
+	expect(users[1].id).eq(2);
+	expect(users[2].id).eq(3);
+	expect(users[3].id).eq(4);
+	expect(users[4].id).eq(5);
+
+	users = [
+		{ id: 1, is_superuser: true, email: '[email protected]' },
+		{ id: 2, is_superuser: true, email: '[email protected]' },
+		{ id: 3, is_superuser: false, email: '[email protected]' }
+	];
+
+	sortUsers(users, 2);
+
+	expect(users[0].id).eq(2);
+	expect(users[1].id).eq(1);
+	expect(users[2].id).eq(3);
+});

src/lib/common/component_utilities.js

@@ -1,21 +1,20 @@
 import { marked } from 'marked';
 import DOMPurify from 'dompurify';
-import semver from 'semver';
+import coerce from 'semver/functions/coerce';
+import gte from 'semver/functions/gte';
+import lte from 'semver/functions/lte';
+import valid from 'semver/functions/valid';
 
 /**
  * @param {import('$lib/types').Task} t1
  * @param {import('$lib/types').Task} t2
  * @returns {-1|0|1}
  */
 export function greatestVersionAsc(t1, t2) {
-	const semverValidationOptions = {
-		loose: true,
-		includePrerelease: true
-	};
-	const t1Version = semver.valid(t1.version, semverValidationOptions);
-	const t2Version = semver.valid(t2.version, semverValidationOptions);
+	const t1Version = validateVersion(t1.version);
+	const t2Version = validateVersion(t2.version);
 	if (t1Version !== null && t2Version !== null) {
-		const t1VersionLt = semver.lte(t1Version, t2Version);
+		const t1VersionLt = lte(t1Version, t2Version);
 		return t1VersionLt ? -1 : 1;
 	}
 	return 0;
@@ -27,19 +26,32 @@ export function greatestVersionAsc(t1, t2) {
  * @returns {-1|0|1}
  */
 export function greatestVersionDesc(t1, t2) {
-	const semverValidationOptions = {
-		loose: true,
-		includePrerelease: true
-	};
-	const t1Version = semver.valid(t1.version, semverValidationOptions);
-	const t2Version = semver.valid(t2.version, semverValidationOptions);
+	const t1Version = validateVersion(t1.version);
+	const t2Version = validateVersion(t2.version);
 	if (t1Version !== null && t2Version !== null) {
-		const t1VersionGt = semver.gte(t1Version, t2Version);
+		const t1VersionGt = gte(t1Version, t2Version);
 		return t1VersionGt ? -1 : 1;
 	}
 	return 0;
 }
 
+const semverValidationOptions = {
+	loose: true,
+	includePrerelease: true
+};
+
+/**
+ * @param {string} version
+ * @returns {string | null}
+ */
+function validateVersion(version) {
+	return (
+		valid(version, semverValidationOptions) ||
+		valid(coerce(version), semverValidationOptions) ||
+		null
+	);
+}
+
 /**
  * @param {import('$lib/types').Task} t1
  * @param {import('$lib/types').Task} t2

src/lib/components/admin/user_utilities.js

@@ -0,0 +1,25 @@
+/**
+ * @param {Array<import('$lib/types').User & {id: number}>} users
+ * @param {number} currentAdminId
+ */
+export function sortUsers(users, currentAdminId) {
+	users.sort((a, b) => {
+		// current admin is always first
+		if (a.id === currentAdminId) {
+			return -1;
+		}
+		if (b.id === currentAdminId) {
+			return 1;
+		}
+		// prioritize superusers
+		if (a.is_superuser && !b.is_superuser) {
+			return -1;
+		}
+		if (!a.is_superuser && b.is_superuser) {
+			return 1;
+		}
+		// then sort by email
+		return a.email < b.email ? -1 : 1;
+	});
+	return users;
+}

src/lib/components/common/jschema/PropertyDiscriminator.svelte

@@ -5,9 +5,10 @@
 	import ArrayProperty from '$lib/components/common/jschema/ArrayProperty.svelte';
 	import ObjectProperty from '$lib/components/common/jschema/ObjectProperty.svelte';
 
+        /** @type {any|undefined} */
 	export let schemaProperty = undefined;
 
-	const key = schemaProperty.key;
+	const key = schemaProperty?.key;
 
 </script>
 
@@ -40,4 +41,4 @@
     <p>Unable to display schema property | unknown property type</p>
   </div>
 
-{/if}
+{/if}

src/lib/components/common/jschema/schema_management.js

@@ -2,8 +2,8 @@ export default class SchemaManager {
 	keySeparator = '###';
 	propertiesMap = new Map();
 	hasUnsavedChanges = false;
-	onPropertyChanges = () => {
-	};
+	/** @type {(hasChanges: boolean) => void} */
+	onPropertyChanges = () => {};
 
 	constructor(schema, schemaData) {
 		this.loadSchema(schema);

src/lib/components/jobs/JobsList.svelte

@@ -92,7 +92,10 @@
 		}
 
 		console.log('Stop running job');
-		const response = await fetch(`/api/v1/project/${job.project_id}/job/${job.id}/stop`, {
+		let stopJobUrl = admin
+			? `/api/admin/job/${job.id}/stop`
+			: `/api/v1/project/${job.project_id}/job/${job.id}/stop`;
+		const response = await fetch(stopJobUrl, {
 			method: 'GET',
 			credentials: 'include'
 		});
@@ -133,6 +136,17 @@
 		updateJobsTimeout = setTimeout(updateJobsInBackground, updateJobsInterval);
 	}
 
+	/**
+	 * @param {import('$lib/types').ApplyWorkflow} row
+	 */
+	function getDownloadUrl(row) {
+		if (admin) {
+			return `/api/admin/job/${row.id}/download`;
+		} else {
+			return `/api/v1/project/${row.project_id}/job/${row.id}/download`;
+		}
+	}
+
 	onMount(() => {
 		updateJobsTimeout = setTimeout(updateJobsInBackground, updateJobsInterval);
 	});
@@ -292,17 +306,17 @@
 									<i class="bi-list-columns-reverse" />
 									Logs
 								</button>
-								{#if row.project_id !== null && row.user_email === $page.data.userInfo.email}
+								{#if (admin && row.id) || (row.project_id !== null && row.user_email === $page.data.userInfo.email)}
 									<a
 										class="btn btn-light"
-										href={`/api/v1/project/${row.project_id}/job/${row.id}/download`}
+										href={getDownloadUrl(row)}
 										download={`${row.id}_logs.zip`}
 									>
 										<i class="bi-arrow-down-circle" />
 									</a>
 								{/if}
 							{/if}
-							{#if row.status === 'running' && !admin}
+							{#if row.status === 'running'}
 								<button class="btn btn-danger" on:click={() => handleJobCancel(row)}>
 									<i class="bi-x-circle" /> Cancel
 								</button>

src/lib/components/projects/CreateUpdateDatasetModal.svelte

@@ -4,9 +4,9 @@
 	import { onMount, tick } from 'svelte';
 	import Modal from '../common/Modal.svelte';
 
-	/** @type {(dataset: (import('$lib/types').Dataset)) => void} */
+	/** @type {(dataset: import('$lib/types').Dataset) => void} */
 	export let createDatasetCallback;
-	/** @type {(dataset: (import('$lib/types').Dataset)) => void} */
+	/** @type {(dataset: import('$lib/types').Dataset) => void} */
 	export let updateDatasetCallback;
 
 	/** @type {Modal} */
@@ -177,6 +177,9 @@
 		return result;
 	}
 
+	/**
+	 * @returns {Promise<import('$lib/types').Dataset>}
+	 */
 	async function callUpdateDataset() {
 		const projectId = $page.params.projectId;
 		const headers = new Headers();
@@ -376,6 +379,7 @@
 				});
 			}),
 			project_id: originalDataset.project_id,
+			project: originalDataset.project,
 			history: []
 		};
 	}