feat(url-clicks): update url repository to use new scopes

opengovsg · Dec 21, 2020 · 4940cf1 · 4940cf1
1 parent bdd88c3
commit 4940cf1
Show file tree

Hide file tree

Showing 7 changed files with 201 additions and 83 deletions.
diff --git a/src/server/mappers/UrlMapper.ts b/src/server/mappers/UrlMapper.ts
@@ -11,11 +11,14 @@ export class UrlMapper implements Mapper<StorableUrl, UrlType> {
     if (!urlType) {
       return null
     }
+    const { UrlClicks: urlClicks } = urlType
+    if (!urlClicks || !Number.isInteger(urlClicks.clicks))
+      throw new Error('UrlClicks object not populated.')
     return {
       shortUrl: urlType.shortUrl,
       longUrl: urlType.longUrl,
       state: urlType.state,
-      clicks: urlType.clicks,
+      clicks: urlClicks.clicks,
       isFile: urlType.isFile,
       createdAt: urlType.createdAt,
       updatedAt: urlType.updatedAt,

diff --git a/src/server/repositories/UrlRepository.ts b/src/server/repositories/UrlRepository.ts
@@ -51,32 +51,40 @@ export class UrlRepository implements UrlRepositoryInterface {
   public findByShortUrl: (
     shortUrl: string,
   ) => Promise<StorableUrl | null> = async (shortUrl) => {
-    return (await Url.findOne({
+    const url = await Url.scope('getClicks').findOne({
       where: { shortUrl },
-    })) as StorableUrl | null
+    })
+    return this.urlMapper.persistenceToDto(url)
   }
 
   public create: (
     properties: { userId: number; shortUrl: string; longUrl?: string },
     file?: StorableFile,
   ) => Promise<StorableUrl> = async (properties, file) => {
     const newUrl = await sequelize.transaction(async (t) => {
-      const url = Url.create(
+      await Url.create(
         {
           ...properties,
           longUrl: file
             ? this.fileBucket.buildFileLongUrl(file.key)
             : properties.longUrl,
           isFile: !!file,
         },
-        { transaction: t },
+        {
+          transaction: t,
+        },
       )
       if (file) {
         await this.fileBucket.uploadFileToS3(file.data, file.key, file.mimetype)
       }
-      return url
+
+      // Do a fresh read which eagerly loads the associated UrlClicks field.
+      return Url.scope('getClicks').findByPk(properties.shortUrl, {
+        transaction: t,
+      })
     })
 
+    if (!newUrl) throw new Error('Newly-created url is null')
     return this.urlMapper.persistenceToDto(newUrl)
   }
 
@@ -86,7 +94,7 @@ export class UrlRepository implements UrlRepositoryInterface {
     file?: StorableFile,
   ) => Promise<StorableUrl> = async (originalUrl, changes, file) => {
     const { shortUrl } = originalUrl
-    const url = await Url.findOne({ where: { shortUrl } })
+    const url = await Url.scope('getClicks').findOne({ where: { shortUrl } })
     if (!url) {
       throw new NotFoundError(
         `url not found in database:\tshortUrl=${shortUrl}`,

diff --git a/test/server/api/setup.ts b/test/server/api/setup.ts
@@ -10,6 +10,7 @@ import {
   mockQuery,
   mockTransaction,
   redisMockClient,
+  urlClicksModelMock,
   urlModelMock,
   userModelMock,
 } from './util'
@@ -85,6 +86,10 @@ jest.mock('../../../src/server/models/url', () => ({
   Url: urlModelMock,
 }))
 
+jest.mock('../../../src/server/models/statistics/clicks', () => ({
+  UrlClicks: urlClicksModelMock,
+}))
+
 // Necessary mock for app to work
 jest.mock('../../../src/server/models/statistics/daily', () => ({
   DailyClicks: clicksModelMock,

diff --git a/test/server/api/util.ts b/test/server/api/util.ts
@@ -99,7 +99,9 @@ export const urlModelMock = sequelizeMock.define(
     shortUrl: 'a',
     longUrl: 'aa',
     state: ACTIVE,
-    clicks: 8,
+    UrlClicks: {
+      clicks: 3,
+    },
   },
   {
     instanceMethods: {
@@ -109,6 +111,11 @@ export const urlModelMock = sequelizeMock.define(
   },
 )
 
+export const urlClicksModelMock = sequelizeMock.define('url_clicks', {
+  shortUrl: 'a',
+  clicks: 3,
+})
+
 export const clicksModelMock = sequelizeMock.define(
   'daily_stats',
   [

diff --git a/test/server/repositories/LinkStatisticsRepository.test.ts b/test/server/repositories/LinkStatisticsRepository.test.ts
@@ -24,6 +24,7 @@ jest.mock('../../../src/server/models/statistics/weekday', () => ({
 }))
 
 const findOne = jest.spyOn(urlModelMock, 'findOne')
+const scope = jest.spyOn(urlModelMock, 'scope')
 
 const shortUrl = 'short-url'
 
@@ -32,6 +33,7 @@ const repository = new LinkStatisticsRepository()
 describe('LinkStatisticsRepository', () => {
   beforeEach(async () => {
     findOne.mockClear()
+    scope.mockClear()
   })
 
   it('returns null on no url', async () => {
@@ -52,24 +54,27 @@ describe('LinkStatisticsRepository', () => {
 
   it('returns values on some device click stats', async () => {
     const url = {
-      clicks: 4,
       DeviceClicks: { toJSON: () => ({ desktop: 1 }) },
       DailyClicks: [{ date: 'today', clicks: 2 }],
       WeekdayClicks: [
         { weekday: 0, hours: 12, clicks: 3 },
         { weekday: 0, hours: 13, clicks: 1 },
       ],
+      UrlClicks: {
+        clicks: 2,
+      },
     }
     findOne.mockResolvedValue(url)
     await expect(repository.findByShortUrl(shortUrl)).resolves.toStrictEqual({
-      totalClicks: url.clicks,
+      totalClicks: url.UrlClicks.clicks,
       deviceClicks: url.DeviceClicks.toJSON(),
       dailyClicks: url.DailyClicks,
       weekdayClicks: url.WeekdayClicks,
     })
     expect(findOne).toBeCalledWith(
       expect.objectContaining({ where: { shortUrl } }),
     )
+    expect(scope).toBeCalledWith('getClicks')
   })
 
   it('correctly interpolates table names into update_link_statistics', () => {