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

feat: Add gender, phone, birthdate metadata by kakao provider #1463

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

doong-jo
Copy link

@doong-jo doong-jo commented Mar 2, 2024

What kind of change does this PR introduce?

https://developers.kakao.com/docs/latest/en/kakaologin/rest-api#req-user-info-response-kakaoaccount

Add KaKaoAcount response to metadata

image

What is the current behavior?

close #1445

What is the new behavior?

Add gender, phone, birthdate if exists.

image

Additional context

These data do not always exist and only exist if the user agrees.

@doong-jo doong-jo requested a review from a team as a code owner March 2, 2024 15:25
@doong-jo doong-jo changed the title Add metadata from kakao provider feat: Add metadata from kakao provider Mar 2, 2024
@doong-jo doong-jo force-pushed the kakao-provider-add-metadata branch from f09b046 to f5ac224 Compare March 2, 2024 15:28
@doong-jo doong-jo changed the title feat: Add metadata from kakao provider feat: Add gender, phone, birthdate metadata by kakao provider Mar 2, 2024
Copy link
Member

@kangmingtay kangmingtay left a comment

Choose a reason for hiding this comment

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

hey @doong-jo, thanks for contributing, can you please add a test the returns the expected values in the mock kakao server for this? you can refer to this section which is where we set up the mock kakao API server

you don't need to write a new test case, modifying the existing successful case to check for the new claims should be sufficient

@MiryangJung
Copy link
Contributor

There are many other options that can be brought in from Kakao Is there a reason to add just these 3?
For example, we are using name gender birthday birthday birthday phone_number account_ci, etc.

@lightofjeju
Copy link

lightofjeju commented Mar 6, 2024

실례합니다. 혹시 현재 이 이슈가 어떠한 상황인지 알 수 있을까요..?
제가 깃허브 활용에 익숙하지 않아 현재 이슈에 대한 상황을 파악하는데 어려움이 있어서요 ㅜ

( 첨부: 아닙니다! 조금 전에 타일러로부터 검토중이라는 답을 받았습니다! )

@doong-jo
Copy link
Author

doong-jo commented Mar 17, 2024

@MiryangJung

https://github.com/supabase/gotrue/blob/f5ac224d615c59041f902d2d1b3ae20e89ec4e6a/internal/api/provider/provider.go#L30-L68

There is a defined Clams type that encapsulates the structure of user data commonly utilized by providers. This stores metadata provided by the providers.

The purpose of this PR is to add missing data within the current structure.

To meet your requirements, it seems we would need to separately define the data structure for Kakao or extend the common structure, which is beyond the scope of this PR. It might be more appropriate to address this in a separate PR, subject to further discussion with the maintainer.


안녕하세요 미량님 반갑습니다. 😁
provider가 공통으로 사용 중인 사용자 데이터에 대한 구조를 정의한 Clams type이 있습니다. 이는 provider가 제공한 metadata를 저장합니다.

이 PR의 목적은 현재의 구조에서 누락된 데이터를 추가하는 것입니다.

말씀하신 요구사항을 만족하려면 kakao를 위한 데이터 구조를 별도로 정의하거나 공통 구조를 확장해야하는데 그것은 이 PR의 범위를 벗어납니다. 다른 PR에서 작성되어 maintainer와 더 논의가 필요할 것 같습니다.

@ChangJunPark
Copy link

I didn't understand the overall test structure and didn't add the test code appropriately. Please make appropriate changes or suggestions to the maintainer. And there are still some tests that remain failed due to this addition. Please check this part as well.

image

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.

Only the information requested by the scope is not imported. and not response phone_number data
5 participants