Dc field replace - #368
Conversation
|
Looks like this broke some tests. I'll tackle those next. |
demiankatz
left a comment
There was a problem hiding this comment.
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.
…level pid, replace whole dc field
… for the new features.
… in the bulk editor
demiankatz
left a comment
There was a problem hiding this comment.
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).
| { "Content-Type": "application/json" }, | ||
| ); | ||
| const resultList = screen.getByTitle("Bulk Edit Results"); | ||
| expect(resultList.innerHTML).toEqual("(1/1) foo: success\n"); |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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."
| <FormControl fullWidth> | ||
| <BlurSavingTextField | ||
| value={replaceField} | ||
| setValue={setReplaceField} | ||
| options={{ id: "replace-whole-field-with", label: "Replace Whole Field With", variant: "outlined" }} | ||
| /> | ||
| </FormControl> |
There was a problem hiding this comment.
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.
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>
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.