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

VerificationAPI #13

Merged
merged 49 commits into from
Jan 26, 2024
Merged

VerificationAPI #13

merged 49 commits into from
Jan 26, 2024

Conversation

650elx
Copy link
Collaborator

@650elx 650elx commented Dec 6, 2023

No description provided.

@650elx 650elx marked this pull request as draft December 6, 2023 15:54
sinch/core/clients/sinch_client_configuration.py Outdated Show resolved Hide resolved
sinch/core/signature.py Outdated Show resolved Hide resolved
@650elx 650elx marked this pull request as ready for review January 2, 2024 14:25
@650elx 650elx requested a review from JPPortier January 19, 2024 17:29

class VerificationMethod(Enum):
SMS = "sms"
FLASHCALL = "flashCall"

Choose a reason for hiding this comment

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

It must be flashcall lowercase

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Docs and OAS file specify this method as flashCall

Copy link
Collaborator Author

@650elx 650elx Jan 22, 2024

Choose a reason for hiding this comment

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

In the end I'm transforming this property into more Python friendly snake_case anyway:

@dataclass
class StartFlashCallInitiateVerificationResponse(StartVerificationResponse):
    flash_call: dict

Choose a reason for hiding this comment

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

The enum value (https://github.com/sinch/sinch-sdk-python/blob/DEVEXP-205-186-verification-API/sinch/domains/verification/enums.py#L6) is still defined as flashCall but, e.g.: https://github.com/sinch/sinch-sdk-python/blob/DEVEXP-205-186-verification-API/sinch/domains/verification/endpoints/get_verification_by_id.py#L26C13-L26C48
Will set the value to flashcall so if end user is using enum value for a test it will failed (and same for other classes)

reference="random"
)

assert isinstance(verification_response, StartVerificationResponse)

Choose a reason for hiding this comment

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

Wouldn't it be better to have a dedicated model for sms (same for the 3 other type) as described in the OAS document? So that when getting a response from this request, the end user has access only to the properties returned by the server.
For reference, it has been done here in java: https://github.com/sinch/sinch-sdk-java/blob/main/client/src/main/com/sinch/sdk/domains/verification/adapters/converters/VerificationsDtoConverter.java#L90

Choose a reason for hiding this comment

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

I was planning to ask for same.
Better added value for end user to have dedicated models defined

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done 👌

Choose a reason for hiding this comment

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

Nice, now is it also possible to update the test in order to verify the response is an instance of a subclass (is should be StartSMSInitiateVerificationResponse if I understand well the inheritance in Python?) instead of the parent StartVerificationResponse ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

JPPortier
JPPortier previously approved these changes Jan 26, 2024
JPPortier
JPPortier previously approved these changes Jan 26, 2024
@650elx 650elx merged commit 83c91f8 into main Jan 26, 2024
4 checks passed
@650elx 650elx deleted the DEVEXP-205-186-verification-API branch August 22, 2024 14:58
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.

3 participants