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.

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

feat(rest): video learning events #1904

Merged
merged 4 commits into from
Oct 4, 2024
Merged

Conversation

jo-elimu
Copy link
Member

@jo-elimu jo-elimu commented Sep 30, 2024

Add REST API endpoint

#1894

Purpose

  • Add data collection for VideoLearningEvents

Technical Details

Testing Instructions

Screenshots


Format Checks

Note

Files in PRs are automatically checked for format violations with mvn spotless:check.

If this PR contains files with format violations, run mvn spotless:apply to fix them.

@jo-elimu jo-elimu requested a review from a team as a code owner September 30, 2024 14:07
@jo-elimu jo-elimu self-assigned this Sep 30, 2024
@jo-elimu jo-elimu linked an issue Sep 30, 2024 that may be closed by this pull request
6 tasks
Copy link
Contributor

coderabbitai bot commented Sep 30, 2024

Walkthrough

The changes introduce a new REST controller, VideoLearningEventsRestController, which is responsible for handling video learning event analytics. It specifically processes POST requests to the /rest/v2/analytics/video-learning-events/csv endpoint, allowing users to upload CSV files. The controller extracts relevant metadata from the uploaded files, organizes them into a structured directory on the server, and provides appropriate JSON responses based on the success or failure of the file upload process. Additionally, unit tests for this controller have been implemented to verify error handling.

Changes

File Path Change Summary
src/main/java/ai/elimu/rest/v2/analytics/VideoLearningEventsRestController.java Added class VideoLearningEventsRestController and method handleUploadCsvRequest(MultipartFile, HttpServletResponse) for handling CSV file uploads.
src/test/java/ai/elimu/rest/v2/analytics/VideoLearningEventsRestControllerTest.java Added unit tests for VideoLearningEventsRestController, testing error handling for invalid filenames and empty files.

Possibly related issues

  • Add VideoLearningEvent #1894: The changes introduce a REST API endpoint for uploading CSV files, which aligns with the issue's objective to add a VideoLearningEvent endpoint and includes input validation for uploaded files.

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

codecov bot commented Sep 30, 2024

Codecov Report

Attention: Patch coverage is 4.76190% with 40 lines in your changes missing coverage. Please review.

Project coverage is 15.00%. Comparing base (442d8c9) to head (bea1699).
Report is 5 commits behind head on main.

Files with missing lines Patch % Lines
...2/analytics/VideoLearningEventsRestController.java 4.76% 40 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #1904      +/-   ##
============================================
- Coverage     15.05%   15.00%   -0.06%     
- Complexity      456      457       +1     
============================================
  Files           249      250       +1     
  Lines          7724     7766      +42     
  Branches        808      810       +2     
============================================
+ Hits           1163     1165       +2     
- Misses         6511     6551      +40     
  Partials         50       50              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 4

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 610ab88 and 7145e7e.

📒 Files selected for processing (1)
  • src/main/java/ai/elimu/rest/v2/analytics/VideoLearningEventsRestController.java (1 hunks)
🔇 Additional comments (3)
src/main/java/ai/elimu/rest/v2/analytics/VideoLearningEventsRestController.java (3)

1-17: LGTM: Imports and package declaration are appropriate.

The imports cover all necessary components for the REST controller, including Spring Framework annotations, file handling utilities, and custom helper classes. The package declaration follows the expected structure for a version 2 REST API.


19-23: LGTM: Class structure and annotations are well-defined.

The @RestController annotation and @RequestMapping are correctly used, following RESTful conventions. The request mapping includes the API version and specifies JSON as the response format. The private logger field is properly initialized using Log4j2.


25-29: LGTM: Method signature and request mapping are correct.

The method correctly handles POST requests to the "/csv" endpoint and accepts a MultipartFile parameter for file upload.

