[PP-3820] Add patron blocking rules editor to the Library Settings UI… by dbernstein · Pull Request #201 · ThePalaceProject/circulation-admin

dbernstein · 2026-02-27T21:30:32Z

… for SIP2 patron auth providers

Description

Implements an editable list of "Patron Blocking Rules" in the Patron Auth Services configuration, scoped per-library. Rules (name, rule, message) are added/removed in the UI and sent as part of the existing libraries payload on save. The feature is gated to SIP2 in v1; the architecture is designed so that additional provider types (SirsiDynix, Millennium, etc.) can be enabled later with no structural changes.

Changes

New files

src/utils/patronBlockingRules.ts — supportsPatronBlockingRules(protocolName) capability gate; SIP2_PROTOCOL constant
src/components/PatronBlockingRulesEditor.tsx — Protocol-agnostic, reusable rule list editor with getValue() ref method, *Add Rule / Delete controls, client-side required-field validation, and error/disabled prop support
src/components/PatronAuthServiceEditForm.tsx — Extends ServiceEditForm; overrides editLibrary, addLibrary, and three hook methods to inject the editor for SIP2
tests/jest/utils/patronBlockingRulesCapability.test.ts
tests/jest/components/PatronBlockingRulesEditor.test.tsx
tests/jest/components/PatronAuthServiceEditForm.test.tsx

Modified files

src/interfaces.ts — New PatronBlockingRule interface; LibraryWithSettingsData widened to allow PatronBlockingRule[] values
src/components/ServiceEditForm.tsx — Three protected hook methods added (protocolHasLibrarySettings, renderExtraExpandedLibrarySettings, renderExtraNewLibrarySettings); no behaviour change to existing callers
src/components/PatronAuthServices.tsx — EditForm swapped from ServiceEditForm to PatronAuthServiceEditForm

@tdilauro : In the last commit I asked Claude to examine the updates I made and to the extent that it made sense, use the most modern React programming patterns that exist within this code base. I'm especially curious to know what you think of it's approach.

Motivation and Context

https://ebce-lyrasis.atlassian.net/browse/PP-3820

How Has This Been Tested?

New unit test coverage added.
This PR was tested extensively on my local instance against it's circulation repository counterpart: ThePalaceProject/circulation#3090

Checklist:

I have updated the documentation accordingly.
All new and existing tests passed.

tdilauro

I've put a first round of comments on this one. This React and Typescript stuff can be a bit tricky. Let me know if you get stuck on any of the feedback.

Also, a lot of the changes are missing test coverage (e.g., there are no test changes for src/components/ServiceEditForm.tsx), so I have not yet gone through the tests carefully.

src/components/PatronAuthServiceEditForm.tsx

tdilauro · 2026-03-04T23:24:31Z

src/components/ServiceEditForm.tsx


