From 8b666b50b82a885c37bf88ef33ff64c74c4a0649 Mon Sep 17 00:00:00 2001 From: Sharad S Date: Wed, 24 Jul 2024 14:06:48 -0400 Subject: [PATCH 1/7] Add strict mode to rgetPromise and resource fetch --- .../lib/components/DataModel/legacyTypes.ts | 4 ++- .../lib/components/DataModel/resourceApi.ts | 25 +++++++++++-------- 2 files changed, 17 insertions(+), 12 deletions(-) diff --git a/specifyweb/frontend/js_src/lib/components/DataModel/legacyTypes.ts b/specifyweb/frontend/js_src/lib/components/DataModel/legacyTypes.ts index bf0d9ee2289..574a747f5a1 100644 --- a/specifyweb/frontend/js_src/lib/components/DataModel/legacyTypes.ts +++ b/specifyweb/frontend/js_src/lib/components/DataModel/legacyTypes.ts @@ -78,7 +78,8 @@ export type SpecifyResource = { SCHEMA['toOneIndependent'])[FIELD_NAME] >( fieldName: FIELD_NAME, - prePopulate?: boolean + prePopulate?: boolean, + strict?: boolean ): readonly [VALUE] extends readonly [never] ? never : Promise< @@ -98,6 +99,7 @@ export type SpecifyResource = { options?: { readonly prePop?: boolean; readonly noBusinessRules?: boolean; + readonly strict?: boolean; } ): readonly [VALUE] extends readonly [never] ? never diff --git a/specifyweb/frontend/js_src/lib/components/DataModel/resourceApi.ts b/specifyweb/frontend/js_src/lib/components/DataModel/resourceApi.ts index 5fb629d22ed..a2f34875df9 100644 --- a/specifyweb/frontend/js_src/lib/components/DataModel/resourceApi.ts +++ b/specifyweb/frontend/js_src/lib/components/DataModel/resourceApi.ts @@ -497,9 +497,9 @@ export const ResourceBase = Backbone.Model.extend({ * REFACTOR: remove the need for this * Like "rget", but returns native promise */ - async rgetPromise(fieldName, prePop = true) { + async rgetPromise(fieldName, prePop = true, strict = true) { return ( - this.getRelated(fieldName, { prePop }) + this.getRelated(fieldName, { prePop, strict }) // GetRelated may return either undefined or null (yuk) .then((data) => (data === undefined ? null : data)) ); @@ -518,7 +518,7 @@ export const ResourceBase = Backbone.Model.extend({ : fieldName.split(backboneFieldSeparator); // First make sure we actually have this object. - return this.fetch() + return this.fetch(options) .then((_this) => _this._rget(path, options)) .then((value) => { /* @@ -530,7 +530,8 @@ export const ResourceBase = Backbone.Model.extend({ if (!value) return value; // Ok if the related resource doesn't exist else if (typeof value.fetchIfNotPopulated === 'function') return value.fetchIfNotPopulated(); - else if (typeof value.fetch === 'function') return value.fetch(); + else if (typeof value.fetch === 'function') + return value.fetch(options); } return value; }); @@ -747,13 +748,15 @@ export const ResourceBase = Backbone.Model.extend({ return this; else if (this._fetch) return this._fetch; else - return (this._fetch = Backbone.Model.prototype.fetch - .call(this, options) - .then(() => { - this._fetch = null; - // BUG: consider doing this.needsSaved=false here - return this; - })); + return (this._fetch = hijackBackboneAjax( + options === undefined || options.strict ? undefined : [Http.NOT_FOUND], + () => + Backbone.Model.prototype.fetch.call(this, options).then(() => { + this._fetch = null; + // BUG: consider doing this.needsSaved=false here + return this; + }) + )); }, parse(_resp) { // Since we are putting in data, the resource in now populated From 69391723254d4ea71df4701a869c225dc00b80dd Mon Sep 17 00:00:00 2001 From: Sharad S Date: Wed, 24 Jul 2024 14:07:18 -0400 Subject: [PATCH 2/7] Catch errors in QCBX --- .../lib/components/QueryComboBox/index.tsx | 51 ++++++++++++------- 1 file changed, 32 insertions(+), 19 deletions(-) diff --git a/specifyweb/frontend/js_src/lib/components/QueryComboBox/index.tsx b/specifyweb/frontend/js_src/lib/components/QueryComboBox/index.tsx index 98605a30a2c..221eaca2c9c 100644 --- a/specifyweb/frontend/js_src/lib/components/QueryComboBox/index.tsx +++ b/specifyweb/frontend/js_src/lib/components/QueryComboBox/index.tsx @@ -5,6 +5,7 @@ import type { State } from 'typesafe-reducer'; import { useAsyncState } from '../../hooks/useAsyncState'; import { useResourceValue } from '../../hooks/useResourceValue'; import { commonText } from '../../localization/common'; +import { formsText } from '../../localization/forms'; import { userText } from '../../localization/user'; import { f } from '../../utils/functools'; import { getValidationAttributes } from '../../utils/parser/definitions'; @@ -119,7 +120,7 @@ export function QueryComboBox({ treeData !== undefined && collectionRelationships !== undefined && typeSearch !== undefined; - const { value, updateValue, validationRef, inputRef, parser } = + const { value, updateValue, validationRef, inputRef, parser, setValidation } = useResourceValue(resource, field, undefined); /** @@ -171,30 +172,42 @@ export function QueryComboBox({ */ field.isDependent()) ? resource - .rgetPromise(field.name) - .then(async (resource) => - resource === undefined || resource === null - ? { - label: localized(''), - resource: undefined, - } - : (value === formattedRef.current?.value && + .rgetPromise(field.name, true, false) + .then(async (resource) => { + setValidation([]); + if (resource === undefined || resource === null) { + return { + label: localized(''), + resource: undefined, + }; + } else { + const formatted = + value === formattedRef.current?.value && typeof formattedRef.current === 'object' - ? Promise.resolve(formattedRef.current.formatted) - : format( + ? await Promise.resolve(formattedRef.current.formatted) + : await format( resource, typeof typeSearch === 'object' ? typeSearch.formatter : undefined, true - ) - ).then((formatted) => ({ - label: - formatted ?? - naiveFormatter(field.relatedTable.label, resource.id), - resource, - })) - ) + ); + + return { + label: + formatted ?? + naiveFormatter(field.relatedTable.label, resource.id), + resource, + }; + } + }) + .catch((_) => { + setValidation([formsText.invalidValue()]); + return { + label: localized(''), + resource: undefined, + }; + }) : { label: userText.noPermission(), resource: undefined }, [version, value, resource, field, typeSearch] ), From 9faedc70ac8cb78dbd676a840770e8ebf1476500 Mon Sep 17 00:00:00 2001 From: Max Patiiuk Date: Wed, 24 Jul 2024 19:10:13 -0700 Subject: [PATCH 3/7] docs(ajax): improve comments documentation --- .../frontend/js_src/lib/utils/ajax/index.ts | 20 ++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/specifyweb/frontend/js_src/lib/utils/ajax/index.ts b/specifyweb/frontend/js_src/lib/utils/ajax/index.ts index 0f2963b21b6..abe6d7160db 100644 --- a/specifyweb/frontend/js_src/lib/utils/ajax/index.ts +++ b/specifyweb/frontend/js_src/lib/utils/ajax/index.ts @@ -40,6 +40,11 @@ export type AjaxMethod = | 'POST' | 'PUT'; +/** + * These methods usually don't modify data, so if they fail it is not a big + * deal. If on the other than POST, PUT, DELETE, or PATCH request fails, it + * may cause data loss or data corruption + */ const safeMethods: ReadonlySet = new Set([ 'OPTIONS', 'GET', @@ -75,16 +80,13 @@ export type AjaxProps = Omit & { /** * All front-end network requests should go through this utility. * - * Wraps native fetch in useful helpers - * It is intended as a replacement for jQuery's ajax - * - * @remarks - * Automatically adds CSRF token to non GET requests - * Casts response to correct typescript type - * Parsers JSON and XML responses - * Handlers errors (including permission errors) + * Wraps native fetch in useful helpers: + * - Automatically adds CSRF token to non GET requests + * - Casts response to correct typescript type + * - Parses JSON and XML responses + * - Handlers errors (including permission errors) + * - Helps with request mocking in tests */ -// "errorMode" is optional for "GET" requests export async function ajax( url: string, /** These options are passed directly to fetch() */ From 948c11832be1f1deec1ff39020e90f938b615520 Mon Sep 17 00:00:00 2001 From: Sharad S Date: Tue, 30 Jul 2024 14:41:49 -0500 Subject: [PATCH 4/7] conditionally call hijackBackbone --- .../lib/components/DataModel/resourceApi.ts | 25 +++++++++++-------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/specifyweb/frontend/js_src/lib/components/DataModel/resourceApi.ts b/specifyweb/frontend/js_src/lib/components/DataModel/resourceApi.ts index a2f34875df9..314e68e3f44 100644 --- a/specifyweb/frontend/js_src/lib/components/DataModel/resourceApi.ts +++ b/specifyweb/frontend/js_src/lib/components/DataModel/resourceApi.ts @@ -747,16 +747,21 @@ export const ResourceBase = Backbone.Model.extend({ ) return this; else if (this._fetch) return this._fetch; - else - return (this._fetch = hijackBackboneAjax( - options === undefined || options.strict ? undefined : [Http.NOT_FOUND], - () => - Backbone.Model.prototype.fetch.call(this, options).then(() => { - this._fetch = null; - // BUG: consider doing this.needsSaved=false here - return this; - }) - )); + else { + const fetchCallback = () => + Backbone.Model.prototype.fetch.call(this, options).then(() => { + this._fetch = null; + // BUG: consider doing this.needsSaved=false here + return this; + }); + if (options === undefined || options.strict) + return (this._fetch = fetchCallback); + else + return (this._fetch = hijackBackboneAjax( + [Http.NOT_FOUND], + fetchCallback + )); + } }, parse(_resp) { // Since we are putting in data, the resource in now populated From d53d25f2403d34fd4d4aa627c1fab999d86b4b3a Mon Sep 17 00:00:00 2001 From: Sharad S Date: Tue, 30 Jul 2024 14:42:12 -0500 Subject: [PATCH 5/7] throw error on an expected 404 error --- specifyweb/frontend/js_src/lib/utils/ajax/backboneAjax.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/specifyweb/frontend/js_src/lib/utils/ajax/backboneAjax.ts b/specifyweb/frontend/js_src/lib/utils/ajax/backboneAjax.ts index 421bb59180d..acf29a15f54 100644 --- a/specifyweb/frontend/js_src/lib/utils/ajax/backboneAjax.ts +++ b/specifyweb/frontend/js_src/lib/utils/ajax/backboneAjax.ts @@ -1,6 +1,7 @@ import { Backbone } from '../../components/DataModel/backbone'; import { promiseToXhr } from '../../components/DataModel/resourceApi'; import { formatUrl } from '../../components/Router/queryString'; +import { f } from '../functools'; import type { RA, ValueOf } from '../types'; import { defined } from '../types'; import { Http } from './definitions'; @@ -63,7 +64,7 @@ Backbone.ajax = function (request): JQueryXHR { ) .then(({ data, status }) => { requestCallbackCopy?.(status); - if (status === Http.CONFLICT) throw new Error(data); + if (f.includes([Http.CONFLICT, Http.NOT_FOUND], status)) throw new Error(data); else if (typeof request.success === 'function') request.success(data, 'success', undefined as never); }) From b6ee081c2a4d0c64faf2b353bb0ec9dd7f1fabb8 Mon Sep 17 00:00:00 2001 From: Sharad S <16229739+sharadsw@users.noreply.github.com> Date: Wed, 31 Jul 2024 09:04:08 -0500 Subject: [PATCH 6/7] fix fetch call Co-authored-by: Max Patiiuk --- .../frontend/js_src/lib/components/DataModel/resourceApi.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/specifyweb/frontend/js_src/lib/components/DataModel/resourceApi.ts b/specifyweb/frontend/js_src/lib/components/DataModel/resourceApi.ts index 314e68e3f44..587091f3c86 100644 --- a/specifyweb/frontend/js_src/lib/components/DataModel/resourceApi.ts +++ b/specifyweb/frontend/js_src/lib/components/DataModel/resourceApi.ts @@ -755,7 +755,7 @@ export const ResourceBase = Backbone.Model.extend({ return this; }); if (options === undefined || options.strict) - return (this._fetch = fetchCallback); + return (this._fetch = fetchCallback()); else return (this._fetch = hijackBackboneAjax( [Http.NOT_FOUND], From 404e701682c10b0ca63d15b52abeda21fef89a15 Mon Sep 17 00:00:00 2001 From: Sharad S <16229739+sharadsw@users.noreply.github.com> Date: Wed, 31 Jul 2024 14:07:33 +0000 Subject: [PATCH 7/7] Lint code with ESLint and Prettier Triggered by b6ee081c2a4d0c64faf2b353bb0ec9dd7f1fabb8 on branch refs/heads/issue-5138 --- .../js_src/lib/components/DataModel/resourceApi.ts | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/specifyweb/frontend/js_src/lib/components/DataModel/resourceApi.ts b/specifyweb/frontend/js_src/lib/components/DataModel/resourceApi.ts index 587091f3c86..9a3158f4793 100644 --- a/specifyweb/frontend/js_src/lib/components/DataModel/resourceApi.ts +++ b/specifyweb/frontend/js_src/lib/components/DataModel/resourceApi.ts @@ -754,13 +754,10 @@ export const ResourceBase = Backbone.Model.extend({ // BUG: consider doing this.needsSaved=false here return this; }); - if (options === undefined || options.strict) - return (this._fetch = fetchCallback()); - else - return (this._fetch = hijackBackboneAjax( - [Http.NOT_FOUND], - fetchCallback - )); + return (this._fetch = + options === undefined || options.strict + ? fetchCallback() + : hijackBackboneAjax([Http.NOT_FOUND], fetchCallback)); } }, parse(_resp) {