Comment on lines 25 to 72
@RequestMapping(value = "/csv", method = RequestMethod.POST)
public String handleUploadCsvRequest(
@RequestParam("file") MultipartFile multipartFile,
HttpServletResponse response
) {
logger.info("handleUploadCsvRequest");

JSONObject jsonResponseObject = new JSONObject();
try {
// Expected format: "7161a85a0e4751cd_3001012_video-learning-events_2020-04-23.csv"
String originalFilename = multipartFile.getOriginalFilename();
logger.info("originalFilename: " + originalFilename);

String androidIdExtractedFromFilename = AnalyticsHelper.extractAndroidIdFromCsvFilename(originalFilename);
logger.info("androidIdExtractedFromFilename: \"" + androidIdExtractedFromFilename + "\"");

Integer versionCodeExtractedFromFilename = AnalyticsHelper.extractVersionCodeFromCsvFilename(originalFilename);
logger.info("versionCodeExtractedFromFilename: " + versionCodeExtractedFromFilename);

byte[] bytes = multipartFile.getBytes();
logger.info("bytes.length: " + bytes.length);

// Store the original CSV file on the filesystem
File elimuAiDir = new File(System.getProperty("user.home"), ".elimu-ai");
File languageDir = new File(elimuAiDir, "lang-" + Language.valueOf(ConfigHelper.getProperty("content.language")));
File analyticsDir = new File(languageDir, "analytics");
File androidIdDir = new File(analyticsDir, "android-id-" + androidIdExtractedFromFilename);
File versionCodeDir = new File(androidIdDir, "version-code-" + versionCodeExtractedFromFilename);
File videoLearningEventsDir = new File(versionCodeDir, "video-learning-events");
videoLearningEventsDir.mkdirs();
File csvFile = new File(videoLearningEventsDir, originalFilename);
logger.info("Storing CSV file at " + csvFile);
multipartFile.transferTo(csvFile);
} catch (Exception ex) {
logger.error(ex);

jsonResponseObject.put("result", "error");
jsonResponseObject.put("errorMessage", ex.getMessage());
response.setStatus(HttpStatus.INTERNAL_SERVER_ERROR.value());
}
jsonResponseObject.put("result", "success");
jsonResponseObject.put("successMessage", "The CSV file was uploaded");
response.setStatus(HttpStatus.OK.value());

String jsonResponse = jsonResponseObject.toString();
logger.info("jsonResponse: " + jsonResponse);
return jsonResponse;
}
Copy link
Contributor

@coderabbitai coderabbitai bot Sep 30, 2024

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider adding unit tests for the controller.

To ensure the reliability and correctness of the file upload process, consider adding unit tests that cover various scenarios such as:

  1. Successful file upload
  2. File upload with invalid filename
  3. File upload with invalid content
  4. Error scenarios (e.g., directory creation failure)

This will help maintain the integrity of the controller as the codebase evolves.

Copy link
Member Author

Choose a reason for hiding this comment

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

@coderabbitai Can you show me an example of a unit test for the controller?

Copy link
Contributor

Choose a reason for hiding this comment

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

@jo-elimu Sure! Below is an example of a unit test for the VideoLearningEventsRestController using JUnit and Mockito. This test covers the scenario of a successful file upload.

import ai.elimu.rest.v2.analytics.VideoLearningEventsRestController;
import ai.elimu.util.AnalyticsHelper;
import ai.elimu.util.ConfigHelper;
import org.json.JSONObject;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.mockito.InjectMocks;
import org.mockito.Mock;
import org.mockito.MockitoAnnotations;
import org.springframework.mock.web.MockMultipartFile;
import org.springframework.web.multipart.MultipartFile;

import javax.servlet.http.HttpServletResponse;
import java.io.File;
import java.io.IOException;

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.mockito.Mockito.*;

public class VideoLearningEventsRestControllerTest {

    @InjectMocks
    private VideoLearningEventsRestController videoLearningEventsRestController;

    @Mock
    private HttpServletResponse response;

    @BeforeEach
    public void setUp() {
        MockitoAnnotations.openMocks(this);
    }

    @Test
    public void testHandleUploadCsvRequest_Success() throws IOException {
        // Mock the MultipartFile
        MultipartFile multipartFile = new MockMultipartFile(
                "file",
                "7161a85a0e4751cd_3001012_video-learning-events_2020-04-23.csv",
                "text/csv",
                "test content".getBytes()
        );

        // Mock the helper methods
        when(AnalyticsHelper.extractAndroidIdFromCsvFilename(anyString())).thenReturn("7161a85a0e4751cd");
        when(AnalyticsHelper.extractVersionCodeFromCsvFilename(anyString())).thenReturn(3001012);
        when(ConfigHelper.getProperty("content.language")).thenReturn("EN");

        // Call the method under test
        String jsonResponse = videoLearningEventsRestController.handleUploadCsvRequest(multipartFile, response);

        // Verify the response
        JSONObject jsonObject = new JSONObject(jsonResponse);
        assertEquals("success", jsonObject.getString("result"));
        assertEquals("The CSV file was uploaded", jsonObject.getString("successMessage"));

        // Verify the response status
        verify(response).setStatus(HttpServletResponse.SC_OK);

        // Verify that the file was stored
        File expectedFile = new File(System.getProperty("user.home") + "/.elimu-ai/lang-EN/analytics/android-id-7161a85a0e4751cd/version-code-3001012/video-learning-events/7161a85a0e4751cd_3001012_video-learning-events_2020-04-23.csv");
        assertEquals(true, expectedFile.exists());
    }
}

