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

[review] 리뷰 상세 딥링크의 regionId 파라미터 null처리 추가 #3266

Merged
merged 3 commits into from
Jun 3, 2024

Conversation

kooinsung
Copy link
Contributor

PR 설명

regionId 파라미터가 null string으로 넘어오는 케이스가 존재하고 있고, null string을 가지고 데이터 조회가 이뤄질때 문제가 발생하는 이슈를 수정
관련 쓰레드: https://interpark.slack.com/archives/C05ES9XF883/p1716947868363169

변경 내역

  • null처리 코드 추가

체크리스트

스크린샷 & URL

@kooinsung kooinsung self-assigned this Jun 3, 2024
@kooinsung kooinsung requested a review from a team as a code owner June 3, 2024 02:51
@kooinsung kooinsung requested review from seulgiyoon, jhyj0521 and dia-triple and removed request for a team June 3, 2024 02:51
@kooinsung kooinsung added this to the v13.26.1 milestone Jun 3, 2024
Copy link

codecov bot commented Jun 3, 2024

Codecov Report

Attention: Patch coverage is 0% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 10.26%. Comparing base (b495cd4) to head (1476c97).

Files Patch % Lines
...ackages/review/src/components/full-list-button.tsx 0.00% 1 Missing ⚠️
...ackages/review/src/services/use-client-actions.tsx 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##              v13    #3266      +/-   ##
==========================================
- Coverage   10.27%   10.26%   -0.02%     
==========================================
  Files         710      710              
  Lines       33713    33763      +50     
  Branches     9166     9183      +17     
==========================================
  Hits         3465     3465              
- Misses      30241    30291      +50     
  Partials        7        7              

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

@kooinsung
Copy link
Contributor Author

release-canary

1 similar comment
@kooinsung
Copy link
Contributor Author

release-canary

@@ -45,7 +45,7 @@ export const FullListButton = ({
const { getWindowId } = useTripleClientActions()

const reviewListUrl = `/reviews/list?_triple_no_navbar&${qs.stringify({
region_id: regionId,
...(regionId && regionId !== null && { region_id: regionId }),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
...(regionId && regionId !== null && { region_id: regionId }),
region_id: regionId ?? undefined,

qs.stringify가 undefined를 받으면 omit 해주니 더 간략하게 작성하면 좋겠습니다.

Copy link
Contributor

@drakang4 drakang4 Jun 3, 2024

Choose a reason for hiding this comment

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

image

Copy link
Contributor Author

@kooinsung kooinsung Jun 3, 2024

Choose a reason for hiding this comment

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

regionId 파라미터가 null string으로 넘어오는 케이스가 존재하고 있는데, qs.stringify가 null string을 정상적인 값으로 인지해버려서 위와 같이 처리를 할수밖에 없다고 판단했습니다.

Copy link
Contributor

Choose a reason for hiding this comment

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

스레드 보고 왔는데 regionId 값이 string으로 'null' 인거에요? 그러면

Suggested change
...(regionId && regionId !== null && { region_id: regionId }),
...(regionId && regionId !== 'null' && { region_id: regionId }),

이렇게 해야되지 않나요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

'null'로 체크해야겠네여! 리뷰 감사합니다! 1476c97

@kooinsung kooinsung requested a review from drakang4 June 3, 2024 03:31
Copy link

github-actions bot commented Jun 3, 2024

v13.26.1-pr-3266.1 has been published!

@kooinsung
Copy link
Contributor Author

release-canary

Copy link

github-actions bot commented Jun 3, 2024

v13.26.1-pr-3266.2 has been published!

@kooinsung kooinsung merged commit 8630d4e into v13 Jun 3, 2024
15 checks passed
@kooinsung kooinsung deleted the fix/review-detail-deeplink branch June 3, 2024 05:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants