[OPS Common SDK] Update Communication Common SDK for Teams Phone Extensibility GA #41219
Conversation
API Change CheckAPIView identified API level changes in this PR and created the following API reviews |
| # PhoneNumberIdentifiers with different asserted_id and anonymous_type=true are equal as raw_id=4:anonymous | ||
| assert PhoneNumberIdentifier( | ||
| value="+112345556789", asserted_id="20eacaae2768", is_anonymous=True | ||
| ) != PhoneNumberIdentifier(value="+112345556789", asserted_id="30eacaae2768", is_anonymous=True) |
There was a problem hiding this comment.
@DominikMe , is this behavior what's expected? I see similar behavior for MicrosoftTeamsUserIdentifier.
If 2 phone numbers have different asserted_ids, but they both have is_anonymous = true, the eq function will return true, as we are only checking the raw_id, and in this case, it's the same for both phone numbers, 4:anonymous
There was a problem hiding this comment.
Let's not add asserted_id and is_anonymous to the init function, then we shouldn't run into this problem. That two anonymous phone numbers are considered equal is likely okay, unless we make an exception that an anonymous phone number is always considered different, but that's maybe even more unexpected ...
| """ | ||
| self.properties = PhoneNumberProperties(value=value) | ||
|
|
||
| raw_id: Optional[str] = kwargs.get("raw_id") |
There was a problem hiding this comment.
@DominikMe should we consider for the PhoneNumberIdentifier to parse here the raw_id and populate the asserted_id and is_anonymous based on the raw_id?
Or is it supposed to be used just to create the raw_id starting from the properties? There's also the identifier_from_raw_id that I think should generally be used for parsing from raw_id to identifier type
There was a problem hiding this comment.
@cemateia Let's follow what we do in the other languages, for example here in C#
=> Don't add asserted_id and is_anonymous to the init args (we can still do that in the future if we must)
=> Inside init, compute asserted_id and is_anonymous based on the raw_id (optionally do it in the properties but probably better to do it in init)
=> in _format_raw_id don't compute the raw_id based on asserted_id and is_anonymous because these two are derived from the raw_id
=> identifier_from_raw_id probably doesn't need changes if asserted_id and is_anonymous get derived within phone number identifier, right?
Description
Please add an informative description that covers that changes made by the pull request and link all relevant issues.
If an SDK is being regenerated based on a new swagger spec, a link to the pull request containing these swagger spec changes has been included above.
All SDK Contribution checklist:
General Guidelines and Best Practices
Testing Guidelines