Skip to content

Conversation

HenriRabalais
Copy link
Collaborator

@HenriRabalais HenriRabalais commented Sep 8, 2025

This PR refactors the acknowledgements page to use a new, more robust and reusable API client and a suite of custom error classes. This change modernizes our data fetching approach by moving away from raw fetch requests and adopting a structured, type-safe methodology.

Key Changes

Introduced a new Core Library: Added a new directory at jslib/core containing:

  • Client.ts: A generic, extensible class for handling API requests (GET, POST, PUT). It includes robust error handling for network issues, HTTP response statuses (e.g., 404), and JSON parsing errors.
  • Query.ts: A class for building type-safe URL query strings, supporting various operators and parameters.
  • Added Custom Error Classes: A comprehensive set of custom error classes are now available at jslib/core/errors to provide more context for debugging API failures. This includes ApiNetworkError, ApiResponseError, JsonParseError, and more.

Introduced a Entities Library: Added a new directory at jslib/entities containing:

  • Acknowledgment entity and affiliated type and client
  • This will eventually house all LORIS entities, and contexts they provide, specific errors, hooks, components, etc.

Updated Acknowledgements Module

  • The acknowledgementsIndex.js file has been refactored to use the new AcknowledgementClient instead of a raw fetch call. This significantly cleans up the component's logic and separates concerns.
  • The AcknowledgementClient is a specialized client that extends the new generic Client.ts, providing an interface for interacting with the acknowledgements API.

Misc

  • Module and Path Configuration: Updated package.json, tsconfig.json, and webpack.config.ts to support the new libraries, including:
  • New dependencies: stream-browserify, pbkdf2, and transliteration.

