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/teacher profile form 선생님 구글폼 프로필 등록 #4

Merged
merged 3 commits into from
Jan 31, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 3 additions & 0 deletions build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,9 @@ dependencies {
// flyway 추가
implementation 'org.flywaydb:flyway-mysql'
implementation 'org.flywaydb:flyway-core'

// aws s3
implementation 'org.springframework.cloud:spring-cloud-starter-aws:2.2.6.RELEASE'
Comment on lines +52 to +54
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix Spring Cloud AWS version compatibility with Spring Boot 3.x.

The current Spring Cloud AWS starter version (2.2.6.RELEASE) is not compatible with Spring Boot 3.x. This will cause runtime issues.

Replace with the compatible AWS SDK dependency:

-    implementation 'org.springframework.cloud:spring-cloud-starter-aws:2.2.6.RELEASE'
+    implementation 'io.awspring.cloud:spring-cloud-aws-starter-s3:3.0.2'

This version is specifically designed for Spring Boot 3.x and provides better integration with AWS S3.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// aws s3
implementation 'org.springframework.cloud:spring-cloud-starter-aws:2.2.6.RELEASE'
// aws s3
implementation 'io.awspring.cloud:spring-cloud-aws-starter-s3:3.0.2'

}

tasks.named('test') {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
package com.yedu.backend.domain.teacher.application.dto.req;

public record TeacherProfileFormRequest(
String phoneNumber
) {
}
Original file line number Diff line number Diff line change
@@ -1,11 +1,16 @@
package com.yedu.backend.domain.teacher.application.usecase;

import com.yedu.backend.domain.teacher.application.dto.req.TeacherInfoFormRequest;
import com.yedu.backend.domain.teacher.application.dto.req.TeacherProfileFormRequest;
import com.yedu.backend.domain.teacher.domain.entity.*;
import com.yedu.backend.domain.teacher.domain.service.TeacherGetService;
import com.yedu.backend.domain.teacher.domain.service.TeacherSaveService;
import com.yedu.backend.domain.teacher.domain.service.TeacherUpdateService;
import com.yedu.backend.global.config.s3.S3UploadService;
import lombok.RequiredArgsConstructor;
import org.springframework.stereotype.Service;
import org.springframework.transaction.annotation.Transactional;
import org.springframework.web.multipart.MultipartFile;

import java.util.ArrayList;
import java.util.List;
Expand All @@ -17,6 +22,9 @@
@Transactional
public class TeacherManageUseCase {
private final TeacherSaveService teacherSaveService;
private final TeacherGetService teacherGetService;
private final TeacherUpdateService teacherUpdateService;
private final S3UploadService s3UploadService;

public void saveTeacher(TeacherInfoFormRequest request) {
Teacher teacher = mapToTeacher(request); // 기본 선생님 정보
Expand Down Expand Up @@ -65,4 +73,10 @@ private List<TeacherDistrict> getTeacherDistricts(TeacherInfoFormRequest request
.map(region -> mapToTeacherDistrict(teacher, region))
.toList(); // 선생님 교육 가능 구역
}

public void saveTeacherProfile(MultipartFile profile, TeacherProfileFormRequest request) {
String profileUrl = s3UploadService.saveProfileFile(profile);
Teacher teacher = teacherGetService.byPhoneNumber(request.phoneNumber());
teacherUpdateService.updateProfile(teacher, profileUrl);
}
Comment on lines +77 to +81
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add error handling for file upload and teacher lookup.

The method needs to handle potential failure scenarios:

  1. Invalid or empty profile file
  2. S3 upload failures
  3. Teacher not found by phone number

Consider adding proper error handling:

 public void saveTeacherProfile(MultipartFile profile, TeacherProfileFormRequest request) {
+    if (profile == null || profile.isEmpty()) {
+        throw new IllegalArgumentException("Profile image is required");
+    }
+    
     String profileUrl = s3UploadService.saveProfileFile(profile);
     Teacher teacher = teacherGetService.byPhoneNumber(request.phoneNumber());
+    
+    if (teacher == null) {
+        throw new EntityNotFoundException("Teacher not found with phone number: " + request.phoneNumber());
+    }
+    
     teacherUpdateService.updateProfile(teacher, profileUrl);
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
public void saveTeacherProfile(MultipartFile profile, TeacherProfileFormRequest request) {
String profileUrl = s3UploadService.saveProfileFile(profile);
Teacher teacher = teacherGetService.byPhoneNumber(request.phoneNumber());
teacherUpdateService.updateProfile(teacher, profileUrl);
}
public void saveTeacherProfile(MultipartFile profile, TeacherProfileFormRequest request) {
if (profile == null || profile.isEmpty()) {
throw new IllegalArgumentException("Profile image is required");
}
String profileUrl = s3UploadService.saveProfileFile(profile);
Teacher teacher = teacherGetService.byPhoneNumber(request.phoneNumber());
if (teacher == null) {
throw new EntityNotFoundException("Teacher not found with phone number: " + request.phoneNumber());
}
teacherUpdateService.updateProfile(teacher, profileUrl);
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -50,4 +50,8 @@ public class Teacher extends BaseEntity {
private int alertMessageCount; //알림톡 발송 횟수
private double responseRate; //답변율
private double responseTime; //답변 평균 시간

public void updateProfile(String profile) {
teacherClassInfo.updateProfile(profile);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -39,4 +39,9 @@ public class TeacherClassInfo {

private String profile; //프로필 사진
private String video; // 과외 영상

public void updateProfile(String profile) {
this.profile = profile;
//todo : 나중에 과외 영상 추가 필요
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
import com.yedu.backend.domain.teacher.domain.entity.Teacher;
import org.springframework.data.jpa.repository.JpaRepository;

public interface TeacherRepository extends JpaRepository<Teacher, Long> {
import java.util.Optional;

public interface TeacherRepository extends JpaRepository<Teacher, Long> {
Optional<Teacher> findByTeacherInfo_PhoneNumber(String PhoneNumber);
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Fix parameter naming convention and consider adding an index.

  1. Parameter name should be camelCase
  2. Consider adding an index on the phone_number column for better query performance

Apply this diff:

-    Optional<Teacher> findByTeacherInfo_PhoneNumber(String PhoneNumber);
+    Optional<Teacher> findByTeacherInfo_PhoneNumber(String phoneNumber);

Add the following annotation to the phoneNumber field in TeacherInfo class:

@Column(name = "phone_number", index = true)
private String phoneNumber;

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
package com.yedu.backend.domain.teacher.domain.service;

import com.yedu.backend.domain.teacher.domain.entity.Teacher;
import com.yedu.backend.domain.teacher.domain.repository.TeacherRepository;
import lombok.RequiredArgsConstructor;
import org.springframework.stereotype.Service;

@Service
@RequiredArgsConstructor
public class TeacherGetService {
private final TeacherRepository teacherRepository;

public Teacher byPhoneNumber(String phoneNumber) {
return teacherRepository.findByTeacherInfo_PhoneNumber(phoneNumber)
.orElseThrow();
}
Comment on lines +13 to +16
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Improve error handling and add phone number validation.

  1. Use custom exception with meaningful message
  2. Validate phone number format before query
  3. Add logging for debugging purposes

Apply this diff:

+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
 @Service
 @RequiredArgsConstructor
 public class TeacherGetService {
+    private static final Logger log = LoggerFactory.getLogger(TeacherGetService.class);
     private final TeacherRepository teacherRepository;
 
     public Teacher byPhoneNumber(String phoneNumber) {
+        if (phoneNumber == null || !phoneNumber.matches("^\\d{10,11}$")) {
+            throw new IllegalArgumentException("Invalid phone number format");
+        }
+        log.debug("Searching for teacher with phone number: {}", phoneNumber);
         return teacherRepository.findByTeacherInfo_PhoneNumber(phoneNumber)
-                .orElseThrow();
+                .orElseThrow(() -> 
+                    new TeacherNotFoundException("Teacher not found with phone number: " + phoneNumber));
     }
 }

Committable suggestion skipped: line range outside the PR's diff.

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
package com.yedu.backend.domain.teacher.domain.service;

import com.yedu.backend.domain.teacher.domain.entity.Teacher;
import org.springframework.stereotype.Service;

@Service
public class TeacherUpdateService {
public void updateProfile(Teacher teacher, String profile) {
teacher.updateProfile(profile);
}
}
Comment on lines +6 to +11
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add transaction management and error handling.

The service implementation should include:

  1. @Transactional annotation for database operations
  2. Error handling for failed profile updates

Apply this diff:

 @Service
+@Transactional
 public class TeacherUpdateService {
     public void updateProfile(Teacher teacher, String profile) {
+        if (teacher == null) {
+            throw new IllegalArgumentException("Teacher cannot be null");
+        }
+        if (profile == null || profile.isBlank()) {
+            throw new IllegalArgumentException("Profile cannot be null or blank");
+        }
         teacher.updateProfile(profile);
     }
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
@Service
public class TeacherUpdateService {
public void updateProfile(Teacher teacher, String profile) {
teacher.updateProfile(profile);
}
}
@Service
@Transactional
public class TeacherUpdateService {
public void updateProfile(Teacher teacher, String profile) {
if (teacher == null) {
throw new IllegalArgumentException("Teacher cannot be null");
}
if (profile == null || profile.isBlank()) {
throw new IllegalArgumentException("Profile cannot be null or blank");
}
teacher.updateProfile(profile);
}
}

Original file line number Diff line number Diff line change
@@ -1,13 +1,12 @@
package com.yedu.backend.domain.teacher.presentation;

import com.yedu.backend.domain.teacher.application.dto.req.TeacherInfoFormRequest;
import com.yedu.backend.domain.teacher.application.dto.req.TeacherProfileFormRequest;
import com.yedu.backend.domain.teacher.application.usecase.TeacherManageUseCase;
import lombok.RequiredArgsConstructor;
import org.springframework.http.ResponseEntity;
import org.springframework.web.bind.annotation.PostMapping;
import org.springframework.web.bind.annotation.RequestBody;
import org.springframework.web.bind.annotation.RequestMapping;
import org.springframework.web.bind.annotation.RestController;
import org.springframework.web.bind.annotation.*;
import org.springframework.web.multipart.MultipartFile;

@RestController
@RequestMapping("/teacher")
Expand All @@ -20,4 +19,10 @@ public ResponseEntity saveByForm(@RequestBody TeacherInfoFormRequest request) {
manageUseCase.saveTeacher(request);
return ResponseEntity.ok().build();
}

@PostMapping("/form/profile")
public ResponseEntity saveProfileByForm(@RequestPart TeacherProfileFormRequest request, @RequestPart MultipartFile profile) {
manageUseCase.saveTeacherProfile(profile, request);
return ResponseEntity.ok().build();
}
Comment on lines +23 to +27
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add validation, documentation and proper response type.

  1. Add file size and type validation
  2. Add Swagger/OpenAPI documentation
  3. Specify response type and error responses

Apply this diff:

+    @Operation(summary = "Save teacher profile from form submission")
+    @ApiResponses(value = {
+        @ApiResponse(responseCode = "200", description = "Profile saved successfully"),
+        @ApiResponse(responseCode = "400", description = "Invalid request"),
+        @ApiResponse(responseCode = "404", description = "Teacher not found"),
+        @ApiResponse(responseCode = "500", description = "Internal server error")
+    })
     @PostMapping("/form/profile")
-    public ResponseEntity saveProfileByForm(@RequestPart TeacherProfileFormRequest request, @RequestPart MultipartFile profile) {
+    public ResponseEntity<Void> saveProfileByForm(
+            @RequestPart @Valid TeacherProfileFormRequest request,
+            @RequestPart @ValidFile(maxSize = 5 * 1024 * 1024, types = {"image/jpeg", "image/png"}) MultipartFile profile) {
         manageUseCase.saveTeacherProfile(profile, request);
         return ResponseEntity.ok().build();
     }

Create a custom validator annotation:

@Target({ElementType.PARAMETER})
@Retention(RetentionPolicy.RUNTIME)
@Constraint(validatedBy = FileValidator.class)
public @interface ValidFile {
    String message() default "Invalid file";
    Class<?>[] groups() default {};
    Class<? extends Payload>[] payload() default {};
    long maxSize() default 1024 * 1024;
    String[] types() default {};
}

}
31 changes: 31 additions & 0 deletions src/main/java/com/yedu/backend/global/config/s3/S3Config.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
package com.yedu.backend.global.config.s3;

import com.amazonaws.auth.AWSCredentials;
import com.amazonaws.auth.AWSStaticCredentialsProvider;
import com.amazonaws.auth.BasicAWSCredentials;
import com.amazonaws.services.s3.AmazonS3Client;
import com.amazonaws.services.s3.AmazonS3ClientBuilder;
import org.springframework.beans.factory.annotation.Value;
import org.springframework.context.annotation.Bean;
import org.springframework.context.annotation.Configuration;

@Configuration
public class S3Config {

@Value("${cloud.aws.credentials.access-key}")
public String accessKey;
@Value("${cloud.aws.credentials.secret-key}")
public String secretKey;
@Value("${cloud.aws.region.static}")
public String region;
Comment on lines +15 to +20
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Enhance security by making fields private and using AWS credential provider chain.

The configuration fields should not be public. Additionally, consider using AWS SDK's default credential provider chain instead of explicitly storing credentials.

Apply this diff to improve security:

-    public String accessKey;
-    public String secretKey;
-    public String region;
+    private String accessKey;
+    private String secretKey;
+    private String region;

Consider using AWS SDK's default credential provider chain by:

  1. Setting up AWS credentials in ~/.aws/credentials
  2. Using environment variables (AWS_ACCESS_KEY_ID, AWS_SECRET_ACCESS_KEY)
  3. Using IAM roles for EC2 instances
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
@Value("${cloud.aws.credentials.access-key}")
public String accessKey;
@Value("${cloud.aws.credentials.secret-key}")
public String secretKey;
@Value("${cloud.aws.region.static}")
public String region;
@Value("${cloud.aws.credentials.access-key}")
private String accessKey;
@Value("${cloud.aws.credentials.secret-key}")
private String secretKey;
@Value("${cloud.aws.region.static}")
private String region;


@Bean
public AmazonS3Client amazonS3Client() {
AWSCredentials credentials = new BasicAWSCredentials(accessKey, secretKey);
return (AmazonS3Client)
AmazonS3ClientBuilder.standard()
.withCredentials(new AWSStaticCredentialsProvider(credentials))
.withRegion(region)
.build();
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
package com.yedu.backend.global.config.s3;

import com.amazonaws.services.s3.AmazonS3;
import com.amazonaws.services.s3.model.ObjectMetadata;
import lombok.RequiredArgsConstructor;
import org.springframework.beans.factory.annotation.Value;
import org.springframework.stereotype.Service;
import org.springframework.web.multipart.MultipartFile;

import java.io.IOException;
import java.io.UnsupportedEncodingException;
import java.net.URLDecoder;
import java.util.UUID;

@Service
@RequiredArgsConstructor
public class S3UploadService {

private final AmazonS3 amazonS3;

@Value("${cloud.aws.s3.bucket}")
private String bucket;
@Value("${cloud.aws.s3.prefix-profile}")
private String profile;

public String saveProfileFile(MultipartFile multipartFile) {
return getString(multipartFile, profile);
}

private String getString(MultipartFile multipartFile, String dir) {
try {
String originalFilename = multipartFile.getOriginalFilename();
String fileName = dir +"/" + originalFilename + UUID.randomUUID();

ObjectMetadata metadata = new ObjectMetadata();
metadata.setContentLength(multipartFile.getSize());
metadata.setContentType(multipartFile.getContentType());

amazonS3.putObject(bucket, fileName, multipartFile.getInputStream(), metadata);
return amazonS3.getUrl(bucket, fileName).toString();
} catch (IOException ex) {
throw new IllegalArgumentException(); //todo : 적절한 예외 처리
}
}
Comment on lines +30 to +44
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Improve file upload security and error handling.

Several improvements needed in the file upload logic:

  1. File name collision possible with UUID appended after original name
  2. Missing file size limits
  3. Generic exception handling
  4. Missing content type validation

Apply these improvements:

     private String getString(MultipartFile multipartFile, String dir) {
+        if (multipartFile.getSize() > 5_000_000) { // 5MB limit
+            throw new FileSizeLimitExceededException("File size exceeds limit", multipartFile.getSize(), 5_000_000);
+        }
+        String contentType = multipartFile.getContentType();
+        if (contentType == null || !contentType.startsWith("image/")) {
+            throw new InvalidFileTypeException("Only image files are allowed");
+        }
         try {
             String originalFilename = multipartFile.getOriginalFilename();
-            String fileName = dir +"/" + originalFilename + UUID.randomUUID();
+            String extension = originalFilename.substring(originalFilename.lastIndexOf("."));
+            String fileName = dir + "/" + UUID.randomUUID() + extension;
 
             ObjectMetadata metadata = new ObjectMetadata();
             metadata.setContentLength(multipartFile.getSize());
             metadata.setContentType(multipartFile.getContentType());
 
             amazonS3.putObject(bucket, fileName, multipartFile.getInputStream(), metadata);
             return amazonS3.getUrl(bucket, fileName).toString();
         } catch (IOException ex) {
-            throw new IllegalArgumentException(); //todo : 적절한 예외 처리
+            throw new FileUploadException("Failed to upload file to S3", ex);
         }
     }

Committable suggestion skipped: line range outside the PR's diff.


public void deleteProfileImage(String url) {
try {
String decodedUrl = URLDecoder.decode(url, "UTF-8");
String fileName = decodedUrl.substring(decodedUrl.lastIndexOf('/') + 1);
amazonS3.deleteObject(bucket + "/" + profile, fileName);
} catch (UnsupportedEncodingException e) {
throw new IllegalArgumentException(); //todo : 적절한 예외 처리
}
}
Comment on lines +46 to +54
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix path construction in deleteProfileImage method.

The bucket path construction is incorrect and the error handling needs improvement.

Apply this diff:

     public void deleteProfileImage(String url)  {
         try {
             String decodedUrl = URLDecoder.decode(url, "UTF-8");
             String fileName = decodedUrl.substring(decodedUrl.lastIndexOf('/') + 1);
-            amazonS3.deleteObject(bucket + "/" + profile, fileName);
+            amazonS3.deleteObject(bucket, profile + "/" + fileName);
         } catch (UnsupportedEncodingException e) {
-            throw new IllegalArgumentException(); //todo : 적절한 예외 처리
+            throw new FileDeleteException("Failed to decode file URL", e);
+        } catch (AmazonS3Exception e) {
+            throw new FileDeleteException("Failed to delete file from S3", e);
         }
     }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
public void deleteProfileImage(String url) {
try {
String decodedUrl = URLDecoder.decode(url, "UTF-8");
String fileName = decodedUrl.substring(decodedUrl.lastIndexOf('/') + 1);
amazonS3.deleteObject(bucket + "/" + profile, fileName);
} catch (UnsupportedEncodingException e) {
throw new IllegalArgumentException(); //todo : 적절한 예외 처리
}
}
public void deleteProfileImage(String url) {
try {
String decodedUrl = URLDecoder.decode(url, "UTF-8");
String fileName = decodedUrl.substring(decodedUrl.lastIndexOf('/') + 1);
amazonS3.deleteObject(bucket, profile + "/" + fileName);
} catch (UnsupportedEncodingException e) {
throw new FileDeleteException("Failed to decode file URL", e);
} catch (AmazonS3Exception e) {
throw new FileDeleteException("Failed to delete file from S3", e);
}
}

}
Loading