From 8b9dbb86133ff0fd6002a391268383d1593918ca Mon Sep 17 00:00:00 2001 From: Joram Wilander Date: Fri, 29 Sep 2017 11:45:59 -0400 Subject: PLT-7404 Return viewed at times in view channel API response (#7428) * Return viewed at times in view channel API response * Updated transaction to read and write once * Remove transaction and only update if new value greater than older --- api/channel.go | 2 +- api4/channel.go | 12 +++++- api4/channel_test.go | 12 ++++-- app/channel.go | 15 ++++--- model/channel_view.go | 25 +++++++++++ model/channel_view_test.go | 38 +++++++++++++++++ model/client4.go | 6 +-- store/sqlstore/channel_store.go | 80 +++++++++++++++++++++++++----------- store/sqlstore/channel_store_test.go | 44 +++++++++++++++++--- 9 files changed, 189 insertions(+), 45 deletions(-) create mode 100644 model/channel_view_test.go diff --git a/api/channel.go b/api/channel.go index 3b033fcb8..7222ba0b1 100644 --- a/api/channel.go +++ b/api/channel.go @@ -779,7 +779,7 @@ func viewChannel(c *Context, w http.ResponseWriter, r *http.Request) { return } - if err := c.App.ViewChannel(view, c.Session.UserId, !c.Session.IsMobileApp()); err != nil { + if _, err := c.App.ViewChannel(view, c.Session.UserId, !c.Session.IsMobileApp()); err != nil { c.Err = err return } diff --git a/api4/channel.go b/api4/channel.go index 9eaa6ec8a..74eb1a972 100644 --- a/api4/channel.go +++ b/api4/channel.go @@ -709,12 +709,20 @@ func viewChannel(c *Context, w http.ResponseWriter, r *http.Request) { return } - if err := c.App.ViewChannel(view, c.Params.UserId, !c.Session.IsMobileApp()); err != nil { + times, err := c.App.ViewChannel(view, c.Params.UserId, !c.Session.IsMobileApp()) + + if err != nil { c.Err = err return } - ReturnStatusOK(w) + // Returning {"status": "OK", ...} for backwards compatability + resp := &model.ChannelViewResponse{ + Status: "OK", + LastViewedAtTimes: times, + } + + w.Write([]byte(resp.ToJson())) } func updateChannelMemberRoles(c *Context, w http.ResponseWriter, r *http.Request) { diff --git a/api4/channel_test.go b/api4/channel_test.go index fbe96728d..517d166dc 100644 --- a/api4/channel_test.go +++ b/api4/channel_test.go @@ -1375,13 +1375,19 @@ func TestViewChannel(t *testing.T) { ChannelId: th.BasicChannel.Id, } - pass, resp := Client.ViewChannel(th.BasicUser.Id, view) + viewResp, resp := Client.ViewChannel(th.BasicUser.Id, view) CheckNoError(t, resp) - if !pass { + if viewResp.Status != "OK" { t.Fatal("should have passed") } + channel, _ := th.App.GetChannel(th.BasicChannel.Id) + + if lastViewedAt := viewResp.LastViewedAtTimes[channel.Id]; lastViewedAt != channel.LastPostAt { + t.Fatal("LastPostAt does not match returned LastViewedAt time") + } + view.PrevChannelId = th.BasicChannel.Id _, resp = Client.ViewChannel(th.BasicUser.Id, view) CheckNoError(t, resp) @@ -1396,7 +1402,7 @@ func TestViewChannel(t *testing.T) { member, resp := Client.GetChannelMember(th.BasicChannel.Id, th.BasicUser.Id, "") CheckNoError(t, resp) - channel, resp := Client.GetChannel(th.BasicChannel.Id, "") + channel, resp = Client.GetChannel(th.BasicChannel.Id, "") CheckNoError(t, resp) if member.MsgCount != channel.TotalMsgCount { diff --git a/app/channel.go b/app/channel.go index 2d3709a0c..88f9cc7d7 100644 --- a/app/channel.go +++ b/app/channel.go @@ -1136,9 +1136,9 @@ func (a *App) SearchChannelsUserNotIn(teamId string, userId string, term string) } } -func (a *App) ViewChannel(view *model.ChannelView, userId string, clearPushNotifications bool) *model.AppError { +func (a *App) ViewChannel(view *model.ChannelView, userId string, clearPushNotifications bool) (map[string]int64, *model.AppError) { if err := a.SetActiveChannel(userId, view.ChannelId); err != nil { - return err + return nil, err } channelIds := []string{} @@ -1157,14 +1157,14 @@ func (a *App) ViewChannel(view *model.ChannelView, userId string, clearPushNotif } if len(channelIds) == 0 { - return nil + return map[string]int64{}, nil } uchan := a.Srv.Store.Channel().UpdateLastViewedAt(channelIds, userId) if pchan != nil { if result := <-pchan; result.Err != nil { - return result.Err + return nil, result.Err } else { if result.Data.(int64) > 0 { a.ClearPushNotification(userId, view.ChannelId) @@ -1172,8 +1172,11 @@ func (a *App) ViewChannel(view *model.ChannelView, userId string, clearPushNotif } } + times := map[string]int64{} if result := <-uchan; result.Err != nil { - return result.Err + return nil, result.Err + } else { + times = result.Data.(map[string]int64) } if *utils.Cfg.ServiceSettings.EnableChannelViewedMessages && model.IsValidId(view.ChannelId) { @@ -1182,7 +1185,7 @@ func (a *App) ViewChannel(view *model.ChannelView, userId string, clearPushNotif go a.Publish(message) } - return nil + return times, nil } func (a *App) PermanentDeleteChannel(channel *model.Channel) *model.AppError { diff --git a/model/channel_view.go b/model/channel_view.go index 8a7ead76f..e7b1de306 100644 --- a/model/channel_view.go +++ b/model/channel_view.go @@ -32,3 +32,28 @@ func ChannelViewFromJson(data io.Reader) *ChannelView { return nil } } + +type ChannelViewResponse struct { + Status string `json:"status"` + LastViewedAtTimes map[string]int64 `json:"last_viewed_at_times"` +} + +func (o *ChannelViewResponse) ToJson() string { + b, err := json.Marshal(o) + if err != nil { + return "" + } else { + return string(b) + } +} + +func ChannelViewResponseFromJson(data io.Reader) *ChannelViewResponse { + decoder := json.NewDecoder(data) + var o ChannelViewResponse + err := decoder.Decode(&o) + if err == nil { + return &o + } else { + return nil + } +} diff --git a/model/channel_view_test.go b/model/channel_view_test.go new file mode 100644 index 000000000..fac455a74 --- /dev/null +++ b/model/channel_view_test.go @@ -0,0 +1,38 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See License.txt for license information. + +package model + +import ( + "strings" + "testing" +) + +func TestChannelViewJson(t *testing.T) { + o := ChannelView{ChannelId: NewId(), PrevChannelId: NewId()} + json := o.ToJson() + ro := ChannelViewFromJson(strings.NewReader(json)) + + if o.ChannelId != ro.ChannelId { + t.Fatal("ChannelIdIds do not match") + } + + if o.PrevChannelId != ro.PrevChannelId { + t.Fatal("PrevChannelIds do not match") + } +} + +func TestChannelViewResponseJson(t *testing.T) { + id := NewId() + o := ChannelViewResponse{Status: "OK", LastViewedAtTimes: map[string]int64{id: 12345}} + json := o.ToJson() + ro := ChannelViewResponseFromJson(strings.NewReader(json)) + + if o.Status != ro.Status { + t.Fatal("ChannelIdIds do not match") + } + + if o.LastViewedAtTimes[id] != ro.LastViewedAtTimes[id] { + t.Fatal("LastViewedAtTimes do not match") + } +} diff --git a/model/client4.go b/model/client4.go index 42e89fd9c..eb8568a5e 100644 --- a/model/client4.go +++ b/model/client4.go @@ -1570,13 +1570,13 @@ func (c *Client4) GetChannelMembersForUser(userId, teamId, etag string) (*Channe } // ViewChannel performs a view action for a user. Synonymous with switching channels or marking channels as read by a user. -func (c *Client4) ViewChannel(userId string, view *ChannelView) (bool, *Response) { +func (c *Client4) ViewChannel(userId string, view *ChannelView) (*ChannelViewResponse, *Response) { url := fmt.Sprintf(c.GetChannelsRoute()+"/members/%v/view", userId) if r, err := c.DoApiPost(url, view.ToJson()); err != nil { - return false, BuildErrorResponse(r, err) + return nil, BuildErrorResponse(r, err) } else { defer closeBody(r) - return CheckStatusOK(r), BuildResponse(r) + return ChannelViewResponseFromJson(r.Body), BuildResponse(r) } } diff --git a/store/sqlstore/channel_store.go b/store/sqlstore/channel_store.go index ed001f2d2..c9a4f695e 100644 --- a/store/sqlstore/channel_store.go +++ b/store/sqlstore/channel_store.go @@ -1284,52 +1284,82 @@ func (s SqlChannelStore) UpdateLastViewedAt(channelIds []string, userId string) go func() { result := store.StoreResult{} - var query string props := make(map[string]interface{}) - idQuery := "" + updateIdQuery := "" for index, channelId := range channelIds { - if len(idQuery) > 0 { - idQuery += " OR " + if len(updateIdQuery) > 0 { + updateIdQuery += " OR " } props["channelId"+strconv.Itoa(index)] = channelId - idQuery += "ChannelId = :channelId" + strconv.Itoa(index) + updateIdQuery += "ChannelId = :channelId" + strconv.Itoa(index) + } + + selectIdQuery := strings.Replace(updateIdQuery, "ChannelId", "Id", -1) + + var lastPostAtTimes []struct { + Id string + LastPostAt int64 + TotalMsgCount int64 + } + + selectQuery := "SELECT Id, LastPostAt, TotalMsgCount FROM Channels WHERE (" + selectIdQuery + ")" + + if _, err := s.GetMaster().Select(&lastPostAtTimes, selectQuery, props); err != nil { + result.Err = model.NewAppError("SqlChannelStore.UpdateLastViewedAt", "store.sql_channel.update_last_viewed_at.app_error", nil, "channel_ids="+strings.Join(channelIds, ",")+", user_id="+userId+", "+err.Error(), http.StatusInternalServerError) + storeChannel <- result + close(storeChannel) + return + } + + times := map[string]int64{} + msgCountQuery := "" + lastViewedQuery := "" + for index, t := range lastPostAtTimes { + times[t.Id] = t.LastPostAt + + props["msgCount"+strconv.Itoa(index)] = t.TotalMsgCount + msgCountQuery += fmt.Sprintf("WHEN :channelId%d THEN GREATEST(MsgCount, :msgCount%d) ", index, index) + + props["lastViewed"+strconv.Itoa(index)] = t.LastPostAt + lastViewedQuery += fmt.Sprintf("WHEN :channelId%d THEN GREATEST(LastViewedAt, :lastViewed%d) ", index, index) + + props["channelId"+strconv.Itoa(index)] = t.Id } + var updateQuery string + if *utils.Cfg.SqlSettings.DriverName == model.DATABASE_DRIVER_POSTGRES { - query = `UPDATE + updateQuery = `UPDATE ChannelMembers SET MentionCount = 0, - MsgCount = Channels.TotalMsgCount, - LastViewedAt = Channels.LastPostAt, - LastUpdateAt = Channels.LastPostAt - FROM - Channels + MsgCount = CAST(CASE ChannelId ` + msgCountQuery + ` END AS BIGINT), + LastViewedAt = CAST(CASE ChannelId ` + lastViewedQuery + ` END AS BIGINT), + LastUpdateAt = CAST(CASE ChannelId ` + lastViewedQuery + ` END AS BIGINT) WHERE - Channels.Id = ChannelMembers.ChannelId - AND UserId = :UserId - AND (` + idQuery + `)` + UserId = :UserId + AND (` + updateIdQuery + `)` } else if *utils.Cfg.SqlSettings.DriverName == model.DATABASE_DRIVER_MYSQL { - query = `UPDATE - ChannelMembers, Channels + updateQuery = `UPDATE + ChannelMembers SET - ChannelMembers.MentionCount = 0, - ChannelMembers.MsgCount = Channels.TotalMsgCount, - ChannelMembers.LastViewedAt = Channels.LastPostAt, - ChannelMembers.LastUpdateAt = Channels.LastPostAt + MentionCount = 0, + MsgCount = CASE ChannelId ` + msgCountQuery + ` END, + LastViewedAt = CASE ChannelId ` + lastViewedQuery + ` END, + LastUpdateAt = CASE ChannelId ` + lastViewedQuery + ` END WHERE - Channels.Id = ChannelMembers.ChannelId - AND UserId = :UserId - AND (` + idQuery + `)` + UserId = :UserId + AND (` + updateIdQuery + `)` } props["UserId"] = userId - _, err := s.GetMaster().Exec(query, props) - if err != nil { + if _, err := s.GetMaster().Exec(updateQuery, props); err != nil { result.Err = model.NewAppError("SqlChannelStore.UpdateLastViewedAt", "store.sql_channel.update_last_viewed_at.app_error", nil, "channel_ids="+strings.Join(channelIds, ",")+", user_id="+userId+", "+err.Error(), http.StatusInternalServerError) + } else { + result.Data = times } storeChannel <- result diff --git a/store/sqlstore/channel_store_test.go b/store/sqlstore/channel_store_test.go index 490300f24..886852583 100644 --- a/store/sqlstore/channel_store_test.go +++ b/store/sqlstore/channel_store_test.go @@ -7,6 +7,8 @@ import ( "testing" "time" + "github.com/stretchr/testify/assert" + "github.com/mattermost/mattermost-server/model" "github.com/mattermost/mattermost-server/store" ) @@ -1249,6 +1251,7 @@ func TestChannelStoreUpdateLastViewedAt(t *testing.T) { o1.Name = "zz" + model.NewId() + "b" o1.Type = model.CHANNEL_OPEN o1.TotalMsgCount = 25 + o1.LastPostAt = 12345 store.Must(ss.Channel().Save(&o1)) m1 := model.ChannelMember{} @@ -1257,13 +1260,44 @@ func TestChannelStoreUpdateLastViewedAt(t *testing.T) { m1.NotifyProps = model.GetDefaultChannelNotifyProps() store.Must(ss.Channel().SaveMember(&m1)) - err := (<-ss.Channel().UpdateLastViewedAt([]string{m1.ChannelId}, m1.UserId)).Err - if err != nil { - t.Fatal("failed to update", err) + o2 := model.Channel{} + o2.TeamId = model.NewId() + o2.DisplayName = "Channel1" + o2.Name = "zz" + model.NewId() + "c" + o2.Type = model.CHANNEL_OPEN + o2.TotalMsgCount = 26 + o2.LastPostAt = 123456 + store.Must(ss.Channel().Save(&o2)) + + m2 := model.ChannelMember{} + m2.ChannelId = o2.Id + m2.UserId = m1.UserId + m2.NotifyProps = model.GetDefaultChannelNotifyProps() + store.Must(ss.Channel().SaveMember(&m2)) + + if result := <-ss.Channel().UpdateLastViewedAt([]string{m1.ChannelId}, m1.UserId); result.Err != nil { + t.Fatal("failed to update", result.Err) + } else if result.Data.(map[string]int64)[o1.Id] != o1.LastPostAt { + t.Fatal("last viewed at time incorrect") } - err = (<-ss.Channel().UpdateLastViewedAt([]string{m1.ChannelId}, "missing id")).Err - if err != nil { + if result := <-ss.Channel().UpdateLastViewedAt([]string{m1.ChannelId, m2.ChannelId}, m1.UserId); result.Err != nil { + t.Fatal("failed to update", result.Err) + } else if result.Data.(map[string]int64)[o2.Id] != o2.LastPostAt { + t.Fatal("last viewed at time incorrect") + } + + rm1 := store.Must(ss.Channel().GetMember(m1.ChannelId, m1.UserId)).(*model.ChannelMember) + assert.Equal(t, rm1.LastViewedAt, o1.LastPostAt) + assert.Equal(t, rm1.LastUpdateAt, o1.LastPostAt) + assert.Equal(t, rm1.MsgCount, o1.TotalMsgCount) + + rm2 := store.Must(ss.Channel().GetMember(m2.ChannelId, m2.UserId)).(*model.ChannelMember) + assert.Equal(t, rm2.LastViewedAt, o2.LastPostAt) + assert.Equal(t, rm2.LastUpdateAt, o2.LastPostAt) + assert.Equal(t, rm2.MsgCount, o2.TotalMsgCount) + + if result := <-ss.Channel().UpdateLastViewedAt([]string{m1.ChannelId}, "missing id"); result.Err != nil { t.Fatal("failed to update") } } -- cgit v1.2.3-1-g7c22