UFAL/Embargo provenance - missing start date and end date #1049
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughextractAccessConditions in ProvenanceServiceImpl now maps each AccessCondition to its name plus optional " [from: ]" and/or " [till: ]" suffixes when present; ProvenanceServiceIT gains a new embargo provenance test (duplicated). No other control-flow or error-handling changes. Changes
Sequence Diagram(s)(Skipped — changes are a small mapping adjustment and test additions without new control-flow worth diagramming.) Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
dspace-api/src/main/java/org/dspace/core/ProvenanceServiceImpl.java (3)
326-332: Add a null/empty guard to avoid NPE when no access conditions are present.Upstream callers may pass a null list; a small guard makes this method safer without changing behavior.
Apply this diff:
private String extractAccessConditions(List<AccessCondition> accessConditions) { - return accessConditions.stream() + if (CollectionUtils.isEmpty(accessConditions)) { + return StringUtils.EMPTY; + } + return accessConditions.stream() .map(ac -> ac.getName() + (ac.getStartDate() != null ? " [from: " + ac.getStartDate().toString() + "]" : "") + (ac.getEndDate() != null ? " [till: " + ac.getEndDate().toString() + "]" : "")) .collect(Collectors.joining(";")); }
328-331: Prefer stable ISO-8601 date formatting and externalize labels for i18n.Provenance entries are audit-like; using ISO-8601 (and avoiding hardcoded English “from”/“till”) improves readability and localization.
Minimal diff to route through a formatter and use “until”:
- .map(ac -> ac.getName() + - (ac.getStartDate() != null ? " [from: " + ac.getStartDate().toString() + "]" : "") + - (ac.getEndDate() != null ? " [till: " + ac.getEndDate().toString() + "]" : "")) + .map(ac -> ac.getName() + + (ac.getStartDate() != null ? " [from: " + formatAccessDate(ac.getStartDate()) + "]" : "") + + (ac.getEndDate() != null ? " [until: " + formatAccessDate(ac.getEndDate()) + "]" : ""))Add this helper within the class (outside the shown range):
// Consider moving formatting and labels into ProvenanceMessageFormatter for full i18n. private static String formatAccessDate(Object date) { if (date == null) return ""; // Prefer java.time.* when available if (date instanceof java.time.LocalDate) { return java.time.format.DateTimeFormatter.ISO_LOCAL_DATE.format((java.time.LocalDate) date); } if (date instanceof java.time.LocalDateTime) { return java.time.format.DateTimeFormatter.ISO_LOCAL_DATE_TIME.format((java.time.LocalDateTime) date); } if (date instanceof java.time.OffsetDateTime) { return java.time.format.DateTimeFormatter.ISO_OFFSET_DATE_TIME.format((java.time.OffsetDateTime) date); } if (date instanceof java.time.ZonedDateTime) { return java.time.format.DateTimeFormatter.ISO_OFFSET_DATE_TIME.format((java.time.ZonedDateTime) date); } if (date instanceof java.util.Date) { return new java.text.SimpleDateFormat("yyyy-MM-dd").format((java.util.Date) date); } return String.valueOf(date); }
326-332: Ensure non-nullAccessCondition.nameand consistent date formatting
Guard against null names (e.g. viaObjects.requireNonNull(ac.getName(), "AccessCondition.name")or a default value) to avoid literal “null” in the output, and replace rawDate.toString()calls with a shared formatter (for example aSimpleDateFormator JSR-310DateTimeFormatteraligned with your ProvenanceMessageFormatter) for consistent, human-readable timestamps. [ProvenanceServiceImpl#extractAccessConditions]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
dspace-api/src/main/java/org/dspace/core/ProvenanceServiceImpl.java(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: dspace / docker-build (linux/amd64, ubuntu-latest, true)
- GitHub Check: dspace-cli / docker-build (linux/amd64, ubuntu-latest, true)
- GitHub Check: dspace-test / docker-build (linux/amd64, ubuntu-latest, true)
- GitHub Check: Run Unit Tests
- GitHub Check: Run Integration Tests
🔇 Additional comments (1)
dspace-api/src/main/java/org/dspace/core/ProvenanceServiceImpl.java (1)
328-331: Change achieves the objective; date suffixes are correctly appended.Both start and end dates are handled and consistently converted to strings; the semicolon-joined format is preserved.
There was a problem hiding this comment.
Pull Request Overview
This PR enhances provenance/audit logging by including time range information for access conditions. When access conditions have start or end dates, these are now displayed in the audit trail to provide better visibility into time-limited access restrictions.
- Added start date display with "from:" prefix when available
- Added end date display with "till:" prefix when available
- Enhanced audit message format to include temporal access condition details
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
this is how it currently looks |
milanmajchrak
left a comment
milanmajchrak
left a comment
There was a problem hiding this comment.
Missing integration tests
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
dspace-server-webapp/src/test/java/org/dspace/app/rest/ProvenanceServiceIT.java (3)
23-27: Prefer java.time over Calendar/TimeZone for deterministic testsUse Instant/ZoneId and ISO formatting to avoid locale/timezone-dependent strings and DST edge cases in tests.
Apply this import refactor if you switch the test to java.time:
-import java.util.Calendar; -import java.util.TimeZone; +import java.time.Instant; +import java.time.ZoneOffset; +import java.time.format.DateTimeFormatter;
385-408: Ensure cleanup in finally to prevent resource leaks on assertion failureIf the assertion fails,
deleteBitstream(bitstream)won’t run, leaving test state behind.Apply:
@@ - Bitstream bitstream = createBitstream(item, Constants.CONTENT_BUNDLE_NAME); - - BulkAccessControlInput bulk = new BulkAccessControlInput(); - AccessConditionBitstream bitstreamNode = new AccessConditionBitstream(); - List<AccessCondition> acList = new ArrayList<>(); - Calendar cal = Calendar.getInstance(TimeZone.getTimeZone("UTC")); - cal.set(2030, Calendar.JANUARY, 1, 0, 0, 0); - cal.set(Calendar.MILLISECOND, 0); - AccessCondition embargo = new AccessCondition("embargo", "test", cal.getTime(), null); - acList.add(embargo); - bitstreamNode.setAccessConditions(acList); - bulk.setBitstream(bitstreamNode); - - provenanceService.setBitstreamPolicies(context, bitstream, item, bulk); - - // Build full expected message - String expected = "Access condition (embargo [from: " + cal.getTime() + "]) was added to bitstream (" - + bitstream.getID() + ") by first (admin) last (admin) (" - + admin.getEmail() + ") on \nNo. of bitstreams: 1\n"; - - objectCheck(itemService.find(context, item.getID()), expected); - - deleteBitstream(bitstream); + Bitstream bitstream = createBitstream(item, Constants.CONTENT_BUNDLE_NAME); + try { + BulkAccessControlInput bulk = new BulkAccessControlInput(); + AccessConditionBitstream bitstreamNode = new AccessConditionBitstream(); + List<AccessCondition> acList = new ArrayList<>(); + Calendar cal = Calendar.getInstance(TimeZone.getTimeZone("UTC")); + cal.set(2030, Calendar.JANUARY, 1, 0, 0, 0); + cal.set(Calendar.MILLISECOND, 0); + AccessCondition embargo = new AccessCondition("embargo", "test", cal.getTime(), null); + acList.add(embargo); + bitstreamNode.setAccessConditions(acList); + bulk.setBitstream(bitstreamNode); + + provenanceService.setBitstreamPolicies(context, bitstream, item, bulk); + + String expected = "Access condition (embargo [from: "; + objectCheck(itemService.find(context, item.getID()), expected); + } finally { + deleteBitstream(bitstream); + }
382-408: Add an end-date (“till:”) coverage testCurrent test only verifies the start-date suffix. Add a second test where endDate is set (and optionally startDate null) to assert presence of “ [till: …]”.
I can draft a companion test if desired.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
dspace-server-webapp/src/test/java/org/dspace/app/rest/ProvenanceServiceIT.java(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
dspace-server-webapp/src/test/java/org/dspace/app/rest/ProvenanceServiceIT.java (3)
dspace-api/src/main/java/org/dspace/app/bulkaccesscontrol/model/AccessCondition.java (1)
AccessCondition(21-59)dspace-api/src/main/java/org/dspace/app/bulkaccesscontrol/model/AccessConditionBitstream.java (1)
AccessConditionBitstream(21-69)dspace-api/src/main/java/org/dspace/app/bulkaccesscontrol/model/BulkAccessControlInput.java (1)
BulkAccessControlInput(42-72)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: dspace / docker-build (linux/amd64, ubuntu-latest, true)
- GitHub Check: dspace-cli / docker-build (linux/amd64, ubuntu-latest, true)
- GitHub Check: dspace-test / docker-build (linux/amd64, ubuntu-latest, true)
- GitHub Check: Run Unit Tests
- GitHub Check: Run Integration Tests
🔇 Additional comments (3)
dspace-server-webapp/src/test/java/org/dspace/app/rest/ProvenanceServiceIT.java (3)
33-36: LGTM: Correct model imports for bulk access controlThese are required for constructing the embargo payload in the test.
63-63: LGTM: ProvenanceService import is appropriateUsed directly in the new test.
81-82: LGTM: Autowiring ProvenanceService in ITMakes sense for invoking policy updates directly.
dspace-server-webapp/src/test/java/org/dspace/app/rest/ProvenanceServiceIT.java
Show resolved
Hide resolved
Kasinhou
requested review from
milanmajchrak
and removed request for
milanmajchrak
* Added start and end date to embargo provenance * Consistent formatting for dates Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Created integration test for embargo provenance * Updated IT to expect whole message * Change the expected message in checkEmbargoProvenanceTest --------- Co-authored-by: Matus Kasak <matus.kasak@dataquest.sk> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Linked issue: #1030
Summary by CodeRabbit
New Features
Tests