Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[DS-4300] Optionally apply given Handles when importing community/collection structure #2469

Merged
merged 10 commits into from Jul 28, 2022
Merged

[DS-4300] Optionally apply given Handles when importing community/collection structure #2469

merged 10 commits into from Jul 28, 2022

Conversation

mwoodiupui
Copy link
Member

@mwoodiupui mwoodiupui commented Jul 15, 2019 •
edited

Fixes #7640
Based on mwoodiupui#13

@mwoodiupui mwoodiupui added new feature work in progress PR is still being worked on & is not currently ready for review interface: command-line DSpace CLI (command-line interface) identifier: Handles Related to integration with Handle System tools: import Related to import of data into the system labels Jul 15, 2019
@mwoodiupui mwoodiupui added this to the 7.0 milestone Jul 15, 2019
@mwoodiupui mwoodiupui requested a review from pnbecker July 15, 2019 16:46
@mwoodiupui mwoodiupui removed the work in progress PR is still being worked on & is not currently ready for review label Aug 15, 2019
@mwoodiupui mwoodiupui changed the base branch from master to main July 14, 2020 13:45
@mwoodiupui mwoodiupui closed this Aug 4, 2020
@mwoodiupui
Copy link
Member Author

Close/open to get Travis to run.

@mwoodiupui mwoodiupui reopened this Aug 4, 2020
@github-actions github-actions bot added the merge conflict PR has a merge conflict that needs resolution label Dec 7, 2020
@mwoodiupui mwoodiupui removed the merge conflict PR has a merge conflict that needs resolution label Mar 14, 2021
@mwoodiupui mwoodiupui removed this from the 7.0 milestone Mar 14, 2021
@mwoodiupui mwoodiupui added the needs triage New issue needs triage and/or scheduling label Mar 14, 2021
@tdonohue
Copy link
Member

@mwoodiupui : Could we add tests for this PR in the existing StructBuilderIT? https://github.com/DSpace/DSpace/blob/main/dspace-api/src/test/java/org/dspace/administer/StructBuilderIT.java

@mwoodiupui mwoodiupui added improvement and removed needs triage New issue needs triage and/or scheduling new feature labels Mar 29, 2022
@github-actions github-actions bot added the merge conflict PR has a merge conflict that needs resolution label Mar 30, 2022
@mwoodiupui mwoodiupui removed the merge conflict PR has a merge conflict that needs resolution label Mar 31, 2022
@github-actions github-actions bot added the merge conflict PR has a merge conflict that needs resolution label Apr 28, 2022
@mwoodiupui mwoodiupui removed the merge conflict PR has a merge conflict that needs resolution label May 5, 2022
@mwoodiupui mwoodiupui added this to Needs Reviewers Assigned in DSpace 7.4 release via automation May 5, 2022
@mwoodiupui mwoodiupui self-assigned this Jun 22, 2022
@mwoodiupui mwoodiupui mentioned this pull request Jul 13, 2022
Closed
DSpace 7.4 release automation moved this from Needs Reviewers Assigned to Under Review Jul 13, 2022
abollini
abollini requested changes Jul 13, 2022
View reviewed changes
Copy link
Member

@abollini abollini left a comment

Choose a reason for hiding this comment

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

hi @mwoodiupui thanks for this PR and to ping us on the recent bug report about it!
I made a quick code review and added inline a question for you about the test, can you take a look?
my colleague will make a functional test in the next days as well

dspace-api/src/test/java/org/dspace/administer/StructBuilderIT.java Outdated
IMPORT_DOCUMENT.getBytes(StandardCharsets.UTF_8)));
Diff myDiff = DiffBuilder.compare(reference).withTest(output)
.normalizeWhitespace()
// .withNodeFilter(new MyNodeFilter())
Copy link
Member

Choose a reason for hiding this comment

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

please remove comment out code

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

