Merge pull request #1006 from 555aaditya/fix/993-unibox-overview-forget

feat: keep the shared unibox overview across read-only POSTs and drop it when opening a message marks it read

internal/api/handler/unibox.go

@@ -653,6 +653,29 @@ func (h *Handler) UniboxReply(c *gin.Context) {
 	c.JSON(http.StatusOK, resp)
 }
 
+// readOnlyPosts take a POST only to carry a query body and are gated on a read permission, so they write nothing the overview counts.
+var readOnlyPosts = map[string]bool{
+	"/v1/campaign-template-preview":   true,
+	"/v1/campaigns-estimate":          true,
+	"/v1/contacts/search":             true,
+	"/v1/segments/preview":            true,
+	"/v1/segments/:id/members/lookup": true,
+	"/v1/templates/:id/render":        true,
+	"/v1/crm/deals/search":            true,
+	"/v1/crm/deals/summary":           true,
+	"/v1/crm/tasks/search":            true,
+	"/v1/crm/tasks/summary":           true,
+}
+
+// ReadOnlyPostRoutes lists readOnlyPosts as "POST path", for the test that keeps each one registered.
+func ReadOnlyPostRoutes() []string {
+	out := make([]string, 0, len(readOnlyPosts))
+	for path := range readOnlyPosts {
+		out = append(out, http.MethodPost+" "+path)
+	}
+	return out
+}
+
 // ForgetUniboxOverviewOnWrite drops the organization's shared unibox overview after any successful write.
 func (h *Handler) ForgetUniboxOverviewOnWrite(c *gin.Context) {
 	c.Next()
@@ -660,6 +683,9 @@ func (h *Handler) ForgetUniboxOverviewOnWrite(c *gin.Context) {
 	case http.MethodGet, http.MethodHead, http.MethodOptions:
 		return
 	}
+	if readOnlyPosts[c.FullPath()] {
+		return
+	}
 	if h.UniboxService == nil || c.Writer.Status() >= http.StatusBadRequest {
 		return
 	}

internal/api/handler/unibox_overview_forget_test.go

@@ -0,0 +1,49 @@
+package handler
+
+import (
+	"net/http"
+	"net/http/httptest"
+	"testing"
+
+	"github.com/gin-gonic/gin"
+	"github.com/google/uuid"
+	"github.com/warmbly/warmbly/internal/api/middleware"
+	"github.com/warmbly/warmbly/internal/app/unibox"
+)
+
+type forgetCountingUnibox struct {
+	unibox.UniboxService
+	forgets int
+}
+
+func (f *forgetCountingUnibox) ForgetOverview(uuid.UUID) { f.forgets++ }
+
+func TestForgetUniboxOverviewOnWrite(t *testing.T) {
+	gin.SetMode(gin.TestMode)
+	tests := []struct {
+		name, method, route, path string
+		status, want              int
+	}{
+		{"a unibox write", http.MethodPatch, "/v1/unibox/seen", "/v1/unibox/seen", http.StatusOK, 1},
+		{"a mailbox write", http.MethodDelete, "/v1/emails/:id", "/v1/emails/x", http.StatusOK, 1},
+		{"a read", http.MethodGet, "/v1/unibox/overview", "/v1/unibox/overview", http.StatusOK, 0},
+		{"a refused write", http.MethodPatch, "/v1/unibox/seen", "/v1/unibox/seen", http.StatusBadRequest, 0},
+		{"a contact search", http.MethodPost, "/v1/contacts/search", "/v1/contacts/search", http.StatusOK, 0},
+		{"a CRM summary", http.MethodPost, "/v1/crm/deals/summary", "/v1/crm/deals/summary", http.StatusOK, 0},
+		{"a template render", http.MethodPost, "/v1/templates/:id/render", "/v1/templates/x/render", http.StatusOK, 0},
+		{"a contact create", http.MethodPost, "/v1/contacts", "/v1/contacts", http.StatusCreated, 1},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			svc := &forgetCountingUnibox{}
+			h := &Handler{UniboxService: svc}
+			r := gin.New()
+			r.Use(func(c *gin.Context) { c.Set(middleware.OrganizationIDKey, uuid.New()) }, h.ForgetUniboxOverviewOnWrite)
+			r.Handle(tt.method, tt.route, func(c *gin.Context) { c.Status(tt.status) })
+			r.ServeHTTP(httptest.NewRecorder(), httptest.NewRequest(tt.method, tt.path, nil))
+			if svc.forgets != tt.want {
+				t.Errorf("forgets = %d, want %d", svc.forgets, tt.want)
+			}
+		})
+	}
+}

internal/api/resource_scope_routes_test.go

