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

Fix minor issues for executive activity #1394

Merged
merged 15 commits into from
Feb 4, 2025

Conversation

wjeongchoi
Copy link
Contributor

@wjeongchoi wjeongchoi commented Jan 27, 2025

요약 *

It closes #1388

  • editedAt, commentedAt 사용
  • 승인 코멘트만 있을 때 반려사유 필드 제거
  • 승인 코멘트 보이는 부분 제거
  • 승인/반려 시 table 최신화
  • 검토 현황 상세 rowLink 추가
  • 집행부 상세 페이지에서 pagehead가 대표 동아리 관리로 뜨는 문제

스크린샷

이후 Task *

  • 없음

@wjeongchoi wjeongchoi added bug Something isn't working front-end labels Jan 27, 2025
@wjeongchoi wjeongchoi self-assigned this Jan 27, 2025
@wjeongchoi wjeongchoi linked an issue Jan 27, 2025 that may be closed by this pull request
@wjeongchoi wjeongchoi marked this pull request as ready for review January 27, 2025 15:34
@@ -248,30 +254,28 @@ const ActivityReportDetailFrame: React.FC<ActivityReportDetailFrameProps> = ({
labels={
getActivityReportProgress(
data.activityStatusEnumId,
data.updatedAt,
data.activityStatusEnumId === 1
Copy link
Contributor

Choose a reason for hiding this comment

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

요런거 숫자 말고 인터페이스에 ActivityStatusEnum 사용하면 좋을 것 같아요

@@ -64,6 +65,7 @@ const useGetActivityReportDetail = (

return {
data: memoizedData,
clubId: activityReport?.clubId ?? 0,
Copy link
Contributor

Choose a reason for hiding this comment

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

혹시 요거 memoizedData 에 있는 clubId 그대로 사용하지 않고 따로 뺀 이유가 있나요?
그렇다면 ActivityReportDetailFrame 199 번째 줄의 !("clubId" in data) 이라는 조건을 !clubId 혹은 그냥 아예 지워도 될거 같아요

Copy link
Contributor Author

Choose a reason for hiding this comment

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

정확한 이유는 모르겠지만 clubId: memoizedData.clubId 이렇게 쓰면 아래 같은 에러가 나요
스크린샷 2025-02-02 오후 11 29 40
!("clubId" in data) 대신 clubId === 0로 바꿨는데 괜찮을까요

Copy link
Contributor

@jooyeongmee jooyeongmee Feb 4, 2025

Choose a reason for hiding this comment

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

요거 에러 나는 이유가 data 가 CurrentActivityReport 타입으로 리턴되는데(13번째 줄) CurrentActivityReport에 clubId가 없어서 그런거 같아요 CurrentActivityReport 여기에 clubId 추가하면 어떨까요?_?

@@ -226,6 +228,10 @@ const ActivityReportDetailFrame: React.FC<ActivityReportDetailFrameProps> = ({
return null;
};

const filteredComments = data.comments.filter(
Copy link
Contributor

Choose a reason for hiding this comment

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

별거 아니긴 한데 요거 수정페이지 쪽에서도 똑같이 필터하고 있는 것 같고 활동이 승인되었습니다 라는 상수 스트링도 사용하고 있어서 함수로 밖으로 빼는건 어떨까?

data.updatedAt,
data.activityStatusEnumId === 1
? data.editedAt
: data.commentedAt || data.editedAt,
Copy link
Contributor

Choose a reason for hiding this comment

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

홈 뭔가 가독성이 조굼 떨어지는거 같은데 그냥 commentedAt이 있으면 그걸로 없으면 editedAt으로 하면 안되나?
commentedAt 이 있는데 activityStatusEnum이 applied 일 수가 있나?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

이미 한 번 검토가 됐는데 다시 수정하면 applied로 바뀌어서 상태가 applied일 때만 editedAt을 보여줘야 하고,
commentedAt은 null이 가능해서 없을 경우에 사용할게 필요해서 updatedAt으로 수정했어

@@ -18,6 +22,12 @@ const useExecutiveApproveActivityReport = (activityId: number) => {
queryClient.invalidateQueries({
queryKey: activityReportDetailQueryKey("executive", activityId),
});
queryClient.invalidateQueries({
queryKey: ["executiveChargedActivities"],
Copy link
Contributor

Choose a reason for hiding this comment

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

요거 딱히 쿼리키 정의된 곳은 없이 사용만 하고 있는거 같은데 요거 뭔가요?

Copy link
Contributor Author

@wjeongchoi wjeongchoi Feb 2, 2025

Choose a reason for hiding this comment

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

이거 #1367 에서 써용

Copy link
Contributor

Choose a reason for hiding this comment

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

그 pr에서 쿼리키 수정한 것 같은데 여기도 하는거죠?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

queryKey인 array에서 앞쪽 원소가 동일하면 다 invalidate이 된다고 하네용
테스트도 했음!

cell: info => formatDateTime(info.getValue()),
cell: info => {
const date = info.getValue();
return date ? formatDateTime(date) : "-";
Copy link
Contributor

Choose a reason for hiding this comment

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

요거 지금 스테이지라서 요런거겠지? 상태가 신청 상태인데 commentedAt이 있어서 "-" 가 안 나와! 프론트에선 상태로 따로 더 검증 안해도 되려나?
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

물어봤는데 commentedAt 오면은 보여주는게 맞대요

@jooyeongmee
Copy link
Contributor

혹시 그때 슬랙에서 논의 한 최종 승인 상태에 따라 토스트 색상 바꾸는거 요기 말고 다른 곳에서 하나요?
image

@jooyeongmee
Copy link
Contributor

칼디 임원진 회의 활보 (1725번) 상세조회 하면 이런 에러가 납니다 확인 부탁해요
image

@wjeongchoi
Copy link
Contributor Author

혹시 그때 슬랙에서 논의 한 최종 승인 상태에 따라 토스트 색상 바꾸는거 요기 말고 다른 곳에서 하나요? image

넹 별도 pr에서 하려고 합니다

@wjeongchoi
Copy link
Contributor Author

칼디 임원진 회의 활보 (1725번) 상세조회 하면 이런 에러가 납니다 확인 부탁해요 image

filteredComments랑 activityReportProgress를 asyncboundary 밖으로 뺐다가 생긴 문제인데 코드 반복되긴 해도 다시 boundary 안으로 넣을까요..?

@jooyeongmee
Copy link
Contributor

jooyeongmee commented Feb 4, 2025

filteredComments랑 activityReportProgress를 asyncboundary 밖으로 뺐다가 생긴 문제인데 코드 반복되긴 해도 다시 boundary 안으로 넣을까요..?

요거 filterActivityComments 함수 파라미터를 optional로 하고 if (!comments) return []; 추가하면 될거 같아요
그리고 근본적으로는 useGetActivityReportDetail 35번째 줄에서 만약 로딩 중이거나 데이터가 없으면 빈 객체를 반환하도록 되어 있는데 ActivityReportDetailFrame 에서는 207번째 줄에서 data가 undefined 혹은 null인지만 비교하고 빈 객체인지는 확인하지 않아서 생기는 문제 같아요
그래서 useGetActivityReportDetail 에서 데이터가 없는 경우 빈 객체 말고 그대로 undefined 반환해서 사용하면 좋을거 같네요
그런 경우 isPastActivity 에도 data 옆에 ? 달아줘야 하는데 maxEndTermDuration 때문에 타입에러 날 수 있어서 리턴 전에 if (!maxEndTermDuration) return false; 추가해주면 될거 같아요..ㅜㅜ (제가 짠 코드라.. 짤 때 요거 생각 못했네요ㅜ)

-- 아니면 제가 임시저장 조금 수정하면서 동윤이 pr에 빈객체인지 확인하는 utils 함수 추가했는데 그거로 빈객체인지 체크해도 될거 같아요

@wjeongchoi
Copy link
Contributor Author

filteredComments랑 activityReportProgress를 asyncboundary 밖으로 뺐다가 생긴 문제인데 코드 반복되긴 해도 다시 boundary 안으로 넣을까요..?

요거 filterActivityComments 함수 파라미터를 optional로 하고 if (!comments) return []; 추가하면 될거 같아요 그리고 근본적으로는 useGetActivityReportDetail 35번째 줄에서 만약 로딩 중이거나 데이터가 없으면 빈 객체를 반환하도록 되어 있는데 ActivityReportDetailFrame 에서는 207번째 줄에서 data가 undefined 혹은 null인지만 비교하고 빈 객체인지는 확인하지 않아서 생기는 문제 같아요 그래서 useGetActivityReportDetail 에서 데이터가 없는 경우 빈 객체 말고 그대로 undefined 반환해서 사용하면 좋을거 같네요 그런 경우 isPastActivity 에도 data 옆에 ? 달아줘야 하는데 maxEndTermDuration 때문에 타입에러 날 수 있어서 리턴 전에 if (!maxEndTermDuration) return false; 추가해주면 될거 같아요..ㅜㅜ (제가 짠 코드라.. 짤 때 요거 생각 못했네요ㅜ)

-- 아니면 제가 임시저장 조금 수정하면서 동윤이 pr에 빈객체인지 확인하는 utils 함수 추가했는데 그거로 빈객체인지 체크해도 될거 같아요

이거 방금 커밋으로 조금 다르게 해결했는데 이렇게 해도 괜찮을까요?

@jooyeongmee
Copy link
Contributor

이거 방금 커밋으로 조금 다르게 해결했는데 이렇게 해도 괜찮을까요?

오호 isPastActivity 쪽에도 에러 안 나는거죠? 홈 에러 안 나는거면 고대로 하고 나중에 문제생기면 다시 바꾸죠~
엇 근데 여전히 에러 나는거 같은데요..?! 근데 이것도 참 이상한게 다른 상세조회는 잘 보이는데 1725 이것만 에러가 나는거 같아요..
image

@wjeongchoi
Copy link
Contributor Author

이거 방금 커밋으로 조금 다르게 해결했는데 이렇게 해도 괜찮을까요?

오호 isPastActivity 쪽에도 에러 안 나는거죠? 홈 에러 안 나는거면 고대로 하고 나중에 문제생기면 다시 바꾸죠~ 엇 근데 여전히 에러 나는거 같은데요..?! 근데 이것도 참 이상한게 다른 상세조회는 잘 보이는데 1725 이것만 에러가 나는거 같아요.. image

max term은 duration length로 필터 걸었고, undefined 이슈는 원래는 clubId 별도로 처리하는게 로딩 되기 전에 값 사용하려는 문제를 해결하고 있었는데 그게 빠지면서 로딩 끝나기 전에 (데이터 받아오기 전에) 없는 데이터를 가져다 쓰려고 해서 문제 생긴 거였어요

@jooyeongmee
Copy link
Contributor

오호 오키오키 👍🏻

@wjeongchoi wjeongchoi merged commit 2038910 into dev Feb 4, 2025
1 check passed
@wjeongchoi wjeongchoi deleted the 1388-use-editedat-and-commentedat-for-activity branch February 4, 2025 11:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working front-end
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix minor issues for executive activity
2 participants