Key Points:

  1. Mocking Dependencies: We mock the MultipartFile and helper methods (AnalyticsHelper and ConfigHelper) to simulate the file upload process.
  2. Assertions: We assert that the response contains the expected success message and that the file is stored in the correct location.
  3. Verifying Response: We verify that the correct HTTP status code is set in the response.

You can extend this test to cover other scenarios like invalid filenames, invalid content, and error handling.

Let me know if you need further assistance!

nya-elimu
nya-elimu previously approved these changes Sep 30, 2024
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Outside diff range and nitpick comments (2)
src/main/java/ai/elimu/rest/v2/analytics/VideoLearningEventsRestController.java (2)

25-29: LGTM: Method declaration is appropriate for file upload handling.

The method is correctly mapped to handle POST requests for CSV file uploads. The use of MultipartFile and HttpServletResponse parameters is appropriate.

Consider using @ResponseBody annotation to explicitly indicate that the method return value should be bound to the web response body. Although not strictly necessary with @RestController, it can improve code clarity:

@ResponseBody
@RequestMapping(value = "/csv", method = RequestMethod.POST)
public String handleUploadCsvRequest(

1-74: Enhance overall code quality with tests, validation, and dependency injection.

While the current implementation is functional, there are several areas where it could be improved:

  1. Unit Tests: Add comprehensive unit tests to ensure the reliability of the file upload process and error handling.
  2. Input Validation: Implement explicit input validation for the uploaded file and extracted metadata.
  3. Dependency Injection: Use Spring's dependency injection for configuration and helper classes to improve testability and flexibility.

Consider the following architectural improvements:

  1. Create a separate service class for file processing logic, which can be easily mocked in unit tests.
  2. Use constructor injection for dependencies:
@RestController
@RequestMapping(value = "/rest/v2/analytics/video-learning-events", produces = MediaType.APPLICATION_JSON_VALUE)
public class VideoLearningEventsRestController {
    
    private final Logger logger = LogManager.getLogger();
    private final VideoLearningEventService videoLearningEventService;
    
    public VideoLearningEventsRestController(VideoLearningEventService videoLearningEventService) {
        this.videoLearningEventService = videoLearningEventService;
    }
    
    // ... rest of the class
}
  1. Implement a custom exception handler for more granular error responses:
@ControllerAdvice
public class GlobalExceptionHandler {
    
    @ExceptionHandler(InvalidFileException.class)
    public ResponseEntity<String> handleInvalidFileException(InvalidFileException ex) {
        // Handle specific exceptions
    }
    
    // ... other exception handlers
}

Would you like assistance in generating unit tests or implementing input validation for this controller?

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 7145e7e and c04eab8.

📒 Files selected for processing (1)
  • src/main/java/ai/elimu/rest/v2/analytics/VideoLearningEventsRestController.java (1 hunks)
🔇 Additional comments (2)
src/main/java/ai/elimu/rest/v2/analytics/VideoLearningEventsRestController.java (2)

1-21: LGTM: Class declaration and imports are well-structured.

The class is properly annotated as a REST controller and mapped to an appropriate endpoint. The use of API versioning (v2) in the request mapping is a good practice for maintaining backward compatibility.


32-57: ⚠️ Potential issue

Improve file processing security and robustness.

While the file processing logic is functional, there are several areas that could be improved for better security and robustness:

  1. Validate the extracted metadata (Android ID and version code) to ensure they meet expected formats and ranges.
  2. Add checks for maximum allowed file size to prevent potential denial of service attacks.
  3. Validate the CSV content before processing to ensure it meets expected format and contains required columns.
  4. Add error handling for directory creation failures.

Here's a suggested improvement for directory creation:

File videoLearningEventsDir = new File(versionCodeDir, "video-learning-events");
if (!videoLearningEventsDir.exists() && !videoLearningEventsDir.mkdirs()) {
    throw new IOException("Failed to create directory: " + videoLearningEventsDir);
}

To ensure the file processing is secure, please run the following verification script:

Comment on lines +59 to +72
jsonResponseObject.put("result", "success");
jsonResponseObject.put("successMessage", "The CSV file was uploaded");
response.setStatus(HttpStatus.OK.value());
} catch (Exception ex) {
logger.error(ex);

jsonResponseObject.put("result", "error");
jsonResponseObject.put("errorMessage", ex.getMessage());
response.setStatus(HttpStatus.INTERNAL_SERVER_ERROR.value());
}

