From c2f0ee2a06fecb78e14e57c6f7a4591e9e705d35 Mon Sep 17 00:00:00 2001 From: Dave Roberts Date: Tue, 21 Jan 2025 16:43:24 +0000 Subject: [PATCH 1/8] Refactored code and added await to stop race conditions --- .../form-group/autosave/lib/modal.js | 32 ++++++++++--------- 1 file changed, 17 insertions(+), 15 deletions(-) diff --git a/src/frontend/components/form-group/autosave/lib/modal.js b/src/frontend/components/form-group/autosave/lib/modal.js index b0166d7c6..f4d69f815 100644 --- a/src/frontend/components/form-group/autosave/lib/modal.js +++ b/src/frontend/components/form-group/autosave/lib/modal.js @@ -11,24 +11,25 @@ class AutosaveModal extends AutosaveBase { async initAutosave() { const $modal = $(this.element); const $form = $('.form-edit'); - + $modal.find('.btn-js-restore-values').on('click', async (e) => { - this.storage.setItem('recovering', true); - let curvalCount = $form.find('.linkspace-field[data-column-type="curval"]').length; - e.preventDefault(); + e.stopPropagation(); + + await this.storage.setItem('recovering', true); + let curvalCount = $form.find('.linkspace-field[data-column-type="curval"]').length; let errored = false; let $list = $(""); const $body = $modal.find(".modal-body"); $body.html("

Restoring values...

").append($list); - await Promise.all($form.find('.linkspace-field').map(async (_,field) => { + Promise.all($form.find('.linkspace-field').map(async (_, field) => { const $field = $(field); - await this.storage.getItem(this.columnKey($field)).then(json => { + try { + const json = await this.storage.getItem(this.columnKey($field)) let values = json ? JSON.parse(json) : undefined; - return values && Array.isArray(values) ? values : undefined; - }).then(values => { + if (!values) return; const $editButton = $field.closest('.card--topic').find('.btn-js-edit'); if ($editButton && $editButton.length) $editButton.trigger('click'); if (Array.isArray(values)) { @@ -41,29 +42,30 @@ class AutosaveModal extends AutosaveBase { curvalCount--; const $li = $(`
  • Error restoring ${name}, please check these values before submission
    • `); $list.append($li); - if(!curvalCount) this.storage.removeItem('recovering'); + if (!curvalCount) this.storage.removeItem('recovering'); }); $field.on("validationPassed", () => { curvalCount--; const $li = $(`
  • Restored ${name}
    • `); $list.append($li); - if(!curvalCount) this.storage.removeItem('recovering'); + if (!curvalCount) this.storage.removeItem('recovering'); }); } setFieldValues($field, values); - if(type !== "curval") { + if (type !== "curval") { const $li = $(`
  • Restored ${name}
    • `); $list.append($li); } + // For some reason a delay is needed here else the promise forgets what it's doing frontend $field.addClass("field--changed"); } - }).catch(e => { + } catch (e) { const name = $field.data("name"); const $li = $(`
  • Failed to restore ${name}
    • `); console.error(e); $list.append($li); errored = true; - }); + } })).then(() => { $body.append(`

    ${errored ? "Values restored with errors." : "All values restored."} Please check that all field values are as expected.

    `); }).catch(e => { @@ -71,13 +73,13 @@ class AutosaveModal extends AutosaveBase { }).finally(() => { $modal.find(".modal-footer").find("button:not(.btn-cancel)").hide(); $modal.find(".modal-footer").find(".btn-cancel").text("Close"); - if(!curvalCount) this.storage.removeItem('recovering'); + if (!curvalCount) this.storage.removeItem('recovering'); }); }); const item = await this.storage.getItem(this.table_key); - if (item){ + if (item) { $modal.modal('show'); $modal.find('.btn-js-delete-values').attr('disabled', 'disabled').hide(); } From b66ea14ce623f79176975dceafc48076d6b57847 Mon Sep 17 00:00:00 2001 From: Dave Roberts Date: Wed, 22 Jan 2025 11:00:38 +0000 Subject: [PATCH 2/8] Added relevant comments --- .../form-group/autosave/lib/modal.js | 20 ++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/src/frontend/components/form-group/autosave/lib/modal.js b/src/frontend/components/form-group/autosave/lib/modal.js index f4d69f815..8500a8e42 100644 --- a/src/frontend/components/form-group/autosave/lib/modal.js +++ b/src/frontend/components/form-group/autosave/lib/modal.js @@ -16,7 +16,9 @@ class AutosaveModal extends AutosaveBase { e.preventDefault(); e.stopPropagation(); + // This need awaiting or it returns before the value is fully set meaning if the recovery is "fast" it will not clear await this.storage.setItem('recovering', true); + // Count the curvals so we don't return too early let curvalCount = $form.find('.linkspace-field[data-column-type="curval"]').length; let errored = false; @@ -24,30 +26,41 @@ class AutosaveModal extends AutosaveBase { let $list = $(""); const $body = $modal.find(".modal-body"); $body.html("

    Restoring values...

    ").append($list); + // Convert the fields to promise functions (using the fields) that are run in parallel + // This is only done because various parts of the codebase use the fields in different ways dependent on types (i.e. curval) Promise.all($form.find('.linkspace-field').map(async (_, field) => { const $field = $(field); + // This was originally a bunch of promises, but as the code is async, we can await things here try { const json = await this.storage.getItem(this.columnKey($field)) let values = json ? JSON.parse(json) : undefined; + // If the value can't be parsed, ignore it if (!values) return; + // If we are in view mode and we need to switch to edit mode, do that const $editButton = $field.closest('.card--topic').find('.btn-js-edit'); if ($editButton && $editButton.length) $editButton.trigger('click'); if (Array.isArray(values)) { const name = $field.data("name"); const type = $field.data("column-type"); if (type === "curval") { + // Curvals need to work event-driven - this is because the modal doesn't always load fully, + // meaning the setvalue doesn't work correctly for dropdowns (mainly) $field.off("validationFailed"); $field.off("validationPassed"); $field.on("validationFailed", (e) => { + // Decrement the curval count curvalCount--; const $li = $(`
  • Error restoring ${name}, please check these values before submission
    • ${e.message}
    • `); $list.append($li); + // If we've done all fields, turn off the recovery flag if (!curvalCount) this.storage.removeItem('recovering'); }); $field.on("validationPassed", () => { + // Decrement the curval count curvalCount--; const $li = $(`
  • Restored ${name}
    • `); $list.append($li); + // If we've done all fields, turn off the recovery flag if (!curvalCount) this.storage.removeItem('recovering'); }); } @@ -56,10 +69,10 @@ class AutosaveModal extends AutosaveBase { const $li = $(`
  • Restored ${name}
    • `); $list.append($li); } - // For some reason a delay is needed here else the promise forgets what it's doing frontend $field.addClass("field--changed"); } } catch (e) { + // Catch anything within the mapped promises const name = $field.data("name"); const $li = $(`
  • Failed to restore ${name}
    • ${e.message}
    • `); console.error(e); @@ -67,16 +80,21 @@ class AutosaveModal extends AutosaveBase { errored = true; } })).then(() => { + // If there are errors, show an appropriate message, otherwise show a success message $body.append(`

    ${errored ? "Values restored with errors." : "All values restored."} Please check that all field values are as expected.

    `); }).catch(e => { + // If there are any errors that can't be handled in the mapped promises, show a critical error message $body.append(`

    Critical error restoring values

    ${e}

    `); }).finally(() => { + // Hide the restore button and show the close button $modal.find(".modal-footer").find("button:not(.btn-cancel)").hide(); $modal.find(".modal-footer").find(".btn-cancel").text("Close"); + // If we've done all fields, turn off the recovery flag if (!curvalCount) this.storage.removeItem('recovering'); }); }); + // Do we need to run an autorecover? const item = await this.storage.getItem(this.table_key); if (item) { From fef244a2ed68107787303f86831f07e642803729 Mon Sep 17 00:00:00 2001 From: Dave Roberts Date: Thu, 23 Jan 2025 11:24:34 +0000 Subject: [PATCH 3/8] Further updates to mitigate curval errors --- .../form-group/autosave/lib/modal.js | 40 ++++++++++++++----- .../modal/modals/curval/lib/component.js | 7 +++- 2 files changed, 36 insertions(+), 11 deletions(-) diff --git a/src/frontend/components/form-group/autosave/lib/modal.js b/src/frontend/components/form-group/autosave/lib/modal.js index 8500a8e42..eda0e2145 100644 --- a/src/frontend/components/form-group/autosave/lib/modal.js +++ b/src/frontend/components/form-group/autosave/lib/modal.js @@ -1,5 +1,6 @@ import { setFieldValues } from "set-field-values"; import AutosaveBase from './autosaveBase'; +import { fromJson } from "util/common"; /** * A modal that allows the user to restore autosaved values. @@ -10,22 +11,30 @@ class AutosaveModal extends AutosaveBase { */ async initAutosave() { const $modal = $(this.element); + console.log("modal", this.element); const $form = $('.form-edit'); - + $modal.find('.btn-js-restore-values').on('click', async (e) => { e.preventDefault(); e.stopPropagation(); + $modal.find(".modal-footer").find("button").hide(); + // This need awaiting or it returns before the value is fully set meaning if the recovery is "fast" it will not clear await this.storage.setItem('recovering', true); // Count the curvals so we don't return too early - let curvalCount = $form.find('.linkspace-field[data-column-type="curval"]').length; + let curvalCount = 0; + const counter = Promise.all($form.find('.linkspace-field[data-column-type="curval"]').map(async (_, field) => { + await this.storage.getItem(this.columnKey($(field))) && (curvalCount += fromJson(await this.storage.getItem(this.columnKey($(field)))).length); + })); + + await counter; let errored = false; let $list = $("
      "); const $body = $modal.find(".modal-body"); - $body.html("

      Restoring values...

      ").append($list); + $body.html("

      Restoring values...

      Please be aware that linked records may take a moment to finish restoring.

      ").append($list); // Convert the fields to promise functions (using the fields) that are run in parallel // This is only done because various parts of the codebase use the fields in different ways dependent on types (i.e. curval) Promise.all($form.find('.linkspace-field').map(async (_, field) => { @@ -53,15 +62,24 @@ class AutosaveModal extends AutosaveBase { const $li = $(`

    • Error restoring ${name}, please check these values before submission
      • ${e.message}
      • `); $list.append($li); // If we've done all fields, turn off the recovery flag - if (!curvalCount) this.storage.removeItem('recovering'); + if (!curvalCount) { + // Hide the restore button and show the close button + $modal.find(".modal-footer").find(".btn-cancel").text("Close").show(); + this.storage.removeItem('recovering'); + } }); $field.on("validationPassed", () => { // Decrement the curval count curvalCount--; + console.log("curvalCount", curvalCount); const $li = $(`
    • Restored ${name}
      • `); $list.append($li); // If we've done all fields, turn off the recovery flag - if (!curvalCount) this.storage.removeItem('recovering'); + if (!curvalCount) { + // Hide the restore button and show the close button + $modal.find(".modal-footer").find(".btn-cancel").text("Close").show(); + this.storage.removeItem('recovering'); + } }); } setFieldValues($field, values); @@ -86,11 +104,13 @@ class AutosaveModal extends AutosaveBase { // If there are any errors that can't be handled in the mapped promises, show a critical error message $body.append(`

      Critical error restoring values

      ${e}

      `); }).finally(() => { - // Hide the restore button and show the close button - $modal.find(".modal-footer").find("button:not(.btn-cancel)").hide(); - $modal.find(".modal-footer").find(".btn-cancel").text("Close"); - // If we've done all fields, turn off the recovery flag - if (!curvalCount) this.storage.removeItem('recovering'); + // Only allow to close once recovery is finished + console.log("curvalCount", curvalCount); + if(!curvalCount || errored) { + // Show the close button + $modal.find(".modal-footer").find(".btn-cancel").text("Close").show(); + this.storage.removeItem('recovering'); + } }); }); diff --git a/src/frontend/components/modal/modals/curval/lib/component.js b/src/frontend/components/modal/modals/curval/lib/component.js index 50d4e18db..7b5db23b1 100644 --- a/src/frontend/components/modal/modals/curval/lib/component.js +++ b/src/frontend/components/modal/modals/curval/lib/component.js @@ -5,6 +5,7 @@ import { guid as Guid } from "guid" import { initializeRegisteredComponents } from 'component' import { validateRadioGroup, validateCheckboxGroup } from 'validation' import gadsStorage from 'util/gadsStorage' +import { fromJson } from 'util/common' class CurvalModalComponent extends ModalComponent { @@ -216,7 +217,8 @@ class CurvalModalComponent extends ModalComponent { const id = location.pathname.split("/").pop() const record_id = isNaN(parseInt(id)) ? 0 : parseInt(id) const parent_key = `linkspace-column-${col_id}-${$('body').data('layout-identifier')}-${record_id}`; - let existing = await gadsStorage.getItem(parent_key) ? (JSON.parse(await gadsStorage.getItem(parent_key))) : [] + let existing = fromJson(await gadsStorage.getItem(parent_key) ?? "[]") + console.log('existing', existing, typeof existing); const identifier = current_id || guid // "existing" is the existing values for this curval // Pull out the current record if it exists @@ -333,6 +335,9 @@ class CurvalModalComponent extends ModalComponent { let url = current_id ? `/record/${current_id}` : `/${instance_name}/record/` + + if(url.endsWith('undefined')) + url=`/${instance_name}/record/` url = `${url}?include_draft&modal=${layout_id}` if (form_data) url = url + `&${form_data}` From e1641d9ab4446f0aaf7ad87a6e14a8051856e2c2 Mon Sep 17 00:00:00 2001 From: Dave Roberts <145559566+droberts-ctrlo@users.noreply.github.com> Date: Thu, 23 Jan 2025 17:03:03 +0000 Subject: [PATCH 4/8] Removed console.log --- src/frontend/components/form-group/autosave/lib/modal.js | 1 - 1 file changed, 1 deletion(-) diff --git a/src/frontend/components/form-group/autosave/lib/modal.js b/src/frontend/components/form-group/autosave/lib/modal.js index 7beb79813..dd0a05cda 100644 --- a/src/frontend/components/form-group/autosave/lib/modal.js +++ b/src/frontend/components/form-group/autosave/lib/modal.js @@ -11,7 +11,6 @@ class AutosaveModal extends AutosaveBase { */ async initAutosave() { const $modal = $(this.element); - console.log("modal", this.element); const $form = $('.form-edit'); $modal.find('.btn-js-restore-values').on('click', async (e) => { From c78b8d4f2f6957ba6524d500fbfd3806cea2eaa8 Mon Sep 17 00:00:00 2001 From: Dave Roberts <145559566+droberts-ctrlo@users.noreply.github.com> Date: Thu, 23 Jan 2025 17:03:49 +0000 Subject: [PATCH 5/8] Removed console statement --- src/frontend/components/modal/modals/curval/lib/component.js | 1 - 1 file changed, 1 deletion(-) diff --git a/src/frontend/components/modal/modals/curval/lib/component.js b/src/frontend/components/modal/modals/curval/lib/component.js index 7b5db23b1..cc379da72 100644 --- a/src/frontend/components/modal/modals/curval/lib/component.js +++ b/src/frontend/components/modal/modals/curval/lib/component.js @@ -218,7 +218,6 @@ class CurvalModalComponent extends ModalComponent { const record_id = isNaN(parseInt(id)) ? 0 : parseInt(id) const parent_key = `linkspace-column-${col_id}-${$('body').data('layout-identifier')}-${record_id}`; let existing = fromJson(await gadsStorage.getItem(parent_key) ?? "[]") - console.log('existing', existing, typeof existing); const identifier = current_id || guid // "existing" is the existing values for this curval // Pull out the current record if it exists From 66381886e1ed009249cda96dfa1cef7477eda083 Mon Sep 17 00:00:00 2001 From: Dave Roberts <145559566+droberts-ctrlo@users.noreply.github.com> Date: Thu, 23 Jan 2025 17:04:54 +0000 Subject: [PATCH 6/8] Removed other console statements --- src/frontend/components/form-group/autosave/lib/modal.js | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/frontend/components/form-group/autosave/lib/modal.js b/src/frontend/components/form-group/autosave/lib/modal.js index dd0a05cda..790ff40ea 100644 --- a/src/frontend/components/form-group/autosave/lib/modal.js +++ b/src/frontend/components/form-group/autosave/lib/modal.js @@ -70,7 +70,6 @@ class AutosaveModal extends AutosaveBase { $field.on("validationPassed", () => { // Decrement the curval count curvalCount--; - console.log("curvalCount", curvalCount); const $li = $(`
    • Restored ${name}
      • `); $list.append($li); // If we've done all fields, turn off the recovery flag @@ -104,7 +103,6 @@ class AutosaveModal extends AutosaveBase { $body.append(`

      Critical error restoring values

      ${e}

      `); }).finally(() => { // Only allow to close once recovery is finished - console.log("curvalCount", curvalCount); if(!curvalCount || errored) { // Show the close button $modal.find(".modal-footer").find(".btn-cancel").text("Close").show(); From a5c1a5b97a71e419203743d24c1adbd6e3246dde Mon Sep 17 00:00:00 2001 From: Dave Roberts Date: Fri, 24 Jan 2025 09:44:41 +0000 Subject: [PATCH 7/8] Added and updated comments as appropriate --- src/frontend/components/form-group/autosave/lib/modal.js | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/frontend/components/form-group/autosave/lib/modal.js b/src/frontend/components/form-group/autosave/lib/modal.js index 790ff40ea..95638700c 100644 --- a/src/frontend/components/form-group/autosave/lib/modal.js +++ b/src/frontend/components/form-group/autosave/lib/modal.js @@ -17,18 +17,18 @@ class AutosaveModal extends AutosaveBase { e.preventDefault(); e.stopPropagation(); + // Hide all the buttons (we don't want any interaction to close the modal or the restore process fails) $modal.find(".modal-footer").find("button").hide(); // This need awaiting or it returns before the value is fully set meaning if the recovery is "fast" it will not clear await this.storage.setItem('recovering', true); // Count the curvals so we don't return too early let curvalCount = 0; - const counter = Promise.all($form.find('.linkspace-field[data-column-type="curval"]').map(async (_, field) => { + // Only count changed curvals - as each in the array has it's own event, we count the number of changes, not the number of fields + await Promise.all($form.find('.linkspace-field[data-column-type="curval"]').map(async (_, field) => { await this.storage.getItem(this.columnKey($(field))) && (curvalCount += fromJson(await this.storage.getItem(this.columnKey($(field)))).length); })); - await counter; - let errored = false; let $list = $("
        "); @@ -119,7 +119,6 @@ class AutosaveModal extends AutosaveBase { $modal.find('.btn-js-delete-values').attr('disabled', 'disabled').hide(); } } - } export default AutosaveModal; From 2ba65c7e8f50913ae551ebeeda9d8998206ba9ff Mon Sep 17 00:00:00 2001 From: Dave Roberts Date: Fri, 24 Jan 2025 13:07:11 +0000 Subject: [PATCH 8/8] Removed unusual undef code --- src/frontend/components/modal/modals/curval/lib/component.js | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/frontend/components/modal/modals/curval/lib/component.js b/src/frontend/components/modal/modals/curval/lib/component.js index cc379da72..3e29fc7b3 100644 --- a/src/frontend/components/modal/modals/curval/lib/component.js +++ b/src/frontend/components/modal/modals/curval/lib/component.js @@ -110,7 +110,7 @@ class CurvalModalComponent extends ModalComponent { const editButton = $( ` `, @@ -334,9 +334,6 @@ class CurvalModalComponent extends ModalComponent { let url = current_id ? `/record/${current_id}` : `/${instance_name}/record/` - - if(url.endsWith('undefined')) - url=`/${instance_name}/record/` url = `${url}?include_draft&modal=${layout_id}` if (form_data) url = url + `&${form_data}`