-
Notifications
You must be signed in to change notification settings - Fork 110
Alex/adding display name #1216
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
Alex/adding display name #1216
Conversation
🦋 Changeset detectedLatest commit: a903bff The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR 💥 An error occurred when fetching the changed packages and changesets in this PR
|
I'm guessing I have had more recent versions of both protoc and protoc-gen-go-grpc and that resulted in some major changes in a lot of files, so this is the result of the minimal set of changes I managed to produce thus far. Please let me know if there's a better way. |
Usually I only use mage proto to make sure the generation is running fine and only commit the .proto files. There is a github action that will generate everything and create a commit for the autogenerated files. |
// 3) Non-empty: Use the specified value as the display name. | ||
optional string display_name = 31; | ||
|
||
// NEXT ID: 32 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dennwc perhaps for another change, but couldn't we reuse CreateSIPParticipantRequest here ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not directly, but yes, this is on my TODO list as well.
Sharing one structure likely won't work well, because internal has a different representation of a few things (e.g. feature flags).
I was thinking that we should slice this structure into a few smaller ones. Everything related to media setup can be one structure, shared for internal Create, public Create and also for inbound trunks. Similarly, there are fields related to how SIP should join LK room, this could be another structure shared for Create and EvaluateDispatch. The plan is also to allow storing these objects directly into the DB, so that we don't have to flatten/unflatten from SQL and add columns each time we add another flag that is never used as a filter for DB queries.
// 3) Non-empty: Use the specified value as the display name. | ||
optional string display_name = 31; | ||
|
||
// NEXT ID: 32 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not directly, but yes, this is on my TODO list as well.
Sharing one structure likely won't work well, because internal has a different representation of a few things (e.g. feature flags).
I was thinking that we should slice this structure into a few smaller ones. Everything related to media setup can be one structure, shared for internal Create, public Create and also for inbound trunks. Similarly, there are fields related to how SIP should join LK room, this could be another structure shared for Create and EvaluateDispatch. The plan is also to allow storing these objects directly into the DB, so that we don't have to flatten/unflatten from SQL and add columns each time we add another flag that is never used as a filter for DB queries.
// 1) Unspecified: Use legacy behavior - display name will be set to be the caller's number. | ||
// 2) Empty string: Do not send a display name, which will result in a CNAM lookup downstream. | ||
// 3) Non-empty: Use the specified value as the display name. | ||
optional string display_name = 31; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In theory, we could avoid optional
here, since we can populate display_name
to a number in NewCreateSIPParticipantRequest
. The only concern would be that the backend still cannot know if it should follow the old logic or a new one.
No description provided.