String jsonResponse = jsonResponseObject.toString();
logger.info("jsonResponse: " + jsonResponse);
return jsonResponse;
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Enhance response handling and logging practices.

The current implementation of response handling is functional but can be improved:

  1. Consider using Spring's ResponseEntity for more idiomatic response handling.
  2. Be cautious about logging entire response objects, as this might expose sensitive information in logs.
  3. Consider using a custom exception handler for more granular error responses.

Here's a suggested refactor using ResponseEntity:

@RequestMapping(value = "/csv", method = RequestMethod.POST)
public ResponseEntity<String> handleUploadCsvRequest(@RequestParam("file") MultipartFile multipartFile) {
    try {
        // ... (existing file processing logic) ...
        
        JSONObject jsonResponseObject = new JSONObject();
        jsonResponseObject.put("result", "success");
        jsonResponseObject.put("successMessage", "The CSV file was uploaded");
        return ResponseEntity.ok(jsonResponseObject.toString());
    } catch (Exception ex) {
        logger.error("Error processing file upload", ex);
        
        JSONObject jsonResponseObject = new JSONObject();
        jsonResponseObject.put("result", "error");
        jsonResponseObject.put("errorMessage", "An error occurred while processing the file");
        return ResponseEntity.status(HttpStatus.INTERNAL_SERVER_ERROR).body(jsonResponseObject.toString());
    }
}

This approach eliminates the need for manually setting response status and provides a more Spring-like way of handling responses.

For logging, consider logging only non-sensitive parts of the response:

logger.info("Response status: " + jsonResponseObject.getString("result"));

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 4

🧹 Outside diff range and nitpick comments (4)
src/test/java/ai/elimu/rest/v2/analytics/VideoLearningEventsRestControllerTest.java (4)

16-27: LGTM: Class setup is well-structured.

The class declaration and setup are correct. The use of @Injectmocks for the controller and @mock for HttpServletResponse is appropriate. The setUp method correctly initializes the mocks.

Consider adding a descriptive comment above the class declaration to explain its purpose and the components being tested.


29-42: LGTM: Invalid filename test is well-implemented.

The test method for handling an invalid filename is correctly implemented. It creates a MockMultipartFile with an invalid name, calls the controller method, and verifies both the JSON response content and the HTTP status code.

Consider adding a constant for the error message "Unexpected filename" to improve maintainability. This could be defined in the controller class and used both in the implementation and the test.


44-57: LGTM: Empty file test is well-implemented, with room for improvement.

The test method for handling an empty file is correctly implemented. It creates a MockMultipartFile with a valid filename but empty content, calls the controller method, and verifies both the JSON response content and the HTTP status code.

Consider the following improvements:

  1. Add a constant for the error message "Empty file" to improve maintainability.
  2. The filename "7161a85a0e4751cd_3001012_video-learning-events_2020-04-23.csv" seems to follow a specific format. Consider adding a comment explaining this format or extracting it to a constant if it's used elsewhere in the codebase.
  3. Add a test case for a valid file to ensure the happy path is also covered.

Example of adding a test for a valid file:

@Test
public void testHandleUploadCsvRequest_validFile() {
    MultipartFile multipartFile = new MockMultipartFile(
            "file",
            "7161a85a0e4751cd_3001012_video-learning-events_2020-04-23.csv",
            "text/csv",
            "valid,csv,content".getBytes()
    );
    String jsonResponse = videoLearningEventsRestController.handleUploadCsvRequest(multipartFile, response);
    JSONObject jsonObject = new JSONObject(jsonResponse);
    assertEquals("success", jsonObject.getString("result"));
    verify(response).setStatus(HttpServletResponse.SC_OK);
}

1-58: Overall, good test implementation with room for enhancement.

The VideoLearningEventsRestControllerTest class provides a solid foundation for testing the new REST API endpoint for VideoLearningEvents. The existing tests for invalid filename and empty file scenarios are well-structured and use appropriate mocking and verification techniques.

To further improve the test coverage and maintainability:

  1. Add a test case for a valid file upload to ensure the happy path is covered.
  2. Consider extracting error messages and filename patterns to constants for better maintainability.
  3. Add tests for edge cases, such as very large files or files with malformed content.
  4. Include comments explaining the expected behavior of the controller for each test case.

