[3단계 - Navigation & Flow | 4단계 - 주문] 허닛 미션 제출합니다.#181
Open
BaekCCI wants to merge 54 commits into
Open
Conversation
Summary by CodeRabbit릴리스 노트
Walkthrough이 PR은 Android Shopping 앱의 네비게이션 아키텍처를 Activity 기반에서 Compose Navigation 기반으로 전환합니다. MainActivity가 단일 진입점이 되어 ShoppingNavHost를 통해 모든 화면을 관리하며, 기존의 CartActivity, ProductDetailActivity, ProductListActivity를 제거하고 Compose 기반으로 통합합니다. 동시에 새로운 쿠폰 시스템(도메인 모델부터 UI까지)과 주문 알림 시스템(AlarmManager, BroadcastReceiver, NotificationManager)을 구현하고, 알림 설정 화면을 추가합니다. 저장소 구현체명을 정리하고, 의존성 구조를 재정의합니다. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (5)
app/src/main/java/woowacourse/shopping/di/AppContainer.kt (1)
64-74: 💤 Low value저장소 타입을 인터페이스로 선언하는 것을 고려해 보세요.
couponRepository가CouponRepositoryImpl구체 타입으로 선언되어 있습니다.cartRepository,productRepository,recentProductRepository는 인터페이스 타입(CartRepository,ProductRepository,RecentProductRepository)으로 선언된 것과 비교하면 일관성이 떨어집니다.
CouponRepository와NotificationRepository인터페이스가 존재한다면, 이들도 인터페이스 타입으로 선언하는 것이 의존성 역전 원칙에 더 부합합니다. 이 부분에 대해 어떻게 생각하시나요?🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/woowacourse/shopping/di/AppContainer.kt` around lines 64 - 74, Declare repository variables using their interface types instead of concrete implementations: change the declaration of couponRepository from CouponRepositoryImpl to the interface CouponRepository and notificationRepository from NotificationRepositoryImpl to NotificationRepository while still instantiating them with CouponRepositoryImpl(couponDataSource) and NotificationRepositoryImpl(context.getSharedPreferences(...)); this matches how CartRepository, ProductRepository, and RecentProductRepository are typed and follows dependency inversion (refer to couponRepository, CouponRepositoryImpl, notificationRepository, NotificationRepositoryImpl, CartRepository).app/src/main/java/woowacourse/shopping/MainActivity.kt (1)
49-53: 💤 Low value권한 요청 거부 시 사용자 안내가 누락되어 있습니다.
shouldShowRequestPermissionRationale이true를 반환하면 사용자가 이전에 권한 요청을 거부한 것입니다. 이 경우 왜 알림 권한이 필요한지 설명하는 UI(예: 다이얼로그)를 표시한 후 다시 권한을 요청하는 것이 일반적인 패턴입니다.현재 구현은 이 경우를 무시하므로, 한 번 거부한 사용자는 다시 권한 요청을 받지 못합니다. 이 부분을 어떻게 처리할지 고민해 보시겠어요?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/woowacourse/shopping/MainActivity.kt` around lines 49 - 53, When shouldShowRequestPermissionRationale(Manifest.permission.POST_NOTIFICATIONS) returns true you must show an explanatory UI before re-requesting permission; implement a dialog (e.g., in MainActivity) that explains why POST_NOTIFICATIONS is needed and offers actions to retry (call requestPermissionLauncher.launch(Manifest.permission.POST_NOTIFICATIONS)) or cancel, and if the user selected "Don't ask again" send them to app settings via an intent; update the branch handling in MainActivity to display this rationale dialog instead of doing nothing and to handle both retry and open-settings flows.app/src/main/java/woowacourse/shopping/constant/Format.kt (1)
6-11: 💤 Low value
formatDiscountPrice의 0원 처리 방식을 검토해보시면 좋을 것 같습니다.현재 구현에서 할인 금액이 0원일 때는
"0원"을 반환하고, 0보다 클 때는"-%,d원"형식으로 마이너스 기호를 붙여 반환하고 있습니다.몇 가지 고려해볼 점이 있습니다:
- 할인이 없을 때(0원) 화면에 어떻게 표시하고 싶으신가요? "0원"을 보여주는 것이 적절한지, 아니면 호출하는 쪽에서 0원일 때는 이 함수를 호출하지 않도록 하는게 나을지 생각해보면 좋을 것 같습니다.
- 음수 입력에 대한 처리는 어떻게 하실 계획인가요? 현재는 음수가 들어오면
"--1000원"같은 형태가 될 수 있습니다.UI에서 할인 금액을 어떻게 표현하고 싶으신지에 따라 이 함수의 책임 범위를 다시 생각해보시면 도움이 될 것 같습니다.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/woowacourse/shopping/constant/Format.kt` around lines 6 - 11, formatDiscountPrice currently returns "0원" for zero and can produce a double-minus for negative inputs; change it so that when price == 0 it returns an empty string (so callers can omit displaying a discount) and for non-zero values format using the absolute value with a single leading "-" only when price > 0 (use kotlin.math.abs on price and conditionally prefix "-" for positive amounts) in the formatDiscountPrice function to avoid "--1000원" and to let callers handle the absence of a discount.app/src/main/java/woowacourse/shopping/data/repository/CartRepositoryImpl.kt (1)
49-54: ⚡ Quick win
in연산자의 시간 복잡도를 고려해 보셨나요?
cartIds가List<Int>일 때,it.id in cartIds는 내부적으로List.contains()를 호출하게 됩니다. 이 경우 각 lookup마다 O(n) 시간이 소요되어, 전체 필터링의 시간 복잡도가 O(cartItems × cartIds)가 됩니다.
- 현재 데이터 규모에서는 문제가 없을 수 있지만,
cartIds의 크기가 커질 경우 성능에 영향을 줄 수 있습니다.- 어떤 자료구조로 변환하면 lookup 성능을 O(1)로 개선할 수 있을까요?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/woowacourse/shopping/data/repository/CartRepositoryImpl.kt` around lines 49 - 54, The current getCartItemsByIds implementation uses "it.id in cartIds" which does repeated O(n) list lookups; convert the incoming cartIds List<Int> to a Set<Int> (e.g., val idSet = cartIds.toSet()) at the start of the getCartItemsByIds function and then filter using idSet.contains(it.id) so lookups become O(1); keep the existing refreshCartItems() and return logic unchanged and reference _cartItems and CartItems as before.app/src/main/java/woowacourse/shopping/notification/AlarmScheduler.kt (1)
20-20: ⚡ Quick win
cartIdsextra key를 상수로 통합해 계약 드리프트를 막아주세요.Line 20의
"cartIds"는NotificationReceiver,MainActivity에서도 문자열로 반복 사용됩니다. 한 곳만 바뀌면 알림 딥링크가 조용히 깨질 수 있어서,NotificationConstants같은 단일 상수로 묶는 편이 안전합니다.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/woowacourse/shopping/notification/AlarmScheduler.kt` at line 20, Replace the hard-coded "cartIds" extra key with a shared constant to avoid contract drift: add a constant (e.g., CART_IDS) in a new or existing NotificationConstants object/class and then update AlarmScheduler (the putIntegerArrayListExtra call), NotificationReceiver, and MainActivity to use NotificationConstants.CART_IDS instead of the literal "cartIds".
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/src/main/java/woowacourse/shopping/data/mapper/CouponMapper.kt`:
- Around line 15-59: The current CouponResponseDto.toDomain() calls
LocalDate.parse/LocalTime.parse directly so a single malformed coupon breaks the
whole mapping; change the mapping to be resilient by making parsing failures
local and skip or mark invalid coupons instead of throwing: update
CouponResponseDto.toDomain() to catch DateTimeParseException (or use
runCatching) and return a nullable or Result, log the coupon identifier (id or
code) and the problematic field/value, and then change callers that do
List<CouponResponseDto>.map { it.toDomain() } to use mapNotNull (or
filterResults) so only successfully parsed coupons become domain objects; ensure
logging includes the unique symbols id/code and the parsing function names
(LocalDate.parse/LocalTime.parse) so broken records are observable.
In `@app/src/main/java/woowacourse/shopping/domain/coupon/Coupon.kt`:
- Line 112: contains(time: LocalTime)에서 start..end 범위가 자정을 넘는 구간(start > end)을
처리하지 않으므로, Coupon.contains 구현을 수정해 자정 교차를 분기 처리하세요: 만약 start <= end이면 기존처럼 time
in start..end를 사용하고, start > end인 경우에는 time >= start || time <= end 로 판단하도록 변경해
자정 이후/이전 시간을 올바르게 포함시킵니다; 대상 식별자: contains, start, end, time.
- Around line 75-82: The discount currently multiplies getQuantity only once,
undercounting when a CartItem has multiple buy+get sets; in discount(cartItems:
CartItems) compute how many full sets apply for each CartItem (e.g.,
applicableSets = item.quantity.value / (buyQuantity + getQuantity)) and multiply
getQuantity by that set count when calculating free units and total discount;
update logic that selects target (targetItems / target) to use freeUnits =
applicableSets * getQuantity and return freeUnits * target.product.price.value
so multiple bundles are correctly discounted.
In `@app/src/main/java/woowacourse/shopping/notification/NotificationReceiver.kt`:
- Around line 49-50: In NotificationReceiver (around the
notify(ORDER_NOTIFICATION_ID, notification) call) guard the notification post by
first checking ContextCompat.checkSelfPermission for
android.Manifest.permission.POST_NOTIFICATIONS (on API 33+) and
NotificationManager.areNotificationsEnabled(); if either check fails do not call
notificationManager.notify(...) and instead branch to the flow that prompts the
user (e.g., start an activity/notification settings intent or enqueue a
permission-request flow) so you avoid SecurityException when POST_NOTIFICATIONS
is denied or notifications are disabled; update the code path that references
ORDER_NOTIFICATION_ID and notification to only call
notificationManager.notify(...) after both checks pass.
In
`@app/src/main/java/woowacourse/shopping/ui/cart/recommend/RecommendScreenContent.kt`:
- Around line 32-87: RecommendScreenContent must branch on
RecommendUiState.loadState before rendering: check recommendUiState.loadState
(Loading/Success/Error) and render a centered CircularProgressIndicator or
skeleton when Loading, an error message (with optional retry callback) when
Error, and the existing product list + RecommentBottomBar only when Success; use
recommendUiState.recommendProducts and existing callbacks
(onAddClick/onIncrease/onDecrease/onClickOrder) inside the Success branch so the
UI accurately reflects loading/failure states.
In
`@app/src/main/java/woowacourse/shopping/ui/cart/recommend/RecommendViewModel.kt`:
- Around line 63-71: The current RecommendViewModel uses
productRepository.getProducts(0, Int.MAX_VALUE) which risks huge network/memory
use; change to fetch a bounded subset (e.g., call a category-scoped repository
method or getProducts(page, size) with a sensible cap) before applying
getCategoryProducts(recommended.category.value) and subtracting cartItemsFlow;
for example add/use productRepository.getProductsByCategory(category, page,
size) or call getProducts(0, limitedSize) where limitedSize >=
MAX_RECOMMEND_SIZE * 2, then compute result and update recommendProductsFlow and
recommendLoadStateFlow as before; ensure you remove Int.MAX_VALUE and respect
MAX_RECOMMEND_SIZE when taking results.
In `@app/src/main/java/woowacourse/shopping/ui/navigation/ShoppingNavHost.kt`:
- Around line 127-131: There is a duplicated LaunchedEffect(startCartIds) block
causing double navigation and missing state cleanup; remove the duplicate block
so only the original LaunchedEffect(startCartIds) remains (the one that calls
navController.navigate(Order(startCartIds)) and then onCartIdsConsumed()),
ensuring the remaining block performs both navigation and calls
onCartIdsConsumed() to clear state; search for occurrences of
LaunchedEffect(startCartIds) and delete the extra instance that omits
onCartIdsConsumed().
In `@app/src/main/java/woowacourse/shopping/ui/order/CouponCard.kt`:
- Around line 39-42: The Checkbox is rendered read-only (Checkbox with
onCheckedChange = null) while the whole card handles clicks via
Column.clickable, which breaks accessibility for screen readers; fix by wiring
an accessibility toggle handler—either replace Column.clickable with
Modifier.toggleable(onValueChange = { ... }, value = coupon.isSelected, role =
Role.Checkbox) on the container OR supply onCheckedChange on the Checkbox that
delegates to the existing selection handler so the accessibility tree exposes a
proper checkbox action (update the selection callback used by Checkbox/Column to
the same handler referenced in CouponCard).
In `@app/src/main/java/woowacourse/shopping/ui/order/OrderScreen.kt`:
- Around line 52-68: The payment button (PaymentBottomBar invoking
orderViewModel::order) is always enabled regardless of uiState.loadState; change
the UI so ordering is only allowed when uiState.loadState == LoadState.Success:
pass a boolean or click handler to PaymentBottomBar that is enabled only for
LoadState.Success, and for other states render a loading indicator or retry UI
(use OrderScreenContent's uiState.loadState to branch to a Loading/Retry
surface). Update PaymentBottomBar usage in OrderScreen to conditionally supply
the orderViewModel::order callback (or a disabled state) and ensure
OrderScreenContent shows retry action for error states.
In `@app/src/main/java/woowacourse/shopping/ui/order/OrderViewModel.kt`:
- Around line 63-70: The amountToPay calculation in OrderViewModel/OrderUiState
is missing shipping and can go negative; compute amountToPay as the non-negative
value max(0, totalPrice - discountAmount + Coupon.SHIPPING_FEE) and pass that
formatted value to amountToPay (use existing formatPrice), ensuring
discountAmount is still computed from selectedCoupon?.discount(cartItems) ?: 0
and shippingFee uses Coupon.SHIPPING_FEE so the UI shows consistent “주문
금액/할인/배송비/총 결제 금액”.
In `@app/src/main/java/woowacourse/shopping/ui/productList/ProductListScreen.kt`:
- Around line 316-323: Replace the plain Icon with an IconButton in
ProductListScreen so the TopAppBar action uses the standard Material touch
target and semantics; locate the Icon block that uses
painterResource(R.drawable.ic_setting) and clickable { onSettingClick() } and
change it to use IconButton (calling onSettingClick on click) while preserving
the contentDescription and visual icon, ensuring the action matches the existing
cart IconButton behavior and provides the default 48dp hit target and proper
accessibility semantics.
In `@app/src/main/java/woowacourse/shopping/ui/setting/SettingScreen.kt`:
- Around line 149-151: The top title Text in SettingScreen (the Text composable
currently showing "결제하기") is incorrect for the settings screen; update the
string literal in the Text call inside SettingScreen (the composable/Text node
used for the screen header) to a settings-appropriate label such as "설정" or
"설정하기" so the header matches the screen context.
In `@app/src/main/java/woowacourse/shopping/ui/util/Mapper.kt`:
- Line 52: The mapping currently uses coupon.expirationDate.toString() in
Mapper.kt which yields the default LocalDate format; change it to format the
date with a fixed DateTimeFormatter (e.g., pattern "yyyy.MM.dd") so the UI
expects "2023.08.31"; update the expiredDate assignment in the mapping function
to use coupon.expirationDate.format(yourFormatter) and either create a private
constant DateTimeFormatter in Mapper.kt or reuse a shared formatter used by
CouponCard/OrderScreen to ensure consistent presentation.
In `@app/src/main/res/xml/network_security_config.xml`:
- Line 4: The XML currently allows the external ELB host
techcourse-lv2-alb-250216202.ap-northeast-2.elb.amazonaws.com under a
cleartextTrafficPermitted="true" domain, which is a release-stage risk; remove
that domain from any cleartext-permitted <domain> entries and ensure
cleartextTrafficPermitted="false" in the release network_security_config.xml,
then create a debug-only network_security_config_debug.xml (or use a debug
build-specific manifest entry) that includes the domain with
cleartextTrafficPermitted="true" for local testing; verify the app manifest
points to the release config for release builds and the debug config for debug
builds so the ELB is only contacted via HTTPS in release.
---
Nitpick comments:
In `@app/src/main/java/woowacourse/shopping/constant/Format.kt`:
- Around line 6-11: formatDiscountPrice currently returns "0원" for zero and can
produce a double-minus for negative inputs; change it so that when price == 0 it
returns an empty string (so callers can omit displaying a discount) and for
non-zero values format using the absolute value with a single leading "-" only
when price > 0 (use kotlin.math.abs on price and conditionally prefix "-" for
positive amounts) in the formatDiscountPrice function to avoid "--1000원" and to
let callers handle the absence of a discount.
In
`@app/src/main/java/woowacourse/shopping/data/repository/CartRepositoryImpl.kt`:
- Around line 49-54: The current getCartItemsByIds implementation uses "it.id in
cartIds" which does repeated O(n) list lookups; convert the incoming cartIds
List<Int> to a Set<Int> (e.g., val idSet = cartIds.toSet()) at the start of the
getCartItemsByIds function and then filter using idSet.contains(it.id) so
lookups become O(1); keep the existing refreshCartItems() and return logic
unchanged and reference _cartItems and CartItems as before.
In `@app/src/main/java/woowacourse/shopping/di/AppContainer.kt`:
- Around line 64-74: Declare repository variables using their interface types
instead of concrete implementations: change the declaration of couponRepository
from CouponRepositoryImpl to the interface CouponRepository and
notificationRepository from NotificationRepositoryImpl to NotificationRepository
while still instantiating them with CouponRepositoryImpl(couponDataSource) and
NotificationRepositoryImpl(context.getSharedPreferences(...)); this matches how
CartRepository, ProductRepository, and RecentProductRepository are typed and
follows dependency inversion (refer to couponRepository, CouponRepositoryImpl,
notificationRepository, NotificationRepositoryImpl, CartRepository).
In `@app/src/main/java/woowacourse/shopping/MainActivity.kt`:
- Around line 49-53: When
shouldShowRequestPermissionRationale(Manifest.permission.POST_NOTIFICATIONS)
returns true you must show an explanatory UI before re-requesting permission;
implement a dialog (e.g., in MainActivity) that explains why POST_NOTIFICATIONS
is needed and offers actions to retry (call
requestPermissionLauncher.launch(Manifest.permission.POST_NOTIFICATIONS)) or
cancel, and if the user selected "Don't ask again" send them to app settings via
an intent; update the branch handling in MainActivity to display this rationale
dialog instead of doing nothing and to handle both retry and open-settings
flows.
In `@app/src/main/java/woowacourse/shopping/notification/AlarmScheduler.kt`:
- Line 20: Replace the hard-coded "cartIds" extra key with a shared constant to
avoid contract drift: add a constant (e.g., CART_IDS) in a new or existing
NotificationConstants object/class and then update AlarmScheduler (the
putIntegerArrayListExtra call), NotificationReceiver, and MainActivity to use
NotificationConstants.CART_IDS instead of the literal "cartIds".
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d796a07b-ad44-4078-8a11-a698431fca45
📒 Files selected for processing (61)
README.mdapp/build.gradle.ktsapp/src/main/AndroidManifest.xmlapp/src/main/java/woowacourse/shopping/MainActivity.ktapp/src/main/java/woowacourse/shopping/ShoppingApplication.ktapp/src/main/java/woowacourse/shopping/constant/Format.ktapp/src/main/java/woowacourse/shopping/data/datasource/cart/CartRemoteDataSource.ktapp/src/main/java/woowacourse/shopping/data/datasource/cart/CartRemoteDataSourceImpl.ktapp/src/main/java/woowacourse/shopping/data/datasource/coupon/CouponRemoteDataSource.ktapp/src/main/java/woowacourse/shopping/data/datasource/coupon/CouponRemoteDataSourceImpl.ktapp/src/main/java/woowacourse/shopping/data/datasource/order/OrderRemoteDataSource.ktapp/src/main/java/woowacourse/shopping/data/datasource/order/OrderRemoteDateSourceImpl.ktapp/src/main/java/woowacourse/shopping/data/mapper/CouponMapper.ktapp/src/main/java/woowacourse/shopping/data/remote/RetrofitProvider.ktapp/src/main/java/woowacourse/shopping/data/remote/api/CouponApi.ktapp/src/main/java/woowacourse/shopping/data/remote/api/ProductApi.ktapp/src/main/java/woowacourse/shopping/data/remote/dto/CartItemDto.ktapp/src/main/java/woowacourse/shopping/data/remote/dto/CouponResponseDto.ktapp/src/main/java/woowacourse/shopping/data/repository/CartRepositoryImpl.ktapp/src/main/java/woowacourse/shopping/data/repository/CouponRepositoryImpl.ktapp/src/main/java/woowacourse/shopping/data/repository/NotificationRepositoryImpl.ktapp/src/main/java/woowacourse/shopping/data/repository/ProductRepositoryImpl.ktapp/src/main/java/woowacourse/shopping/data/repository/RecentProductRepositoryImpl.ktapp/src/main/java/woowacourse/shopping/di/AppContainer.ktapp/src/main/java/woowacourse/shopping/domain/coupon/Coupon.ktapp/src/main/java/woowacourse/shopping/domain/coupon/Coupons.ktapp/src/main/java/woowacourse/shopping/domain/product/Products.ktapp/src/main/java/woowacourse/shopping/domain/repository/CartRepository.ktapp/src/main/java/woowacourse/shopping/domain/repository/CouponRepository.ktapp/src/main/java/woowacourse/shopping/domain/repository/NotificationRepository.ktapp/src/main/java/woowacourse/shopping/notification/AlarmScheduler.ktapp/src/main/java/woowacourse/shopping/notification/NotificationConstants.ktapp/src/main/java/woowacourse/shopping/notification/NotificationReceiver.ktapp/src/main/java/woowacourse/shopping/ui/cart/CartActivity.ktapp/src/main/java/woowacourse/shopping/ui/cart/CartScreen.ktapp/src/main/java/woowacourse/shopping/ui/cart/CartUiState.ktapp/src/main/java/woowacourse/shopping/ui/cart/CartViewModel.ktapp/src/main/java/woowacourse/shopping/ui/cart/RecommendScreenContent.ktapp/src/main/java/woowacourse/shopping/ui/cart/recommend/RecommendScreen.ktapp/src/main/java/woowacourse/shopping/ui/cart/recommend/RecommendScreenContent.ktapp/src/main/java/woowacourse/shopping/ui/cart/recommend/RecommendUiState.ktapp/src/main/java/woowacourse/shopping/ui/cart/recommend/RecommendViewModel.ktapp/src/main/java/woowacourse/shopping/ui/navigation/Routes.ktapp/src/main/java/woowacourse/shopping/ui/navigation/ShoppingNavHost.ktapp/src/main/java/woowacourse/shopping/ui/order/CouponCard.ktapp/src/main/java/woowacourse/shopping/ui/order/CouponUiModel.ktapp/src/main/java/woowacourse/shopping/ui/order/OrderScreen.ktapp/src/main/java/woowacourse/shopping/ui/order/OrderUiState.ktapp/src/main/java/woowacourse/shopping/ui/order/OrderViewModel.ktapp/src/main/java/woowacourse/shopping/ui/productDetail/ProductDetailActivity.ktapp/src/main/java/woowacourse/shopping/ui/productDetail/ProductDetailScreen.ktapp/src/main/java/woowacourse/shopping/ui/productDetail/ProductDetailViewModel.ktapp/src/main/java/woowacourse/shopping/ui/productList/ProductListActivity.ktapp/src/main/java/woowacourse/shopping/ui/productList/ProductListScreen.ktapp/src/main/java/woowacourse/shopping/ui/setting/SettingScreen.ktapp/src/main/java/woowacourse/shopping/ui/setting/SettingViewModel.ktapp/src/main/java/woowacourse/shopping/ui/util/Mapper.ktapp/src/main/res/drawable/ic_setting.xmlapp/src/main/res/xml/network_security_config.xmlbuild.gradle.ktsgradle/libs.versions.toml
💤 Files with no reviewable changes (9)
- app/src/main/java/woowacourse/shopping/ui/cart/RecommendScreenContent.kt
- app/src/main/java/woowacourse/shopping/ui/productDetail/ProductDetailActivity.kt
- app/src/main/java/woowacourse/shopping/ui/cart/CartActivity.kt
- app/src/main/java/woowacourse/shopping/domain/product/Products.kt
- app/src/main/java/woowacourse/shopping/data/datasource/cart/CartRemoteDataSource.kt
- app/src/main/java/woowacourse/shopping/ui/productList/ProductListActivity.kt
- app/src/main/java/woowacourse/shopping/data/datasource/cart/CartRemoteDataSourceImpl.kt
- app/src/main/java/woowacourse/shopping/data/remote/api/ProductApi.kt
- app/src/main/java/woowacourse/shopping/ui/cart/CartUiState.kt
Comment on lines
+15
to
+59
| fun CouponResponseDto.toDomain(): Coupon = | ||
| when (this) { | ||
| is CouponResponseDto.Fixed -> | ||
| Coupon.FixedCoupon( | ||
| id = id, | ||
| code = code, | ||
| description = description, | ||
| expirationDate = LocalDate.parse(expirationDate), | ||
| discountAmount = discount, | ||
| minimumAmount = minimumAmount, | ||
| ) | ||
|
|
||
| is CouponResponseDto.Percentage -> | ||
| Coupon.PercentageCoupon( | ||
| id = id, | ||
| code = code, | ||
| description = description, | ||
| expirationDate = LocalDate.parse(expirationDate), | ||
| discountPercentage = discount, | ||
| availableTime = | ||
| TimeRange( | ||
| start = LocalTime.parse(availableTime.start), | ||
| end = LocalTime.parse(availableTime.end), | ||
| ), | ||
| ) | ||
|
|
||
| is CouponResponseDto.BuyXGetY -> | ||
| Coupon.BuyXGetYCoupon( | ||
| id = id, | ||
| code = code, | ||
| description = description, | ||
| expirationDate = LocalDate.parse(expirationDate), | ||
| buyQuantity = buyQuantity, | ||
| getQuantity = getQuantity, | ||
| ) | ||
|
|
||
| is CouponResponseDto.FreeShipping -> | ||
| Coupon.FreeShipping( | ||
| id = id, | ||
| code = code, | ||
| description = description, | ||
| expirationDate = LocalDate.parse(expirationDate), | ||
| minimumAmount = minimumAmount, | ||
| ) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Coupon mapper parse 지점 확인 =="
rg -n -C2 'LocalDate\.parse|LocalTime\.parse|toDomain\(' --type=kt
echo
echo "== CouponRepository 사용처에서 예외 처리(runCatching/try-catch) 확인 =="
rg -n -C3 'getCoupons\(|runCatching|try\s*\{' --type=ktRepository: woowacourse/android-shopping-order
Length of output: 155
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== 1) CouponMapper.kt 내부 LocalDate/LocalTime parse 호출 확인 =="
rg -n -C2 'LocalDate\.parse|LocalTime\.parse' --glob '*.kt' app/src/main/java/woowacourse/shopping/data/mapper/CouponMapper.kt || true
echo
echo "== 2) CouponResponseDto.toDomain() 호출(매핑) 사용처 확인 =="
rg -n -C2 'toDomain\(\)' --glob '*.kt' app/src/main/java/woowacourse/shopping/data | head -n 200 || true
echo
echo "== 3) getCoupons/매핑 경로 주변의 예외 처리 확인 =="
rg -n -C3 'getCoupons\(|runCatching|try\s*\{' --glob '*.kt' app/src/main/java/woowacourse/shopping | head -n 200 || trueRepository: woowacourse/android-shopping-order
Length of output: 25311
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== OrderViewModel.kt: loadData() 주변 =="
nl -ba app/src/main/java/woowacourse/shopping/ui/order/OrderViewModel.kt | sed -n '70,150p'
echo
echo "== CouponRepositoryImpl.kt: getCoupons() =="
nl -ba app/src/main/java/woowacourse/shopping/data/repository/CouponRepositoryImpl.kt | sed -n '1,80p'
echo
echo "== CouponMapper.kt: parse 호출 포함 구간 =="
nl -ba app/src/main/java/woowacourse/shopping/data/mapper/CouponMapper.kt | sed -n '1,120p'Repository: woowacourse/android-shopping-order
Length of output: 178
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== OrderViewModel.kt: loadData() 본문 =="
sed -n '70,140p' app/src/main/java/woowacourse/shopping/ui/order/OrderViewModel.kt
echo
echo "== CouponRepositoryImpl.kt: getCoupons() 전체 =="
sed -n '1,120p' app/src/main/java/woowacourse/shopping/data/repository/CouponRepositoryImpl.kt
echo
echo "== CouponMapper.kt: toDomain() 매핑 구조(컬렉션 map) =="
sed -n '1,120p' app/src/main/java/woowacourse/shopping/data/mapper/CouponMapper.ktRepository: woowacourse/android-shopping-order
Length of output: 5341
쿠폰 날짜/시간 파싱 예외가 주문 화면 로딩 전체를 LoadState.Error로 만들 수 있음
CouponMapper.kt에서 LocalDate.parse/LocalTime.parse를 직접 호출하고, List<CouponResponseDto>.toDomain()이 map { it.toDomain() }로 모든 쿠폰을 일괄 변환합니다. 이때 한 쿠폰이라도 파싱에 실패하면 예외가 CouponRepositoryImpl.getCoupons()을 타고 올라가고, OrderViewModel.loadData()의 runCatching { ... couponRepository.getCoupons() ... }가 한 번 실패하면 loadState가 LoadState.Error로 전환되어 availableCoupons.update 및 쿠폰 적용 계산이 실행되지 않습니다.
- “실패 쿠폰만 제외(부분 성공)할지” vs “전체 쿠폰 로딩을 실패로 볼지” 정책을 정하고, 그에 맞춰 예외 경계를 두는 게 필요
- 관측/로그를 남겨(어떤 필드/쿠폰이 깨졌는지) 재발 방지에 도움 되도록 처리 방식을 고민해보기
- 정책이 정해지면,
OrderViewModel.loadData()에서 예외를 어떤 범위로 묶을지(장바구니/쿠폰 분리)도 같이 확인하면 좋습니다
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/src/main/java/woowacourse/shopping/data/mapper/CouponMapper.kt` around
lines 15 - 59, The current CouponResponseDto.toDomain() calls
LocalDate.parse/LocalTime.parse directly so a single malformed coupon breaks the
whole mapping; change the mapping to be resilient by making parsing failures
local and skip or mark invalid coupons instead of throwing: update
CouponResponseDto.toDomain() to catch DateTimeParseException (or use
runCatching) and return a nullable or Result, log the coupon identifier (id or
code) and the problematic field/value, and then change callers that do
List<CouponResponseDto>.map { it.toDomain() } to use mapNotNull (or
filterResults) so only successfully parsed coupons become domain objects; ensure
logging includes the unique symbols id/code and the parsing function names
(LocalDate.parse/LocalTime.parse) so broken records are observable.
Comment on lines
+75
to
+82
| override fun discount(cartItems: CartItems): Int { | ||
| val targetItems = | ||
| cartItems.values.filter { it.quantity.value >= buyQuantity + getQuantity } | ||
| if (targetItems.isEmpty()) return 0 | ||
|
|
||
| val target = targetItems.maxBy { it.product.price.value } | ||
| return target.product.price.value * getQuantity | ||
| } |
There was a problem hiding this comment.
Buy X Get Y 할인 수량이 1회로 고정되어 있습니다.
Line 81에서 getQuantity를 한 번만 곱하고 있어, 같은 상품이 여러 묶음(예: buy+get를 2세트 이상)인 경우 할인액이 과소 계산됩니다.
quantity에서 “적용 가능한 세트 수”를 먼저 구해보면 어떨까요? 그리고 그 세트 수만큼 무료 수량이 누적되는지 케이스(경계값 포함)로 확인해보세요.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/src/main/java/woowacourse/shopping/domain/coupon/Coupon.kt` around lines
75 - 82, The discount currently multiplies getQuantity only once, undercounting
when a CartItem has multiple buy+get sets; in discount(cartItems: CartItems)
compute how many full sets apply for each CartItem (e.g., applicableSets =
item.quantity.value / (buyQuantity + getQuantity)) and multiply getQuantity by
that set count when calculating free units and total discount; update logic that
selects target (targetItems / target) to use freeUnits = applicableSets *
getQuantity and return freeUnits * target.product.price.value so multiple
bundles are correctly discounted.
| val start: LocalTime, | ||
| val end: LocalTime, | ||
| ) { | ||
| fun contains(time: LocalTime): Boolean = time in start..end |
There was a problem hiding this comment.
자정 교차 시간대 쿠폰이 비활성화될 수 있습니다.
Line 112의 time in start..end는 start > end인 구간(예: 22:00~02:00)을 표현하지 못합니다.
운영 데이터에 야간 쿠폰이 들어올 가능성이 있다면, start <= end와 start > end를 분기해서 포함 조건을 점검해보는 게 안전합니다.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/src/main/java/woowacourse/shopping/domain/coupon/Coupon.kt` at line 112,
contains(time: LocalTime)에서 start..end 범위가 자정을 넘는 구간(start > end)을 처리하지 않으므로,
Coupon.contains 구현을 수정해 자정 교차를 분기 처리하세요: 만약 start <= end이면 기존처럼 time in
start..end를 사용하고, start > end인 경우에는 time >= start || time <= end 로 판단하도록 변경해 자정
이후/이전 시간을 올바르게 포함시킵니다; 대상 식별자: contains, start, end, time.
Comment on lines
+49
to
+50
| val notificationManager = context.getSystemService(NotificationManager::class.java) | ||
| notificationManager.notify(ORDER_NOTIFICATION_ID, notification) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Android 13 (API 33)+에서 POST_NOTIFICATIONS 권한이 없는 상태로 NotificationManager.notify()를 호출하면 SecurityException이 발생하는지, 공식 Android 문서 기준으로 확인해줘.
💡 Result:
결론부터 말하면: Android 13(API 33)+에서 POST_NOTIFICATIONS 런타임 권한을 부여받지 않은 상태로 NotificationManager.notify를 호출하면 SecurityException이 “발생할 수 있습니다.” 다만 Android 공식 문서에서 “권한이 없으면 notify에서 반드시 SecurityException이 발생한다”처럼 예외 발생 여부를 1:1로 단정하는 문구는 확인되지 않았고, 대신 공식 문서에서는 POST_NOTIFICATIONS 권한을 사용자로부터 받은 뒤에야 알림을 보내라고(미허용 시 알림이 기본적으로 꺼져 opt-in 모델) 요구합니다.[1][2] 공식 문서 근거(요청하신 ‘공식 Android 문서’ 기준) 1) POST_NOTIFICATIONS는 Android 13(API 33)부터 알림 전송을 위한 런타임 권한이며, 설치 후 기본값이 꺼져 있으므로 사용자가 권한을 승인할 때까지 알림을 보내지 말아야 한다고 명시합니다.[1] 2) AOSP(공식) 문서에서는 Android 13부터 앱이 알림을 보내려면 android.permission.POST_NOTIFICATION runtime permission을 시스템에 요청(사용자 승인)해야 하고, 사용자가 승인하기 전까지 알림 전송이 차단된다고 설명합니다.[2] 3) Jetpack/공식 안내에서는 실제로 알림을 보내기 전에 권한이 부여되었는지 확인(checkSelfPermission)하고, 부여되지 않았다면 권한 요청/대기 흐름을 넣으라고 예시를 제공합니다.[3] 정리 - “권한 없음 → SecurityException 필연”을 공식 문구로 단정할 수는 없지만,[1][2] - 공식 문서가 요구하는 모델이 “권한을 승인받기 전에는 알림을 보내면 안 됨(차단됨)”이므로, 앱이 권한 미허용 상태에서 notify를 호출하면(시스템이 알림 게시를 거부하는 방식으로) SecurityException이 관측될 가능성은 있습니다.[1][2] - 따라서 안전한 구현은 notify 호출 전 POST_NOTIFICATIONS 권한(및 필요 시 areNotificationsEnabled 등)을 확인하고, 미허용이면 권한 요청 후 사용자 허용을 기다리는 것입니다.[3][1] 원하시면, (1) NotificationManager vs NotificationManagerCompat 경로 차이, (2) targetSdk/compileSdk 조합, (3) POST_NOTIFICATIONS가 거부된 경우 vs 알림이 꺼진 경우(areNotificationsEnabled)로 나눠서 “어떤 경우에 어떤 예외/동작이 관측되는지”를 공식/소스 근거로 더 좁혀 드릴게요.
Citations:
- 1: https://developer.android.com/develop/ui/views/notifications/notification-permission?hl=en
- 2: https://source.android.com/docs/core/display/notification-perm
- 3: https://developer.android.com/develop/ui/views/notifications/build-notification
🏁 Script executed:
#!/bin/bash
set -euo pipefail
repo_root="$(pwd)"
file="app/src/main/java/woowacourse/shopping/notification/NotificationReceiver.kt"
if [ ! -f "$file" ]; then
echo "Missing file: $file"
exit 1
fi
echo "=== $file (lines 1-120) ==="
nl -ba "$file" | sed -n '1,120p'
echo "=== $file (lines 35-80) ==="
nl -ba "$file" | sed -n '35,80p'Repository: woowacourse/android-shopping-order
Length of output: 238
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="app/src/main/java/woowacourse/shopping/notification/NotificationReceiver.kt"
echo "=== $file (lines 1-120) ==="
awk 'NR>=1 && NR<=120 {printf "%d:%s\n", NR, $0}' "$file"
echo "=== $file (lines 1-220) - searching relevant permission checks ==="
awk 'NR>=1 && NR<=220 {print NR ":" $0}' "$file" | rg -n "POST_NOTIFICATIONS|checkSelfPermission|areNotificationsEnabled|notify\\(|NotificationManager" -n || trueRepository: woowacourse/android-shopping-order
Length of output: 2534
🌐 Web query:
NotificationManager.notify POST_NOTIFICATIONS SecurityException Android 13
💡 Result:
On Android 13 (API 33, targetSdkVersion 33+), a SecurityException from calling NotificationManager.notify(...) or NotificationManagerCompat.notify(...) is typically caused by the app not having been granted the runtime permission android.permission.POST_NOTIFICATIONS. Android 13 introduced a runtime notification permission for sending non-exempt notifications; newly installed apps have notifications turned off by default until the user grants permission after you request it at runtime [1][2]. What to do (Android 13+): 1) Declare the permission in your manifest: android.permission.POST_NOTIFICATIONS [1][2]. 2) Request the permission at runtime (e.g., via ActivityResultContracts.RequestPermission or ActivityCompat.requestPermissions) and wait for the user grant before posting notifications [1][2]. 3) Before calling notify(...), check whether the permission is granted and/or whether notifications are enabled for your app (there’s an API areNotificationsEnabled). If not granted, don’t call notify; prompt the user instead [1][2]. If the user denies the permission, your app must not assume notifications will appear; notifications are off by default and you need the user grant to proceed [1][2]. Also handle the case where a runtime-revocable permission is rejected by being prepared for notify to fail/throw (as evidenced by reported “missing permissions required by NotificationManagerCompat.notify: android.permission.POST_NOTIFICATIONS” errors) [3].
Citations:
- 1: https://developer.android.com/develop/ui/views/notifications/notification-permission?hl=en
- 2: https://developer.android.com/develop/ui/compose/notifications/notification-permission
- 3: [Bug]: Missing permissions required by NotificationManagerCompat.notify Baseflow/flutter-geolocator#1393
API 33+에서 알림 권한/허용 상태 확인 없이 notify() 호출하면 예외/차단 위험
NotificationReceiver.kt 49-50에서 POST_NOTIFICATIONS 런타임 권한과 앱의 알림 허용 상태를 다시 확인하지 않고 바로 notificationManager.notify(...)를 호출하고 있습니다. Android 13(API 33)+에서는 사용자가 권한을 승인/철회할 수 있고, 권한이 없거나(또는 알림이 꺼져 있으면) 알림 게시가 차단되며 SecurityException 등 예외가 발생할 수 있습니다.
notify()직전(리시버 진입 직후가 아니라 호출 직전)POST_NOTIFICATIONS승인 여부와areNotificationsEnabled를 확인- 미허용이면 알림을 게시하지 말고 권한 요청/알림 설정 유도 흐름으로 분기
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/src/main/java/woowacourse/shopping/notification/NotificationReceiver.kt`
around lines 49 - 50, In NotificationReceiver (around the
notify(ORDER_NOTIFICATION_ID, notification) call) guard the notification post by
first checking ContextCompat.checkSelfPermission for
android.Manifest.permission.POST_NOTIFICATIONS (on API 33+) and
NotificationManager.areNotificationsEnabled(); if either check fails do not call
notificationManager.notify(...) and instead branch to the flow that prompts the
user (e.g., start an activity/notification settings intent or enqueue a
permission-request flow) so you avoid SecurityException when POST_NOTIFICATIONS
is denied or notifications are disabled; update the code path that references
ORDER_NOTIFICATION_ID and notification to only call
notificationManager.notify(...) after both checks pass.
Comment on lines
+63
to
+70
| val totalPrice = cartItems.totalPrice | ||
| val discountAmount = selectedCoupon?.discount(cartItems) ?: 0 | ||
| OrderUiState( | ||
| totalPrice = formatPrice(totalPrice), | ||
| discountAmount = formatPrice(discountAmount), | ||
| shippingFee = formatPrice(Coupon.SHIPPING_FEE), | ||
| amountToPay = formatPrice(totalPrice - discountAmount), | ||
| coupons = availableCoupons.toUiModel(selectedCoupon), |
There was a problem hiding this comment.
총 결제 금액 계산식이 요약 금액과 불일치할 수 있습니다.
Line 69는 배송비를 포함하지 않아, 화면의 “주문 금액/할인/배송비/총 결제 금액” 구조와 계산이 어긋납니다. 또한 할인액이 주문 금액보다 크면 음수 결제금액이 노출될 수 있습니다.
총 결제 금액 = max(0, 주문금액 - 할인금액 + 배송비) 형태로 경계값까지 함께 점검해 보실래요?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/src/main/java/woowacourse/shopping/ui/order/OrderViewModel.kt` around
lines 63 - 70, The amountToPay calculation in OrderViewModel/OrderUiState is
missing shipping and can go negative; compute amountToPay as the non-negative
value max(0, totalPrice - discountAmount + Coupon.SHIPPING_FEE) and pass that
formatted value to amountToPay (use existing formatPrice), ensuring
discountAmount is still computed from selectedCoupon?.discount(cartItems) ?: 0
and shippingFee uses Coupon.SHIPPING_FEE so the UI shows consistent “주문
금액/할인/배송비/총 결제 금액”.
Comment on lines
+149
to
+151
| Text( | ||
| text = "결제하기", | ||
| color = Color.White, |
There was a problem hiding this comment.
설정 화면 상단 타이틀이 현재 화면 맥락과 맞지 않습니다.
Line 150이 "결제하기"로 되어 있어 설정 화면 진입 시 문맥이 어긋납니다. 설정 전용 라벨로 맞춰주세요.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/src/main/java/woowacourse/shopping/ui/setting/SettingScreen.kt` around
lines 149 - 151, The top title Text in SettingScreen (the Text composable
currently showing "결제하기") is incorrect for the settings screen; update the
string literal in the Text call inside SettingScreen (the composable/Text node
used for the screen header) to a settings-appropriate label such as "설정" or
"설정하기" so the header matches the screen context.
| CouponUiModel( | ||
| id = coupon.id, | ||
| description = coupon.description, | ||
| expiredDate = coupon.expirationDate.toString(), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="app/src/main/java/woowacourse/shopping/ui/util/Mapper.kt"
echo "== File =="
ls -l "$FILE"
echo "== Lines 35-80 =="
nl -ba "$FILE" | sed -n '35,80p'
echo "== Search for date/formatter usage in Mapper.kt =="
rg -n "expire|expiration|date|formatter|format" "$FILE" || true
echo "== Search for shared UI date formatting util in ui/util =="
ls -1 app/src/main/java/woowacourse/shopping/ui/util || true
rg -n "format|toString\\(\\)|LocalDate|expiration|expire" app/src/main/java/woowacourse/shopping/ui/util || trueRepository: woowacourse/android-shopping-order
Length of output: 275
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="app/src/main/java/woowacourse/shopping/ui/util/Mapper.kt"
echo "== Lines 35-80 (with line numbers via awk) =="
awk 'NR>=35 && NR<=80 {printf "%4d:%s\n", NR, $0}' "$FILE" | sed -n '1,200p'
echo "== Search for date/formatter usage in Mapper.kt =="
rg -n "(expire|expiration|date|formatter|format|toString\\(\\))" "$FILE" || true
echo "== List ui/util directory =="
ls -1 app/src/main/java/woowacourse/shopping/ui/util || true
echo "== Search for shared date formatting utils under ui/util =="
rg -n "(format|toString\\(\\)|LocalDate|DateTime|expiration|expire)" app/src/main/java/woowacourse/shopping/ui/util || true
echo "== Search in ui package for similar expiration/toString patterns =="
rg -n "(expirationDate\\.toString\\(\\)|expirationDate|expiredDate|expire(d)?Date)" app/src/main/java/woowacourse/shopping/ui || trueRepository: woowacourse/android-shopping-order
Length of output: 3768
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "expirationDate" app/src/main/java || true
echo "== CouponUiModel =="
cat -n app/src/main/java/woowacourse/shopping/ui/order/CouponUiModel.kt | head -n 120
echo "== CouponCard usage =="
cat -n app/src/main/java/woowacourse/shopping/ui/order/CouponCard.kt | sed -n '1,200p'
echo "== Coupon model definitions (search) =="
rg -n "class Coupon|sealed class Coupon|interface Coupon" app/src/main/java/woowacourse || trueRepository: woowacourse/android-shopping-order
Length of output: 7382
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show Coupon domain/model and mapper pieces
FILE1="app/src/main/java/woowacourse/shopping/domain/coupon/Coupon.kt"
FILE2="app/src/main/java/woowacourse/shopping/data/mapper/CouponMapper.kt"
FILE3="app/src/main/java/woowacourse/shopping/data/remote/dto/CouponResponseDto.kt"
FILE4="app/src/main/java/woowacourse/shopping/ui/order/OrderScreen.kt"
FILE5="app/src/main/java/woowacourse/shopping/ui/util/Mapper.kt"
for f in "$FILE1" "$FILE2" "$FILE3" "$FILE4" "$FILE5"; do
echo "== $f =="
awk 'NR>=1 && NR<=140 {print}' "$f" | sed -n '1,140p'
echo
doneRepository: woowacourse/android-shopping-order
Length of output: 15135
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Search for date formatters =="
rg -n "DateTimeFormatter|DateTime|LocalDate\\.|toString\\(\\)|yyyy\\.MM\\.dd|MM\\.dd|yyyy-MM-dd|\\.MM\\.|\\bformat\\w*Date\\b" app/src/main/java || true
echo
echo "== Search for sample expiration strings in UI/tests =="
rg -n "\"\\d{4}[\\.-]\\d{2}[\\.-]\\d{2}\"" app/src/main/java || trueRepository: woowacourse/android-shopping-order
Length of output: 2676
만료일 날짜 포맷을 명시적으로 고정해 주세요.
Mapper.kt의 expiredDate = coupon.expirationDate.toString()은 LocalDate의 기본 출력(yyyy-MM-dd)을 그대로 써서 화면에 만료일: 2023-08-31처럼 하이픈 포맷이 들어갈 수 있습니다. 반면 CouponCard/OrderScreen 프리셋에서는 2023.08.31처럼 점(yyyy.MM.dd) 포맷을 기대하므로, UI에서 보여주는 규격에 맞춰 DateTimeFormatter(또는 공용 포맷터)로 변환하도록 매핑 계층에서 고정해 주세요.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/src/main/java/woowacourse/shopping/ui/util/Mapper.kt` at line 52, The
mapping currently uses coupon.expirationDate.toString() in Mapper.kt which
yields the default LocalDate format; change it to format the date with a fixed
DateTimeFormatter (e.g., pattern "yyyy.MM.dd") so the UI expects "2023.08.31";
update the expiredDate assignment in the mapping function to use
coupon.expirationDate.format(yourFormatter) and either create a private constant
DateTimeFormatter in Mapper.kt or reuse a shared formatter used by
CouponCard/OrderScreen to ensure consistent presentation.
| <domain-config cleartextTrafficPermitted="true"> | ||
| <domain includeSubdomains="true"> | ||
| techcourse-lv2-alb-974870821.ap-northeast-2.elb.amazonaws.com | ||
| techcourse-lv2-alb-250216202.ap-northeast-2.elb.amazonaws.com |
There was a problem hiding this comment.
릴리스에서 외부 도메인 cleartext 허용은 보안 리스크가 큽니다.
Line 4 도메인이 cleartextTrafficPermitted="true" 영역에 포함되어, 릴리스에서도 HTTP 평문 통신이 허용될 수 있습니다. 외부 ELB 도메인은 HTTPS-only로 유지하고, cleartext 예외는 debug 전용 설정으로 분리하는 편이 안전합니다.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/src/main/res/xml/network_security_config.xml` at line 4, The XML
currently allows the external ELB host
techcourse-lv2-alb-250216202.ap-northeast-2.elb.amazonaws.com under a
cleartextTrafficPermitted="true" domain, which is a release-stage risk; remove
that domain from any cleartext-permitted <domain> entries and ensure
cleartextTrafficPermitted="false" in the release network_security_config.xml,
then create a debug-only network_security_config_debug.xml (or use a debug
build-specific manifest entry) that includes the domain with
cleartextTrafficPermitted="true" for local testing; verify the app manifest
points to the release config for release builds and the debug config for debug
builds so the ELB is only contacted via HTTPS in release.
krrong
requested changes
May 25, 2026
krrong
left a comment
krrong
left a comment
There was a problem hiding this comment.
안녕하세요 허닛,
3, 4단계 미션도 고생 많으셨습니다!
미션을 마무리하기전에 한 번씩 확인하면 좋을 부분들에 코멘트를 달아두었습니다.
잘 구현해주셔서 미션을 마무리하기 전에 가볍게 확인해주시면 좋을 것 같아요 🙇
Comment on lines
+32
to
+33
| cartIds.value = intent.getIntegerArrayListExtra("cartIds") | ||
| requestNotificationPermission() |
There was a problem hiding this comment.
알림을 통해 결제 페이지로 접근 → 백버튼을 통해 홈 화면으로 이동 → 화면 회전을 해보면 어떤 문제가 발생하고 있는 것일까요?
Comment on lines
+41
to
+44
| LaunchedEffect(orderViewModel.events) { | ||
| orderViewModel.events.collect { event -> | ||
| when (event) { | ||
| OrderEvent.OrderSuccess -> onOrderSuccess() |
There was a problem hiding this comment.
현재 구조에서 만약 화면이 백그라운드로 갔을 때 event의 수집은 어떻게 되고 있을까요?
또한 LaunchedEffect의 키 값으로 Flow 객체를 전달해주고 있는데, 참조가 변경되면 어떻게 될까요?
Comment on lines
+61
to
+62
| super.onNewIntent(intent) | ||
| cartIds.value = intent.getIntegerArrayListExtra("cartIds") |
Comment on lines
+127
to
+131
| LaunchedEffect(startCartIds) { | ||
| if (startCartIds != null) { | ||
| navController.navigate(Order(startCartIds)) | ||
| } | ||
| } |
There was a problem hiding this comment.
이야기를 덧붙여보면,
LaunchedEffect(startCartIds)가 여기서 한 번, 아래에서도 한 번 총 두 번 선언되어 있습니다.
그래서 알림을 타고 들어와서 뒤로가기를 해보면 어떤 문제가 있는지 알 수 있을거에요.
또한 LaunchedEffect가 NavHost 선언보다 앞에 있으면 어떤 문제가 발생할 수 있을까요?
Comment on lines
+52
to
+68
| Column( | ||
| modifier = | ||
| Modifier | ||
| .fillMaxSize() | ||
| .systemBarsPadding(), | ||
| ) { | ||
| PaymentTopAppBar(onNavigateBack = onClickClose) | ||
| OrderScreenContent( | ||
| uiState = uiState, | ||
| onCouponClick = orderViewModel::selectCoupon, | ||
| modifier = | ||
| Modifier | ||
| .fillMaxWidth() | ||
| .weight(1f), | ||
| ) | ||
| PaymentBottomBar(orderViewModel::order) | ||
| } |
There was a problem hiding this comment.
만약 앱이 확장된다면 Route가 더 많아질 수도 있을거에요.
그리고 각자의 Nav Graph가 만들어졌을 때 어떻게 하면 각 화면의 이동을 쉽게 유추해볼 수 있을까요?
Comment on lines
+112
to
+114
| navController.navigate(ProductList) { | ||
| popUpTo<ProductList> { inclusive = true } | ||
| } |
There was a problem hiding this comment.
inclusive는 어떤 역할을 하여 왜 true로 설정해주어야 하나요?
이 점을 고민해보시면 어떠한 객체가 불필요하게 재생성되고 있는 것을 알아차리실 수 있을거에요.
| remember(entry) { | ||
| navController.getBackStackEntry<CartGraph>() | ||
| } | ||
| val cartViewModel: CartViewModel = |
There was a problem hiding this comment.
CartViewModel을 공유하고 있는 방식이 적절한 지에 대해서도 리뷰받고 싶습니다!
CartGraph 스코프 안에서 ViewModelStoreOwner를 지정해주었기 때문에 Cart와 Recommend는 모두 같은 CartViewModel 인스턴스를 참조할거에요. 👍
Comment on lines
+106
to
+109
| composable<Order> { | ||
| val viewModel: OrderViewModel = viewModel(factory = OrderViewModel.Factory) | ||
| OrderScreen( | ||
| orderViewModel = viewModel, |
There was a problem hiding this comment.
허닛이 정의하는 NavHost는 어떤 관심사를 가진 친구인가요?
그리고 ViewModel의 생성도 NavHost의 관심사 중 하나인가요?
Comment on lines
+113
to
+117
| fun order() { | ||
| viewModelScope.launch { | ||
| runCatching { | ||
| cartRepository.order(cartItems.value.values.map { it.id }) | ||
| alarmScheduler.cancel() |
There was a problem hiding this comment.
알림 로직이 흐름에 맞게 구현되어있는 지에 대해서 확신 없이 코드를 구현한 것 같습니다.
Alarm을 담당하는 AlarmScheduler를 만들어 주었고, 이를 OrderViewModel에서 알림 로직을 적절하게 구현하고 있는 것 같아요.
한 가지 고민해볼점은 결제 화면에 접근할 때마다 알람이 다시 예약이 되는데요.
알림을 통해 접근한 경우에도 역시 알람이 다시 예약되어 결제화면 → 알람 예약 → 결제화면의 무한루프가 돌고 있습니다.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
셀프 체크리스트
README.md에 기능 목록을 정리하고 명확히 기술하였는가?어떤 부분에 집중하여 리뷰해야 할까요?
README.md에 기능 목록을 정리하고 명확히 기술하였는가?어떤 부분에 집중하여 리뷰해야 할까요?
CartViewModel을 공유하고 있는 방식이 적절한 지에 대해서도 리뷰받고 싶습니다!