From 96d34092d90d619174ad7e55904c6efe4c862ad3 Mon Sep 17 00:00:00 2001
From: Lewis Marshall <lewis.marshall@ably.com>
Date: Wed, 2 Aug 2023 19:26:03 +0100
Subject: [PATCH] space: Fix clobbering presence data

Before this commit, space.updateProfileData would call presence.update
with just the updated profile data, removing the previous and current
locations that might be set in the presence data.

This commit fixes the issue by introducing an updateSelf function which
allows callers to mutate the member retrieved from getSelf and trigger a
presence update which maintains unrelated fields within the resulting
presence data.

Signed-off-by: Lewis Marshall <lewis.marshall@ably.com>
---
 src/Locations.mockClient.test.ts | 17 +++++++++++++++++
 src/Locations.ts                 |  9 ++++-----
 src/Space.mockClient.test.ts     | 28 ++++++++++++++++++++++++++++
 src/Space.ts                     | 30 ++++++++++++++++++++++++++++--
 4 files changed, 77 insertions(+), 7 deletions(-)

diff --git a/src/Locations.mockClient.test.ts b/src/Locations.mockClient.test.ts
index 7f4f628d..fca2d725 100644
--- a/src/Locations.mockClient.test.ts
+++ b/src/Locations.mockClient.test.ts
@@ -38,6 +38,23 @@ describe('Locations (mockClient)', () => {
       await space.enter();
       space.locations.set('location1');
       expect(spy).toHaveBeenCalledOnce();
+      expect(spy).toHaveBeenCalledWith({
+        profileData: {},
+        currentLocation: 'location1',
+      });
+    });
+
+    it<SpaceTestContext>('sets previousLocation after setting location multiple times', async ({ space, presence }) => {
+      const spy = vi.spyOn(presence, 'update');
+      await space.enter();
+      space.locations.set('location1');
+      space.locations.set('location2');
+      expect(spy).toHaveBeenCalledTimes(2);
+      expect(spy).toHaveBeenNthCalledWith(2, {
+        profileData: {},
+        previousLocation: 'location1',
+        currentLocation: 'location2',
+      });
     });
 
     it<SpaceTestContext>('fires an event when a location is set', async ({ space }) => {
diff --git a/src/Locations.ts b/src/Locations.ts
index f8839ce0..0b2fe98c 100644
--- a/src/Locations.ts
+++ b/src/Locations.ts
@@ -37,11 +37,10 @@ export default class Locations extends EventEmitter<LocationEventMap> {
       throw new Error('Must enter a space before setting a location');
     }
 
-    return this.channel.presence.update({
-      profileData: self.profileData,
-      previousLocation: self.location,
-      currentLocation: location,
-    });
+    self.previousLocation = self.location;
+    self.location = location;
+
+    return this.space.updateSelf(self);
   }
 
   subscribe<K extends EventKey<LocationEventMap>>(
diff --git a/src/Space.mockClient.test.ts b/src/Space.mockClient.test.ts
index 1ee6056c..02db72ed 100644
--- a/src/Space.mockClient.test.ts
+++ b/src/Space.mockClient.test.ts
@@ -148,6 +148,34 @@ describe('Space (mockClient)', () => {
         await space.updateProfileData((profileData) => ({ ...profileData, a: 2 }));
         expect(updateSpy).toHaveBeenNthCalledWith(1, { profileData: { a: 2 } });
       });
+
+      it<SpaceTestContext>('maintains the members location in presence data', async ({ presence, space }) => {
+        const spy = vi.spyOn(presence, 'update');
+        vi.spyOn(space, 'getSelf').mockReturnValue({
+          clientId: '1',
+          connectionId: 'testConnectionId',
+          isConnected: true,
+          location: null,
+          lastEvent: {
+            name: 'enter',
+            timestamp: 1,
+          },
+          profileData: {
+            a: 1,
+          },
+        });
+        await space.locations.set('location1');
+        await space.updateProfileData({ a: 2 });
+        expect(spy).toHaveBeenCalledTimes(2);
+        expect(spy).toHaveBeenNthCalledWith(1, {
+          profileData: { a: 1 },
+          currentLocation: 'location1',
+        });
+        expect(spy).toHaveBeenNthCalledWith(2, {
+          profileData: { a: 2 },
+          currentLocation: 'location1',
+        });
+      });
     });
   });
 
diff --git a/src/Space.ts b/src/Space.ts
index c7ee02de..bc87ca21 100644
--- a/src/Space.ts
+++ b/src/Space.ts
@@ -20,6 +20,7 @@ export type SpaceMember = {
   isConnected: boolean;
   profileData: { [key: string]: any };
   location: any;
+  previousLocation?: any;
   lastEvent: {
     name: Types.PresenceAction;
     timestamp: number;
@@ -32,6 +33,12 @@ type SpaceLeaver = {
   timeoutId: ReturnType<typeof setTimeout>;
 };
 
+interface SpaceMemberData {
+  profileData?: any;
+  currentLocation?: any;
+  previousLocation?: any;
+}
+
 const SPACE_OPTIONS_DEFAULTS = {
   offlineTimeout: 120_000,
 };
@@ -210,11 +217,13 @@ class Space extends EventEmitter<SpaceEventsMap> {
       return;
     } else if (isFunction(profileDataOrUpdateFn) && self) {
       const update = profileDataOrUpdateFn(self.profileData);
-      await this.channel.presence.update({ profileData: update });
+      self.profileData = update;
+      await this.updateSelf(self);
       return;
     }
 
-    await this.channel.presence.update({ profileData: profileDataOrUpdateFn });
+    self.profileData = profileDataOrUpdateFn;
+    await this.updateSelf(self);
     return;
   }
 
@@ -234,6 +243,23 @@ class Space extends EventEmitter<SpaceEventsMap> {
     return;
   }
 
+  updateSelf(member: SpaceMember): Promise<void> {
+    const data: SpaceMemberData = {};
+
+    if (member.profileData) {
+      data.profileData = member.profileData;
+    }
+
+    if (member.location) {
+      data.currentLocation = member.location;
+    }
+    if (member.previousLocation) {
+      data.previousLocation = member.previousLocation;
+    }
+
+    return this.channel.presence.update(data);
+  }
+
   subscribe<K extends EventKey<SpaceEventsMap>>(
     listenerOrEvents?: K | K[] | EventListener<SpaceEventsMap[K]>,
     listener?: EventListener<SpaceEventsMap[K]>,