fix(server): skip existing users when sharing albums (#28884)

Co-authored-by: Daniel Dietzler <mail@ddietzler.dev>

e2e/src/specs/server/api/album.e2e-spec.ts

@@ -730,8 +730,8 @@ describe('/albums', () => {
         .set('Authorization', `Bearer ${user1.accessToken}`)
         .send({ albumUsers: [{ userId: user1.userId, role: AlbumUserRole.Editor }] });
 
-      expect(status).toBe(400);
+      expect(status).toBe(200);
-      expect(body).toEqual(errorDto.badRequest('User already added'));
+      expect(body.albumUsers.length).toEqual(1);
     });
 
     it('should not be able to add existing user to shared album', async () => {
@@ -745,8 +745,8 @@ describe('/albums', () => {
         .set('Authorization', `Bearer ${user1.accessToken}`)
         .send({ albumUsers: [{ userId: user2.userId, role: AlbumUserRole.Editor }] });
 
-      expect(status).toBe(400);
+      expect(status).toBe(200);
-      expect(body).toEqual(errorDto.badRequest('User already added'));
+      expect(body.albumUsers.length).toEqual(2);
     });
   });
 

server/src/services/album.service.spec.ts

@@ -472,17 +472,19 @@ describe(AlbumService.name, () => {
       expect(mocks.album.update).not.toHaveBeenCalled();
     });
 
-    it('should throw an error if the userId is already added', async () => {
+    it('should skip if the userId is already added', async () => {
       const userId = newUuid();
       const album = AlbumFactory.from().albumUser({ userId }).build();
       const { user: owner } = album.albumUsers.find(({ role }) => role === AlbumUserRole.Owner)!;
       mocks.access.album.checkOwnerAccess.mockResolvedValue(new Set([album.id]));
       mocks.album.getById.mockResolvedValue(getForAlbum(album));
-      await expect(
+      await expect(sut.addUsers(AuthFactory.create(owner), album.id, { albumUsers: [{ userId }] })).resolves.toEqual(
-        sut.addUsers(AuthFactory.create(owner), album.id, { albumUsers: [{ userId }] }),
+        expect.objectContaining({ id: album.id }),
-      ).rejects.toBeInstanceOf(BadRequestException);
+      );
       expect(mocks.album.update).not.toHaveBeenCalled();
       expect(mocks.user.get).not.toHaveBeenCalled();
+      expect(mocks.albumUser.create).not.toHaveBeenCalled();
+      expect(mocks.event.emit).not.toHaveBeenCalled();
     });
 
     it('should throw an error if the userId does not exist', async () => {
@@ -498,7 +500,7 @@ describe(AlbumService.name, () => {
       expect(mocks.user.get).toHaveBeenCalledWith('unknown-user', {});
     });
 
-    it('should throw an error if the userId is the ownerId', async () => {
+    it('should skip if the userId is the ownerId', async () => {
       const album = AlbumFactory.create();
       const { user: owner } = album.albumUsers.find(({ role }) => role === AlbumUserRole.Owner)!;
       mocks.access.album.checkOwnerAccess.mockResolvedValue(new Set([album.id]));
@@ -507,9 +509,11 @@ describe(AlbumService.name, () => {
         sut.addUsers(AuthFactory.create(owner), album.id, {
           albumUsers: [{ userId: owner.id }],
         }),
-      ).rejects.toBeInstanceOf(BadRequestException);
+      ).resolves.toEqual(expect.objectContaining({ id: album.id }));
       expect(mocks.album.update).not.toHaveBeenCalled();
       expect(mocks.user.get).not.toHaveBeenCalled();
+      expect(mocks.albumUser.create).not.toHaveBeenCalled();
+      expect(mocks.event.emit).not.toHaveBeenCalled();
     });
 
     it('should add valid shared users', async () => {
@@ -534,6 +538,35 @@ describe(AlbumService.name, () => {
         senderName: owner.name,
       });
     });
+
+    it('should add new users when already-added users are included', async () => {
+      const existingUserId = newUuid();
+      const album = AlbumFactory.from().albumUser({ userId: existingUserId }).build();
+      const { user: owner } = album.albumUsers.find(({ role }) => role === AlbumUserRole.Owner)!;
+      const user = UserFactory.create();
+      mocks.access.album.checkOwnerAccess.mockResolvedValue(new Set([album.id]));
+      mocks.album.getById.mockResolvedValue(getForAlbum(album));
+      mocks.user.get.mockResolvedValue(user);
+      mocks.albumUser.create.mockResolvedValue(AlbumUserFactory.from().album(album).user(user).build());
+
+      await sut.addUsers(AuthFactory.create(owner), album.id, {
+        albumUsers: [{ userId: existingUserId }, { userId: user.id }],
+      });
+
+      expect(mocks.user.get).toHaveBeenCalledTimes(1);
+      expect(mocks.user.get).toHaveBeenCalledWith(user.id, {});
+      expect(mocks.albumUser.create).toHaveBeenCalledTimes(1);
+      expect(mocks.albumUser.create).toHaveBeenCalledWith({
+        userId: user.id,
+        albumId: album.id,
+      });
+      expect(mocks.event.emit).toHaveBeenCalledTimes(1);
+      expect(mocks.event.emit).toHaveBeenCalledWith('AlbumInvite', {
+        id: album.id,
+        userId: user.id,
+        senderName: owner.name,
+      });
+    });
   });
 
   describe('removeUser', () => {

server/src/services/album.service.ts

@@ -290,7 +290,7 @@ export class AlbumService extends BaseService {
 
       const exists = album.albumUsers.find(({ user: { id } }) => id === userId);
       if (exists) {
-        throw new BadRequestException('User already added');
+        continue;
       }
 
       const user = await this.userRepository.get(userId, {});