From 66cc18bedb1f5bbc948b733bafdf430edf0bfba7 Mon Sep 17 00:00:00 2001 From: TooSeriuz Date: Thu, 11 Apr 2024 15:55:38 +0700 Subject: [PATCH 1/4] #584 Fix converter cannot take null value, Fix update stream name get default timezone and UTC, add more tests --- common/converter/conversion.js | 10 +- core/internal/arbimon/stream.int.test.js | 129 ++++++++++++++++++++++- core/streams/dao/index.js | 16 ++- core/streams/update.int.test.js | 96 +++++++++++++++-- 4 files changed, 238 insertions(+), 13 deletions(-) diff --git a/common/converter/conversion.js b/common/converter/conversion.js index 44292a1b8..a4d710206 100644 --- a/common/converter/conversion.js +++ b/common/converter/conversion.js @@ -10,10 +10,10 @@ module.exports = class Conversion { this.target = target this.property = property - this.value = null + this.value = undefined this.conversions = [] this.required = true - this.defaultValue = null + this.defaultValue = undefined } execute () { @@ -23,8 +23,12 @@ module.exports = class Conversion { this.value = this.src[this.property] !== undefined ? this.src[this.property] : this.defaultValue + if (!this.required && this.value === undefined) { + return + } if (!this.required && this.value === null) { - return this.value + this.target[this.property] = this.value + return } for (const executeValidation of this.conversions) { diff --git a/core/internal/arbimon/stream.int.test.js b/core/internal/arbimon/stream.int.test.js index b1d5d7934..45547a9da 100644 --- a/core/internal/arbimon/stream.int.test.js +++ b/core/internal/arbimon/stream.int.test.js @@ -28,11 +28,13 @@ beforeEach(async () => { const PROJECT_1 = { id: 'testproj0001', name: 'Test project 1', externalId: 1, createdById: seedValues.primaryUserId } const PROJECT_2 = { id: 'testproj0002', name: 'Test project 2', externalId: 2, createdById: seedValues.otherUserId } const PROJECT_3 = { id: 'testproj0003', name: 'Test project 3', externalId: 3, createdById: seedValues.primaryUserId } -const PROJECTS = [PROJECT_1, PROJECT_2, PROJECT_3] +const PROJECT_4 = { id: 'testproj0004', name: 'Test project 4', externalId: 4, createdById: seedValues.primaryUserId } +const PROJECTS = [PROJECT_1, PROJECT_2, PROJECT_3, PROJECT_4] const STREAM_1 = { id: 'LilSjZJkRK43', name: 'Stream 1', projectId: PROJECT_1.id, externalId: 1, createdById: seedValues.primaryUserId } const STREAM_2 = { id: 'LilSjZJkRK46', name: 'Stream 2', projectId: PROJECT_2.id, externalId: 2, isPublic: true, createdById: seedValues.otherUserId } -const STREAMS = [STREAM_1, STREAM_2] +const STREAM_3 = { id: 'LilSjZJkRK49', name: 'Stream 3', projectId: PROJECT_4.id, externalId: 3, latitude: 10, longitude: 103, createdById: seedValues.primaryUserId, countryCode: 'TH', timezone: 'Asia/Bangkok' } +const STREAMS = [STREAM_1, STREAM_2, STREAM_3] async function commonSetup () { await models.Project.bulkCreate(PROJECTS) @@ -54,6 +56,127 @@ describe('PATCH /internal/arbimon/streams/:externalId', () => { expect(stream.name).toBe(body.name) }) + test('with [`name`] params should not change timezone and country code', async () => { + const body = { + name: 'New Stream 3' + } + + const response = await request(app).patch(`/streams/${STREAM_3.externalId}`).send(body) + + expect(response.statusCode).toBe(200) + const stream = await models.Stream.findByPk(STREAM_3.id) + expect(stream.name).toBe(body.name) + expect(stream.latitude).toBe(STREAM_3.latitude) + expect(stream.longitude).toBe(STREAM_3.longitude) + expect(stream.timezone).toBe(STREAM_3.timezone) + expect(stream.countryCode).toBe(STREAM_3.countryCode) + }) + + test('with [`name`, `latitude` = undefined, `longitude` = undefined] params should not change timezone and country code', async () => { + const body = { + name: 'New Stream 3', + latitude: undefined, + longitude: undefined + } + + const response = await request(app).patch(`/streams/${STREAM_3.externalId}`).send(body) + + expect(response.statusCode).toBe(200) + const stream = await models.Stream.findByPk(STREAM_3.id) + expect(stream.name).toBe(body.name) + expect(stream.latitude).toBe(STREAM_3.latitude) + expect(stream.longitude).toBe(STREAM_3.longitude) + expect(stream.timezone).toBe(STREAM_3.timezone) + expect(stream.countryCode).toBe(STREAM_3.countryCode) + }) + + test('with [`name`, `latitude` = 0, `longitude` = 0] params should change timezone and country code to default values', async () => { + const body = { + name: 'New Stream 3', + latitude: 0, + longitude: 0 + } + + const response = await request(app).patch(`/streams/${STREAM_3.externalId}`).send(body) + + expect(response.statusCode).toBe(200) + const stream = await models.Stream.findByPk(STREAM_3.id) + expect(stream.name).toBe(body.name) + expect(stream.latitude).toBeNull() + expect(stream.longitude).toBeNull() + expect(stream.timezone).toBe('UTC') + expect(stream.countryCode).toBeNull() + }) + + test('with [`name`, `latitude`, `longitude` = null] params should change timezone and country code to default values', async () => { + const body = { + name: 'New Stream 3', + latitude: 10.1, + longitude: null + } + + const response = await request(app).patch(`/streams/${STREAM_3.externalId}`).send(body) + + expect(response.statusCode).toBe(200) + const stream = await models.Stream.findByPk(STREAM_3.id) + expect(stream.name).toBe(body.name) + expect(stream.latitude).toBe(body.latitude) + expect(stream.longitude).toBeNull() + expect(stream.timezone).toBe('UTC') + expect(stream.countryCode).toBeNull() + }) + + test('with [`name`, `latitude`= null, `longitude` = null] params should change timezone and country code to default values', async () => { + const body = { + name: 'New Stream 3', + latitude: null, + longitude: null + } + + const response = await request(app).patch(`/streams/${STREAM_3.externalId}`).send(body) + + expect(response.statusCode).toBe(200) + const stream = await models.Stream.findByPk(STREAM_3.id) + expect(stream.name).toBe(body.name) + expect(stream.latitude).toBeNull() + expect(stream.longitude).toBeNull() + expect(stream.timezone).toBe('UTC') + expect(stream.countryCode).toBeNull() + }) + + test('with [`name`, `latitude`, `longitude`] params should change timezone and country code correctly', async () => { + const body = { + name: 'New Stream 3', + longitude: 111 + } + const mockCountry = jest.spyOn(googleMap, 'getCountry') + mockCountry.mockReturnValueOnce({ + data: { + results: [{ + address_components: [{ + short_name: 'MY' + }] + }] + } + }) + const mockTimezone = jest.spyOn(googleMap, 'getTimezone') + mockTimezone.mockReturnValueOnce({ + data: { + timeZoneId: 'Asia/Myanmar' + } + }) + + const response = await request(app).patch(`/streams/${STREAM_3.externalId}`).send(body) + + expect(response.statusCode).toBe(200) + const stream = await models.Stream.findByPk(STREAM_3.id) + expect(stream.name).toBe(body.name) + expect(stream.latitude).toBe(STREAM_3.latitude) + expect(stream.longitude).toBe(body.longitude) + expect(stream.timezone).toBe('Asia/Myanmar') + expect(stream.countryCode).toBe('MY') + }) + test('with [`name`, `latitude`] params', async () => { const body = { name: 'New Stream 1', @@ -195,7 +318,7 @@ describe('PATCH /internal/arbimon/streams/:externalId', () => { const body = { name: 'New Stream 1', latitude: 19.9, - project_external_id: 4 + project_external_id: 5 } const response = await request(app).patch(`/streams/${STREAM_1.externalId}`).send(body) diff --git a/core/streams/dao/index.js b/core/streams/dao/index.js index 8ea3424a3..e1ceea38d 100644 --- a/core/streams/dao/index.js +++ b/core/streams/dao/index.js @@ -246,7 +246,21 @@ async function update (id, data, options = {}) { if (options.updatableBy && !(await hasPermission(UPDATE, options.updatableBy, id, STREAM))) { throw new ForbiddenError() } - const fullStream = { ...data, ...(await computedAdditions(data, stream)) } + // data.latitude or data.longitude is undefined mean it is not in the updated blob unlike null + // So need to assign the current stream latitude or longitude value + // If both are undefined mean there is no intention to update coordinate so no need to get additional data for timezone and country code + // Avoid change timezone and countryCode to default value + let additions = {} + if (data.latitude !== undefined || data.longitude !== undefined) { + if (data.latitude === undefined) { + data.latitude = stream.latitude + } + if (data.longitude === undefined) { + data.longitude = stream.longitude + } + additions = (data.latitude === undefined && data.longitude === undefined) ? {} : await computedAdditions(data, stream) + } + const fullStream = { ...data, ...additions } if (fullStream.name) { if (stream && stream.project_id && stream.name !== fullStream.name) { const duplicateStreamInProject = await query({ names: [fullStream.name], projects: [fullStream.project_id || stream.project_id] }, { fields: 'id', transaction }) diff --git a/core/streams/update.int.test.js b/core/streams/update.int.test.js index a6747d70b..73e137873 100644 --- a/core/streams/update.int.test.js +++ b/core/streams/update.int.test.js @@ -323,17 +323,36 @@ describe('PATCH /streams/:id', () => { expect(streamUpdated.countryCode).toBe('PL') }) - test('country code is changed to null and timezone is UTC for undefined lat', async () => { - const stream = { id: 'qwertyuiop40', name: 'my stream 4', latitude: 54.2, longitude: -4.5, timezone: 'Europe/Britain', countryCode: 'GB', createdById: seedValues.primaryUserId } + test('country code is not changed to null and timezone is UTC for undefined lat', async () => { + const stream = { id: 'qwertyuiop40', name: 'my stream 4', latitude: 54.2, longitude: -4.5, timezone: 'Europe/Isle_of_Man', countryCode: 'GB', createdById: seedValues.primaryUserId } await models.Stream.create(stream) await models.UserStreamRole.create({ stream_id: stream.id, user_id: stream.createdById, role_id: seedValues.roleOwner }) + const mockCountry = jest.spyOn(googleMap, 'getCountry') + mockCountry.mockReturnValueOnce({ + data: { + results: [{ + address_components: [{ + short_name: 'GB' + }] + }] + } + }) + const mockTimezone = jest.spyOn(googleMap, 'getTimezone') + mockTimezone.mockReturnValueOnce({ + data: { + timeZoneId: 'Europe/Isle_of_Man' + } + }) const requestBody = { latitude: undefined, longitude: -4.5 } const response = await request(app).patch(`/${stream.id}`).send(requestBody) + expect(response.statusCode).toBe(204) const streamUpdated = await models.Stream.findByPk(stream.id) - expect(streamUpdated.timezone).toBe('UTC') - expect(streamUpdated.countryCode).toBeNull() + expect(streamUpdated.latitude).toBe(stream.latitude) + expect(streamUpdated.longitude).toBe(requestBody.longitude) + expect(streamUpdated.timezone).toBe(stream.timezone) + expect(streamUpdated.countryCode).toBe(stream.countryCode) }) test('country code is changed to null and timezone is UTC for null lat', async () => { @@ -345,6 +364,8 @@ describe('PATCH /streams/:id', () => { const response2 = await request(app).patch(`/${stream.id}`).send(requestBody) expect(response2.statusCode).toBe(204) const streamUpdated = await models.Stream.findByPk(stream.id) + expect(streamUpdated.latitude).toBeNull() + expect(streamUpdated.longitude).toBe(requestBody.longitude) expect(streamUpdated.timezone).toBe('UTC') expect(streamUpdated.countryCode).toBeNull() }) @@ -358,6 +379,8 @@ describe('PATCH /streams/:id', () => { const response2 = await request(app).patch(`/${stream.id}`).send(requestBody) expect(response2.statusCode).toBe(204) const streamUpdated = await models.Stream.findByPk(stream.id) + expect(streamUpdated.latitude).toBeNull() + expect(streamUpdated.longitude).toBeNull() expect(streamUpdated.timezone).toBe('UTC') expect(streamUpdated.countryCode).toBeNull() }) @@ -371,6 +394,8 @@ describe('PATCH /streams/:id', () => { const response2 = await request(app).patch(`/${stream.id}`).send(requestBody) expect(response2.statusCode).toBe(204) const streamUpdated = await models.Stream.findByPk(stream.id) + expect(streamUpdated.latitude).toBeNull() + expect(streamUpdated.longitude).toBeNull() expect(streamUpdated.timezone).toBe('UTC') expect(streamUpdated.countryCode).toBeNull() }) @@ -384,6 +409,8 @@ describe('PATCH /streams/:id', () => { const response2 = await request(app).patch(`/${stream.id}`).send(requestBody) expect(response2.statusCode).toBe(204) const streamUpdated = await models.Stream.findByPk(stream.id) + expect(streamUpdated.latitude).toBeNull() + expect(streamUpdated.longitude).toBe(requestBody.longitude) expect(streamUpdated.timezone).toBe('UTC') expect(streamUpdated.countryCode).toBeNull() }) @@ -407,6 +434,8 @@ describe('PATCH /streams/:id', () => { const response2 = await request(app).patch(`/${stream.id}`).send(requestBody) expect(response2.statusCode).toBe(204) const streamUpdated = await models.Stream.findByPk(stream.id) + expect(streamUpdated.latitude).toBe(requestBody.latitude) + expect(streamUpdated.longitude).toBe(requestBody.longitude) expect(streamUpdated.countryCode).toBe(null) }) @@ -418,11 +447,24 @@ describe('PATCH /streams/:id', () => { const stream2 = { id: 'jagu2', createdById: seedValues.primaryUserId, name: 'Jaguar Station 2', latitude: 66.2, longitude: -10.5, projectId: project.id } await models.Stream.create(stream) await models.Stream.create(stream2) + const mockCountry = jest.spyOn(googleMap, 'getCountry') + mockCountry.mockReturnValueOnce({ + data: { + results: [] + } + }) + const mockTimezone = jest.spyOn(googleMap, 'getTimezone') + mockTimezone.mockReturnValueOnce({ + data: {} + }) const requestBody = { latitude: 40.2 } const response = await request(app).patch(`/${stream.id}`).send(requestBody) expect(response.statusCode).toBe(204) + const streamUpdated = await models.Stream.findByPk(stream.id) + expect(streamUpdated.latitude).toBe(requestBody.latitude) + expect(streamUpdated.longitude).toBe(stream.longitude) const projectAfterUpdated = await models.Project.findByPk(project.id) expect(projectAfterUpdated.minLatitude).toBe(requestBody.latitude) expect(projectAfterUpdated.maxLatitude).toBe(stream2.latitude) @@ -438,11 +480,25 @@ describe('PATCH /streams/:id', () => { const stream2 = { id: 'jagu2', createdById: seedValues.primaryUserId, name: 'Jaguar Station 2', latitude: 66.2, longitude: -10.5, projectId: project.id } await models.Stream.create(stream) await models.Stream.create(stream2) + const mockCountry = jest.spyOn(googleMap, 'getCountry') + mockCountry.mockReturnValueOnce({ + data: { + results: [] + } + }) + const mockTimezone = jest.spyOn(googleMap, 'getTimezone') + mockTimezone.mockReturnValueOnce({ + data: {} + }) const requestBody = { latitude: 40.2, hidden: true } const response = await request(app).patch(`/${stream.id}`).send(requestBody) expect(response.statusCode).toBe(204) + const streamUpdated = await models.Stream.findByPk(stream.id) + expect(streamUpdated.latitude).toBe(requestBody.latitude) + expect(streamUpdated.longitude).toBe(stream.longitude) + expect(streamUpdated.hidden).toBe(requestBody.hidden) const projectAfterUpdated = await models.Project.findByPk(project.id) expect(projectAfterUpdated.minLatitude).toBe(stream2.latitude) expect(projectAfterUpdated.maxLatitude).toBe(stream2.latitude) @@ -456,11 +512,25 @@ describe('PATCH /streams/:id', () => { await models.UserProjectRole.create({ user_id: seedValues.primaryUserId, project_id: project.id, role_id: seedValues.roleAdmin }) const stream = { id: 'jagu1', createdById: seedValues.primaryUserId, name: 'Jaguar Station', latitude: 54.2, longitude: -4.5, projectId: project.id } await models.Stream.create(stream) + const mockCountry = jest.spyOn(googleMap, 'getCountry') + mockCountry.mockReturnValueOnce({ + data: { + results: [] + } + }) + const mockTimezone = jest.spyOn(googleMap, 'getTimezone') + mockTimezone.mockReturnValueOnce({ + data: {} + }) const requestBody = { latitude: 40.2, hidden: true } const response = await request(app).patch(`/${stream.id}`).send(requestBody) expect(response.statusCode).toBe(204) + const streamUpdated = await models.Stream.findByPk(stream.id) + expect(streamUpdated.latitude).toBe(requestBody.latitude) + expect(streamUpdated.longitude).toBe(stream.longitude) + expect(streamUpdated.hidden).toBe(requestBody.hidden) const projectAfterUpdated = await models.Project.findByPk(project.id) expect(projectAfterUpdated.minLatitude).toBeNull() expect(projectAfterUpdated.maxLatitude).toBeNull() @@ -481,6 +551,9 @@ describe('PATCH /streams/:id', () => { const response = await request(app).patch(`/${stream.id}`).send(requestBody) expect(response.statusCode).toBe(204) + const streamUpdated = await models.Stream.findByPk(stream.id) + expect(streamUpdated.latitude).toBeNull() + expect(streamUpdated.longitude).toBe(stream.longitude) const projectAfterUpdated = await models.Project.findByPk(project.id) expect(projectAfterUpdated.minLatitude).toBe(stream2.latitude) expect(projectAfterUpdated.maxLatitude).toBe(stream2.latitude) @@ -501,6 +574,9 @@ describe('PATCH /streams/:id', () => { const response = await request(app).patch(`/${stream.id}`).send(requestBody) expect(response.statusCode).toBe(204) + const streamUpdated = await models.Stream.findByPk(stream.id) + expect(streamUpdated.latitude).toBeNull() + expect(streamUpdated.longitude).toBeNull() const projectAfterUpdated = await models.Project.findByPk(project.id) expect(projectAfterUpdated.minLatitude).toBe(stream2.latitude) expect(projectAfterUpdated.maxLatitude).toBe(stream2.latitude) @@ -514,11 +590,15 @@ describe('PATCH /streams/:id', () => { await models.UserProjectRole.create({ user_id: seedValues.primaryUserId, project_id: project.id, role_id: seedValues.roleAdmin }) const stream1 = { id: 'jagu1', createdById: seedValues.primaryUserId, name: 'Jaguar-Station', latitude: null, longitude: -4.5, projectId: project.id } await models.Stream.create(stream1) - const requestBody = { name: 'Jaguar_Station' } + const requestBody = { name: 'Jaguar_Station' } const response = await request(app).patch(`/${stream1.id}`).send(requestBody) expect(response.statusCode).toBe(204) + const streamUpdated = await models.Stream.findByPk(stream1.id) + expect(streamUpdated.name).toBe(requestBody.name) + expect(streamUpdated.latitude).toBe(stream1.latitude) + expect(streamUpdated.longitude).toBe(stream1.longitude) const stream = await models.Stream.findByPk(stream1.id) expect(stream.name).toBe(requestBody.name) }) @@ -529,11 +609,15 @@ describe('PATCH /streams/:id', () => { await models.UserProjectRole.create({ user_id: seedValues.primaryUserId, project_id: project.id, role_id: seedValues.roleAdmin }) const stream1 = { id: 'jagu1', createdById: seedValues.primaryUserId, name: 'Jaguar-Station-1', latitude: null, longitude: -4.5, projectId: project.id } await models.Stream.create(stream1) - const requestBody = { name: 'Jaguar_Station-1' } + const requestBody = { name: 'Jaguar_Station-1' } const response = await request(app).patch(`/${stream1.id}`).send(requestBody) expect(response.statusCode).toBe(204) + const streamUpdated = await models.Stream.findByPk(stream1.id) + expect(streamUpdated.name).toBe(requestBody.name) + expect(streamUpdated.latitude).toBe(stream1.latitude) + expect(streamUpdated.longitude).toBe(stream1.longitude) const stream = await models.Stream.findByPk(stream1.id) expect(stream.name).toBe(requestBody.name) }) From 91ed53e7f84c2f08e2ec3b8392180106b0b6322e Mon Sep 17 00:00:00 2001 From: TooSeriuz Date: Thu, 11 Apr 2024 16:04:44 +0700 Subject: [PATCH 2/4] #528 Add Changelog --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index bbe849929..c42ac10a9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,7 @@ +## 1.3.9 (2024-04-xx) +### Bug Fixes +* **core**: Fix update only stream name also set default to timezone and country code + ## 1.3.8 (2024-03-xx) ### Common * **core**: Use Google Map API for getting timezone and country from latitude and longitude From cf4380ea2b996aa2f26fcb83ac2abdfb34fd64ee Mon Sep 17 00:00:00 2001 From: TooSeriuz Date: Wed, 17 Apr 2024 01:18:59 +0700 Subject: [PATCH 3/4] Remove undefined check --- core/streams/dao/index.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/streams/dao/index.js b/core/streams/dao/index.js index e1ceea38d..a171e42a6 100644 --- a/core/streams/dao/index.js +++ b/core/streams/dao/index.js @@ -258,7 +258,7 @@ async function update (id, data, options = {}) { if (data.longitude === undefined) { data.longitude = stream.longitude } - additions = (data.latitude === undefined && data.longitude === undefined) ? {} : await computedAdditions(data, stream) + additions = await computedAdditions(data, stream) } const fullStream = { ...data, ...additions } if (fullStream.name) { From 7db1bbb46b685b612636974b7088ca921e1f598d Mon Sep 17 00:00:00 2001 From: TooSeriuz Date: Wed, 17 Apr 2024 13:34:25 +0700 Subject: [PATCH 4/4] New way checking null and undefined --- common/converter/conversion.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/common/converter/conversion.js b/common/converter/conversion.js index a4d710206..bf96deecb 100644 --- a/common/converter/conversion.js +++ b/common/converter/conversion.js @@ -17,7 +17,7 @@ module.exports = class Conversion { } execute () { - if (this.required && this.src[this.property] == null) { + if (this.required && this.src[this.property] === undefined) { this.throwError('the parameter is required but was not provided') }