@@ -25,3 +25,17 @@ func TestScopeAwareRoutesExist(t *testing.T) {
 		}
 	}
 }
+
+// A read-only POST that is renamed falls out of the list and forgets the overview again; this keeps the list honest.
+func TestReadOnlyPostRoutesExist(t *testing.T) {
+	r := Run(&handler.Handler{}, &middleware.Handler{}, &middleware.OidcHandler{}, "", gin.TestMode, nil)
+	registered := map[string]bool{}
+	for _, rt := range r.Routes() {
+		registered[rt.Method+" "+rt.Path] = true
+	}
+	for _, route := range handler.ReadOnlyPostRoutes() {
+		if !registered[route] {
+			t.Errorf("read-only POST %q is not registered", route)
+		}
+	}
+}

internal/app/unibox/email.go

@@ -55,6 +55,9 @@ func (s *uniboxService) getByID(ctx context.Context, orgID, id uuid.UUID, markSe
 			if changed, err := s.uniboxRepository.MarkSeenBulk(ctx, orgID, []uuid.UUID{id}, true); err == nil {
 				msg.Seen = true
 				s.relaySeen(ctx, orgID, changed)
+				if len(changed) > 0 {
+					s.ForgetOverview(orgID)
+				}
 			}
 		}
 		resp.ID = msg.ID

internal/app/unibox/overview_forget_test.go

@@ -0,0 +1,78 @@
+package unibox
+
+import (
+	"context"
+	"testing"
+
+	"github.com/google/uuid"
+	"github.com/warmbly/warmbly/internal/errx"
+	"github.com/warmbly/warmbly/internal/models"
+)
+
+// fakeOpenRepo is the forward fake plus the one write opening a message makes.
+type fakeOpenRepo struct {
+	*fakeForwardRepo
+	marked int
+}
+
+func (f *fakeOpenRepo) MarkSeenBulk(_ context.Context, _ uuid.UUID, ids []uuid.UUID, _ bool) ([]uuid.UUID, error) {
+	f.marked++
+	return ids, nil
+}
+
+// cachedOverviewService returns a service whose overview for orgID is already computed once.
+func cachedOverviewService(t *testing.T, seen bool) (*uniboxService, uuid.UUID, uuid.UUID, *int) {
+	t.Helper()
+	s, msg := forwardService(t, nil)
+	msg.Seen = seen
+	s.uniboxRepository = &fakeOpenRepo{fakeForwardRepo: s.uniboxRepository.(*fakeForwardRepo)}
+	computes := 0
+	s.overview = newOverviewCache(func(context.Context, overviewScope) (*models.UniboxOverview, error) {
+		computes++
+		return &models.UniboxOverview{}, nil
+	})
+	orgID := uuid.New()
+	if _, err := s.overview.get(context.Background(), overviewScope{OrgID: orgID}); err != nil {
+		t.Fatal(err)
+	}
+	return s, orgID, msg.ID, &computes
+}
+
+func TestOpeningAnUnreadMessageForgetsTheOverview(t *testing.T) {
+	s, orgID, id, computes := cachedOverviewService(t, false)
+	if _, xerr := s.GetByID(context.Background(), orgID, id); xerr != nil {
+		t.Fatal(xerr)
+	}
+	if _, err := s.overview.get(context.Background(), overviewScope{OrgID: orgID}); err != nil {
+		t.Fatal(err)
+	}
+	if *computes != 2 {
+		t.Errorf("overview computed %d times, want a fresh one after the open marked a message read", *computes)
+	}
+}
+
+func TestOpeningWithoutChangingReadStateKeepsTheOverview(t *testing.T) {
+	for name, open := range map[string]func(*uniboxService, uuid.UUID, uuid.UUID) *errx.Error{
+		"an already read message": func(s *uniboxService, org, id uuid.UUID) *errx.Error {
+			_, xerr := s.GetByID(context.Background(), org, id)
+			return xerr
+		},
+		"a peek": func(s *uniboxService, org, id uuid.UUID) *errx.Error {
+			_, xerr := s.Peek(context.Background(), org, id)
+			return xerr
+		},
+	} {
+		t.Run(name, func(t *testing.T) {
+			s, orgID, id, computes := cachedOverviewService(t, name == "an already read message")
+			if xerr := open(s, orgID, id); xerr != nil {
+				t.Fatal(xerr)
+			}
+			if _, err := s.overview.get(context.Background(), overviewScope{OrgID: orgID}); err != nil {
+				t.Fatal(err)
+			}
+			if *computes != 1 {
+				t.Errorf("overview computed %d times, want the cached one", *computes)
+			}
+		})
+	}
+}