dspace-api/src/test/java/org/dspace/administer/StructBuilderIT.java Outdated
Comment on lines 222 to 232
for (Community community : communityService.findAllTop(context)) {
for (Handle handle : community.getHandles()) {
if (handle.getHandle().equals(COMMUNITY_1_HANDLE)) {
found = true;
break;
}
if (found) {
break;
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

is there a reason to apply a different test strategy than the one used in testImportStructure via the DiffBuilder?
checking for just a single handle could hide lot of (future) issues (maybe the import skip all the community without a handle or handle are not set for collection, etc.)

Copy link
Member Author

Choose a reason for hiding this comment

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

I've added a spot-check for a Collection handle and a Community with handle not specified.

Below the cited code, this test also applies the strategy from testImportStructure.

@tdonohue tdonohue requested a review from abollini July 21, 2022 14:52
pnbecker
pnbecker approved these changes Jul 21, 2022
View reviewed changes
Copy link
Member

@pnbecker pnbecker left a comment

Choose a reason for hiding this comment

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

👍 by code review
I must mention: a small part of that code goes back to a PR I created for @mwoodiupui (see mwoodiupui#13). So I'm kind of reviewing a small part of my own code. Even if it is a small PR, it should get a second review.

@mwoodiupui
Copy link
Member Author

Yes, thanks to @pnbecker for the original code. I chose to treat it as a separate issue rather than merge it into that other PR.

@vins01-4science
Copy link
Contributor

vins01-4science commented Jul 25, 2022 •
edited

hi @mwoodiupui thanks for this PR and to ping us on the recent bug report about it! I made a quick code review and added inline a question for you about the test, can you take a look? my colleague will make a functional test in the next days as well

Hi @abollini, the functional tests are done, and I think we can accept these changes @tdonohue.
Thank you, @mwoodiupui.

@tdonohue tdonohue moved this from Under Review to Reviewer Approved in DSpace 7.4 release Jul 28, 2022
@tdonohue
Copy link
Member

Merging as this is at +2. Thanks @mwoodiupui !

@tdonohue tdonohue merged commit 4d4f129 into DSpace:main Jul 28, 2022
DSpace 7.4 release automation moved this from Reviewer Approved to Done Jul 28, 2022
@tdonohue tdonohue added this to the 7.4 milestone Jul 28, 2022
@mwoodiupui mwoodiupui deleted the DS-4300 branch July 28, 2022 17:33
alejandratenorio pushed a commit to CIMMYT/DSpace that referenced this pull request Sep 8, 2022
@mwoodiupui @alejandratenorio
Remove commented-out code. DSpace#2469
88a4035
alejandratenorio pushed a commit to CIMMYT/DSpace that referenced this pull request Sep 8, 2022
@mwoodiupui @alejandratenorio
Add spot-check for a Collection handle. DSpace#2469
d9fe7b7
alejandratenorio pushed a commit to CIMMYT/DSpace that referenced this pull request Sep 8, 2022
@mwoodiupui @alejandratenorio
Test with a community with handle not specified. DSpace#2469
ee84c0e
milanmajchrak pushed a commit to dataquest-dev/DSpace that referenced this pull request Aug 7, 2023
@mwoodiupui @milanmajchrak
Remove commented-out code. DSpace#2469
08a8ddb
milanmajchrak pushed a commit to dataquest-dev/DSpace that referenced this pull request Aug 7, 2023
@mwoodiupui @milanmajchrak
Add spot-check for a Collection handle. DSpace#2469
d20bc1a
milanmajchrak pushed a commit to dataquest-dev/DSpace that referenced this pull request Aug 7, 2023
@mwoodiupui @milanmajchrak
Test with a community with handle not specified. DSpace#2469
27c5ea5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pnbecker pnbecker pnbecker approved these changes

@abollini abollini Awaiting requested review from abollini

Assignees

@mwoodiupui mwoodiupui

Labels
identifier: Handles Related to integration with Handle System improvement interface: command-line DSpace CLI (command-line interface) tools: import Related to import of data into the system
Projects
No open projects
DSpace 7.4 release
  
Done
Milestone
7.4
Development

Successfully merging this pull request may close these issues.

[DS-4300] Optionally apply given Handles when importing community/collection structure
5 participants
@mwoodiupui @tdonohue @vins01-4science @abollini @pnbecker