fix(links): just lookup existence and ownership once

opengovsg · Sep 29, 2020 · 5e12ca5 · 5e12ca5
1 parent 1fc6f14
commit 5e12ca5
Show file tree

Hide file tree

Showing 2 changed files with 10 additions and 10 deletions.
diff --git a/src/server/services/UrlManagementService.ts b/src/server/services/UrlManagementService.ts
@@ -45,11 +45,10 @@ export class UrlManagementService implements UrlManagementServiceInterface {
       throw new NotFoundError('User not found')
     }
 
-    const existsShortUrl = await this.urlRepository.findByShortUrl(shortUrl)
-    if (existsShortUrl) {
-      const owner = await this.userRepository.findUserByUrl(shortUrl)
+    const owner = await this.userRepository.findUserByUrl(shortUrl)
+    if (owner) {
       throw new AlreadyExistsError(
-        `Short link "${shortUrl}" is owned by ${owner?.email}`,
+        `Short link "${shortUrl}" is owned by ${owner.email}`,
       )
     }
 

diff --git a/test/server/services/UrlManagementService.test.ts b/test/server/services/UrlManagementService.test.ts
@@ -32,6 +32,7 @@ describe('UrlManagementService', () => {
 
     beforeEach(() => {
       userRepository.findById.mockReset()
+      userRepository.findUserByUrl.mockReset()
       urlRepository.findByShortUrl.mockReset()
       urlRepository.create.mockReset()
     })
@@ -42,22 +43,22 @@ describe('UrlManagementService', () => {
         service.createUrl(userId, shortUrl, longUrl),
       ).rejects.toBeInstanceOf(NotFoundError)
       expect(userRepository.findById).toHaveBeenCalledWith(userId)
-      expect(urlRepository.findByShortUrl).not.toHaveBeenCalled()
+      expect(userRepository.findUserByUrl).not.toHaveBeenCalled()
       expect(urlRepository.create).not.toHaveBeenCalled()
     })
 
     it('throws AlreadyExistsError on existing url', async () => {
       userRepository.findById.mockResolvedValue({ id: userId })
-      urlRepository.findByShortUrl.mockResolvedValue({
+      userRepository.findUserByUrl.mockResolvedValue({
         shortUrl,
         longUrl,
-        userId,
+        email: userId,
       })
       await expect(
         service.createUrl(userId, shortUrl, longUrl),
       ).rejects.toBeInstanceOf(AlreadyExistsError)
       expect(userRepository.findById).toHaveBeenCalledWith(userId)
-      expect(urlRepository.findByShortUrl).toHaveBeenCalledWith(shortUrl)
+      expect(userRepository.findUserByUrl).toHaveBeenCalledWith(shortUrl)
       expect(urlRepository.create).not.toHaveBeenCalled()
     })
 
@@ -69,7 +70,7 @@ describe('UrlManagementService', () => {
         service.createUrl(userId, shortUrl, longUrl),
       ).resolves.toStrictEqual({ userId, longUrl, shortUrl })
       expect(userRepository.findById).toHaveBeenCalledWith(userId)
-      expect(urlRepository.findByShortUrl).toHaveBeenCalledWith(shortUrl)
+      expect(userRepository.findUserByUrl).toHaveBeenCalledWith(shortUrl)
       expect(urlRepository.create).toHaveBeenCalledWith(
         { userId, longUrl, shortUrl },
         undefined,
@@ -89,7 +90,7 @@ describe('UrlManagementService', () => {
         service.createUrl(userId, shortUrl, longUrl, file),
       ).resolves.toStrictEqual({ userId, longUrl, shortUrl })
       expect(userRepository.findById).toHaveBeenCalledWith(userId)
-      expect(urlRepository.findByShortUrl).toHaveBeenCalledWith(shortUrl)
+      expect(userRepository.findUserByUrl).toHaveBeenCalledWith(shortUrl)
       expect(urlRepository.create).toHaveBeenCalledWith(
         { userId, longUrl, shortUrl },
         {