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

DEVEXP-464: Update Verification API #31

Closed
wants to merge 19 commits into from

Conversation

650elx
Copy link
Collaborator

@650elx 650elx commented Aug 14, 2024

@650elx 650elx marked this pull request as ready for review August 16, 2024 14:18
Copy link

@JPPortier JPPortier left a comment

Choose a reason for hiding this comment

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

Outside of implementation details, there is a lot of breaking changes but not highlighted by deprecation nor major version change.
The 'main' branch target do not fit with our branching strategy.
Let' talk about these points

@@ -4,6 +4,42 @@
from sinch.domains.verification.models import VerificationIdentity


class ReporPhoneCallVerificationDataTransfromationMixin:

Choose a reason for hiding this comment

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

Check for typos:

  1. ReporPhoneCallVerificationDataTransfromationMixin
  2. ReporFlashCallVerificationDataTransfromationMixin
  3. ReporSMSVerificationDataTransfromationMixin

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

indeed; copy/paste typos
fixed

@@ -156,16 +211,11 @@ def get_by_reference(self, reference) -> GetVerificationStatusByReferenceRespons
)
)

def get_by_id(self, id) -> GetVerificationStatusByIdResponse:

Choose a reason for hiding this comment

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

I'm not sure to understand why this endpoint has been removed (moreover without any deprecation warning as mentioned by @JPPortier), it still exists in the specification: https://gitlab.com/sinch/sinch-projects/enterprise-and-messaging/documentation/developer-experience/oas-documentation/-/blob/master/services/verification-oas/verification.yaml?ref_type=heads#L105

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oops! I've misunderstood the spec file. The endpoint is back.

@650elx
Copy link
Collaborator Author

650elx commented Aug 21, 2024

Outside of implementation details, there is a lot of breaking changes but not highlighted by deprecation nor major version change. The 'main' branch target do not fit with our branching strategy. Let' talk about these points

Indeed. I'm going to bump the major version on the next release.

@650elx 650elx changed the base branch from main to v2.0-next August 22, 2024 15:04
@650elx 650elx requested a review from JPPortier August 22, 2024 15:05
@650elx
Copy link
Collaborator Author

650elx commented Oct 14, 2024

Closing due to #36

@650elx 650elx closed this Oct 14, 2024
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