From 871a69ae197475aef1f0d45ceb8f940c5386c6e6 Mon Sep 17 00:00:00 2001 From: "Michal Dziekonski (mdz)" Date: Fri, 3 May 2024 21:25:20 +0200 Subject: [PATCH] fix: Protect against incomplete data coming from Spoolman (GH-43) (#44) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: Enable actual typescript code verification (GH-43) Signed-off-by: Michał Dziekoński * fix: Provide basic typings (global vars & own code) (GH-43) Signed-off-by: Michał Dziekoński * fix: Fix typescript include (GH-43) Signed-off-by: Michał Dziekoński * fix: Fix spool typings (GH-43) Signed-off-by: Michał Dziekoński * fix: When rendering spools data, use safer, more error resistant field access method (GH-43) Signed-off-by: Michał Dziekoński * fix: Return failure on empty get spools response (GH-43) Signed-off-by: Michał Dziekoński * fix: Fix Spool.filament.vendor typing (GH-43) Signed-off-by: Michał Dziekoński * fix: Add spool data display formatter (GH-43) Signed-off-by: Michał Dziekoński * fix: Use safe and formatted spool data in confirmation modal (GH-43) Signed-off-by: Michał Dziekoński * fix: Display invalid spool warning (GH-43) Signed-off-by: Michał Dziekoński * fix: Use safe and formatted spool data in selector modal (GH-43) Signed-off-by: Michał Dziekoński * fix: Do not allow to select invalid spools (GH-43) Signed-off-by: Michał Dziekoński * fix: Use safe and formatted spool data in sidebar list (GH-43) Signed-off-by: Michał Dziekoński * fix: Use safe and formatted spool data in selector modal, current selection (GH-43) Signed-off-by: Michał Dziekoński * fix: Improve sidebar layout in edge cases (GH-43) Signed-off-by: Michał Dziekoński --------- Signed-off-by: Michał Dziekoński --- octoprint_Spoolman/SpoolmanPlugin.py | 1 + octoprint_Spoolman/static/js/Spoolman_api.js | 45 +++++++--- .../static/js/Spoolman_modal_confirmSpool.js | 32 ++++++- .../static/js/Spoolman_modal_selectSpool.js | 18 +++- .../static/js/Spoolman_sidebar.js | 1 + .../static/js/api/getSpoolmanSpools.js | 63 +++++++++++--- octoprint_Spoolman/static/js/common/api.js | 15 +++- .../static/js/common/formatting.js | 65 ++++++++++++++ .../static/js/common/promiseCache.js | 39 ++++++++- .../static/js/types/global.d.ts | 60 +++++++++++++ .../Spoolman_modal_confirmspool.jinja2 | 52 +++++++---- .../templates/Spoolman_sidebar.jinja2 | 86 ++++++++++++------- .../Spoolman_sidebar_modal_selectspool.jinja2 | 73 ++++++++++------ tsconfig.json | 13 +++ 14 files changed, 452 insertions(+), 111 deletions(-) create mode 100644 octoprint_Spoolman/static/js/common/formatting.js create mode 100644 octoprint_Spoolman/static/js/types/global.d.ts create mode 100644 tsconfig.json diff --git a/octoprint_Spoolman/SpoolmanPlugin.py b/octoprint_Spoolman/SpoolmanPlugin.py index 0d0a325..d3000be 100644 --- a/octoprint_Spoolman/SpoolmanPlugin.py +++ b/octoprint_Spoolman/SpoolmanPlugin.py @@ -88,6 +88,7 @@ def get_assets(self): "js/common/api.js", "js/common/promiseCache.js", "js/common/octoprint.js", + "js/common/formatting.js", "js/api/getSpoolmanSpools.js", "js/api/getCurrentJobRequirements.js", "js/api/updateActiveSpool.js", diff --git a/octoprint_Spoolman/static/js/Spoolman_api.js b/octoprint_Spoolman/static/js/Spoolman_api.js index 75d6afe..316c6c3 100644 --- a/octoprint_Spoolman/static/js/Spoolman_api.js +++ b/octoprint_Spoolman/static/js/Spoolman_api.js @@ -1,20 +1,21 @@ -$(() => { +const createApi = () => { // from setup.py plugin_identifier const PLUGIN_ID = "Spoolman"; const cache = new PromiseCache(); const apiClient = new APIClient(PLUGIN_ID, BASEURL); - const cacheGetSpoolmanSpoolsResult = cache.registerResource("getSpoolmanSpools", { - getter: async () => { - const request = await getSpoolmanSpools(apiClient); + const sharedGetSpoolmanSpools = async () => { + const request = await getSpoolmanSpools(apiClient); - if (!request.isSuccess) { - console.error("Request error", request.error); - } + if (!request.isSuccess) { + console.error("Request error", request.error); + } - return request; - }, + return request; + }; + const cacheGetSpoolmanSpoolsResult = cache.registerResource("getSpoolmanSpools", { + getter: sharedGetSpoolmanSpools, }); const sharedGetCurrentJobRequirements = async () => { const request = await getCurrentJobRequirements(apiClient); @@ -25,6 +26,9 @@ $(() => { return request; }; + /** + * @param {Parameters[1]} params + */ const sharedUpdateActiveSpool = async ({ toolIdx, spoolId }) => { const request = await updateActiveSpool(apiClient, { toolIdx, spoolId }); @@ -39,11 +43,30 @@ $(() => { throw new Error('[Spoolman][api] Could not create cached "getSpoolmanSpools"'); } - window.pluginSpoolmanApi = { + const pluginSpoolmanApiNamespace = { cache, + /** @type {typeof sharedGetSpoolmanSpools & { invalidate: CachedGetterFn['invalidate'] }} */ + // @ts-ignore getSpoolmanSpools: cacheGetSpoolmanSpoolsResult.getter, getCurrentJobRequirements: sharedGetCurrentJobRequirements, updateActiveSpool: sharedUpdateActiveSpool, }; -}); + + /** + * @typedef {typeof pluginSpoolmanApiNamespace} PluginSpoolmanApiType + */ + + // @ts-ignore + window.pluginSpoolmanApi = pluginSpoolmanApiNamespace; + + return { + pluginSpoolmanApiNamespace, + }; +}; + +$(createApi); + +/** + * @typedef {ReturnType['pluginSpoolmanApiNamespace']} PluginSpoolmanApiType + */ diff --git a/octoprint_Spoolman/static/js/Spoolman_modal_confirmSpool.js b/octoprint_Spoolman/static/js/Spoolman_modal_confirmSpool.js index 821fccb..fd1d0d1 100644 --- a/octoprint_Spoolman/static/js/Spoolman_modal_confirmSpool.js +++ b/octoprint_Spoolman/static/js/Spoolman_modal_confirmSpool.js @@ -20,6 +20,8 @@ $(() => { * Events: * - onConfirm * - onHidden + * + * @param {*} params */ function SpoolmanModalConfirmSpoolViewModel(params) { const self = this; @@ -109,6 +111,8 @@ $(() => { const spoolId = selectedSpoolIds[extruderIdx]?.spoolId(); const spoolData = spoolmanSpools.find((spool) => String(spool.id) === spoolId); + const safeSpool = spoolData && toSafeSpool(spoolData); + const spoolDisplayData = spoolData && toSpoolForDisplay(spoolData, { constants: self.constants }); const toolFilamentUsage = currentJobRequirements.tools[extruderIdx]; if ( @@ -118,6 +122,8 @@ $(() => { return { spoolId, spoolData, + spoolDisplayData, + isSpoolValid: safeSpool?.isSpoolValid, /** @type false */ isToolInUse: false, isToolMissingSelection: undefined, @@ -126,13 +132,20 @@ $(() => { } const isToolMissingSelection = !toolFilamentUsage.spoolId; - const isEnoughFilamentAvailable = isToolMissingSelection || !spoolData - ? undefined - : toolFilamentUsage.filamentWeight <= spoolData.remaining_weight; + + const isEnoughFilamentAvailable = ( + !isToolMissingSelection && + safeSpool && + safeSpool.isSpoolValid + ) + ? toolFilamentUsage.filamentWeight <= safeSpool.spoolData.remaining_weight + : undefined; return { spoolId, - spoolData, + spoolData: safeSpool && safeSpool.spoolData, + spoolDisplayData, + isSpoolValid: safeSpool?.isSpoolValid, /** @type true */ isToolInUse: true, isToolMissingSelection, @@ -155,6 +168,16 @@ $(() => { ? self.constants.filament_problems.NOT_ENOUGH_FILAMENT : undefined ), + ( + selectedSpools.some((spool) => { + return ( + spool.isToolInUse && + spool.isSpoolValid === false + ); + }) + ? self.constants.filament_problems.INVALID_SPOOL + : undefined + ), ( selectedSpools.some((spool) => spool.isToolMissingSelection === true) ? self.constants.filament_problems.MISSING_SPOOL_SELECTION @@ -204,6 +227,7 @@ $(() => { length_unit: 'mm', filament_problems: { + INVALID_SPOOL: 'INVALID_SPOOL', NO_FILAMENT_USAGE_DATA: 'NO_FILAMENT_USAGE_DATA', NOT_ENOUGH_FILAMENT: 'NOT_ENOUGH_FILAMENT', MISSING_SPOOL_SELECTION: 'MISSING_SPOOL_SELECTION', diff --git a/octoprint_Spoolman/static/js/Spoolman_modal_selectSpool.js b/octoprint_Spoolman/static/js/Spoolman_modal_selectSpool.js index f188133..8a662ce 100644 --- a/octoprint_Spoolman/static/js/Spoolman_modal_selectSpool.js +++ b/octoprint_Spoolman/static/js/Spoolman_modal_selectSpool.js @@ -69,9 +69,23 @@ $(() => { return String(spool.id) === toolSpoolId; }); + const spoolmanSafeSpools = spoolmanSpools.map((spool) => { + return { + ...toSafeSpool(spool), + displayData: toSpoolForDisplay(spool, { constants: self.constants }), + }; + }); + self.templateData.toolCurrentSpoolId(toolSpoolId); - self.templateData.toolCurrentSpool(toolSpool); - self.templateData.tableItemsOnCurrentPage(spoolmanSpools); + self.templateData.toolCurrentSpool( + toolSpool + ? { + ...toSafeSpool(toolSpool), + displayData: toSpoolForDisplay(toolSpool, { constants: self.constants }), + } + : undefined + ); + self.templateData.tableItemsOnCurrentPage(spoolmanSafeSpools); self.templateData.spoolmanUrl(getPluginSettings().spoolmanUrl()); diff --git a/octoprint_Spoolman/static/js/Spoolman_sidebar.js b/octoprint_Spoolman/static/js/Spoolman_sidebar.js index 5efea94..ffc42fa 100644 --- a/octoprint_Spoolman/static/js/Spoolman_sidebar.js +++ b/octoprint_Spoolman/static/js/Spoolman_sidebar.js @@ -84,6 +84,7 @@ $(() => { return { spoolId, spoolData, + spoolDisplayData: spoolData && toSpoolForDisplay(spoolData, { constants: self.constants }), }; }); diff --git a/octoprint_Spoolman/static/js/api/getSpoolmanSpools.js b/octoprint_Spoolman/static/js/api/getSpoolmanSpools.js index ad89c4c..ac37fab 100644 --- a/octoprint_Spoolman/static/js/api/getSpoolmanSpools.js +++ b/octoprint_Spoolman/static/js/api/getSpoolmanSpools.js @@ -3,28 +3,29 @@ * @property {Object} data * @property {Array} data.spools * + * @typedef {Object} FilamentVendor + * @property {number} id + * @property {string} name + * * @typedef {Object} Spool * @property {number} id * @property {boolean} archived * @property {string} registered * @property {number} used_length * @property {number} used_weight - * @property {number} remaining_length - * @property {number} remaining_weight - * @property {boolean} archived + * @property {number | undefined} remaining_length + * @property {number | undefined} remaining_weight * @property {Object} filament * @property {number} filament.id + * @property {string} filament.registered * @property {number} filament.diameter * @property {number} filament.density - * @property {number} filament.weight - * @property {number} filament.spool_weight - * @property {string} filament.material - * @property {string} filament.name - * @property {string} filament.registered - * @property {string} filament.color_hex - * @property {Object} filament.vendor - * @property {number} filament.vendor.id - * @property {string} filament.vendor.name + * @property {number | undefined} filament.weight + * @property {number | undefined} filament.spool_weight + * @property {string | undefined} filament.material + * @property {string | undefined} filament.name + * @property {string | undefined} filament.color_hex + * @property {FilamentVendor | undefined} filament.vendor */ /** @@ -37,8 +38,44 @@ async function getSpoolmanSpools(apiClient) { return request; } if (!request.payload.response) { - return /** @type Success<{ response: undefined }> */ (request); + return /** @type Failure */ ({ + isSuccess: false, + error: undefined, + }); } return /** @type Success<{ response: GetSpoolsResponse }> */ (request); } + +/** + * Checks whether spool has "complete-enough" data, so that the plugin + * can function properly. + * + * @type {(spool: Spool) => spool is Spool & { remaining_weight: number }} + */ +const isSpoolValid = (spool) => { + return ( + spool.remaining_weight !== undefined + ); +}; + +/** + * @param {Spool} spool + */ +const toSafeSpool = (spool) => { + if (isSpoolValid(spool)) { + return { + spoolId: spool.id, + /** @type true */ + isSpoolValid: true, + spoolData: spool, + }; + } + + return { + spoolId: spool.id, + /** @type false */ + isSpoolValid: false, + spoolData: spool, + }; +}; diff --git a/octoprint_Spoolman/static/js/common/api.js b/octoprint_Spoolman/static/js/common/api.js index 0dde9c3..c082428 100644 --- a/octoprint_Spoolman/static/js/common/api.js +++ b/octoprint_Spoolman/static/js/common/api.js @@ -76,6 +76,9 @@ const methodsWithBody = [ * @param {string} baseUrl */ function APIClient(pluginId, baseUrl) { + /** + * @param {string | Record} data + */ const _buildRequestQuery = function (data) { if (typeof (data) === 'string') { return data; @@ -92,10 +95,20 @@ function APIClient(pluginId, baseUrl) { .join('&'); }; + /** + * @param {string} url + */ const buildApiUrl = (url) => { return `${baseUrl}plugin/${pluginId}/${url}`; }; + /** + * @param {{ + * method?: string; + * headers?: Record; + * body?: string; + * }} options + */ const buildFetchOptions = (options) => { const requestMethod = (options.method ?? "GET").toUpperCase(); @@ -120,7 +133,7 @@ function APIClient(pluginId, baseUrl) { /** * @param {string} url - * @param {Object} options + * @param {Parameters[0]} options */ const callApi = async (url, options) => { const endpointUrl = buildApiUrl(url); diff --git a/octoprint_Spoolman/static/js/common/formatting.js b/octoprint_Spoolman/static/js/common/formatting.js new file mode 100644 index 0000000..8704e5a --- /dev/null +++ b/octoprint_Spoolman/static/js/common/formatting.js @@ -0,0 +1,65 @@ +/** + * @param {Spool} spool + * @param {{ +* constants: Record +* }} params +*/ +const toSpoolForDisplay = (spool, params) => { + return { + filament: { + color: { + cssProperty: ( + spool.filament.color_hex + ? 'background-color' + : 'background' + ), + cssValue: ( + spool.filament.color_hex + ? `#${spool.filament.color_hex}` + : 'linear-gradient(45deg, #000000 -25%, #ffffff)' + ), + }, + name: ( + spool.filament.name + ? { + displayValue: spool.filament.name, + } : { + displayValue: "Unnamed filament", + } + ), + material: ( + spool.filament.material + ? { + displayShort: spool.filament.material, + displayValue: spool.filament.material, + } : { + displayShort: "Unknown", + displayValue: "Unknown material", + } + ), + vendor: { + name: ( + spool.filament.vendor?.name + ? { + displayValue: spool.filament.vendor.name, + } : { + displayValue: "Unknown filament vendor", + } + ), + }, + }, + used_weight: { + displayValue: spool.used_weight.toFixed(1), + }, + remaining_weight: ( + spool.remaining_weight !== undefined + ? { + isValid: true, + displayValue: `${spool.remaining_weight.toFixed(1)}${params.constants['weight_unit']}`, + } : { + isValid: false, + displayValue: "Unavailable", + } + ), + }; +}; diff --git a/octoprint_Spoolman/static/js/common/promiseCache.js b/octoprint_Spoolman/static/js/common/promiseCache.js index bc9d95d..e20947e 100644 --- a/octoprint_Spoolman/static/js/common/promiseCache.js +++ b/octoprint_Spoolman/static/js/common/promiseCache.js @@ -1,9 +1,15 @@ +/** + * @typedef {(...args: unknown[]) => Promise} GetterFn + +* @typedef {GetterFn & { invalidate: () => void }} CachedGetterFn + */ + class PromiseCache { constructor() { /** * @type {Record Promise, - * cachedGetter: ((...args: unknown[]) => Promise) & { invalidate: () => void }, + * getter: GetterFn, + * cachedGetter: CachedGetterFn, * resourceState: * | { * isCached: false, @@ -16,10 +22,19 @@ class PromiseCache { */ this.resourcesByKey = {}; + /** + * @type {Record void>>} + */ this.invalidationHandlersByKey = {}; } + /** + * @param {string} resourceKey + */ _createCachedGetter(resourceKey) { + /** + * @param {...unknown} args + */ const getter = async (...args) => { const resourceCache = this.resourcesByKey[resourceKey]; @@ -42,6 +57,9 @@ class PromiseCache { return getter; } + /** + * @param {string[]} resourceKeys + */ _triggerInvalidationHandlersFor(resourceKeys) { const handlersToRun = new Set(); @@ -58,9 +76,14 @@ class PromiseCache { }); } + /** + * @param {string} resourceKey + * @param {{ getter: GetterFn }} params + */ registerResource(resourceKey, params) { if (this.resourcesByKey[resourceKey]) { return { + /** @type false */ isSuccess: false, error: { resourceAlreadyExists: true, @@ -77,11 +100,16 @@ class PromiseCache { }; return { + /** @type true */ isSuccess: true, getter: this.resourcesByKey[resourceKey].cachedGetter, }; } + /** + * @param {string} resourceKey + * @param {{ preventHandlersTrigger?: boolean }} options + */ invalidateResource(resourceKey, options = {}) { const resource = this.resourcesByKey[resourceKey]; @@ -108,6 +136,10 @@ class PromiseCache { }; } + /** + * @param {string[]} resourceKeys + * @param {{ preventHandlersTrigger?: boolean }} options + */ invalidateResources(resourceKeys, options = {}) { resourceKeys.forEach((resourceKey) => { this.invalidateResource(resourceKey, { preventHandlersTrigger: true }); @@ -125,6 +157,9 @@ class PromiseCache { * * Note: does not properly support async handlers, in a sense that * each executed handler will be executed synchronously, not awaiting the resulting promise. + * + * @param {string[]} resourceKeys + * @param {() => void} handler */ onResourcesInvalidated(resourceKeys, handler) { resourceKeys.forEach((resourceKey) => { diff --git a/octoprint_Spoolman/static/js/types/global.d.ts b/octoprint_Spoolman/static/js/types/global.d.ts new file mode 100644 index 0000000..50d84a1 --- /dev/null +++ b/octoprint_Spoolman/static/js/types/global.d.ts @@ -0,0 +1,60 @@ +declare global { + // Plugin global definitions + const pluginSpoolmanApi: PluginSpoolmanApiType; + + // Octoprint related definitions + const OctoPrint: { + socket: { + onMessage: ( + messageType: string, + callback: (message: { + data: { + type?: string; + payload: unknown; + }; + }) => void + ) => void; + }; + }; + const OctoPrintClient: { + new(params: unknown): typeof OctoPrintClient; + + getCookie: (name: string) => string; + }; + const OCTOPRINT_VIEWMODELS: { + push: (viewModel: { + construct: unknown; + dependencies: string[]; + elements: (Element | null)[]; + }) => void; + }; + + const PNotify: { + new (params: { + title: string; + text: string; + type: string; + hide: boolean; + addclass: string; + }): void + }; + + const BASEURL: string; + + // Knockout + const ko: { + observable: (arg?: ValueType) => ((value: ValueType) => void); + components: { + register: (name: string, params: { viewModel: unknown; template: Record}) => void; + }; + }; + + // jQuery + const $: { + (...args: unknown[]): typeof $; + + on(eventName: string, selector: string, callback: () => void): void; + }; +} + +export {} diff --git a/octoprint_Spoolman/templates/Spoolman_modal_confirmspool.jinja2 b/octoprint_Spoolman/templates/Spoolman_modal_confirmspool.jinja2 index be3de2a..9279816 100644 --- a/octoprint_Spoolman/templates/Spoolman_modal_confirmspool.jinja2 +++ b/octoprint_Spoolman/templates/Spoolman_modal_confirmspool.jinja2 @@ -81,6 +81,9 @@
  • One of the selected spools does not have enough filament.
    • + +
  • One of the selected spools does not include all necessary information.
    • +
  • One of the tools is missing spool selection.
    • @@ -95,7 +98,7 @@
    - +
    @@ -126,7 +129,7 @@ - + - +
    @@ -155,16 +158,24 @@
    - - [] - - - - () - + [] + + ()
    @@ -172,7 +183,17 @@
    Required: - + Remaining on spool: - - + +
    diff --git a/octoprint_Spoolman/templates/Spoolman_sidebar.jinja2 b/octoprint_Spoolman/templates/Spoolman_sidebar.jinja2 index 75639e3..60498a4 100644 --- a/octoprint_Spoolman/templates/Spoolman_sidebar.jinja2 +++ b/octoprint_Spoolman/templates/Spoolman_sidebar.jinja2 @@ -66,22 +66,24 @@ data-bind="foreach: templateData.selectedSpoolsByToolIdx" >
    - -
    - Tool #: -
    - - - -
    - - Unknown spool selected - - - No spool selected + +
    + +
    + Tool #: +
    + +
    + + Unknown spool selected + + + No spool selected + +
    -
    +