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

๐Ÿ„ :: (Meogo-48) modify my page #49

Merged
merged 5 commits into from
Oct 8, 2024
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
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package org.meogo.domain.review.service
import org.meogo.domain.review.domain.ReviewRepository
import org.meogo.domain.review.presentation.dto.response.ReviewListResponse
import org.meogo.domain.review.presentation.dto.response.ReviewResponse
import org.meogo.domain.user.exception.UserNotFoundException
import org.meogo.domain.user.facade.UserFacade
import org.meogo.global.s3.FileUtil
import org.meogo.global.s3.Path
Expand All @@ -23,7 +24,7 @@ class QueryAllBySchoolIdService(
return ReviewListResponse(
count = reviews.size,
reviews = reviews.map { review ->
val user = userFacade.getUserById(review.userId)
val user = userFacade.getUserById(review.userId) ?: throw UserNotFoundException
val profile = fileUtil.generateObjectUrl(user.profile, Path.USER)
val image = review.picture?.split(",")?.map { pic ->
fileUtil.generateObjectUrl(pic.trim(), Path.REVIEW)
Expand Down
12 changes: 8 additions & 4 deletions src/main/kotlin/org/meogo/domain/user/domain/User.kt
Original file line number Diff line number Diff line change
Expand Up @@ -12,23 +12,27 @@ class User(
id: UUID? = null,

@Column(nullable = false, length = 4)
val name: String,
var name: String,

@Column(name = "account_id", nullable = false, length = 15, unique = true)
val accountId: String,

val password: String,

@Column(name = "enrolled_school", nullable = true)
val enrolledSchool: Int? = 0,
var enrolledSchool: Int? = 0,

var profile: String,

@Enumerated(EnumType.STRING)
val role: UserRole
) : BaseUUIDEntity(id) {
fun updateProfile(profile: String): User {
this.profile = profile
fun updateProfile(name: String, enrolledSchool: Int, profile: String?): User {
this.name = name
this.enrolledSchool = enrolledSchool
if (profile != null) {
this.profile = profile
}
meltapplee marked this conversation as resolved.
Show resolved Hide resolved
return this
}
}
Original file line number Diff line number Diff line change
@@ -1,12 +1,14 @@
package org.meogo.domain.user.domain

import org.springframework.data.jpa.repository.JpaRepository
import org.springframework.data.repository.Repository
meltapplee marked this conversation as resolved.
Show resolved Hide resolved
import java.util.UUID

interface UserRepository : JpaRepository<User, UUID> {
interface UserRepository : Repository<User, UUID> {
meltapplee marked this conversation as resolved.
Show resolved Hide resolved
fun save(entity: User): User

fun findByAccountId(accountId: String): User?
fun findById(id: UUID): User?

fun findByAccountId(accountId: String): User
Copy link

Choose a reason for hiding this comment

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

โš ๏ธ Potential issue

findByAccountId ๋ฉ”์„œ๋“œ์˜ ๋ฐ˜ํ™˜ ํƒ€์ž… ๋ณ€๊ฒฝ์— ๋Œ€ํ•œ ์šฐ๋ ค

findByAccountId ๋ฉ”์„œ๋“œ์˜ ๋ฐ˜ํ™˜ ํƒ€์ž…์„ User?์—์„œ User๋กœ ๋ณ€๊ฒฝํ•œ ๊ฒƒ์€ ์ž ์žฌ์ ์ธ ๋ฌธ์ œ๋ฅผ ์•ผ๊ธฐํ•  ์ˆ˜ ์žˆ์Šต๋‹ˆ๋‹ค:

  • ์ด ๋ณ€๊ฒฝ์€ ๋ฉ”์„œ๋“œ๊ฐ€ ํ•ญ์ƒ User ๊ฐ์ฒด๋ฅผ ๋ฐ˜ํ™˜ํ•œ๋‹ค๊ณ  ๊ฐ€์ •ํ•ฉ๋‹ˆ๋‹ค.
  • ์กด์žฌํ•˜์ง€ ์•Š๋Š” accountId๋กœ ์กฐํšŒํ•  ๊ฒฝ์šฐ, ์˜ˆ๊ธฐ์น˜ ์•Š์€ NullPointerException์ด ๋ฐœ์ƒํ•  ์ˆ˜ ์žˆ์Šต๋‹ˆ๋‹ค.
  • Kotlin์˜ null ์•ˆ์ „์„ฑ ์›์น™์— ์œ„๋ฐฐ๋ฉ๋‹ˆ๋‹ค.

๋‹ค์Œ๊ณผ ๊ฐ™์ด ์ˆ˜์ •์„ ์ œ์•ˆํ•ฉ๋‹ˆ๋‹ค:

-fun findByAccountId(accountId: String): User
+fun findByAccountId(accountId: String): User?

์ด๋ ‡๊ฒŒ ํ•˜๋ฉด accountId์— ํ•ด๋‹นํ•˜๋Š” ์‚ฌ์šฉ์ž๊ฐ€ ์—†์„ ๊ฒฝ์šฐ๋ฅผ ์•ˆ์ „ํ•˜๊ฒŒ ์ฒ˜๋ฆฌํ•  ์ˆ˜ ์žˆ์Šต๋‹ˆ๋‹ค.

๐Ÿ“ 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
fun findByAccountId(accountId: String): User
fun findByAccountId(accountId: String): User?


fun existsByAccountId(accountId: String): Boolean
}
9 changes: 4 additions & 5 deletions src/main/kotlin/org/meogo/domain/user/facade/UserFacade.kt
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package org.meogo.domain.user.facade

import org.meogo.domain.user.domain.User
import org.meogo.domain.user.domain.UserRepository
import org.meogo.domain.user.exception.UserNotFoundException
import org.springframework.security.core.context.SecurityContextHolder
import org.springframework.stereotype.Component
import java.util.UUID
Expand All @@ -17,9 +16,9 @@ class UserFacade(
return accountId?.let { getUserByAccountId(it) }
}

fun getUserByAccountId(accountId: String): User =
userRepository.findByAccountId(accountId) ?: throw UserNotFoundException
fun getUserByAccountId(accountId: String): User? =
userRepository.findByAccountId(accountId)
meltapplee marked this conversation as resolved.
Show resolved Hide resolved

fun getUserById(id: UUID): User =
userRepository.findById(id).orElseThrow { UserNotFoundException }
fun getUserById(id: UUID): User? =
userRepository.findById(id)
meltapplee marked this conversation as resolved.
Show resolved Hide resolved
}
Original file line number Diff line number Diff line change
@@ -1,12 +1,13 @@
package org.meogo.domain.user.presentation

import org.meogo.domain.user.presentation.dto.request.UserCheckRequest
import org.meogo.domain.user.presentation.dto.request.UserModifyRequest
import org.meogo.domain.user.presentation.dto.request.UserSignInRequest
import org.meogo.domain.user.presentation.dto.request.UserSignUpRequest
import org.meogo.domain.user.presentation.dto.response.MyPageResponse
import org.meogo.domain.user.service.CheckAccountIdService
import org.meogo.domain.user.service.MyPageService
import org.meogo.domain.user.service.UploadProfileService
import org.meogo.domain.user.service.ModifyUserInfoService
import org.meogo.domain.user.service.QueryMyPageService
import org.meogo.domain.user.service.UserSignInService
import org.meogo.domain.user.service.UserSignUpService
import org.meogo.global.jwt.dto.TokenResponse
Expand All @@ -28,8 +29,8 @@ class UserController(
private val userSignUpService: UserSignUpService,
private val userSignInService: UserSignInService,
private val userCheckAccountIdService: CheckAccountIdService,
private val myPageService: MyPageService,
private val uploadProfileService: UploadProfileService
private val queryMyPageService: QueryMyPageService,
private val modifyUserInfoService: ModifyUserInfoService
) {
@PostMapping("/signup")
@ResponseStatus(HttpStatus.CREATED)
Expand All @@ -49,9 +50,13 @@ class UserController(

@GetMapping("/my")
fun myPage(): MyPageResponse =
myPageService.execute()
queryMyPageService.execute()

@PatchMapping("/profile")
fun updateProfile(@RequestPart(name = "image") file: MultipartFile) =
uploadProfileService.uploadProfile(file)
@PatchMapping("/modify")
fun updateProfile(
@RequestPart(name = "image")
file: MultipartFile?,
@RequestPart(name = "request")
request: UserModifyRequest
) = modifyUserInfoService.execute(request, file)
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
package org.meogo.domain.user.presentation.dto.request

data class UserModifyRequest(
val name: String,
val enrolledSchool: Int
meltapplee marked this conversation as resolved.
Show resolved Hide resolved
)
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package org.meogo.domain.user.service
import org.meogo.domain.user.domain.UserRepository
import org.meogo.domain.user.exception.UserNotFoundException
import org.meogo.domain.user.facade.UserFacade
import org.meogo.domain.user.presentation.dto.request.UserModifyRequest
import org.meogo.global.s3.FileUtil
import org.meogo.global.s3.Path
import org.springframework.beans.factory.annotation.Value
Expand All @@ -11,22 +12,23 @@ import org.springframework.transaction.annotation.Transactional
import org.springframework.web.multipart.MultipartFile

@Service
class UploadProfileService(
private val fileUtil: FileUtil,
private val userFacade: UserFacade,
class ModifyUserInfoService(
private val userRepository: UserRepository,
private val userFacade: UserFacade,
@Value("\${cloud.aws.s3.default-image}")
private val defaultImage: String

private val defaultImage: String,
private val fileUtil: FileUtil
) {

@Transactional
fun uploadProfile(file: MultipartFile) {
fun execute(request: UserModifyRequest, file: MultipartFile?) {
val user = userFacade.currentUser() ?: throw UserNotFoundException
Copy link

Choose a reason for hiding this comment

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

๐Ÿ› ๏ธ Refactor suggestion

UserNotFoundException ์ฒ˜๋ฆฌ ๊ฐœ์„  ๊ถŒ์žฅ

ํ˜„์žฌ UserNotFoundException์„ ๋˜์ง€๋Š” ๋ฐฉ์‹์ด ๊ฐœ์„ ๋˜์—ˆ์ง€๋งŒ, ๋” ๋‚˜์€ ์—๋Ÿฌ ์ฒ˜๋ฆฌ๋ฅผ ์œ„ํ•ด ๋‹ค์Œ๊ณผ ๊ฐ™์ด ์ˆ˜์ •ํ•˜๋Š” ๊ฒƒ์ด ์ข‹๊ฒ ์Šต๋‹ˆ๋‹ค:

val user = userFacade.currentUser() ?: throw UserNotFoundException()

์ด๋ ‡๊ฒŒ ํ•˜๋ฉด ์˜ˆ์™ธ์— ์ถ”๊ฐ€ ์ •๋ณด๋ฅผ ํฌํ•จ์‹œํ‚ฌ ์ˆ˜ ์žˆ๊ณ , ์˜ˆ์™ธ ์ฒ˜๋ฆฌ์˜ ์ผ๊ด€์„ฑ์„ ์œ ์ง€ํ•  ์ˆ˜ ์žˆ์Šต๋‹ˆ๋‹ค.


if (user.profile != defaultImage) fileUtil.delete(user.profile, Path.USER)
val url = fileUtil.upload(file, Path.USER)
user.updateProfile(url)
userRepository.save(user)
if (file != null && user.profile != defaultImage) fileUtil.delete(user.profile, Path.USER)
Copy link

Choose a reason for hiding this comment

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

๐Ÿ› ๏ธ Refactor suggestion

ํ”„๋กœํ•„ ์ด๋ฏธ์ง€ ์‚ญ์ œ ์กฐ๊ฑด ๊ฐœ์„  ์ œ์•ˆ

ํ˜„์žฌ ๊ตฌํ˜„์€ ์ด์ „ ๋ฆฌ๋ทฐ ์˜๊ฒฌ์„ ๋ถ€๋ถ„์ ์œผ๋กœ ๋ฐ˜์˜ํ–ˆ์ง€๋งŒ, ๋” ๊ฐœ์„ ํ•  ์ˆ˜ ์žˆ์Šต๋‹ˆ๋‹ค. ๋‹ค์Œ๊ณผ ๊ฐ™์ด ์ˆ˜์ •ํ•˜๋Š” ๊ฒƒ์ด ์ข‹๊ฒ ์Šต๋‹ˆ๋‹ค:

if (file != null && user.profile != defaultImage) {
    fileUtil.delete(user.profile, Path.USER)
}

์ด๋ ‡๊ฒŒ ํ•˜๋ฉด ์ฝ”๋“œ์˜ ๊ฐ€๋…์„ฑ์ด ํ–ฅ์ƒ๋˜๊ณ , ์กฐ๊ฑด์˜ ์˜๋„๊ฐ€ ๋” ๋ช…ํ™•ํ•ด์ง‘๋‹ˆ๋‹ค.

val url = file?.let { fileUtil.upload(it, Path.USER) }

userRepository.save(
user.updateProfile(request.name, request.enrolledSchool, url)
)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import org.springframework.stereotype.Service
import org.springframework.transaction.annotation.Transactional

@Service
class MyPageService(
class QueryMyPageService(
private val userFacade: UserFacade

) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package org.meogo.domain.user.service

import org.meogo.domain.user.domain.UserRepository
import org.meogo.domain.user.exception.PasswordMismatchException
import org.meogo.domain.user.exception.UserNotFoundException
import org.meogo.domain.user.presentation.dto.request.UserSignInRequest
import org.meogo.global.jwt.JwtTokenProvider
import org.meogo.global.jwt.dto.TokenResponse
Expand All @@ -19,7 +18,6 @@ class UserSignInService(
@Transactional
fun execute(request: UserSignInRequest): TokenResponse {
val user = userRepository.findByAccountId(request.accountId)
?: throw UserNotFoundException

if (!passwordEncoder.matches(request.password, user.password)) throw PasswordMismatchException

Expand Down
3 changes: 2 additions & 1 deletion src/main/kotlin/org/meogo/global/auth/AuthDetailsService.kt
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package org.meogo.global.auth

import org.meogo.domain.user.exception.UserNotFoundException
import org.meogo.domain.user.facade.UserFacade
import org.springframework.security.core.userdetails.UserDetails
import org.springframework.security.core.userdetails.UserDetailsService
Expand All @@ -10,7 +11,7 @@ class AuthDetailsService(
private val userFacade: UserFacade
) : UserDetailsService {
override fun loadUserByUsername(username: String): UserDetails {
val user = userFacade.getUserByAccountId(username)
val user = userFacade.getUserByAccountId(username) ?: throw UserNotFoundException
return AuthDetails(user.accountId, user.role)
}
}
Loading