startchat: support creating group #43
Conversation
pkg/connector/startchat.go
Outdated
| _ bridgev2.GroupCreatingNetworkAPI = (*LinkedInClient)(nil) | ||
| ) | ||
|
|
||
| func (l *LinkedInClient) ResolveIdentifier(ctx context.Context, id string, _ bool) (*bridgev2.ResolveIdentifierResponse, error) { |
There was a problem hiding this comment.
do i need to implement this? CreateGroup seems to work without it
There was a problem hiding this comment.
Yes, clients can't create groups if they can't get user IDs for the group participants. Starting DMs should be implemented too.
It might be sufficient for ResolveIdentifier to accept internal IDs, but you can also allow usernames/emails/phone numbers if linkedin has sensible query APIs for those (search-like APIs probably shouldn't be used for resolve identifier). SearchUsers and/or GetContactList are also good to have depending on what APIs are available.
There was a problem hiding this comment.
linkedin can only message connected people, I tried but didn't find an api to get all connections. Maybe the best we can do is pulling all participants from existing conversations
pkg/connector/capabilities.go
Outdated
| CreateDM: true, | ||
| LookupUsername: true, | ||
| ContactList: true, |
There was a problem hiding this comment.
Faking these is not allowed, they need to reflect what ResolveIdentifier, SearchUsers and GetContactList can actually do
There was a problem hiding this comment.
with these at least I can create group with participants from existing conversations
without these, linkedin participants don't show up in the options list at all
pkg/linkedingo/messages.go
Outdated
| body := SendMessageBody{ | ||
| Text: topic, | ||
| } | ||
| if topic == "" { | ||
| body.Text = title | ||
| } |
There was a problem hiding this comment.
Is this going to send an actual message to the chat? 🤔
There was a problem hiding this comment.
yes, the message body is required. I tried to set Topic: bridgev2.GroupFieldCapability{Allowed: true, Required: true}, but beeper desktop doesn't seem to have a topic input when creating group
There was a problem hiding this comment.
Topic is for the group description, not initial message. If group descriptions don't exist, it shouldn't be advertised at all. For now, probably best to hardcode some temporary message like "Group created"
pkg/connector/startchat.go
Outdated
|
|
||
| func (l *LinkedInClient) ResolveIdentifier(ctx context.Context, identifier string, createChat bool) (*bridgev2.ResolveIdentifierResponse, error) { | ||
| id := networkid.UserID(identifier) | ||
| ghost, _ := l.main.Bridge.GetGhostByID(ctx, id) |
There was a problem hiding this comment.
Errors should be propagated rather than ignored
| ) | ||
|
|
||
| func (l *LinkedInClient) ResolveIdentifier(ctx context.Context, identifier string, createChat bool) (*bridgev2.ResolveIdentifierResponse, error) { | ||
| id := networkid.UserID(identifier) |
There was a problem hiding this comment.
The input can be absolutely anything, so this needs to validate that it looks like a linkedin identifier. Implementing ValidateUserID on the network connector and calling that should be sufficient
pkg/connector/startchat.go
Outdated
|
|
||
| func (l *LinkedInClient) CreateGroup(ctx context.Context, params *bridgev2.GroupCreateParams) (*bridgev2.CreateChatResponse, error) { | ||
| chatInfo := &bridgev2.ChatInfo{ | ||
| Type: ptr.Ptr(database.RoomTypeGroupDM), |
There was a problem hiding this comment.
I had missed that chatinfo.go makes this mistake too, but groups are not group DMs, the default room type should be used instead
| Membership: event.MembershipJoin, | ||
| } | ||
| } | ||
| resp, err := l.client.NewChat(ctx, ptr.Val(chatInfo.Name), participants) |
There was a problem hiding this comment.
Do we get the proper conversation info from this? I don't like the way this ChatInfo is formed completely separately from the code in chatinfo.go, would be preferable to reuse conversationToChatInfo if we get the canonical info from the server
(if we don't get the info, then this is fine for now)
There was a problem hiding this comment.
we only get conversation urn
res = {
value: {
renderContentUnions: [],
entityUrn: '',
backendConversationUrn: '',
senderUrn: '',
originToken: '',
body: { attributes: [], text: 'c' },
backendUrn: '',
conversationUrn: '',
deliveredAt: 1764378815883,
},
}
There was a problem hiding this comment.
Then the current code is fine for now
pkg/connector/capabilities.go
Outdated
|
|
||
| func (*LinkedInConnector) GetBridgeInfoVersion() (info, capabilities int) { | ||
| return 1, 7 | ||
| return 1, 8 |
There was a problem hiding this comment.
The capability version only applies to room features, not general capabilities, no need to bump it here
rnons
force-pushed
the
rnons/plat-34404
branch
from
82e8f29 to
c5bd86c
Compare
pkg/connector/startchat.go
Outdated
| ) | ||
|
|
||
| func (l *LinkedInConnector) ValidateUserID(id networkid.UserID) bool { | ||
| return strings.HasPrefix(string(id), "ACoAA") |
There was a problem hiding this comment.
not sure how reliable this prefix is, should we check id length instead?
There was a problem hiding this comment.
If it's fixed length, then definitely check the length. Also if it's always base64 or something, can try decoding it
There was a problem hiding this comment.
it looks like base64, but decoded to binary string
There was a problem hiding this comment.
So probably check length (greater than X if it's not fixed length) and that it's valid base64, probably no need to inspect the contents further
No description provided.