Merge pull request #3546 from gradido/refactor_updateField

refactor(backend): add and use template function updateAllDefinedAndChanged in update user and community

backend/src/graphql/resolver/CommunityResolver.ts

@@ -4,7 +4,6 @@ import {
   getHomeCommunity 
 } from 'database'
 import { Arg, Args, Authorized, Mutation, Query, Resolver } from 'type-graphql'
-
 import { Paginated } from '@arg/Paginated'
 import { EditCommunityInput } from '@input/EditCommunityInput'
 import { AdminCommunityView } from '@model/AdminCommunityView'
@@ -19,6 +18,7 @@ import {
   getCommunityByIdentifier,
   getCommunityByUuid,
 } from './util/communities'
+import { updateAllDefinedAndChanged } from 'shared'
 
 import { CONFIG } from '@/config'
 
@@ -78,16 +78,20 @@ export class CommunityResolver {
     if (homeCom.foreign) {
       throw new LogError('Error: Only the HomeCommunity could be modified!')
     }
-    if (
-      homeCom.gmsApiKey !== gmsApiKey ||
-      homeCom.location !== location ||
-      homeCom.hieroTopicId !== hieroTopicId
-    ) {
-      homeCom.gmsApiKey = gmsApiKey ?? null
-      if (location) {
-        homeCom.location = Location2Point(location)
+    let updated = false
+    // if location is undefined, it should not be changed
+    // if location is null, it should be set to null
+    if (typeof location !== 'undefined') {
+      const newLocation = location ? Location2Point(location) : null
+      if (newLocation !== homeCom.location) {
+        homeCom.location = newLocation
+        updated = true
       }
-      homeCom.hieroTopicId = hieroTopicId ?? null
+    }
+    if (updateAllDefinedAndChanged(homeCom, { gmsApiKey, hieroTopicId })) {
+      updated = true
+    }
+    if (updated) {
       await DbCommunity.save(homeCom)
     }
     return new AdminCommunityView(homeCom)

backend/src/graphql/resolver/UserResolver.ts

@@ -25,7 +25,7 @@ import {
   Root,
 } from 'type-graphql'
 import { IRestResponse } from 'typed-rest-client'
-import { EntityNotFoundError, In, Point } from 'typeorm'
+import { EntityManager, EntityNotFoundError, In, Point } from 'typeorm'
 import { v4 as uuidv4 } from 'uuid'
 
 import { UserArgs } from '@arg//UserArgs'
@@ -104,6 +104,7 @@ import { deleteUserRole, setUserRole } from './util/modifyUserRole'
 import { sendUserToGms } from './util/sendUserToGms'
 import { syncHumhub } from './util/syncHumhub'
 import { validateAlias } from 'core'
+import { updateAllDefinedAndChanged } from 'shared'
 
 const LANGUAGES = ['de', 'en', 'es', 'fr', 'nl']
 const DEFAULT_LANGUAGE = 'de'
@@ -727,18 +728,22 @@ export class UserResolver {
       user.humhubPublishName as PublishNameType,
     )
 
-    // try {
-    if (firstName) {
-      user.firstName = firstName
-    }
-
-    if (lastName) {
-      user.lastName = lastName
-    }
+    let updated = updateAllDefinedAndChanged(user, { 
+      firstName, 
+      lastName, 
+      hideAmountGDD, 
+      hideAmountGDT, 
+      humhubAllowed, 
+      gmsAllowed, 
+      gmsPublishName: gmsPublishName?.valueOf(),
+      humhubPublishName: humhubPublishName?.valueOf(),
+      gmsPublishLocation: gmsPublishLocation?.valueOf(),
+    })
 
     // currently alias can only be set, not updated
     if (alias && !user.alias && (await validateAlias(alias))) {
       user.alias = alias
+      updated = true
     }
 
     if (language) {
@@ -748,6 +753,7 @@ export class UserResolver {
       }
       user.language = language
       i18n.setLocale(language)
+      updated = true
     }
 
     if (password && passwordNew) {
@@ -768,55 +774,28 @@ export class UserResolver {
       // Save new password hash and newly encrypted private key
       user.passwordEncryptionType = PasswordEncryptionType.GRADIDO_ID
       user.password = await encryptPassword(user, passwordNew)
+      updated = true
     }
 
-    // Save hideAmountGDD value
-    if (hideAmountGDD !== undefined) {
-      user.hideAmountGDD = hideAmountGDD
-    }
-    // Save hideAmountGDT value
-    if (hideAmountGDT !== undefined) {
-      user.hideAmountGDT = hideAmountGDT
-    }
-    if (humhubAllowed !== undefined) {
-      user.humhubAllowed = humhubAllowed
-    }
-    if (gmsAllowed !== undefined) {
-      user.gmsAllowed = gmsAllowed
-    }
-    if (gmsPublishName !== null && gmsPublishName !== undefined) {
-      user.gmsPublishName = gmsPublishName
-    }
-    if (humhubPublishName !== null && humhubPublishName !== undefined) {
-      user.humhubPublishName = humhubPublishName
-    }
     if (gmsLocation) {
       user.location = Location2Point(gmsLocation)
+      updated = true
     }
-    if (gmsPublishLocation !== null && gmsPublishLocation !== undefined) {
-      user.gmsPublishLocation = gmsPublishLocation
+
+    // early exit if no update was made
+    if (!updated) {
+      return true
     }
-    // } catch (err) {
-    //   console.log('error:', err)
-    // }
-    const queryRunner = db.getDataSource().createQueryRunner()
-    await queryRunner.connect()
-    await queryRunner.startTransaction('REPEATABLE READ')
 
     try {
-      await queryRunner.manager.save(user).catch((error) => {
-        throw new LogError('Error saving user', error)
-      })
-
-      await queryRunner.commitTransaction()
-      logger.debug('writing User data successful...', new UserLoggingView(user))
-    } catch (e) {
-      await queryRunner.rollbackTransaction()
-      throw new LogError('Error on writing updated user data', e)
-    } finally {
-      await queryRunner.release()
+      await DbUser.save(user)
+    } catch (error) {
+      const errorMessage = 'Error saving user'
+      logger.error(errorMessage, error)
+      throw new Error(errorMessage)
     }
     logger.info('updateUserInfos() successfully finished...')
+    logger.debug('writing User data successful...', new UserLoggingView(user))
     await EVENT_USER_INFO_UPDATE(user)
 
     // validate if user settings are changed with relevance to update gms-user

shared/src/helper/index.ts

@@ -0,0 +1 @@
+export * from './updateField'

shared/src/helper/updateField.test.ts

@@ -0,0 +1,68 @@
+import { updateAllDefinedAndChanged, updateIfDefinedAndChanged } from './updateField'
+
+describe('updateIfDefinedAndChanged', () => {
+  it('should update field if incoming is different from current', () => {
+    const current = { field: 'current' }
+    const incoming = 'incoming'
+    const result = updateIfDefinedAndChanged(current, 'field', incoming)
+    expect(result).toBe(true)
+    expect(current.field).toBe('incoming')
+  })
+  it('should not update field if incoming is the same as current', () => {
+    const current = { field: 'current' }
+    const incoming = 'current'
+    const result = updateIfDefinedAndChanged(current, 'field', incoming)
+    expect(result).toBe(false)
+    expect(current.field).toBe('current')
+  })
+  it('should not update field if incoming is undefined', () => {
+    const current = { field: 'current' }
+    const incoming = undefined
+    const result = updateIfDefinedAndChanged(current, 'field', incoming)
+    expect(result).toBe(false)
+    expect(current.field).toBe('current')
+  })
+  it('should update field if incoming is null', () => {
+    type TestEntity = { field: string | null }
+    const current: TestEntity = { field: 'current' }
+    const incoming = null
+    const result = updateIfDefinedAndChanged(current, 'field', incoming)
+    expect(result).toBe(true)
+    expect(current.field).toBe(null)
+  })
+})
+
+describe('updateAllDefinedAndChanged', () => {
+  it('should update all fields if incoming is different from current', () => {
+    type TestEntity = { field1: string | null, field2: string | null, field3: string | null }
+    const current: TestEntity = { field1: 'current', field2: 'current', field3: 'current' }
+    const incoming = { field1: 'incoming', field2: 'incoming', otherField: 'incoming' }
+    const result = updateAllDefinedAndChanged(current, incoming)
+    expect(result).toBe(true)
+    expect(current).toEqual({ field1: 'incoming', field2: 'incoming', field3: 'current' })
+  })
+  it('should not update any field if incoming is the same as current', () => {
+    const current = { field1: 'current', field2: 'current' }
+    const incoming = { field1: 'current', field2: 'current' }
+    const result = updateAllDefinedAndChanged(current, incoming)
+    expect(result).toBe(false)
+    expect(current).toEqual({ field1: 'current', field2: 'current' })
+  })
+  it('should not update any field if incoming is undefined', () => {
+    const current = { field1: 'current', field2: 'current' }
+    const incoming = { field1: undefined, field2: undefined }
+    const result = updateAllDefinedAndChanged(current, incoming)
+    expect(result).toBe(false)
+    expect(current).toEqual({ field1: 'current', field2: 'current' })
+  })
+  it('should update field if incoming is null', () => {
+    type TestEntity = { field1: string | null, field2: string | null }
+    type TestInput = { field1: string | null }
+    const current: TestEntity = { field1: 'current', field2: 'current' }
+    const incoming: TestInput = { field1: null }
+    const result = updateAllDefinedAndChanged(current, incoming)
+    expect(result).toBe(true)
+    expect(current).toEqual({ field1: null, field2: 'current' })
+  })
+})
+  

shared/src/helper/updateField.ts

@@ -0,0 +1,39 @@
+/**
+ * Updates a field if the incoming value is not undefined and not equal to the current value.
+ * So basically undefined means don't touch value, null means set value to null.
+ * @param current The current value of the field.
+ * @param incoming The incoming value of the field.
+ * @returns True if the field was updated, false otherwise.
+ */
+export function updateIfDefinedAndChanged<T, K extends keyof T>(
+  entity: T,
+  key: K,
+  incoming: T[K] | undefined
+): boolean {
+  if (typeof incoming === 'undefined') {
+    return false 
+  }
+  // Object.is compare actual values and return true if they are identical
+  if (Object.is(entity[key], incoming)) {
+    return false
+  }
+  entity[key] = incoming
+  return true
+}
+
+/**
+ * Check all keys of incoming and if exist on entity, call {@link updateIfDefinedAndChanged} 
+ * to update entity if value isn't undefined and not equal to current value.
+ * @param entity The entity to update.
+ * @param incoming The incoming values to update the entity with.
+ * @returns True if at least one field was updated, false otherwise.
+ */
+export function updateAllDefinedAndChanged<T extends object>(entity: T, incoming: Partial<T>): boolean {
+	let updated = false
+	for (const [key, value] of Object.entries(incoming)) {
+		if (key in entity && updateIfDefinedAndChanged(entity, key as keyof T, value as T[keyof T])) {
+			updated = true
+		}
+	}
+	return updated
+}

shared/src/index.ts

@@ -1,5 +1,6 @@
 export * from './schema'
 export * from './enum'
+export * from './helper'
 export * from './logic/decay'
 export * from './jwt/JWT'
 export * from './jwt/payloadtypes/AuthenticationJwtPayloadType'
@@ -14,3 +15,4 @@ export * from './jwt/payloadtypes/SendCoinsJwtPayloadType'
 export * from './jwt/payloadtypes/SendCoinsResponseJwtPayloadType'
 export * from './jwt/payloadtypes/SignedTransferPayloadType'
 
+