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

Fix/deserialization of callback events #92

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

Dovchik
Copy link
Contributor

@Dovchik Dovchik commented Oct 25, 2024

https://tickets.sinch.com/browse/DEVEXP-622
Add JsonPropertyName for hooks models where it was missing. This is required if System.Text.Json.Serializer is used.
Add methods to Conversation.Webhooks.Deserialize to help deserialize hooks out of the box.
Add tests for hooks deserialization

@Dovchik Dovchik marked this pull request as ready for review October 28, 2024 15:10
@asein-sinch
Copy link

The updates in this PR are going to the right direction, but in the current state, it's difficult to provide an approval:

  • the referenced ticket is too vague about what needs to be done
  • the PR is targeting the main branch which will trigger a new release upon merge. We have seen the difficulty to communicate about a version upgrade in the past, it would be better to cover all the deserialization cases in a single release
  • to updated code is handling only a subset of messages types. Can you clarify the scope of this PR and confirm whether the other types of messages need to be updated or not?
    The E2E tests targeting the mocks from our official mockserver are passing because only a few types of messages are present. It would be better to add unit tests for this kind of code update.

@Dovchik
Copy link
Contributor Author

Dovchik commented Dec 11, 2024

Thanks for the review!

the referenced ticket is too vague about what needs to be done

Updated, it is more clear?

the PR is targeting the main branch which will trigger a new release upon merge. We have seen the difficulty to communicate about a version upgrade in the past, it would be better to cover all the deserialization cases in a single release
to updated code is handling only a subset of messages types. Can you clarify the scope of this PR and confirm whether the other types of messages need to be updated or not?
The E2E tests targeting the mocks from our official mockserver are passing because only a few types of messages are present. It would be better to add unit tests for this kind of code update.

Updated the description, it's more clear?
Regarding tests, do you expect to see specifically added Unit tests for models not present in E2E mocks?

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