From ecb9e1f5400183097a4c0cbbc11285f64d3c2a8f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tim=20Z=C3=B6ller?= Date: Sun, 14 Dec 2025 16:12:20 +0100 Subject: [PATCH] Don't duplicated updates activities --- .../fitpub/controller/ActivityController.java | 25 +-- .../fitpub/service/FitFileService.java | 25 +-- .../fitpub/service/FitFileServiceTest.java | 160 ++++++++++++++++++ 3 files changed, 186 insertions(+), 24 deletions(-) diff --git a/src/main/java/org/operaton/fitpub/controller/ActivityController.java b/src/main/java/org/operaton/fitpub/controller/ActivityController.java index 3732830..748e269 100644 --- a/src/main/java/org/operaton/fitpub/controller/ActivityController.java +++ b/src/main/java/org/operaton/fitpub/controller/ActivityController.java @@ -307,20 +307,21 @@ public class ActivityController { UUID userId = getUserId(userDetails); - Activity activity = fitFileService.getActivity(id, userId); - if (activity == null) { + try { + Activity updated = fitFileService.updateActivity( + id, + userId, + request.getTitle(), + request.getDescription(), + request.getVisibility() + ); + + ActivityDTO dto = ActivityDTO.fromEntity(updated); + return ResponseEntity.ok(dto); + } catch (IllegalArgumentException e) { + log.warn("Activity update failed: {}", e.getMessage()); return ResponseEntity.notFound().build(); } - - // Update fields - activity.setTitle(request.getTitle()); - activity.setDescription(request.getDescription()); - activity.setVisibility(request.getVisibility()); - - Activity updated = fitFileService.updateActivity(activity); - - ActivityDTO dto = ActivityDTO.fromEntity(updated); - return ResponseEntity.ok(dto); } /** diff --git a/src/main/java/org/operaton/fitpub/service/FitFileService.java b/src/main/java/org/operaton/fitpub/service/FitFileService.java index eec562c..c4a6f84 100644 --- a/src/main/java/org/operaton/fitpub/service/FitFileService.java +++ b/src/main/java/org/operaton/fitpub/service/FitFileService.java @@ -382,25 +382,26 @@ public class FitFileService { /** * Update an existing activity's metadata. * - * @param activity the activity with updated fields + * @param activityId the activity ID + * @param userId the user ID (for authorization) + * @param title new title + * @param description new description + * @param visibility new visibility * @return the updated activity * @throws IllegalArgumentException if activity doesn't exist or user doesn't own it */ @Transactional - public Activity updateActivity(Activity activity) { - // Verify activity exists and belongs to the user - Activity existing = activityRepository.findById(activity.getId()) - .orElseThrow(() -> new IllegalArgumentException("Activity not found: " + activity.getId())); - - if (!existing.getUserId().equals(activity.getUserId())) { - throw new IllegalArgumentException("User does not own this activity"); - } + public Activity updateActivity(UUID activityId, UUID userId, String title, String description, Activity.Visibility visibility) { + // Fetch the existing activity within the transaction + Activity existing = activityRepository.findByIdAndUserId(activityId, userId) + .orElseThrow(() -> new IllegalArgumentException("Activity not found or user does not own it: " + activityId)); // Update allowed fields - existing.setTitle(activity.getTitle()); - existing.setDescription(activity.getDescription()); - existing.setVisibility(activity.getVisibility()); + existing.setTitle(title); + existing.setDescription(description); + existing.setVisibility(visibility); + // Save will UPDATE because the entity is already managed by the persistence context return activityRepository.save(existing); } } diff --git a/src/test/java/org/operaton/fitpub/service/FitFileServiceTest.java b/src/test/java/org/operaton/fitpub/service/FitFileServiceTest.java index 7ca0d03..643626b 100644 --- a/src/test/java/org/operaton/fitpub/service/FitFileServiceTest.java +++ b/src/test/java/org/operaton/fitpub/service/FitFileServiceTest.java @@ -346,6 +346,166 @@ class FitFileServiceTest { assertEquals(testParsedData.getTotalDuration().getSeconds(), savedActivity.getTotalDurationSeconds()); } + @Test + @DisplayName("Should update existing activity without creating a new one") + void testUpdateActivity_Success() { + // Arrange + UUID activityId = UUID.randomUUID(); + UUID userId = UUID.randomUUID(); + + Activity existingActivity = Activity.builder() + .id(activityId) + .userId(userId) + .title("Old Title") + .description("Old Description") + .visibility(Activity.Visibility.PRIVATE) + .activityType(Activity.ActivityType.RUN) + .totalDistance(BigDecimal.valueOf(5000)) + .build(); + + when(activityRepository.findByIdAndUserId(activityId, userId)) + .thenReturn(Optional.of(existingActivity)); + when(activityRepository.save(any(Activity.class))) + .thenAnswer(invocation -> invocation.getArgument(0)); + + // Act + Activity result = fitFileService.updateActivity( + activityId, + userId, + "New Title", + "New Description", + Activity.Visibility.PUBLIC + ); + + // Assert + assertNotNull(result); + assertEquals(activityId, result.getId(), "Activity ID should remain the same (no new entity created)"); + assertEquals("New Title", result.getTitle()); + assertEquals("New Description", result.getDescription()); + assertEquals(Activity.Visibility.PUBLIC, result.getVisibility()); + + // Verify that only the allowed fields were updated + assertEquals(Activity.ActivityType.RUN, result.getActivityType(), "Activity type should not change"); + assertEquals(BigDecimal.valueOf(5000), result.getTotalDistance(), "Distance should not change"); + + // Verify repository interactions + verify(activityRepository).findByIdAndUserId(activityId, userId); + verify(activityRepository).save(existingActivity); + + // Verify that save was called exactly once (UPDATE, not INSERT) + ArgumentCaptor activityCaptor = ArgumentCaptor.forClass(Activity.class); + verify(activityRepository, times(1)).save(activityCaptor.capture()); + Activity savedActivity = activityCaptor.getValue(); + assertEquals(activityId, savedActivity.getId(), "Saved activity should have the same ID"); + } + + @Test + @DisplayName("Should throw exception when updating non-existent activity") + void testUpdateActivity_NotFound() { + // Arrange + UUID activityId = UUID.randomUUID(); + UUID userId = UUID.randomUUID(); + + when(activityRepository.findByIdAndUserId(activityId, userId)) + .thenReturn(Optional.empty()); + + // Act & Assert + IllegalArgumentException exception = assertThrows( + IllegalArgumentException.class, + () -> fitFileService.updateActivity( + activityId, + userId, + "New Title", + "New Description", + Activity.Visibility.PUBLIC + ) + ); + + assertTrue(exception.getMessage().contains("Activity not found or user does not own it")); + verify(activityRepository).findByIdAndUserId(activityId, userId); + verify(activityRepository, never()).save(any()); + } + + @Test + @DisplayName("Should throw exception when user does not own the activity") + void testUpdateActivity_WrongUser() { + // Arrange + UUID activityId = UUID.randomUUID(); + UUID ownerId = UUID.randomUUID(); + UUID differentUserId = UUID.randomUUID(); + + // Repository returns empty when querying with wrong user ID + when(activityRepository.findByIdAndUserId(activityId, differentUserId)) + .thenReturn(Optional.empty()); + + // Act & Assert + IllegalArgumentException exception = assertThrows( + IllegalArgumentException.class, + () -> fitFileService.updateActivity( + activityId, + differentUserId, + "Hacked Title", + "Hacked Description", + Activity.Visibility.PUBLIC + ) + ); + + assertTrue(exception.getMessage().contains("Activity not found or user does not own it")); + verify(activityRepository).findByIdAndUserId(activityId, differentUserId); + verify(activityRepository, never()).save(any()); + } + + @Test + @DisplayName("Should only update allowed fields (title, description, visibility)") + void testUpdateActivity_OnlyAllowedFields() { + // Arrange + UUID activityId = UUID.randomUUID(); + UUID userId = UUID.randomUUID(); + LocalDateTime originalStartTime = LocalDateTime.of(2024, 1, 15, 8, 0); + BigDecimal originalDistance = BigDecimal.valueOf(10000); + + Activity existingActivity = Activity.builder() + .id(activityId) + .userId(userId) + .title("Original Title") + .description("Original Description") + .visibility(Activity.Visibility.PRIVATE) + .activityType(Activity.ActivityType.RIDE) + .startedAt(originalStartTime) + .totalDistance(originalDistance) + .totalDurationSeconds(3600L) + .elevationGain(BigDecimal.valueOf(200)) + .build(); + + when(activityRepository.findByIdAndUserId(activityId, userId)) + .thenReturn(Optional.of(existingActivity)); + when(activityRepository.save(any(Activity.class))) + .thenAnswer(invocation -> invocation.getArgument(0)); + + // Act + Activity result = fitFileService.updateActivity( + activityId, + userId, + "Updated Title", + "Updated Description", + Activity.Visibility.PUBLIC + ); + + // Assert - verify updated fields + assertEquals("Updated Title", result.getTitle()); + assertEquals("Updated Description", result.getDescription()); + assertEquals(Activity.Visibility.PUBLIC, result.getVisibility()); + + // Assert - verify fields that should NOT change + assertEquals(activityId, result.getId()); + assertEquals(userId, result.getUserId()); + assertEquals(Activity.ActivityType.RIDE, result.getActivityType()); + assertEquals(originalStartTime, result.getStartedAt()); + assertEquals(originalDistance, result.getTotalDistance()); + assertEquals(3600L, result.getTotalDurationSeconds()); + assertEquals(BigDecimal.valueOf(200), result.getElevationGain()); + } + /** * Creates test parsed FIT data with realistic values. */