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

#456 퀘스트 시스템 활성화 #465

Merged
merged 32 commits into from
Feb 19, 2024
Merged

#456 퀘스트 시스템 활성화 #465

merged 32 commits into from
Feb 19, 2024

Conversation

kmc7468
Copy link
Member

@kmc7468 kmc7468 commented Feb 13, 2024

Summary

It closes #456

비활성화 됐던 lottery 모듈을 다시 활성화하고, 2024 봄학기 이벤트에 맞춰 코드를 수정합니다.

Further Work

  • 추천인 시스템 구현
  • 어뷰징 대응 자동화 시스템 구현

@kmc7468 kmc7468 self-assigned this Feb 13, 2024
@kmc7468 kmc7468 linked an issue Feb 13, 2024 that may be closed by this pull request
3 tasks
@kmc7468
Copy link
Member Author

kmc7468 commented Feb 13, 2024

현재 남아 있는 작업 목록입니다.

  • 더 이상 쓰이지 않는 Endpoint 삭제하기
  • 퀘스트 썸네일 주소 수정하기 (원활한 테스트를 위해, 일단 dev로 머지한 후 차후에 수정해도 될 듯 합니다.)

@kmc7468
Copy link
Member Author

kmc7468 commented Feb 13, 2024

이벤트 참여 동의시 학번 체크 우회하는 방법

학번 체크는 production 환경에서만 이루어집니다. 따라서, 로컬 개발 환경에서는 기본적으로 학번 체크가 우회됩니다. production 환경에서 우회하려면 src/lottery/services/globalState.js 파일의 84번 ~ 94번 줄을 삭제하시면 됩니다.

@kmc7468 kmc7468 marked this pull request as ready for review February 14, 2024 15:11
@kmc7468
Copy link
Member Author

kmc7468 commented Feb 14, 2024

  • 퀘스트 썸네일은 다른 브랜치에서 변경하는 것이 좋을 것 같습니다.
  • 2023년 가을학기 이벤트 때의 코드와 최대한 하위호환성을 유지하려고 노력했으나, 여러 가지 방법을 생각해 봤음에도 유지보수가 어려울 것 같아 호환성 유지는 포기하고 작업했습니다.
  • lottery 모듈 내에서는 eventConfig가 정상적으로 설정되어 있다고 가정하고, null check를 하지 않고 그냥 가져다 쓰도록 변경했습니다. lottery 모듈 외부에서는 항상 index.js를 거쳐 필요한 항목을 가져오게 되고, index.js에서 eventConfig가 적절히 정의된 경우에만 lottery 모듈 내의 다른 파일들을 불러오도록 코드를 수정했기 때문입니다.

properties: {
credit: {
type: "number",
description: "완료 보상 중 재화의 개수입니다.",
example: 100,
},
ticket1: {
Copy link
Member Author

Choose a reason for hiding this comment

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

Swagger랑 노션에 남겨둔 API 명세에서는 ticket1 항목이 전송되지 않는다고 했지만... 사실은 0으로 채워진채로 전송됩니다. 이걸 수정하려면 globalStateHandler 쪽에서 ticket1 항목을 지워서 전송하거나, modules/quests.js의 buildQuests 함수를 수정해야 하는데, 전자는 (물론 큰 문제는 아니겠지만) 성능 이슈가 있을 것 같고, 후자는 lottery 모듈의 재사용성을 떨어뜨리는 것 같아 ticket1 항목을 여전히 전송하도록 냅뒀습니다.

@kmc7468 kmc7468 requested a review from cokia February 17, 2024 05:17
Copy link
Contributor

@cokia cokia left a comment

Choose a reason for hiding this comment

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

LGTM!

다만 , Nullcheck 관련해서만 한번 더 고민해주시면 좋을 듯 합니다.
또한 Config에 이벤트 관련 기본값 넣는 것 관련해서, ENV를 사용하지않고 기본값만 사용하거나, ENV만 사용하거나 둘중 하나를 택하는 편이 좋을 듯 합니다.

},
},
};
// 다음 Endpoint는 2024 봄학기 이벤트에서 사용되지 않습니다.
Copy link
Member Author

Choose a reason for hiding this comment

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

Q. 왜 사용되지 않는 코드를 삭제하지 않으셨나요?
A. 다음 이벤트에서 활용될 수도 있는 코드들은 현재 이벤트에서 사용하지 않더라도 삭제하지 않고 주석 처리하여 유지하는 방향으로 결정했습니다. 매번 GitHub Commit Log나 PR List를 확인하여 필요할 때마다 코드를 복구하는 것이 번거로울 것이라 생각했기 때문입니다.

Q. eventConfig.mode 를 확인하여 이벤트에 따라 적절히 핸들링할 수 있었을텐데, 그렇게 하지 않으신 이유가 무엇인가요?
A. 처음에는 lottery 모듈의 하위호환성(최신 버전의 lottery 모듈을 기존 코드에 이식해도 퀘스트 시스템이 오류 없이 작동하는 것)을 최대한 유지할 수 있도록 코딩하려 했으나, 이벤트마다 세세하게 달라지는 요구사항을 매번 eventConfig.mode 를 확인하며 핸들링하기엔 부담이 너무 컸고, 하위호환성을 유지해서 얻을 수 있는 이점이 없는 것 같아 lottery 모듈은 현재 이벤트에서만 사용될 수 있도록 간결하게 구성하고자 했습니다.

loadenv.js Outdated Show resolved Hide resolved
src/lottery/modules/contracts.js Outdated Show resolved Hide resolved
src/lottery/index.js Outdated Show resolved Hide resolved
src/lottery/modules/contracts.js Outdated Show resolved Hide resolved
src/lottery/modules/quests.js Outdated Show resolved Hide resolved
Comment on lines 91 to 93
return res.status(400).json({
error: "GlobalState/Create : not an undergraduate freshman",
});
Copy link
Member

Choose a reason for hiding this comment

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

잉?? 저희 errror 를 { error: message } 형태의 JSON으로 반환하는 이유가 있나용?

Copy link
Member Author

Choose a reason for hiding this comment

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

가을학기에 처음 lottery 모듈을 만들면서 참고했던 코드에서 저런 방식으로 오류 메세지를 전달하고 있었어서, lottery 모듈에서도 같은 방식을 사용하고 있었습니다. (rooms.js 파일 같은 곳에서 저런 방식을 쓰고 있습니다!) 그냥 메세지만 바로 send하도록 변경할까요? 이 부분은 이벤트 끝난 후 이슈를 만들어서 lottery 모듈 밖에 있는 코드들도 같이 손보는게 좋을 것 같기도 합니다.

Copy link
Member Author

Choose a reason for hiding this comment

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

이슈 #469 개설하였습니다.

Copy link
Member

@14KGun 14KGun left a comment

Choose a reason for hiding this comment

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

👍

await eventStatus.save();
if (
req.body.inviter &&
(await eventStatusModel.findOne({ _id: req.body.inviter }).lean())
Copy link
Member Author

Choose a reason for hiding this comment

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

이 부분 조건식이 잘못 되었는데, 이슈 #457 에서 추천인 시스템 구현하면서 함께 해결하도록 하겠습니다.

@kmc7468 kmc7468 merged commit d5d302e into dev Feb 19, 2024
1 check passed
@kmc7468 kmc7468 deleted the #456-enable-quest-system branch February 19, 2024 15:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

퀘스트 시스템 활성화
3 participants