@github-actions github-actions bot added Language: PHP PR or issue that update PHP code Language: Javascript PR or issue that update Javascript code Module: acknowledgements PR or issue related to acknowledgements module labels Sep 8, 2025
$factory->config(),
[
__DIR__ . "/../project/",
__DIR__ . "/../project/modules",
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

needed to get things to load...

fallback: {
fs: false,
path: false,
stream: require.resolve("stream-browserify"),
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

npm compile was giving errors without this

"jstat": "^1.9.5",
"jszip": "^3.10.1",
"papaparse": "^5.3.0",
"pbkdf2": "^3.1.3",
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

npm compile was giving errors without these

@HenriRabalais HenriRabalais changed the title [jslib] Refactored Acknowledgements Client with Enhanced API Utilities [jslib] Refactored Acknowledgements with Enhanced HTTP Utilities Sep 9, 2025
export class Client<T> {
protected baseUrl: string;
protected subEndpoint?: string;
public getCustomMessage: (
Copy link
Collaborator Author

@HenriRabalais HenriRabalais Sep 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is to allow each Client concrete class to define it's own errors across all the error types, not sure if it's the best implementation, but it seems to work — maybe it just needs a better name. return type be something like Record<string, string>

@HenriRabalais
Copy link
Collaborator Author

@driusan @ridz1208 would love your thoughts on this! Hoping to extend this to more parts of loris after and add more entities. I'm also curious what you think of the directory structure I've started to build out, as it will likely get expanded. Should everything live in jslib? I'm thinking the top level components that currently live in jsx/ should also get moved into what ever the top level javascript directory is and live under a /components directory of sorts.

* Base class for all custom API-related errors.
*/
export class BaseError extends Error {
public name: string;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need to delete this since name is already a property of Error

@HenriRabalais
Copy link
Collaborator Author

@kongtiaowang could you help me with these errors?

 ✘ Filters
   │
   │ Facebook\WebDriver\Exception\NoSuchElementException: Unable to locate element: input[name="fullName"]
   │
   │ /app/vendor/php-webdriver/webdriver/lib/Exception/WebDriverException.php:120
   │ /app/vendor/php-webdriver/webdriver/lib/Remote/HttpCommandExecutor.php:359
   │ /app/vendor/php-webdriver/webdriver/lib/Remote/RemoteWebDriver.php:601
   │ /app/vendor/php-webdriver/webdriver/lib/Remote/RemoteWebDriver.php:217
   │ /app/vendor/php-webdriver/webdriver/lib/WebDriverExpectedCondition.php:177
   │ /app/vendor/php-webdriver/webdriver/lib/WebDriverWait.php:54
   │ /app/test/integrationtests/LorisIntegrationTest.class.inc:544
   │ /app/test/integrationtests/LorisIntegrationTest.class.inc:707
   │ /app/modules/acknowledgements/test/AcknowledgementsIntegrationTest.php:170
   │
 ✔ Cant add new record
 ✘ Config setting policy
   │
   │ Facebook\WebDriver\Exception\NoSuchElementException: Unable to locate element: #citationPolicy
   │
   │ /app/vendor/php-webdriver/webdriver/lib/Exception/WebDriverException.php:120
   │ /app/vendor/php-webdriver/webdriver/lib/Remote/HttpCommandExecutor.php:359
   │ /app/vendor/php-webdriver/webdriver/lib/Remote/RemoteWebDriver.php:601
   │ /app/vendor/php-webdriver/webdriver/lib/Remote/RemoteWebDriver.php:217
   │ /app/vendor/php-webdriver/webdriver/lib/WebDriverExpectedCondition.php:177
   │ /app/vendor/php-webdriver/webdriver/lib/WebDriverWait.php:54
   │ /app/test/integrationtests/LorisIntegrationTest.class.inc:544
   │ /app/modules/acknowledgements/test/AcknowledgementsIntegrationTest.php:234
   │

@kongtiaowang
Copy link
Contributor

@HenriRabalais I will handle it.

@kongtiaowang
Copy link
Contributor

I made some changes in 3b59957

@kongtiaowang kongtiaowang added the Passed manual tests PR has been successfully tested by at least one peer label Oct 18, 2025
@HenriRabalais
Copy link
Collaborator Author

Thanks @kongtiaowang !

@driusan do you have time to take a look at this? Otherwise, can I assign it to someone else?

@ridz1208
Copy link
Collaborator

@skarya22 @marandmart I assigned the both of you here because its a JSX heavy PR but also brongs some nice additions to what we already have. This is not exactly a critical priority but definitely a nice have so if you ca take a bit of time here and there to review it it would be perfect. We always talk baout improving our react side and this is doing just that.

@@ -0,0 +1,14 @@
import {Acknowledgement} from '../../';
import {Http} from 'jsx/../jslib/core';

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there isn't a specific reason for this import being done like this, change to /jslib/core


// 1. Handle HTTP status errors (e.g., 404, 500)
if (!response.ok) {
const message = this.getMessage(response.status, request, response);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't the first value being passed here be 'ApiResponseError' instead of response.status? The response object is already being passed as the third parameter, therefore it would be more useful to have the type of error like the examples bellow, I think

/**
* Function to retrieve a custom error message for a given error context.
*/
public getMessage: (

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this method handles error messages, I think its name should make that purpose more immediately clear

* @param baseUrl The base URL for the API requests.
*/
constructor(baseUrl: string) {
this.baseUrl = loris.BaseURL+'/'+baseUrl+'/';

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of concatenating strings to build the URL, I recommend using the URL object for cleaner and more reliable construction

case Operator.GreaterThan: return '>';
case Operator.LessThanOrEqual: return '<=';
case Operator.GreaterThanOrEqual: return '>=';
case Operator.Like: return '_like';

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Operator.Includes case is missing

// Handle responses with no content
const contentType = response.headers.get('content-type');
if (!contentType || !contentType.includes('application/json')) {
return null as U;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of typing null as U, return null directly and include it in the return type. This way, it is required to handle the null case instead of always expecting an object of type U

protected async fetchJSON<U>(url: string, options: RequestInit): Promise<U> {
const request = new Request(url, options);
try {
const response = await fetch(url, options);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't really change much, but it looks cleaner to use the request const from line 121 here instead of passing url and options again

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Language: Javascript PR or issue that update Javascript code Language: PHP PR or issue that update PHP code Module: acknowledgements PR or issue related to acknowledgements module Passed manual tests PR has been successfully tested by at least one peer

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants