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

결제기능 추가 #249

Merged
merged 11 commits into from
Sep 26, 2023
Merged

결제기능 추가 #249

merged 11 commits into from
Sep 26, 2023

Conversation

Monsteel
Copy link
Contributor

안녕하세요.
카드결제 기능을 추가했습니다.

Copy link
Owner

@ryanking13 ryanking13 left a comment

Choose a reason for hiding this comment

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

감사합니다~ 아무래도 결제 기능은 잘못되었을 때 문제가 될 수 있다보니 좀 엄밀하게 테스트를 할 필요가 있을 것 같아요.

여러 API에 대한 응답 값을 mocking해서 테스트 하는 것이 계획에 있는데 (#230), 제가 바쁘고 귀찮아서 구현이 되어있지는 않은 상태입니다. 그렇지만 결제 기능에는 다양한 결제 성공, 실패에 대해서 응답을 저장해놓고 테스트를 하는 것이 꼭 필요할 것 같아요. 관련해서 작업을 할 의향이 있으실까요?

SRT/srt.py Outdated Show resolved Hide resolved
@Monsteel
Copy link
Contributor Author

@ryanking13 감사합니다. mock데이터 기반으로 테스트 환경을 구축하는 방향은 이해했습니다.
다만, reverse engineering특성 상 해당 mock test가 어디까지 유효할 것인지에 대한 의문은 듭니다.

제가 python환경에서 test환경 구축 경험이 없어서.. 혹시 생각하고 계신 방향이 있으시다면, maintainer님께서 선행작업해주시면 좋을것 같아요 :)

@ryanking13
Copy link
Owner

ryanking13 commented Sep 21, 2023

네, 결국 서버에서 뭔가 변화가 생기는 것을 알 수 없다보니 크게 도움이 안될 수도 있겠습니다만, 그래도 매번 CI에서 직접 결제를 하고 취소를 하는 것은 말이 안되다 보니 ㅎㅎ 어뷰징같기도 하구요. 그래서 지금은 예약의 경우도 다른 유저에게 피해를 줄 수 있어서 CI에서는 테스트를 하고 있지 않고 있습니다.

제가 python환경에서 test환경 구축 경험이 없어서.. 혹시 생각하고 계신 방향이 있으시다면, maintainer님께서 선행작업해주시면 좋을것 같아요 :)

네 제가 주말에 시간이 되면 다른 간단한 API에 대해서 mock response를 이용한 테스트를 작성해 놓을게요. 참고하셔서 작업하시면 될 것 같습니다.

@ryanking13
Copy link
Owner

login / logout API에 대해서 모킹된 응답을 테스트하는 간단한 테스트를 작성하여 머지해두었습니다.

  1. tests 디렉토리의 mock_responses 페이지에 서버의 json 응답을 저장하시고 (개인정보가 유출되지 않게 잘 확인하셔서 지우시구요 :) )
  2. test_srt_mock.py 파일에 테스트를 추가하시면 됩니다.

코드에서 이상한 부분이나 이해가 안 가시는 부분이 있다면 편하게 알려주세요.

@Monsteel
Copy link
Contributor Author

@ryanking13 mock data 기반 test 코드 추가 완료했습니다 :) 확인 부탁드려요.

Copy link
Owner

@ryanking13 ryanking13 left a comment

Choose a reason for hiding this comment

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

감사합니다. 몇가지 마이너한 코멘트를 빼곤 코드 측면에서는 좋아보여요.

문서 업데이트가 필요할 듯 한데, advanced usage 페이지에 결제 API를 사용하는 법을 적어주시면 좋을 것 같아요, 각각의 파라미터를 사용하면서 헷갈릴 것 같은 부분, 예를 들어

  • 카드번호는 -를 포함하는지,
  • validation number는 무엇인지
  • expire date의 포맷은 어떻게 되는지

등이 예시로 들어가 있으면 좋을 것 같습니다. docstring에 있긴 한데, 이 라이브러리는 파이썬을 처음 사용하시는 분들도 종종 사용해서, 친절하게 작성하는 편이 아무래도 좋습니다.

덤으로, 문서에 주의사항도 명시를 같이 했으면 좋겠습니다. 가령,

주의: payment API는 실제로 결제를 수행합니다. 이 API는 충분히 테스트 된 것이 아니며, 비공식적인 API를 사용하기 때문에 언제든지 제대로 동작하지 않을 수 있습니다. 이에 따른 문제 발생 시 이 라이브러리의 기여자들은 책임을 지지 않습니다.

정도의 문구를 포함하면 좋을 것 같아요.

SRT/srt.py Show resolved Hide resolved
SRT/srt.py Outdated Show resolved Hide resolved
SRT/srt.py Show resolved Hide resolved
tests/test_srt_mock.py Outdated Show resolved Hide resolved
@Monsteel
Copy link
Contributor Author

@ryanking13 감사합니다. 말씀하신 내용 반영했습니다 :) 확인 부탁드려요

Copy link
Owner

@ryanking13 ryanking13 left a comment

Choose a reason for hiding this comment

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

감사합니다~ 마지막으로 한가지 수정을 하면 좋겠는데. payment는 명사형인 관계로 메소드 이름을 수정했으면 좋겠습니다.

단순히 pay로 해도 되는데, 계획은 없습니다만 타 결제 수단을 지원할 수도 있음을 고려해서 pay_with_card 정도로 이름을 바꾸면 좋을 것 같아요.

그렇게 하면 파라미터의 이름에서도 굳이 중복해서 card가 들어갈 필요가 없어져서.

  • card_number ==> number
  • card_password ==> password

등으로 바꿀 수 있을 것 같습니다. (type은 reserved keyword라서 card_type으로 남겨둬도 될 것 같구요.)

여러 번 수정 요청을 드려서 죄송합니다 🙇

@Monsteel
Copy link
Contributor Author

@ryanking13 아닙니다 :) 피드백 주셔서 감사합니다 :) 말씀 주신내용 반영했습니다. 확인 부탁드려요

@ryanking13 ryanking13 merged commit 75e2ff9 into ryanking13:master Sep 26, 2023
6 checks passed
@ryanking13
Copy link
Owner

감사합니다!

@ryanking13
Copy link
Owner

약간 parameter를 수정해서 v2.3.0으로 릴리즈 했습니다. #253

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants