Skip to content

Commit

Permalink
fixup new conv sysmsg
Browse files Browse the repository at this point in the history
  • Loading branch information
joshblum committed Feb 7, 2020
1 parent bf155c3 commit 6b8a2e6
Show file tree
Hide file tree
Showing 11 changed files with 54 additions and 46 deletions.
6 changes: 4 additions & 2 deletions go/chat/bots/commands.go
Expand Up @@ -144,15 +144,17 @@ func (b *CachingBotCommandManager) createConv(ctx context.Context, param chat1.A
}
switch param.Typ {
case chat1.BotCommandsAdvertisementTyp_PUBLIC:
return b.G().ChatHelper.NewConversation(ctx, b.uid, username, &commandsPublicTopicName,
res, _, err = b.G().ChatHelper.NewConversation(ctx, b.uid, username, &commandsPublicTopicName,
chat1.TopicType_DEV, chat1.ConversationMembersType_IMPTEAMNATIVE, keybase1.TLFVisibility_PUBLIC)
return res, err
case chat1.BotCommandsAdvertisementTyp_TLFID_MEMBERS, chat1.BotCommandsAdvertisementTyp_TLFID_CONVS:
if param.TeamName == nil {
return res, errors.New("missing team name")
}
topicName := fmt.Sprintf("___keybase_botcommands_team_%s_%v", username, param.Typ)
return b.G().ChatHelper.NewConversationSkipFindExisting(ctx, b.uid, *param.TeamName, &topicName,
res, _, err = b.G().ChatHelper.NewConversationSkipFindExisting(ctx, b.uid, *param.TeamName, &topicName,
chat1.TopicType_DEV, chat1.ConversationMembersType_TEAM, keybase1.TLFVisibility_PRIVATE)
return res, err
default:
return res, errors.New("unknown bot advertisement typ")
}
Expand Down
2 changes: 1 addition & 1 deletion go/chat/devstorage.go
Expand Up @@ -38,7 +38,7 @@ func (s *DevConversationBackedStorage) Put(ctx context.Context, uid gregor1.UID,
if err != nil {
return err
}
conv, err := NewConversation(ctx, s.G(), uid, username, &name, chat1.TopicType_DEV,
conv, _, err := NewConversation(ctx, s.G(), uid, username, &name, chat1.TopicType_DEV,
chat1.ConversationMembersType_IMPTEAMNATIVE, keybase1.TLFVisibility_PRIVATE, s.ri,
NewConvFindExistingNormal)
if err != nil {
Expand Down
2 changes: 1 addition & 1 deletion go/chat/flipmanager.go
Expand Up @@ -983,7 +983,7 @@ func (m *FlipManager) StartFlip(ctx context.Context, uid gregor1.UID, hostConvID
membersType)
default:
}
conv, err = NewConversationWithMemberSourceConv(ctx, m.G(), uid, tlfName, &topicName,
conv, _, err = NewConversationWithMemberSourceConv(ctx, m.G(), uid, tlfName, &topicName,
chat1.TopicType_DEV, membersType,
keybase1.TLFVisibility_PRIVATE, m.ri, NewConvFindExistingSkip, &hostConvID)
convCreatedCh <- err
Expand Down
48 changes: 24 additions & 24 deletions go/chat/helper.go
Expand Up @@ -40,21 +40,21 @@ func NewHelper(g *globals.Context, ri func() chat1.RemoteInterface) *Helper {

func (h *Helper) NewConversation(ctx context.Context, uid gregor1.UID, tlfName string,
topicName *string, topicType chat1.TopicType, membersType chat1.ConversationMembersType,
vis keybase1.TLFVisibility) (chat1.ConversationLocal, error) {
vis keybase1.TLFVisibility) (chat1.ConversationLocal, bool, error) {
return NewConversation(ctx, h.G(), uid, tlfName, topicName,
topicType, membersType, vis, h.ri, NewConvFindExistingNormal)
}

func (h *Helper) NewConversationSkipFindExisting(ctx context.Context, uid gregor1.UID, tlfName string,
topicName *string, topicType chat1.TopicType, membersType chat1.ConversationMembersType,
vis keybase1.TLFVisibility) (chat1.ConversationLocal, error) {
vis keybase1.TLFVisibility) (chat1.ConversationLocal, bool, error) {
return NewConversation(ctx, h.G(), uid, tlfName, topicName,
topicType, membersType, vis, h.ri, NewConvFindExistingSkip)
}

func (h *Helper) NewConversationWithMemberSourceConv(ctx context.Context, uid gregor1.UID, tlfName string,
topicName *string, topicType chat1.TopicType, membersType chat1.ConversationMembersType,
vis keybase1.TLFVisibility, memberSourceConv *chat1.ConversationID) (chat1.ConversationLocal, error) {
vis keybase1.TLFVisibility, memberSourceConv *chat1.ConversationID) (chat1.ConversationLocal, bool, error) {
return NewConversationWithMemberSourceConv(ctx, h.G(), uid, tlfName, topicName,
topicType, membersType, vis, h.ri, NewConvFindExistingNormal, memberSourceConv)
}
Expand Down Expand Up @@ -403,7 +403,7 @@ func (s *sendHelper) conversation(ctx context.Context) error {
return err
}
uid := gregor1.UID(kuid.ToBytes())
conv, err := NewConversation(ctx, s.G(), uid, s.name, s.topicName,
conv, _, err := NewConversation(ctx, s.G(), uid, s.name, s.topicName,
chat1.TopicType_CHAT, s.membersType, keybase1.TLFVisibility_PRIVATE, s.remoteInterface,
NewConvFindExistingNormal)
if err != nil {
Expand Down Expand Up @@ -950,14 +950,14 @@ const (

func NewConversation(ctx context.Context, g *globals.Context, uid gregor1.UID, tlfName string,
topicName *string, topicType chat1.TopicType, membersType chat1.ConversationMembersType,
vis keybase1.TLFVisibility, ri func() chat1.RemoteInterface, findExistingMode NewConvFindExistingMode) (chat1.ConversationLocal, error) {
vis keybase1.TLFVisibility, ri func() chat1.RemoteInterface, findExistingMode NewConvFindExistingMode) (chat1.ConversationLocal, bool, error) {
return NewConversationWithMemberSourceConv(ctx, g, uid, tlfName, topicName, topicType, membersType, vis, ri, findExistingMode, nil)
}

func NewConversationWithMemberSourceConv(ctx context.Context, g *globals.Context, uid gregor1.UID,
tlfName string, topicName *string, topicType chat1.TopicType, membersType chat1.ConversationMembersType,
vis keybase1.TLFVisibility, ri func() chat1.RemoteInterface,
findExistingMode NewConvFindExistingMode, memberSourceConv *chat1.ConversationID) (chat1.ConversationLocal, error) {
findExistingMode NewConvFindExistingMode, memberSourceConv *chat1.ConversationID) (chat1.ConversationLocal, bool, error) {
defer utils.SuspendComponent(ctx, g, g.ConvLoader)()
helper := newNewConversationHelper(g, uid, tlfName, topicName, topicType, membersType, vis, ri,
findExistingMode, memberSourceConv)
Expand Down Expand Up @@ -1084,7 +1084,7 @@ func (n *newConversationHelper) findExistingViaInboxSearch(ctx context.Context)
return nil
}

func (n *newConversationHelper) create(ctx context.Context) (res chat1.ConversationLocal, reserr error) {
func (n *newConversationHelper) create(ctx context.Context) (res chat1.ConversationLocal, created bool, reserr error) {
defer n.Trace(ctx, func() error { return reserr }, "newConversationHelper")()
// Handle a nil topic name with default values for the members type specified
if n.topicName == nil {
Expand All @@ -1107,9 +1107,9 @@ func (n *newConversationHelper) create(ctx context.Context) (res chat1.Conversat
// This can happen if a user tries to create a conversation with the same person as a conversation
// in which they are currently locked out due to reset.
if conv := n.findExistingViaInboxSearch(ctx); conv != nil {
return *conv, nil
return *conv, false, nil
}
return res, err
return res, false, err
}
n.tlfName = info.CanonicalName

Expand All @@ -1128,7 +1128,7 @@ func (n *newConversationHelper) create(ctx context.Context) (res chat1.Conversat
// If we find one conversation, then just return it as if we created it.
if len(convs) == 1 {
n.Debug(ctx, "found previous conversation that matches, returning")
return convs[0], err
return convs[0], false, nil
}

if n.G().ExternalG().Env.GetChatMemberType() == "impteam" {
Expand Down Expand Up @@ -1156,7 +1156,7 @@ func (n *newConversationHelper) create(ctx context.Context) (res chat1.Conversat
for i := 0; i < 5; i++ {
triple.TopicID, err = utils.NewChatTopicID()
if err != nil {
return res, fmt.Errorf("error creating topic ID: %s", err)
return res, false, fmt.Errorf("error creating topic ID: %s", err)
}
n.Debug(ctx, "attempt: %v [tlfID: %s topicType: %d topicID: %s name: %s public: %v mt: %v]",
i, triple.Tlfid, triple.TopicType, triple.TopicID, info.CanonicalName, isPublic,
Expand All @@ -1173,14 +1173,14 @@ func (n *newConversationHelper) create(ctx context.Context) (res chat1.Conversat
types.InboxSourceDataSourceRemoteOnly)
if len(convs) == 1 {
n.Debug(ctx, "found previous conversation that matches, returning")
return convs[0], findErr
return convs[0], false, findErr
}
n.Debug(ctx, "failed to find previous conversation on second attempt: len(convs): %d err: %s",
len(convs), findErr)
default:
// Nothing to do for other error types.
}
return res, err
return res, false, err
}

var ncrres chat1.NewConversationRemoteRes
Expand Down Expand Up @@ -1232,23 +1232,23 @@ func (n *newConversationHelper) create(ctx context.Context) (res chat1.Conversat
n.ri, n.uid, n.tlfName, n.topicType, n.membersType, n.vis, topicName, nil)
if err != nil {
n.Debug(ctx, "failed trying FindConversations after client error: %s", err)
return res, reserr
return res, false, reserr
} else if len(fcRes) > 0 {
convID = fcRes[0].GetConvID()
} else {
return res, reserr
return res, false, reserr
}
case libkb.ChatNotInTeamError:
if n.membersType == chat1.ConversationMembersType_TEAM {
teamID, tmpErr := TLFIDToTeamID(triple.Tlfid)
if tmpErr == nil && teamID.IsSubTeam() {
n.Debug(ctx, "For tlf ID %s, inferring NotExplicitMemberOfSubteamError, from error: %s", triple.Tlfid, reserr.Error())
return res, teams.NewNotExplicitMemberOfSubteamError()
return res, false, teams.NewNotExplicitMemberOfSubteamError()
}
}
return res, fmt.Errorf("error creating conversation: %s", reserr)
return res, false, fmt.Errorf("error creating conversation: %s", reserr)
default:
return res, fmt.Errorf("error creating conversation: %s", reserr)
return res, false, fmt.Errorf("error creating conversation: %s", reserr)
}
}

Expand All @@ -1261,11 +1261,11 @@ func (n *newConversationHelper) create(ctx context.Context) (res chat1.Conversat
ConvIDs: []chat1.ConversationID{convID},
})
if err != nil {
return res, err
return res, false, err
}

if len(ib.Convs) != 1 {
return res,
return res, false,
fmt.Errorf("newly created conversation fetch error: found %d conversations", len(ib.Convs))
}
res = ib.Convs[0]
Expand All @@ -1275,11 +1275,11 @@ func (n *newConversationHelper) create(ctx context.Context) (res chat1.Conversat
// Update inbox cache
updateConv := ib.ConvsUnverified[0]
if err = n.G().InboxSource.NewConversation(ctx, n.uid, 0, updateConv.Conv); err != nil {
return res, err
return res, false, err
}

if res.Error != nil {
return res, errors.New(res.Error.Message)
return res, false, errors.New(res.Error.Message)
}

// Send a message to the channel after joining.
Expand Down Expand Up @@ -1310,9 +1310,9 @@ func (n *newConversationHelper) create(ctx context.Context) (res chat1.Conversat
n.Debug(ctx, "failed to send complex team intro message: %s", err)
}
}
return res, nil
return res, true, nil
}
return res, reserr
return res, false, reserr
}

func (n *newConversationHelper) makeFirstMessage(ctx context.Context, triple chat1.ConversationIDTriple,
Expand Down
11 changes: 7 additions & 4 deletions go/chat/helper_test.go
Expand Up @@ -143,19 +143,20 @@ func TestTopicNameRace(t *testing.T) {

// spam create conversation with same name
type ncRes struct {
convID chat1.ConversationID
err error
convID chat1.ConversationID
created bool
err error
}
topicName := "LOSERS"
attempts := 2
retCh := make(chan ncRes, attempts)
for i := 0; i < attempts; i++ {
go func() {
ctx = globals.CtxAddLogTags(ctx, tc.Context())
conv, err := NewConversation(ctx, tc.Context(), uid, first.TlfName, &topicName,
conv, created, err := NewConversation(ctx, tc.Context(), uid, first.TlfName, &topicName,
chat1.TopicType_DEV, mt, keybase1.TLFVisibility_PRIVATE,
func() chat1.RemoteInterface { return ri }, NewConvFindExistingNormal)
retCh <- ncRes{convID: conv.GetConvID(), err: err}
retCh <- ncRes{convID: conv.GetConvID(), created: created, err: err}
}()
}
var convID chat1.ConversationID
Expand All @@ -164,8 +165,10 @@ func TestTopicNameRace(t *testing.T) {
require.NoError(t, res.err)
if convID.IsNil() {
convID = res.convID
require.True(t, res.created)
} else {
require.Equal(t, convID, res.convID)
require.False(t, res.created)
}
}
})
Expand Down
3 changes: 2 additions & 1 deletion go/chat/sender_test.go
Expand Up @@ -1035,10 +1035,11 @@ func TestKBFSFileEditSize(t *testing.T) {
uid := u.User.GetUID().ToBytes()
tlfName := u.Username
tc := userTc(t, world, u)
conv, err := NewConversation(ctx, tc.Context(), uid, tlfName, nil, chat1.TopicType_KBFSFILEEDIT,
conv, created, err := NewConversation(ctx, tc.Context(), uid, tlfName, nil, chat1.TopicType_KBFSFILEEDIT,
chat1.ConversationMembersType_IMPTEAMNATIVE, keybase1.TLFVisibility_PRIVATE,
func() chat1.RemoteInterface { return ri }, NewConvFindExistingNormal)
require.NoError(t, err)
require.True(t, created)

body := strings.Repeat("M", 100000)
_, _, err = blockingSender.Send(ctx, conv.GetConvID(), chat1.MessagePlaintext{
Expand Down
4 changes: 2 additions & 2 deletions go/chat/server.go
Expand Up @@ -429,7 +429,7 @@ func (h *Server) NewConversationLocal(ctx context.Context, arg chat1.NewConversa
return chat1.NewConversationLocalRes{}, err
}

conv, err := NewConversation(ctx, h.G(), uid, arg.TlfName, arg.TopicName,
conv, created, err := NewConversation(ctx, h.G(), uid, arg.TlfName, arg.TopicName,
arg.TopicType, arg.MembersType, arg.TlfVisibility, h.remoteClient, NewConvFindExistingNormal)
if err != nil {
return res, err
Expand All @@ -441,7 +441,7 @@ func (h *Server) NewConversationLocal(ctx context.Context, arg chat1.NewConversa

// If we are making a new channel in a team, send a system message to
// indicate this.
if arg.MembersType == chat1.ConversationMembersType_TEAM &&
if created && arg.MembersType == chat1.ConversationMembersType_TEAM &&
arg.TopicType == chat1.TopicType_CHAT &&
arg.TopicName != nil && *arg.TopicName != globals.DefaultTeamTopic {
subBody := chat1.NewMessageSystemWithNewchannel(chat1.MessageSystemNewChannel{
Expand Down
3 changes: 2 additions & 1 deletion go/chat/server_test.go
Expand Up @@ -6109,10 +6109,11 @@ func TestChatSrvNewConvAfterReset(t *testing.T) {
require.NoError(t, libkb.ResetAccount(ctc.as(t, users[0]).m, users[0].NormalizedUsername(),
users[0].Passphrase))
require.NoError(t, users[0].Login(tc.G))
conv2, err := NewConversation(ctx, tc.Context(), uid, users[0].Username+","+users[1].Username, nil,
conv2, created, err := NewConversation(ctx, tc.Context(), uid, users[0].Username+","+users[1].Username, nil,
chat1.TopicType_CHAT, chat1.ConversationMembersType_IMPTEAMNATIVE, keybase1.TLFVisibility_PRIVATE,
func() chat1.RemoteInterface { return ctc.as(t, users[0]).ri }, NewConvFindExistingNormal)
require.NoError(t, err)
require.False(t, created)
require.Equal(t, conv.Id, conv2.Info.Id)
}

Expand Down
3 changes: 2 additions & 1 deletion go/chat/sync_test.go
Expand Up @@ -36,9 +36,10 @@ func localizeConv(ctx context.Context, t *testing.T, tc *kbtest.ChatTestContext,
func newBlankConvWithMembersType(ctx context.Context, t *testing.T, tc *kbtest.ChatTestContext,
uid gregor1.UID, ri chat1.RemoteInterface, sender types.Sender, tlfName string,
membersType chat1.ConversationMembersType) chat1.Conversation {
res, err := NewConversation(ctx, tc.Context(), uid, tlfName, nil, chat1.TopicType_CHAT, membersType,
res, created, err := NewConversation(ctx, tc.Context(), uid, tlfName, nil, chat1.TopicType_CHAT, membersType,
keybase1.TLFVisibility_PRIVATE, func() chat1.RemoteInterface { return ri }, NewConvFindExistingNormal)
require.NoError(t, err)
require.True(t, created)
convID := res.GetConvID()
ires, err := ri.GetInboxRemote(ctx, chat1.GetInboxRemoteArg{
Query: &chat1.GetInboxQuery{
Expand Down
12 changes: 6 additions & 6 deletions go/kbtest/chat.go
Expand Up @@ -1450,20 +1450,20 @@ func (m *MockChatHelper) JourneycardDebugState(ctx context.Context, uid gregor1.

func (m *MockChatHelper) NewConversation(ctx context.Context, uid gregor1.UID, tlfName string,
topicName *string, topicType chat1.TopicType, membersType chat1.ConversationMembersType,
vis keybase1.TLFVisibility) (chat1.ConversationLocal, error) {
return chat1.ConversationLocal{}, nil
vis keybase1.TLFVisibility) (chat1.ConversationLocal, bool, error) {
return chat1.ConversationLocal{}, false, nil
}

func (m *MockChatHelper) NewConversationSkipFindExisting(ctx context.Context, uid gregor1.UID, tlfName string,
topicName *string, topicType chat1.TopicType, membersType chat1.ConversationMembersType,
vis keybase1.TLFVisibility) (chat1.ConversationLocal, error) {
return chat1.ConversationLocal{}, nil
vis keybase1.TLFVisibility) (chat1.ConversationLocal, bool, error) {
return chat1.ConversationLocal{}, false, nil
}

func (m *MockChatHelper) NewConversationWithMemberSourceConv(ctx context.Context, uid gregor1.UID, tlfName string,
topicName *string, topicType chat1.TopicType, membersType chat1.ConversationMembersType,
vis keybase1.TLFVisibility, memberSourceConv *chat1.ConversationID) (chat1.ConversationLocal, error) {
return chat1.ConversationLocal{}, nil
vis keybase1.TLFVisibility, memberSourceConv *chat1.ConversationID) (chat1.ConversationLocal, bool, error) {
return chat1.ConversationLocal{}, false, nil
}

func (m *MockChatHelper) JoinConversationByID(ctx context.Context, uid gregor1.UID,
Expand Down
6 changes: 3 additions & 3 deletions go/libkb/interfaces.go
Expand Up @@ -1034,13 +1034,13 @@ type ServiceSummaryMapper interface {
type ChatHelper interface {
NewConversation(ctx context.Context, uid gregor1.UID, tlfName string,
topicName *string, topicType chat1.TopicType, membersType chat1.ConversationMembersType,
vis keybase1.TLFVisibility) (chat1.ConversationLocal, error)
vis keybase1.TLFVisibility) (chat1.ConversationLocal, bool, error)
NewConversationSkipFindExisting(ctx context.Context, uid gregor1.UID, tlfName string,
topicName *string, topicType chat1.TopicType, membersType chat1.ConversationMembersType,
vis keybase1.TLFVisibility) (chat1.ConversationLocal, error)
vis keybase1.TLFVisibility) (chat1.ConversationLocal, bool, error)
NewConversationWithMemberSourceConv(ctx context.Context, uid gregor1.UID, tlfName string,
topicName *string, topicType chat1.TopicType, membersType chat1.ConversationMembersType,
vis keybase1.TLFVisibility, memberSourceConv *chat1.ConversationID) (chat1.ConversationLocal, error)
vis keybase1.TLFVisibility, memberSourceConv *chat1.ConversationID) (chat1.ConversationLocal, bool, error)
SendTextByID(ctx context.Context, convID chat1.ConversationID,
tlfName string, text string, vis keybase1.TLFVisibility) error
SendMsgByID(ctx context.Context, convID chat1.ConversationID,
Expand Down

0 comments on commit 6b8a2e6

Please sign in to comment.