+  /** Hook for subclasses to inject extra fields into the expanded per-library settings panel.
+   *  Rendered after protocol library_settings fields and before the Save button. */
+  renderExtraExpandedLibrarySettings(


Suggestion - Rename this function to renderExtraAssociatedLibrarySettings or renderExtraActiveLibrarySettings to parallel with the "New" in renderExtraNewLibrarySettings below. The current name was confusing me when I looked at other parts of this PR.

The problem here may just be me misunderstanding the intent here. I don't think of these as expanded libraries, but rather as associated (with the integration) libraries. It's true that they are expanded in the interface when we are editing them, but that is more a property of the UI design than something intrinsic to the libraries themselves.

tdilauro · 2026-03-04T23:41:31Z

src/components/PatronBlockingRulesEditor.tsx

+        <Button
+          type="button"
+          className="add-patron-blocking-rule"
+          disabled={disabled}


Suggestion - This button should also be disabled whenever there is an incomplete rule (missing any required field) in the form. Otherwise, a user can just create a bunch of incomplete rules.

Currently, this is disabled with just the prop that is passed when the this component is mounted.

tdilauro · 2026-03-04T23:46:34Z

src/components/PatronBlockingRulesEditor.tsx

+          return (
+            <li key={index} className="patron-blocking-rule-row">
+              <WithRemoveButton
+                disabled={disabled}
+                onRemove={() => removeRule(index)}
+              >
+                <div className="patron-blocking-rule-fields">
+                  {nameClientError && (
+                    <p className="patron-blocking-rule-field-error text-danger">
+                      Rule Name is required.
+                    </p>
+                  )}
+                  <EditableInput
+                    elementType="input"
+                    type="text"
+                    label="Rule Name"
+                    name={`patron_blocking_rule_name_${index}`}
+                    value={rule.name}
+                    required={true}
+                    disabled={disabled}
+                    readOnly={disabled}
+                    optionalText={false}
+                    clientError={rowErrors.name}
+                    error={error}
+                    onChange={(value) => updateRule(index, "name", value)}
+                  />
+                  {ruleClientError && (
+                    <p className="patron-blocking-rule-field-error text-danger">
+                      Rule Expression is required.
+                    </p>
+                  )}
+                  <EditableInput
+                    elementType="textarea"
+                    label="Rule Expression"
+                    name={`patron_blocking_rule_rule_${index}`}
+                    value={rule.rule}
+                    required={true}
+                    disabled={disabled}
+                    readOnly={disabled}
+                    optionalText={false}
+                    clientError={rowErrors.rule}
+                    error={error}
+                    onChange={(value) => updateRule(index, "rule", value)}
+                  />
+                  <EditableInput
+                    elementType="textarea"
+                    label="Message (optional)"
+                    name={`patron_blocking_rule_message_${index}`}
+                    value={rule.message || ""}
+                    disabled={disabled}
+                    readOnly={disabled}
+                    optionalText={false}
+                    onChange={(value) => updateRule(index, "message", value)}
+                  />
+                </div>
+              </WithRemoveButton>
+            </li>


Suggestion - Factor this out into its own little RuleFormListItem (or similarly named) component. This is really a lot to pack into the middle of some JSX and makes it difficult to reason over the function of this code.

tdilauro · 2026-03-05T00:23:36Z

src/interfaces.ts

  short_name: string;
-  [key: string]: string;
+  patron_blocking_rules?: PatronBlockingRule[];
+  [key: string]: string | PatronBlockingRule[] | undefined;


Do we actually need to change this line?

Can keys other than patron_blocking_rules, which is already listed as optional above, contain a list of PatronBlockingRule?

Are we planning to return undefined settings?

I'm not sure about this: But here is what my robot overlord said about it:

"The widening of [key: string]: string | PatronBlockingRule[] | undefined is required by TypeScript. The rule is: every named property in an interface must be assignable to the index signature type. Since patron_blocking_rules?: PatronBlockingRule[] has type PatronBlockingRule[] | undefined, the index signature must include both. There's no way to keep the original [key: string]: string while also having a typed patron_blocking_rules property — TypeScript will reject it."

I tried to have it remove undefined, but Claude said typescript would not accept it. Does this make sense to you?

Ah, that makes sense. Learn something new everyday.

tdilauro · 2026-03-05T00:36:01Z

src/utils/patronBlockingRules.ts

@@ -0,0 +1,5 @@
+export const SIP2_PROTOCOL = "api.sip";
+
+export function supportsPatronBlockingRules(protocolName: string): boolean {


We should change the type for protocolName here to 'string | null | undefined'.

In PatronAuthServiceEditForm it is called with protocol && protocol.name, which can evaluate to null, undefined, or string. The tests call it with null and undefined, as well.

tdilauro · 2026-03-05T00:44:36Z

src/components/PatronBlockingRulesEditor.tsx

+          const ruleClientError = !!rowErrors.rule;
+
+          return (
+            <li key={index} className="patron-blocking-rule-row">


Using index for the key here is potentially problematic because it is not unique for a given rule. So, it is possible that the wrong data could be display because the DOM is confused. For example, if we have more than one rule and we delete the first one, then the second one will end up with the index that used to belong to the first one. When that happens, the DOM may not understand that it needs to update that <li>.

tdilauro · 2026-03-05T00:47:38Z

src/components/PatronBlockingRulesEditor.tsx

+      {serverErrorMessage && rules.length > 0 && (
+        <p className="patron-blocking-rule-field-error text-danger">
+          {serverErrorMessage}
+        </p>
+      )}


Do we want to suppress a server error message just because there are no rules? Would that message be displayed somewhere else or just hidden entirely?

tdilauro · 2026-03-05T01:00:15Z

src/components/ServiceEditForm.tsx

+  /** Returns true when this protocol has any editable per-library settings,
+   *  either from the protocol definition or injected by a subclass.
+   *  Subclasses should override this when they add extra library-level fields. */
+  protocolHasLibrarySettings(protocol: ProtocolData): boolean {
+    return this.protocolLibrarySettings(protocol).length > 0;
+  }
+


Minor (style) - We might want to put this up with the other subclass hooks.

… for SIP2 patron auth providers

…o change everything at once, the style of the new components to the newer React development style. Here are the changes: PatronBlockingRulesEditor.tsx — full function component conversion * React.forwardRef + useImperativeHandle: replaces the class instance methods (getValue, validateAndGetValue) with an explicit typed PatronBlockingRulesEditorHandle interface. Callers now get a clean contract rather than reaching into a class instance. useState for rules and clientErrors: replaces this.state and this.setState. * Functional updater pattern (e.g. setRules(prev => ...)) in removeRule and updateRule to avoid stale closure issues. * useImperativeHandle deps array [rules]: ensures the exposed getValue/validateAndGetValue always close over the latest rules without needing a separate ref. * displayName: set explicitly since forwardRef components lose the inferred name in React DevTools. PatronAuthServiceEditForm.tsx — modernized refs, kept as class * Cannot be a function component — it extends ServiceEditForm, a class. Function components can't extend anything; that constraint belongs to the inheritance structure, which a full refactor of ServiceEditForm would be needed to change. * React.createRef<PatronBlockingRulesEditorHandle>() for the new-library editor, replacing the NEW_LIBRARY_RULES_REF string constant. * Map<string, React.RefObject<PatronBlockingRulesEditorHandle>> (libraryRulesRefs) for per-library editors, with a getOrCreateLibraryRef() helper — replaces the deprecated string refs (this.refs["library_patron_blocking_rules"]). * Optional chaining (editorRef?.current) throughout for cleaner null safety. * PatronBlockingRulesEditor.test.tsx — type fix only * Updated two React.createRef<PatronBlockingRulesEditor>() calls to React.createRef<PatronBlockingRulesEditorHandle>(), since the component is no longer a class whose instance type can be used as a ref type.

Co-authored-by: Tim DiLauro <tdilauro@users.noreply.github.com>

… from string to string | null | undefined so it safely handles all values callers may pass.

…rarySettings in ServiceEditForm and PatronAuthServiceEditForm

…t, renderExtraAssociatedLibrarySettings, and renderExtraNewLibrarySettings

The fix requires giving each rule a stable internal ID that doesn't change when other rules are deleted. Introduce a local RuleEntry type (internal to the component only) that augments PatronBlockingRule with a _id: number field Use a useRef counter to generate a monotonically increasing ID each time a rule is added (or on initial load) Use entry._id as the key prop instead of index Strip the _id when returning values from getValue() / validateAndGetValue() The PatronBlockingRule interface in interfaces.ts stays untouched — the ID is purely a UI concern internal to the component. Add a test to improve coverage of this functionalily.

…ield. Also adds tests to cover change: The first test checks the button is disabled immediately after adding an empty rule, and the second verifies it re-enables once both required fields (name and rule) are filled.

…ge check alone is sufficient — it is only non-null when there is an actual error to display. Also add test coverage.

Co-authored-by: Tim DiLauro <tdilauro@users.noreply.github.com>

TypeScript required widening the index signature to string | PatronBlockingRule[] | undefined when patron_blocking_rules was added as a named property. Instead, remove the index signature entirely and use localized unknown-casts at the three call sites that do dynamic string-key access, keeping the interface semantically precise.

editLibrary — the closing } for the if (supportsPatronBlockingRules(...)) block was missing, which caused a duplicate editorRef declaration and pulled libraries.push / setState inside the malformed block. addLibrary — the supportsPatronBlockingRules protocol guard was dropped, meaning patron blocking rules would have been collected regardless of the protocol type.

src/api/patronBlockingRules.ts (new) — validatePatronBlockingRuleExpression(serviceId, rule, csrfToken) — sends FormData POST, returns null on 200 or the error detail string on failure. src/components/PatronAuthServices.tsx — Added additionalData: { csrfToken: ownProps.csrfToken } to mapStateToProps to thread the CSRF token through to the edit form. src/components/PatronAuthServiceEditForm.tsx — Passes csrfToken and serviceId (this.props.item?.id) to both PatronBlockingRulesEditor instances. src/components/PatronBlockingRulesEditor.tsx — Added csrfToken/serviceId props, serverErrors state, handleRuleBlur async handler, clear-on-edit logic in updateRule, and <div onBlur> wrapper + serverRuleError display in RuleFormListItem. tests/jest/api/patronBlockingRules.test.ts (new) — 5 tests covering the API function (null on 200, detail string on 400, fallback string, correct headers, undefined serviceId). tests/jest/components/PatronBlockingRulesEditor.test.tsx — Added 6 blur/server-error tests and added beforeEach/afterEach fetch mock setup to both describe blocks to prevent incidental blur events from failing unrelated tests. Other changes: .eslintrc — Added a @typescript-eslint/no-unused-vars rule override with three options: argsIgnorePattern: "^_" — suppresses the _library, _protocol, _disabled parameter warnings in ServiceEditForm.tsx (and any similar _-prefixed intentionally-unused parameters elsewhere) ignoreRestSiblings: true — suppresses the _id warnings in PatronBlockingRulesEditor.tsx, which is the standard ESLint way to allow the { _id, ...rest } "strip a key" pattern varsIgnorePattern: "^_" — consistent _-prefix convention for variables too ServiceEditForm.tsx — Renamed library → _library in isLibraryRemovalPermitted (the only parameter that didn't already have the _ prefix the others used) PatronAuthServiceEditForm.tsx — Removed the unused PatronBlockingRule import PatronBlockingRulesEditor.tsx — Added // eslint-disable-next-line jsx-a11y/no-static-element-interactions with an explanatory comment. The wrapper <div> is a focus-event delegation container (React's synthetic blur bubbles from the textarea), not an interactive element — an eslint-disable with a clear rationale is the honest and correct fix here rather than misusing a role to silence the rule.

… rules validation errors.

…ot yet saved, the "Add Library" button is not active if there is an invalid patron block rule. Also, inline errors for duplicate names across patron block rules within a library.

dbernstein · 2026-03-08T23:57:12Z

@tdilauro : I think this PR should be good now. In addition to addressing the issues you raised, I made some other changes to ensure that the user can't "Add Library" or click "Save" if there are outstanding validation issues. I also added support for inline validation checks (both changed here and new functionality added on the backend) to ensure that the user gets instant feedback about the validity of a rule (uniqueness of name as well as proper syntax and templated values based on live feedback from the SIP2 api).

I tested it extensively locally before committing the changes.

tdilauro

I like the async validation you've added here. There's still an important issue to resolve with removeRule. Additionally, there are a bunch of mostly "non happy path" cases that are not covered by tests, mostly in src/components/PatronBlockingRulesEditor.tsx and src/components/PatronAuthServiceEditForm.tsx.

tdilauro · 2026-03-10T15:18:23Z

src/components/PatronBlockingRulesEditor.tsx

+
+    const removeRule = (index: number) => {
+      const removedId = rules[index]._id;
+      setRules((prev) => prev.filter((_, i) => i !== index));


Change requested - It looks like there is still the same problem with removeRule. I think it would be helpful to write a test for this case where you start with two rules, the first of which validates, the second of which does not. Then delete the first rule. Verify that what was the second rule is now the first (and only) rule and that the error message is associated with that rule is still at the right place on the page.

I manually ran this locally just to verify that it is behaving as I described above.

src/components/PatronBlockingRulesEditor.tsx

tdilauro · 2026-03-10T15:56:39Z

src/components/PatronBlockingRulesEditor.tsx

+      const errorMessage = await validatePatronBlockingRuleExpression(
+        serviceId,
+        rule,
+        csrfToken
+      );
+      setServerErrors((prev) => ({ ...prev, [index]: errorMessage }));


There's a possible race condition here because we're rendering multiple rules down below, each with their own onRuleBlur. That means that there might be multiple validatePatronBlockingRuleExpression fetches in flight at the same time and the last one to resolve will win, which might not end up being what you want.

It would be more likely to happen with slower services and an ILS that has a lot of variability. I don't think this will be a big problem, but since it is a possibility, it's probably worth adding a comment about it, in case we actually run into it.

tdilauro · 2026-03-10T16:10:11Z

src/components/ServiceEditForm.tsx

 export interface ServiceEditFormProps<T> {
  data: T;
  item?: ServiceData;
+  additionalData?: any;


Minor - It would be good to narrow this from any to per-service effective types. We'd need to use generics for that.

tdilauro · 2026-03-10T16:23:42Z

src/components/PatronBlockingRulesEditor.tsx

+          <Button
+            type="button"
+            className="add-patron-blocking-rule"
+            disabled={disabled || hasIncompleteRule}
+            callback={addRule}
+            content="Add Rule"
+          />


Minor, especially since this is MVP, but might need future design - This button is a bit confusing, possibly due to the proximity of the library's Save button.

Yes - I agree. I'll make a note for future review.

tdilauro · 2026-03-10T16:26:28Z

src/components/PatronBlockingRulesEditor.tsx

+            />
+          ))}
+        </ul>
+      </div>


Minor - It would be useful to fetch the supported ILS attributes /functions to display at the bottom of the list of rules to provide some initial guidance to the user.

I agree - I'll create a new ticket for this.

Co-authored-by: Tim DiLauro <tdilauro@users.noreply.github.com>

…ditor, adding coverage for expected error conditions as well as testing for expected button enabled/disabled states.

… was deleted and add a test to confirm the fix.

dbernstein · 2026-03-10T18:01:40Z

@tdilauro : thanks for the second round of comments. I added a bunch of new tests, fixed the remove bug (and added the test to prove it as well as manually confirmed it is fixed), and added a note regarding the potential race condition. I have added a new ticket for providing guidance. https://ebce-lyrasis.atlassian.net/browse/PP-3866

tdilauro

🎉

dbernstein force-pushed the feature/PP-3820-add-patron-blocks branch from 4065f00 to d7fca0d Compare March 3, 2026 20:24

dbernstein marked this pull request as ready for review March 3, 2026 21:16

dbernstein requested a review from a team March 3, 2026 21:16

dbernstein mentioned this pull request Mar 4, 2026

[PP-3772] Add patron blocking rules per library ThePalaceProject/circulation#3090

Open

2 tasks

tdilauro requested changes Mar 5, 2026

View reviewed changes

dbernstein and others added 13 commits March 6, 2026 10:21

[PP-3820] Add patron blocking rules editor to the Library Settings UI…

279629a

… for SIP2 patron auth providers

Cosmetic improvements.

5548ff4

Ensure that the error message appears next to the rule expression field.

5fa5431

Update src/components/PatronAuthServiceEditForm.tsx

dbf00db

Co-authored-by: Tim DiLauro <tdilauro@users.noreply.github.com>

Update src/components/PatronAuthServiceEditForm.tsx

c89add6

Co-authored-by: Tim DiLauro <tdilauro@users.noreply.github.com>

Change the protocolName parameter type in supportsPatronBlockingRules…

b166d77

… from string to string | null | undefined so it safely handles all values callers may pass.

Rename renderExtraExpandedLibrarySettings to renderExtraAssociatedLib…

a44924b

…rarySettings in ServiceEditForm and PatronAuthServiceEditForm

protocolHasLibrarySettings is now grouped with renderAdditionalConten…

9d399e7

…t, renderExtraAssociatedLibrarySettings, and renderExtraNewLibrarySettings

Remove the rules.length > 0 condition entirely. The serverErrorMessa…

cf8f1e8

…ge check alone is sufficient — it is only non-null when there is an actual error to display. Also add test coverage.

Extract a RuleFormListItem function component

b727a1a

dbernstein force-pushed the feature/PP-3820-add-patron-blocks branch from 4229b46 to b727a1a Compare March 6, 2026 21:04

dbernstein and others added 3 commits March 6, 2026 13:05

Update src/components/PatronAuthServiceEditForm.tsx

a20be10

Co-authored-by: Tim DiLauro <tdilauro@users.noreply.github.com>

Update src/components/PatronAuthServiceEditForm.tsx

40c7d4f

Co-authored-by: Tim DiLauro <tdilauro@users.noreply.github.com>

dbernstein force-pushed the feature/PP-3820-add-patron-blocks branch from e118dc0 to b7963a1 Compare March 6, 2026 21:30

dbernstein added 5 commits March 6, 2026 13:37

Ensure that save is disabled while there are client side patron block…

adca26c

… rules validation errors.

This update ensures that when a new library is newly configured but n…

2052416

…ot yet saved, the "Add Library" button is not active if there is an invalid patron block rule. Also, inline errors for duplicate names across patron block rules within a library.

Fix linting issue.

bc8aa7d

dbernstein requested a review from tdilauro March 8, 2026 23:57

tdilauro requested changes Mar 10, 2026

View reviewed changes

dbernstein and others added 4 commits March 10, 2026 09:46

Update src/components/PatronBlockingRulesEditor.tsx

055e842

Co-authored-by: Tim DiLauro <tdilauro@users.noreply.github.com>

Beefs up tests for PatronAuthServiceEditForm and PatronBlockingRulesE…

c07d2e7

…ditor, adding coverage for expected error conditions as well as testing for expected button enabled/disabled states.

Fix bug where rule error messages were disappearing when a valid rule…

efb9ef0

… was deleted and add a test to confirm the fix.

Added note regarding possible race condition.

d7101d3

tdilauro approved these changes Mar 10, 2026

View reviewed changes

		@@ -0,0 +1,5 @@
		export const SIP2_PROTOCOL = "api.sip";

		export function supportsPatronBlockingRules(protocolName: string): boolean {

Conversation

dbernstein commented Feb 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Changes

New files

Modified files

Motivation and Context

How Has This Been Tested?

Checklist:

Uh oh!

tdilauro left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbernstein Mar 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbernstein commented Mar 8, 2026

Uh oh!

tdilauro left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbernstein commented Mar 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tdilauro left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

dbernstein commented Feb 27, 2026 •

edited

Loading

dbernstein Mar 6, 2026 •

edited

Loading

dbernstein commented Mar 10, 2026 •

edited

Loading