Skip to content

Dc field replace - #368

Open
Jason-Benson wants to merge 9 commits into
FalveyLibraryTechnology:devfrom
Jason-Benson:dcFieldReplace
Open

Dc field replace#368
Jason-Benson wants to merge 9 commits into
FalveyLibraryTechnology:devfrom
Jason-Benson:dcFieldReplace

Conversation

@Jason-Benson

Copy link
Copy Markdown
Contributor

This is an update the the bulk editor that allows you to select any DC fields from the currently selected set of records and replace any given text within them.

@Jason-Benson

Copy link
Copy Markdown
Contributor Author

Looks like this broke some tests. I'll tackle those next.

@demiankatz demiankatz left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Excellent work, @Jason-Benson, this looks great. I have one small suggestion to streamline the code (see below), but other than that, it looks pretty much exactly the way I would have done it myself. :-)

I might find more things to suggest after I test it hands-on, but I'll let you fix the tests and reply to my feedback first so that I'm testing the finished product.

Also, one other minor point: I like to update the changelog in the PRs where I add new features. Might as well do that here. Since this is the first change since the last release, I would recommend just copying and pasting the whole 2.6.2 section, changing the heading to "2.7.0 - TBD" (since we'll want a minor version number due to the new feature, and the release date is not yet determined), reset all the subsections to "Nothing" except put "Added Dublin Core search-and-replace support to Bulk Editor." in the "Added" section.

Comment thread client/components/bulk/BulkEditor.tsx Outdated

@demiankatz demiankatz left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks, @Jason-Benson, this is continuing to shape up very nicely. See below for a few comments.

General thought: there's some added complexity here that doesn't have test coverage yet. My preference would be to decide on a minimum viable product, and either add test coverage or remove functionality until we meet the MVP and have 100% test coverage... then we can get this merged as a baseline, and add further features as separate PRs. I think that may make the review and development easier to manage. (Open to other approaches, though, if you don't think that's a good idea).

Comment thread client/components/bulk/BulkEditor.test.tsx Outdated
Comment thread client/components/bulk/BulkEditor.test.tsx Outdated
Comment thread CHANGELOG.md Outdated
{ "Content-Type": "application/json" },
);
const resultList = screen.getByTitle("Bulk Edit Results");
expect(resultList.innerHTML).toEqual("(1/1) foo: success\n");

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Maybe it's also worth doing a test case to cover the preview button. Or maybe we can just do both buttons in this single test (i.e. preview first, then apply).

<h1>Bulk Editor</h1>
<h2>Record Selector</h2>
<FormControl fullWidth>
<BlurSavingTextField

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

It might make sense to use a <PidPicker> control here to allow more robust PID selection -- though if we did that, we might also want to put it inside some kind of collapsible container, since it would otherwise use a lot of vertical real estate.

Maybe we need to make the label a little more clear, like "Restrict edits to this PID and its children."

Comment on lines +277 to +283
<FormControl fullWidth>
<BlurSavingTextField
value={replaceField}
setValue={setReplaceField}
options={{ id: "replace-whole-field-with", label: "Replace Whole Field With", variant: "outlined" }}
/>
</FormControl>

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I wonder if this would be more clear with radio buttons instead of multiple text inputs -- i.e.

"( ) Perform search and replace within values of matching fields ; ( ) Fully replace matching fields with this value ; ( ) Add this value to records containing matching fields"

(I should also note that the implementation appears to replace ALL values on a records with the SINGLE suggested replacement value -- that's maybe not the most desirable approach since it might destroy data unexpectedly).

Maybe we could also have separate radio buttons for "search and replace as literal text" vs. "search and replace as regular expression" in the future.

Of course, I'm not suggesting we need to implement all these possibilities right away. I'd rather finish a "minimum viable product" and add more later. But I'm just thinking ahead to how we can make this most manageable and easiest to understand.

Jason-Benson and others added 3 commits August 24, 2026 11:22
Co-authored-by: Demian Katz <demian.katz@villanova.edu>
Co-authored-by: Demian Katz <demian.katz@villanova.edu>
Co-authored-by: Demian Katz <demian.katz@villanova.edu>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants