popup: apply #222 review changes (#227)

* popup: encode popup text as Windows-1252

* popup: return 200 and de-dupe recipients in the popup service

api.yml

@@ -587,7 +587,7 @@ paths:
                   type: string
                   minLength: 1
                   maxLength: 8000
-                  description: The text content of the popup. Supports HTML. Encoded with ICBM_LEGACY_CHARSET, and characters outside it become '?'.
+                  description: The text content of the popup. Supports HTML. Encoded as Windows-1252, and characters outside it become '?'.
                 url:
                   type: string
                   maxLength: 2048
@@ -607,20 +607,8 @@ paths:
       responses:
         '200':
           description: Popup sent.
-          content:
-            application/json:
-              schema:
-                type: object
-                properties:
-                  offline:
-                    type: array
-                    items:
-                      type: string
-                    description: Recipients who were offline and did not receive the popup.
         '400':
           description: Bad request. Invalid input data.
-        '404':
-          description: None of the recipients are online.
         '500':
           description: Internal server error.
 

cmd/server/factory.go

@@ -406,7 +406,7 @@ func MgmtAPI(deps Container) *http.Server {
 		deps.sqLiteUserStore,
 		deps.sqLiteUserStore,
 	)
-	popupService := foodgroup.NewPopupService(deps.inMemorySessionManager, deps.legacyCharset)
+	popupService := foodgroup.NewPopupService(deps.inMemorySessionManager)
 	return http.NewManagementAPI(
 		bld,
 		deps.cfg.APIListener,

foodgroup/popup.go

@@ -3,11 +3,12 @@ package foodgroup
 import (
 	"context"
 	"fmt"
+	"slices"
 
 	"github.com/mk6i/open-oscar-server/state"
 	"github.com/mk6i/open-oscar-server/wire"
 
-	"golang.org/x/text/encoding"
+	"golang.org/x/text/encoding/charmap"
 )
 
 const (
@@ -16,26 +17,31 @@ const (
 )
 
 // NewPopupService returns a new instance of PopupService.
-func NewPopupService(messageRelayer MessageRelayer, legacyCharset encoding.Encoding) PopupService {
+func NewPopupService(messageRelayer MessageRelayer) PopupService {
 	return PopupService{
-		legacyCharset:  legacyCharset,
 		messageRelayer: messageRelayer,
 	}
 }
 
 // PopupService implements the Popup food group.
 type PopupService struct {
-	legacyCharset  encoding.Encoding
 	messageRelayer MessageRelayer
 }
 
-// Display shows a popup to the given screen names.
+// Display shows a popup to the given screen names. Each screen name receives
+// the popup once.
 func (s PopupService) Display(ctx context.Context, screenNames []state.IdentScreenName, message string, url string, width uint16, height uint16) error {
 	msg, err := s.popupDisplay(message, url, width, height)
 	if err != nil {
 		return err
 	}
-	s.messageRelayer.RelayToScreenNames(ctx, screenNames, msg)
+	recipients := make([]state.IdentScreenName, 0, len(screenNames))
+	for _, screenName := range screenNames {
+		if !slices.Contains(recipients, screenName) {
+			recipients = append(recipients, screenName)
+		}
+	}
+	s.messageRelayer.RelayToScreenNames(ctx, recipients, msg)
 	return nil
 }
 
@@ -50,7 +56,7 @@ func (s PopupService) DisplayAll(ctx context.Context, message string, url string
 }
 
 func (s PopupService) popupDisplay(message string, url string, width uint16, height uint16) (wire.SNACMessage, error) {
-	text, err := encodeLegacy(s.legacyCharset, message)
+	text, err := encodeLegacy(charmap.Windows1252, message)
 	if err != nil {
 		return wire.SNACMessage{}, fmt.Errorf("encodeLegacy: %w", err)
 	}

foodgroup/popup_test.go

@@ -8,7 +8,6 @@ import (
 	"github.com/mk6i/open-oscar-server/wire"
 
 	"github.com/stretchr/testify/assert"
-	"golang.org/x/text/encoding/charmap"
 )
 
 func popupMessage(tlvs ...wire.TLV) wire.SNACMessage {
@@ -33,19 +32,21 @@ func TestPopupService_Display(t *testing.T) {
 	}
 
 	cases := []struct {
-		name       string
-		message    string
-		url        string
-		width      uint16
-		height     uint16
-		mockParams mockParams
+		name        string
+		screenNames []state.IdentScreenName
+		message     string
+		url         string
+		width       uint16
+		height      uint16
+		mockParams  mockParams
 	}{
 		{
-			name:    "all fields set",
-			message: "hello",
-			url:     "http://example.com",
-			width:   400,
-			height:  300,
+			name:        "all fields set",
+			screenNames: screenNames,
+			message:     "hello",
+			url:         "http://example.com",
+			width:       400,
+			height:      300,
 			mockParams: mockParams{
 				messageRelayerParams: messageRelayerParams{
 					relayToScreenNamesParams: relayToScreenNamesParams{
@@ -63,10 +64,11 @@ func TestPopupService_Display(t *testing.T) {
 			},
 		},
 		{
-			name:    "no url",
-			message: "hello",
-			width:   400,
-			height:  300,
+			name:        "no url",
+			screenNames: screenNames,
+			message:     "hello",
+			width:       400,
+			height:      300,
 			mockParams: mockParams{
 				messageRelayerParams: messageRelayerParams{
 					relayToScreenNamesParams: relayToScreenNamesParams{
@@ -83,10 +85,11 @@ func TestPopupService_Display(t *testing.T) {
 			},
 		},
 		{
-			name:    "non-ascii text",
-			message: "caf\u00e9 \u2122 \u65e5",
-			width:   400,
-			height:  300,
+			name:        "non-ascii text",
+			screenNames: screenNames,
+			message:     "caf\u00e9 \u2122 \u65e5",
+			width:       400,
+			height:      300,
 			mockParams: mockParams{
 				messageRelayerParams: messageRelayerParams{
 					relayToScreenNamesParams: relayToScreenNamesParams{
@@ -103,8 +106,30 @@ func TestPopupService_Display(t *testing.T) {
 			},
 		},
 		{
-			name:    "default dimensions",
-			message: "hello",
+			name:        "duplicate screen names",
+			screenNames: []state.IdentScreenName{screenNames[0], screenNames[1], screenNames[0]},
+			message:     "hello",
+			width:       400,
+			height:      300,
+			mockParams: mockParams{
+				messageRelayerParams: messageRelayerParams{
+					relayToScreenNamesParams: relayToScreenNamesParams{
+						{
+							screenNames: screenNames,
+							message: popupMessage(
+								wire.NewTLVBE(wire.PopupTLVMessage, []byte("hello")),
+								wire.NewTLVBE(wire.PopupTLVWidth, uint16(400)),
+								wire.NewTLVBE(wire.PopupTLVHeight, uint16(300)),
+							),
+						},
+					},
+				},
+			},
+		},
+		{
+			name:        "default dimensions",
+			screenNames: screenNames,
+			message:     "hello",
 			mockParams: mockParams{
 				messageRelayerParams: messageRelayerParams{
 					relayToScreenNamesParams: relayToScreenNamesParams{
@@ -130,8 +155,8 @@ func TestPopupService_Display(t *testing.T) {
 					RelayToScreenNames(matchContext(), params.screenNames, params.message)
 			}
 
-			svc := NewPopupService(messageRelayer, charmap.Windows1252)
-			err := svc.Display(context.Background(), screenNames, tc.message, tc.url, tc.width, tc.height)
+			svc := NewPopupService(messageRelayer)
+			err := svc.Display(context.Background(), tc.screenNames, tc.message, tc.url, tc.width, tc.height)
 			assert.NoError(t, err)
 		})
 	}
@@ -148,7 +173,7 @@ func TestPopupService_DisplayAll(t *testing.T) {
 	messageRelayer := newMockMessageRelayer(t)
 	messageRelayer.EXPECT().RelayToAll(matchContext(), want)
 
-	svc := NewPopupService(messageRelayer, charmap.Windows1252)
+	svc := NewPopupService(messageRelayer)
 	err := svc.DisplayAll(context.Background(), "hello", "http://example.com", 400, 300)
 	assert.NoError(t, err)
 }

server/http/mgmt_api.go

@@ -104,7 +104,7 @@ func NewManagementAPI(bld config.Build, listener string, userManager UserManager
 
 	// Handlers for '/popup' route
 	mux.HandleFunc("POST /popup", func(w http.ResponseWriter, r *http.Request) {
-		postPopupHandler(w, r, sessionRetriever, popupDisplayer, logger)
+		postPopupHandler(w, r, popupDisplayer, logger)
 	})
 
 	// Handlers for '/version' route
@@ -714,7 +714,7 @@ const (
 )
 
 // postPopupHandler handles the POST /popup endpoint.
-func postPopupHandler(w http.ResponseWriter, r *http.Request, sessionRetriever SessionRetriever, popupDisplayer PopupDisplayer, logger *slog.Logger) {
+func postPopupHandler(w http.ResponseWriter, r *http.Request, popupDisplayer PopupDisplayer, logger *slog.Logger) {
 	input := popup{}
 	if err := json.NewDecoder(r.Body).Decode(&input); err != nil {
 		http.Error(w, "malformed input", http.StatusBadRequest)
@@ -734,27 +734,14 @@ func postPopupHandler(w http.ResponseWriter, r *http.Request, sessionRetriever S
 	}
 
 	var err error
-	out := popupResult{Offline: []string{}}
 	if input.Broadcast {
 		err = popupDisplayer.DisplayAll(r.Context(), input.Message, input.URL, input.Width, input.Height)
 	} else {
-		online := make([]state.IdentScreenName, 0, len(input.Recipients))
+		screenNames := make([]state.IdentScreenName, 0, len(input.Recipients))
 		for _, recipient := range input.Recipients {
-			screenName := state.NewIdentScreenName(recipient)
-			if slices.Contains(online, screenName) {
-				continue
-			}
-			if sessionRetriever.RetrieveSession(screenName) == nil {
-				out.Offline = append(out.Offline, recipient)
-				continue
-			}
-			online = append(online, screenName)
-		}
-		if len(online) == 0 {
-			http.Error(w, "no recipients are online", http.StatusNotFound)
-			return
+			screenNames = append(screenNames, state.NewIdentScreenName(recipient))
 		}
-		err = popupDisplayer.Display(r.Context(), online, input.Message, input.URL, input.Width, input.Height)
+		err = popupDisplayer.Display(r.Context(), screenNames, input.Message, input.URL, input.Width, input.Height)
 	}
 	if err != nil {
 		logger.Error("error displaying popup POST /popup", "err", err.Error())
@@ -762,10 +749,8 @@ func postPopupHandler(w http.ResponseWriter, r *http.Request, sessionRetriever S
 		return
 	}
 
-	w.Header().Set("Content-Type", "application/json")
-	if err := json.NewEncoder(w).Encode(out); err != nil {
-		http.Error(w, err.Error(), http.StatusInternalServerError)
-	}
+	w.WriteHeader(http.StatusOK)
+	_, _ = fmt.Fprintln(w, "Popup sent successfully.")
 }
 
 // getUserBuddyIconHandler handles the GET /user/{screenname}/icon endpoint.

server/http/mgmt_api_test.go

@@ -1822,7 +1822,6 @@ func TestPopupHandler_POST(t *testing.T) {
 	tt := []struct {
 		name          string
 		body          string
-		sessions      map[state.IdentScreenName]*state.Session
 		wantDisplay   []state.IdentScreenName
 		wantBroadcast bool
 		wantMessage   string
@@ -1841,65 +1840,26 @@ func TestPopupHandler_POST(t *testing.T) {
 			wantURL:       "http://example.com",
 			wantWidth:     400,
 			wantHeight:    300,
-			want:          `{"offline":[]}`,
+			want:          `Popup sent successfully.`,
 			statusCode:    http.StatusOK,
 		},
 		{
-			name: "all recipients online",
-			body: `{"recipients":["alice","bob"],"message":"hello"}`,
-			sessions: map[state.IdentScreenName]*state.Session{
-				alice: state.NewSession(),
-				bob:   state.NewSession(),
-			},
+			name:        "recipients",
+			body:        `{"recipients":["Alice","bob"],"message":"hello"}`,
 			wantDisplay: []state.IdentScreenName{alice, bob},
 			wantMessage: "hello",
-			want:        `{"offline":[]}`,
+			want:        `Popup sent successfully.`,
 			statusCode:  http.StatusOK,
 		},
 		{
-			name: "some recipients offline",
-			body: `{"recipients":["alice","bob"],"message":"hello"}`,
-			sessions: map[state.IdentScreenName]*state.Session{
-				alice: state.NewSession(),
-				bob:   nil,
-			},
-			wantDisplay: []state.IdentScreenName{alice},
-			wantMessage: "hello",
-			want:        `{"offline":["bob"]}`,
-			statusCode:  http.StatusOK,
-		},
-		{
-			name: "duplicate recipients",
-			body: `{"recipients":["alice","ALICE","Ali Ce"],"message":"hello"}`,
-			sessions: map[state.IdentScreenName]*state.Session{
-				alice: state.NewSession(),
-			},
-			wantDisplay: []state.IdentScreenName{alice},
-			wantMessage: "hello",
-			want:        `{"offline":[]}`,
-			statusCode:  http.StatusOK,
-		},
-		{
-			name: "display fails",
-			body: `{"recipients":["alice"],"message":"hello"}`,
-			sessions: map[state.IdentScreenName]*state.Session{
-				alice: state.NewSession(),
-			},
+			name:        "display fails",
+			body:        `{"recipients":["alice"],"message":"hello"}`,
 			wantDisplay: []state.IdentScreenName{alice},
 			wantMessage: "hello",
 			displayErr:  io.EOF,
 			want:        `internal server error`,
 			statusCode:  http.StatusInternalServerError,
 		},
-		{
-			name: "no recipients online",
-			body: `{"recipients":["alice"],"message":"hello"}`,
-			sessions: map[state.IdentScreenName]*state.Session{
-				alice: nil,
-			},
-			want:       `no recipients are online`,
-			statusCode: http.StatusNotFound,
-		},
 		{
 			name:       "neither recipients nor broadcast",
 			body:       `{"message":"hello"}`,
@@ -1946,11 +1906,6 @@ func TestPopupHandler_POST(t *testing.T) {
 
 	for _, tc := range tt {
 		t.Run(tc.name, func(t *testing.T) {
-			sessionRetriever := newMockSessionRetriever(t)
-			for screenName, sess := range tc.sessions {
-				sessionRetriever.EXPECT().RetrieveSession(screenName).Return(sess)
-			}
-
 			popupDisplayer := newMockPopupDisplayer(t)
 			if tc.wantBroadcast {
 				popupDisplayer.EXPECT().
@@ -1966,7 +1921,7 @@ func TestPopupHandler_POST(t *testing.T) {
 			request := httptest.NewRequest(http.MethodPost, "/popup", strings.NewReader(tc.body))
 			responseRecorder := httptest.NewRecorder()
 
-			postPopupHandler(responseRecorder, request, sessionRetriever, popupDisplayer, slog.Default())
+			postPopupHandler(responseRecorder, request, popupDisplayer, slog.Default())
 
 			assert.Equal(t, tc.statusCode, responseRecorder.Code)
 			assert.Equal(t, tc.want, strings.TrimSpace(responseRecorder.Body.String()))

server/http/types.go

@@ -308,10 +308,6 @@ type popup struct {
 	Height     uint16   `json:"height"`
 }
 
-type popupResult struct {
-	Offline []string `json:"offline"`
-}
-
 type directoryKeyword struct {
 	ID   uint8  `json:"id"`
 	Name string `json:"name"`