These enhancements will provide more comprehensive test coverage and make the tests more robust and easier to maintain as the codebase evolves.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between c04eab8 and bea1699.

📒 Files selected for processing (2)
  • src/main/java/ai/elimu/rest/v2/analytics/VideoLearningEventsRestController.java (1 hunks)
  • src/test/java/ai/elimu/rest/v2/analytics/VideoLearningEventsRestControllerTest.java (1 hunks)
🔇 Additional comments (2)
src/test/java/ai/elimu/rest/v2/analytics/VideoLearningEventsRestControllerTest.java (1)

1-15: LGTM: Package declaration and imports are correct.

The package declaration is appropriate, and all necessary imports for JUnit 5, Mockito, and Spring's MockMultipartFile are present. There are no unused imports.

src/main/java/ai/elimu/rest/v2/analytics/VideoLearningEventsRestController.java (1)

1-23: LGTM: Class structure and imports are appropriate.

The class declaration, package structure, and imports are well-organized and relevant to the REST controller's functionality.

Comment on lines +25 to +33
@RequestMapping(value = "/csv", method = RequestMethod.POST)
public String handleUploadCsvRequest(
@RequestParam("file") MultipartFile multipartFile,
HttpServletResponse response
) {
logger.info("handleUploadCsvRequest");

JSONObject jsonResponseObject = new JSONObject();
try {
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider using ResponseEntity for improved response handling.

While the current implementation works, using Spring's ResponseEntity can provide more idiomatic and flexible response handling. It allows for easier manipulation of response headers, status codes, and body in a type-safe manner.

Here's a suggested refactor for the method signature:

@RequestMapping(value = "/csv", method = RequestMethod.POST)
public ResponseEntity<String> handleUploadCsvRequest(@RequestParam("file") MultipartFile multipartFile) {
    logger.info("handleUploadCsvRequest");
    
    try {
        // Existing logic here...
        
        return ResponseEntity.ok(jsonResponseObject.toString());
    } catch (Exception ex) {
        logger.error("Error processing file upload", ex);
        return ResponseEntity.status(HttpStatus.INTERNAL_SERVER_ERROR)
                             .body(new JSONObject().put("error", ex.getMessage()).toString());
    }
}

This approach eliminates the need for manually setting response status and provides a more Spring-like way of handling responses.

Comment on lines +34 to +54
String contentType = multipartFile.getContentType();
logger.info("contentType: " + contentType);

long size = multipartFile.getSize();
logger.info("size: " + size);
if (size == 0) {
throw new IllegalArgumentException("Empty file");
}

// Expected format: "7161a85a0e4751cd_3001012_video-learning-events_2020-04-23.csv"
String originalFilename = multipartFile.getOriginalFilename();
logger.info("originalFilename: " + originalFilename);
if (originalFilename.length() != 61) {
throw new IllegalArgumentException("Unexpected filename");
}

String androidIdExtractedFromFilename = AnalyticsHelper.extractAndroidIdFromCsvFilename(originalFilename);
logger.info("androidIdExtractedFromFilename: \"" + androidIdExtractedFromFilename + "\"");

Integer versionCodeExtractedFromFilename = AnalyticsHelper.extractVersionCodeFromCsvFilename(originalFilename);
logger.info("versionCodeExtractedFromFilename: " + versionCodeExtractedFromFilename);
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Enhance input validation and error handling.

While the current validation is good, consider the following improvements:

  1. Validate the content type to ensure it's a CSV file.
  2. Provide more specific error messages for different validation failures.
  3. Consider using a custom exception for validation errors.

Here's a suggested refactor:

if (!"text/csv".equals(contentType)) {
    throw new IllegalArgumentException("Invalid file type. Expected CSV.");
}

if (size == 0) {
    throw new IllegalArgumentException("File is empty.");
}

if (originalFilename == null || originalFilename.length() != 61) {
    throw new IllegalArgumentException("Invalid filename format.");
}

// Consider creating a custom exception for more granular error handling
class FileValidationException extends RuntimeException {
    public FileValidationException(String message) {
        super(message);
    }
}

// Use the custom exception in your validation logic
if (condition) {
    throw new FileValidationException("Specific error message");
}

This approach provides more informative error messages and allows for more granular error handling.

Comment on lines +56 to +69
byte[] bytes = multipartFile.getBytes();
logger.info("bytes.length: " + bytes.length);

// Store the original CSV file on the filesystem
File elimuAiDir = new File(System.getProperty("user.home"), ".elimu-ai");
File languageDir = new File(elimuAiDir, "lang-" + Language.valueOf(ConfigHelper.getProperty("content.language")));
File analyticsDir = new File(languageDir, "analytics");
File androidIdDir = new File(analyticsDir, "android-id-" + androidIdExtractedFromFilename);
File versionCodeDir = new File(androidIdDir, "version-code-" + versionCodeExtractedFromFilename);
File videoLearningEventsDir = new File(versionCodeDir, "video-learning-events");
videoLearningEventsDir.mkdirs();
File csvFile = new File(videoLearningEventsDir, originalFilename);
logger.info("Storing CSV file at " + csvFile);
multipartFile.transferTo(csvFile);
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Improve error handling for file storage and consider externalizing the storage configuration.

The current implementation has a few areas for improvement:

  1. Add explicit error handling for directory creation and file transfer.
  2. Consider externalizing the storage location configuration for better flexibility across different environments.

Here's a suggested refactor:

// Externalize this configuration
@Value("${app.storage.root:/tmp/.elimu-ai}")
private String storageRoot;

// In the method
File storageDir = new File(storageRoot, 
    String.format("lang-%s/analytics/android-id-%s/version-code-%d/video-learning-events",
    Language.valueOf(ConfigHelper.getProperty("content.language")),
    androidIdExtractedFromFilename,
    versionCodeExtractedFromFilename));

if (!storageDir.exists() && !storageDir.mkdirs()) {
    throw new IOException("Failed to create directory: " + storageDir);
}

File csvFile = new File(storageDir, originalFilename);
logger.info("Storing CSV file at " + csvFile);

try {
    multipartFile.transferTo(csvFile);
} catch (IOException e) {
    throw new IOException("Failed to save file: " + csvFile, e);
}

This approach provides better error handling and allows for easier configuration of the storage location across different environments.

Comment on lines +71 to +85
jsonResponseObject.put("result", "success");
jsonResponseObject.put("successMessage", "The CSV file was uploaded");
response.setStatus(HttpStatus.OK.value());
} catch (Exception ex) {
logger.error(ex);

jsonResponseObject.put("result", "error");
jsonResponseObject.put("errorMessage", ex.getMessage());
response.setStatus(HttpStatus.INTERNAL_SERVER_ERROR.value());
}

String jsonResponse = jsonResponseObject.toString();
logger.info("jsonResponse: " + jsonResponse);
return jsonResponse;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Enhance response handling and logging practices.

Consider the following improvements:

  1. Use ResponseEntity for more idiomatic response handling (as suggested earlier).
  2. Be cautious about logging entire response objects to avoid exposing sensitive information.
  3. Provide more specific error responses for different types of exceptions.

Here's a suggested refactor:

try {
    // Existing logic...
    
    JSONObject successResponse = new JSONObject()
        .put("result", "success")
        .put("message", "The CSV file was uploaded successfully");
    logger.info("File upload successful: " + originalFilename);
    return ResponseEntity.ok(successResponse.toString());
} catch (IllegalArgumentException e) {
    logger.warn("Invalid input: " + e.getMessage());
    JSONObject errorResponse = new JSONObject()
        .put("result", "error")
        .put("message", "Invalid input: " + e.getMessage());
    return ResponseEntity.badRequest().body(errorResponse.toString());
} catch (IOException e) {
    logger.error("File processing error", e);
    JSONObject errorResponse = new JSONObject()
        .put("result", "error")
        .put("message", "An error occurred while processing the file");
    return ResponseEntity.status(HttpStatus.INTERNAL_SERVER_ERROR)
                         .body(errorResponse.toString());
} catch (Exception e) {
    logger.error("Unexpected error", e);
    JSONObject errorResponse = new JSONObject()
        .put("result", "error")
        .put("message", "An unexpected error occurred");
    return ResponseEntity.status(HttpStatus.INTERNAL_SERVER_ERROR)
                         .body(errorResponse.toString());
}

This approach provides more specific error handling, uses ResponseEntity for consistent response generation, and avoids logging potentially sensitive information.

@nya-elimu nya-elimu merged commit 39b7593 into main Oct 4, 2024
13 of 14 checks passed
@nya-elimu nya-elimu deleted the 1894-add-videolearningevent branch October 4, 2024 08:01
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.

Add VideoLearningEvent
2 participants