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

feat(rest): video learning events #1904

Merged
merged 4 commits into from
Oct 4, 2024
Merged

feat(rest): video learning events #1904

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7145e7e
feat(rest): video learning events
jo-elimu Sep 30, 2024
7975194
fix: logical error in the success response (#1894)
jo-elimu Sep 30, 2024
c04eab8
Merge branch 'main' into 1894-add-videolearningevent
jo-elimu Oct 3, 2024
bea1699
test(rest): video learning events
jo-elimu Oct 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
86 changes: 86 additions & 0 deletions src/main/java/ai/elimu/rest/v2/analytics/VideoLearningEventsRestController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
package ai.elimu.rest.v2.analytics;

import ai.elimu.model.v2.enums.Language;
import ai.elimu.util.AnalyticsHelper;
import ai.elimu.util.ConfigHelper;
import java.io.File;
import javax.servlet.http.HttpServletResponse;
import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
import org.json.JSONObject;
import org.springframework.http.HttpStatus;
import org.springframework.http.MediaType;
import org.springframework.web.bind.annotation.RequestMapping;
import org.springframework.web.bind.annotation.RequestMethod;
import org.springframework.web.bind.annotation.RequestParam;
import org.springframework.web.bind.annotation.RestController;
import org.springframework.web.multipart.MultipartFile;

@RestController
@RequestMapping(value = "/rest/v2/analytics/video-learning-events", produces = MediaType.APPLICATION_JSON_VALUE)
public class VideoLearningEventsRestController {

private Logger logger = LogManager.getLogger();

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

JSONObject jsonResponseObject = new JSONObject();
try {
Comment on lines +25 to +33
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.

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);
Comment on lines +34 to +54
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.


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);
jo-elimu marked this conversation as resolved.
Show resolved Hide resolved
Comment on lines +56 to +69
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.


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;
Comment on lines +71 to +84
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"));

}
Comment on lines +71 to +85
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.

}
58 changes: 58 additions & 0 deletions src/test/java/ai/elimu/rest/v2/analytics/VideoLearningEventsRestControllerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
package ai.elimu.rest.v2.analytics;

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 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_invalidFilename() {
MultipartFile multipartFile = new MockMultipartFile(
"file",
"invalid_filename.csv",
"text/csv",
"test content".getBytes()
);
String jsonResponse = videoLearningEventsRestController.handleUploadCsvRequest(multipartFile, response);
JSONObject jsonObject = new JSONObject(jsonResponse);
assertEquals("error", jsonObject.getString("result"));
assertEquals("Unexpected filename", jsonObject.getString("errorMessage"));
verify(response).setStatus(HttpServletResponse.SC_INTERNAL_SERVER_ERROR);
}

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