UFAL/Matomo fix tracking of the statistics#912
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis pull request enhances the tracking of bitstream downloads across various components. The Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant RESTController
participant Tracker
participant Utils
participant Matomo
Client->>RESTController: Request download (bitstream or ZIP)
RESTController->>Tracker: trackBitstreamDownload(context, request, bit, isZip)
Tracker->>Utils: fetchUUIDFromUrl(actionURL)
Utils-->>Tracker: Return UUID or error
alt isZip == true
Tracker->>Matomo: logUserDownloadingZip(item)
else
Tracker->>Matomo: logUserDownloadingBitstream(bit)
end
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
dspace-api/src/main/java/org/dspace/app/statistics/clarin/ClarinMatomoBitstreamTracker.java (1)
80-91: Improved URL construction with proper error handling.The changes improve how download URLs are constructed by extracting the Bitstream UUID from the action URL. This provides more specific tracking information while maintaining fallback behavior if extraction fails.
I would suggest enhancing the error logging to include the exception details for easier troubleshooting:
- log.error("Cannot get the Bitstream UUID from the URL {}", matomoRequest.getActionUrl()); + log.error("Cannot get the Bitstream UUID from the URL {}: {}", matomoRequest.getActionUrl(), e.getMessage(), e);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
dspace-api/src/main/java/org/dspace/app/statistics/clarin/ClarinMatomoBitstreamTracker.java(6 hunks)dspace-api/src/main/java/org/dspace/core/Utils.java(3 hunks)dspace-api/src/test/java/org/dspace/statistics/ClarinMatomoBitstreamTrackerTest.java(1 hunks)dspace-server-webapp/src/main/java/org/dspace/app/rest/BitstreamRestController.java(1 hunks)dspace-server-webapp/src/main/java/org/dspace/app/rest/MetadataBitstreamController.java(4 hunks)
🧰 Additional context used
🧬 Code Definitions (3)
dspace-server-webapp/src/main/java/org/dspace/app/rest/MetadataBitstreamController.java (1)
dspace-api/src/main/java/org/dspace/app/statistics/clarin/ClarinMatomoBitstreamTracker.java (1)
ClarinMatomoBitstreamTracker(41-194)
dspace-api/src/test/java/org/dspace/statistics/ClarinMatomoBitstreamTrackerTest.java (1)
dspace-api/src/main/java/org/dspace/app/statistics/clarin/ClarinMatomoBitstreamTracker.java (1)
ClarinMatomoBitstreamTracker(41-194)
dspace-api/src/main/java/org/dspace/app/statistics/clarin/ClarinMatomoBitstreamTracker.java (1)
dspace-api/src/main/java/org/dspace/core/Utils.java (1)
Utils(54-581)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: dspace-dependencies / docker-build (linux/amd64, ubuntu-latest, true)
- GitHub Check: Run Integration Tests
- GitHub Check: Run Unit Tests
🔇 Additional comments (20)
dspace-server-webapp/src/main/java/org/dspace/app/rest/BitstreamRestController.java (1)
168-168: Updated method call to support the enhanced download tracking.The
trackBitstreamDownloadmethod call now includes a boolean parameterfalseto indicate this is a single bitstream download rather than a ZIP file. This aligns with the updated method signature in theClarinMatomoBitstreamTrackerclass.dspace-server-webapp/src/main/java/org/dspace/app/rest/MetadataBitstreamController.java (4)
31-31: Appropriately added import for the new dependency.The import for
ClarinMatomoBitstreamTrackeris needed to support the new tracking functionality.
75-76: Added dependency injection for tracking functionality.The autowired tracker instance will be used to track ZIP file downloads.
115-139: Added tracking for the first bitstream in ZIP downloads.A reference to the first encountered bitstream is stored to be used for statistics tracking. This is a pragmatic approach since we need at least one bitstream to track the download event.
143-143: Added tracking for ZIP file downloads.The method now tracks ZIP file downloads, passing
trueas the fourth parameter to indicate this is a ZIP download rather than a single bitstream. This complements the tracking for single bitstreams inBitstreamRestController.dspace-api/src/main/java/org/dspace/core/Utils.java (2)
60-62: Added UUID pattern for regex matching.A compiled regex pattern to efficiently match UUIDs in standard format has been defined. This pattern will be reused by the new utility method.
556-580: Added robust utility method to extract UUIDs from URLs.The new
fetchUUIDFromUrlmethod provides a centralized way to extract UUIDs from URLs using regex pattern matching. The implementation includes:
- Proper URI parsing
- Comprehensive error handling with descriptive error messages
- Thorough documentation explaining the method's purpose and expected inputs
This utility method helps standardize UUID extraction across the application, particularly for bitstream tracking purposes.
dspace-api/src/test/java/org/dspace/statistics/ClarinMatomoBitstreamTrackerTest.java (8)
1-41: Well-structured test class setup with appropriate imports and dependencies.The test file includes all necessary imports and properly sets up mock objects for testing. The use of
@Mockand@InjectMocksannotations follows testing best practices.
42-70: Test class properly extends AbstractDSpaceTest with well-defined constants.The test class is appropriately set up with:
- Constants for test data like handles and URLs
- Mock objects for all required dependencies
- Proper inheritance from the base test class
71-75: Test setup initializes necessary context.The
setUpmethod establishes a fresh context for each test, following testing best practices.
76-88: Test case for successful bitstream download tracking.This test verifies that when a valid UUID is present in the URL, the tracker correctly extracts it and uses it in the download URL construction. The test:
- Sets up test data with a random UUID
- Mocks the necessary objects and behavior
- Calls the method under test
- Verifies the expected URL format is used
This ensures the tracker works correctly with valid input.
90-103: Test case for tracking with invalid UUID in URL.This test verifies the tracker's behavior when faced with an invalid UUID in the URL. It ensures that:
- The tracker gracefully handles the situation
- Falls back to using the original URL when UUID extraction fails
This test increases confidence in the code's error handling capabilities.
105-111: Well-factored helper method for request setup.The
mockRequestmethod encapsulates the common setup for HTTP requests, making the test cases more readable and maintainable.
113-124: Comprehensive helper method for bitstream and item setup.The
mockBitstreamAndItemmethod properly sets up all the necessary mock objects and their relationships, including metadata values, which is crucial for testing the tracker functionality.
126-134: Robust verification helper using ArgumentCaptor.The
verifyMatomoRequestmethod uses ArgumentCaptor to capture and verify the properties of the MatomoRequest sent to the tracker. This ensures the tracking mechanism is functioning correctly with the expected values.dspace-api/src/main/java/org/dspace/app/statistics/clarin/ClarinMatomoBitstreamTracker.java (5)
27-27: Import addition looks appropriate.The addition of the Utils import is correctly used to access the new
fetchUUIDFromUrlmethod needed for URL processing.
128-129: Method signature enhancement to distinguish download types.The addition of the
isZipparameter is an effective way to differentiate between individual file downloads and ZIP downloads, allowing for specialized tracking behavior.
139-142: Added defensive null check for bitstream.Good defensive programming practice by adding a null check for the bitstream before proceeding, with appropriate error logging.
156-162: Implemented conditional logic for download types.The code correctly implements different logging behaviors based on whether the download is a single file or a ZIP archive, providing more accurate metrics.
183-193: Well-implemented logging for ZIP downloads.The new method for logging ZIP downloads maintains consistent formatting with the existing logging method while providing the appropriate context-specific information. The logging pattern clearly identifies the action as downloading a ZIP file containing all bitstreams from an item.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
dspace-api/src/test/java/org/dspace/statistics/ClarinMatomoBitstreamTrackerTest.java (3)
95-108: Test handles invalid UUID case properly, but contains duplicate Context initialization.The test correctly verifies the behavior when an invalid UUID is in the URL. However, there's an unnecessary Context initialization that duplicates the one in the setUp method.
@Test public void testTrackBitstreamDownloadWrongUrl() throws SQLException { - context = new Context(); UUID bitstreamId = UUID.randomUUID(); mockRequest("/bitstreams/NOT_EXISTING_UUID/download"); mockBitstreamAndItem(bitstreamId); when(matomoTracker.sendRequestAsync(any(MatomoRequest.class))) .thenReturn(CompletableFuture.completedFuture(null)); clarinMatomoBitstreamTracker.trackBitstreamDownload(context, request, bitstream, false); String expectedUrl = BASE_URL + "/bitstreams/NOT_EXISTING_UUID/download"; verifyMatomoRequest(expectedUrl); }
110-139: Well-implemented helper methods, but missing documentation.The helper methods effectively encapsulate common testing logic, but would benefit from JavaDoc comments to explain their purpose and parameters.
Add JavaDoc to the helper methods, for example:
+/** + * Mock an HttpServletRequest with specific URI and properties. + * + * @param requestURI The URI to use in the mocked request + */ private void mockRequest(String requestURI) { when(request.getRequestURI()).thenReturn(requestURI); when(request.getScheme()).thenReturn("http"); when(request.getServerName()).thenReturn("example.com"); when(request.getServerPort()).thenReturn(80); when(request.getHeader("Range")).thenReturn(null); }
47-139: Insufficient test coverage for important scenarios.The tests only cover the
isZip=falsecase for bitstream tracking. There are several important scenarios missing:
- Test for when
isZip=trueto verify ZIP file download tracking- Test when Range header is non-null (should skip tracking per the implementation)
- Test when tracking is disabled in configuration
- Test error paths (e.g., empty results from
findByBitstreamUUID)Consider adding these test methods:
@Test public void testTrackZipDownload() throws SQLException { UUID bitstreamId = UUID.randomUUID(); mockRequest("/bitstreams/" + bitstreamId + "/download"); mockBitstreamAndItem(bitstreamId); when(matomoTracker.sendRequestAsync(any(MatomoRequest.class))) .thenReturn(CompletableFuture.completedFuture(null)); clarinMatomoBitstreamTracker.trackBitstreamDownload(context, request, bitstream, true); String expectedUrl = LOCALHOST_URL + "/bitstream/handle/" + HANDLE + "/" + bitstreamId; verifyMatomoRequest(expectedUrl); } @Test public void testSkipTrackingWithRangeHeader() throws SQLException { UUID bitstreamId = UUID.randomUUID(); mockRequest("/bitstreams/" + bitstreamId + "/download"); // Override Range header to be non-null when(request.getHeader("Range")).thenReturn("bytes=0-1000"); mockBitstreamAndItem(bitstreamId); clarinMatomoBitstreamTracker.trackBitstreamDownload(context, request, bitstream, false); // Verify that no tracking request was sent verify(matomoTracker, times(0)).sendRequestAsync(any(MatomoRequest.class)); } @Test public void testSkipTrackingWhenDisabled() throws SQLException { UUID bitstreamId = UUID.randomUUID(); mockRequest("/bitstreams/" + bitstreamId + "/download"); mockBitstreamAndItem(bitstreamId); // Configure tracking to be disabled when(configurationService.getBooleanProperty("matomo.track.enabled")).thenReturn(false); clarinMatomoBitstreamTracker.trackBitstreamDownload(context, request, bitstream, false); // Verify that no tracking request was sent verify(matomoTracker, times(0)).sendRequestAsync(any(MatomoRequest.class)); } @Test public void testNoItemFound() throws SQLException { UUID bitstreamId = UUID.randomUUID(); mockRequest("/bitstreams/" + bitstreamId + "/download"); when(bitstream.getID()).thenReturn(bitstreamId); // Return empty list to simulate no items found when(clarinItemService.findByBitstreamUUID(context, bitstreamId)).thenReturn(Collections.emptyList()); clarinMatomoBitstreamTracker.trackBitstreamDownload(context, request, bitstream, false); // Verify that no tracking request was sent verify(matomoTracker, times(0)).sendRequestAsync(any(MatomoRequest.class)); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
dspace-api/src/test/java/org/dspace/statistics/ClarinMatomoBitstreamTrackerTest.java(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
dspace-api/src/test/java/org/dspace/statistics/ClarinMatomoBitstreamTrackerTest.java (1)
dspace-api/src/main/java/org/dspace/app/statistics/clarin/ClarinMatomoBitstreamTracker.java (1)
ClarinMatomoBitstreamTracker(41-200)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Run Integration Tests
- GitHub Check: dspace-dependencies / docker-build (linux/amd64, ubuntu-latest, true)
- GitHub Check: Run Unit Tests
🔇 Additional comments (3)
dspace-api/src/test/java/org/dspace/statistics/ClarinMatomoBitstreamTrackerTest.java (3)
1-47: Well-structured test class with clear purpose.The test class is well-organized with proper imports, documentation, and mock setup to test the
ClarinMatomoBitstreamTrackerfunctionality.
74-79: Good setup method.The setup method correctly initializes a new Context object before each test.
81-93: Well-implemented test for single file download tracking.The test correctly verifies that tracking a bitstream download works as expected by:
- Creating a random UUID for the bitstream
- Mocking the necessary dependencies
- Calling the method under test
- Verifying the correct URL is sent to the Matomo tracker
dspace-api/src/main/java/org/dspace/app/statistics/clarin/ClarinMatomoBitstreamTracker.java
Outdated
Show resolved
Hide resolved
* Bitstream preview wrong file name according to it's mimetype (#890) * The owning community was null. (#891) * The + characted was wrongly encoded in the URL (#893) * Set limit when splitting key/value using = (#894) * File preview - Added the method for extracting the file into try catch block (#909) * Fix parts identifiers resolution (#913) * Renamed property dspace.url to dspace.ui.url (#906) * Update clarin-dspace.cfg - handle.plugin.checknameauthority (#897) * File preview - Return empty list if an error has occured (#915) * Matomo fix tracking of the statistics (#912)
Merging latest dataquest-dev/dspace:dtq-dev This contains the following commits: Run build action every 4h for every customer/ branch UFAL/Do not use not-existing metadatafield `hasMetadata` in the submission-forms-cz (dataquest-dev#888) UFAL/Created job to generate preview for every item or for a specific one (dataquest-dev#887) UFAL/bitstream preview wrong file name according to it's mimetype (dataquest-dev#890) Fixed typo in the error exception The owning community was null. (dataquest-dev#891) The `+` characted was wrongly encoded in the URL (dataquest-dev#893) Set limit when splitting key/value using `=` (dataquest-dev#894) Ufal/header value could have equals char (dataquest-dev#895) UFAL/File preview - Added the method for extracting the file into try catch block (dataquest-dev#909) UFAL/File preview better logs (dataquest-dev#910) UFAL/File preview - Return empty list if an error has occured (dataquest-dev#915) UFAL/Matomo fix tracking of the statistics (dataquest-dev#912) UFAL/Matomo statistics - Use the bitstream name instead of the UUID in the tracking download url (dataquest-dev#917) UFAL/Matomo bitstream tracker has error when bitstream name was null (dataquest-dev#918) UFAL/Endpoints leaks private information (dataquest-dev#924) UFAL/Fix parts identifiers resolution (dataquest-dev#913) UFAL/Update `clarin-dspace.cfg` - handle.plugin.checknameauthority (dataquest-dev#897) Creating Legal check (dataquest-dev#863) import/comment-license-script (dataquest-dev#882) UFAL/Renamed property dspace.url to dspace.ui.url (dataquest-dev#906)
Problem description
Summary by CodeRabbit
New Features
Tests