From 4f602bdee47d17218f6e62203fad052ba6ec4a7b Mon Sep 17 00:00:00 2001 From: Benjamin Seber Date: Wed, 3 Jul 2024 22:29:55 +0200 Subject: [PATCH] add transaction handling to TimeClock handling --- .../timeclock/TimeClockController.java | 19 ++++--- .../TimeClockNotStartedException.java | 2 +- .../timeclock/TimeClockService.java | 49 ++++++++++++++++++- .../timeclock/TimeClockControllerTest.java | 13 ++--- .../timeclock/TimeClockServiceTest.java | 11 +++++ 5 files changed, 78 insertions(+), 16 deletions(-) diff --git a/src/main/java/de/focusshift/zeiterfassung/timeclock/TimeClockController.java b/src/main/java/de/focusshift/zeiterfassung/timeclock/TimeClockController.java index 3d2cb12d1..390452e7d 100644 --- a/src/main/java/de/focusshift/zeiterfassung/timeclock/TimeClockController.java +++ b/src/main/java/de/focusshift/zeiterfassung/timeclock/TimeClockController.java @@ -4,9 +4,11 @@ import de.focusshift.zeiterfassung.user.UserId; import jakarta.servlet.http.HttpServletRequest; import jakarta.validation.Valid; +import org.slf4j.Logger; import org.springframework.security.core.annotation.AuthenticationPrincipal; import org.springframework.security.oauth2.core.oidc.user.DefaultOidcUser; import org.springframework.stereotype.Controller; +import org.springframework.transaction.TransactionSystemException; import org.springframework.ui.Model; import org.springframework.validation.BindingResult; import org.springframework.web.bind.annotation.GetMapping; @@ -21,7 +23,8 @@ import java.util.Optional; import static java.lang.String.format; -import static org.springframework.http.HttpStatus.CONFLICT; +import static java.lang.invoke.MethodHandles.lookup; +import static org.slf4j.LoggerFactory.getLogger; import static org.springframework.http.HttpStatus.PRECONDITION_REQUIRED; import static org.springframework.http.HttpStatus.UNPROCESSABLE_ENTITY; import static org.springframework.util.StringUtils.hasText; @@ -30,6 +33,8 @@ @RequestMapping("timeclock") class TimeClockController implements HasTimeClock, HasLaunchpad { + private static final Logger LOG = getLogger(lookup().lookupClass()); + private final TimeClockService timeClockService; TimeClockController(TimeClockService timeClockService) { @@ -78,14 +83,14 @@ public String startTimeClock(@AuthenticationPrincipal DefaultOidcUser principal, final UserId userId = principalToUserId(principal); - // TODO should we do this in the service? - final Optional maybeCurrentTimeClock = timeClockService.getCurrentTimeClock(userId); - if (maybeCurrentTimeClock.isPresent()) { - throw new ResponseStatusException(CONFLICT, "Time clock has been started already."); + try { + timeClockService.startTimeClock(userId); + } catch (TransactionSystemException e) { + // just log and redirect the user to the previous page + // since the time clock should be running already + LOG.warn("TimeClock could not be started. Maybe it is running already and user submitted the form twice with a double click.", e); } - timeClockService.startTimeClock(userId); - return redirectToPreviousPage(request); } diff --git a/src/main/java/de/focusshift/zeiterfassung/timeclock/TimeClockNotStartedException.java b/src/main/java/de/focusshift/zeiterfassung/timeclock/TimeClockNotStartedException.java index a6bdc07d4..5fbd71b32 100644 --- a/src/main/java/de/focusshift/zeiterfassung/timeclock/TimeClockNotStartedException.java +++ b/src/main/java/de/focusshift/zeiterfassung/timeclock/TimeClockNotStartedException.java @@ -2,7 +2,7 @@ import de.focusshift.zeiterfassung.user.UserId; -class TimeClockNotStartedException extends Exception { +public class TimeClockNotStartedException extends Exception { TimeClockNotStartedException(UserId userId) { super("time clock for userId=%s is not running.".formatted(userId)); diff --git a/src/main/java/de/focusshift/zeiterfassung/timeclock/TimeClockService.java b/src/main/java/de/focusshift/zeiterfassung/timeclock/TimeClockService.java index 76dbabe14..8c22c129e 100644 --- a/src/main/java/de/focusshift/zeiterfassung/timeclock/TimeClockService.java +++ b/src/main/java/de/focusshift/zeiterfassung/timeclock/TimeClockService.java @@ -3,16 +3,25 @@ import de.focusshift.zeiterfassung.timeentry.TimeEntryService; import de.focusshift.zeiterfassung.user.UserId; import de.focusshift.zeiterfassung.user.UserSettingsProvider; +import org.slf4j.Logger; import org.springframework.stereotype.Service; +import org.springframework.transaction.annotation.Isolation; +import org.springframework.transaction.annotation.Propagation; +import org.springframework.transaction.annotation.Transactional; import java.time.ZoneId; import java.time.ZonedDateTime; import java.util.List; import java.util.Optional; +import static java.lang.invoke.MethodHandles.lookup; +import static org.slf4j.LoggerFactory.getLogger; + @Service public class TimeClockService { + private static final Logger LOG = getLogger(lookup().lookupClass()); + private final TimeClockRepository timeClockRepository; private final TimeEntryService timeEntryService; private final UserSettingsProvider userSettingsProvider; @@ -27,10 +36,27 @@ Optional getCurrentTimeClock(UserId userId) { return timeClockRepository.findByOwnerAndStoppedAtIsNull(userId.value()).map(TimeClockService::toTimeClock); } - void startTimeClock(UserId userId) { + /** + * Starts the {@linkplain TimeClock} for the given user. Does nothing when one is running already. + * + * @param userId user to start the {@linkplain TimeClock} + * + * @throws org.springframework.transaction.TransactionSystemException + * when there are concurrent hits and the TimeClock cannot be written anymore + */ + @Transactional(propagation = Propagation.REQUIRED, isolation = Isolation.SERIALIZABLE) + public void startTimeClock(UserId userId) { + + final boolean present = timeClockRepository.findByOwnerAndStoppedAtIsNull(userId.value()).isPresent(); + if (present) { + LOG.info("Not starting TimeClock for user {} since already started", userId.value()); + return; + } + final ZonedDateTime now = ZonedDateTime.now(userSettingsProvider.zoneId()); final TimeClock timeClock = new TimeClock(userId, now); + LOG.info("Starting TimeClock for user {}", userId.value()); timeClockRepository.save(toEntity(timeClock)); } @@ -60,7 +86,21 @@ public List findAllTimeClocks(UserId userId) { .toList(); } - TimeClock updateTimeClock(UserId userId, TimeClockUpdate timeClockUpdate) throws TimeClockNotStartedException { + /** + * Updates the current {@linkplain TimeClock}. + * + * @param userId user to update the {@linkplain TimeClock} + * @param timeClockUpdate new timeClock info + * + * @return the updated {@linkplain TimeClock} + * + * @throws TimeClockNotStartedException + * when there is no running timeClock + * @throws org.springframework.transaction.TransactionSystemException + * when there are concurrent hits and the TimeClock cannot be written anymore + */ + @Transactional(propagation = Propagation.REQUIRED, isolation = Isolation.SERIALIZABLE) + public TimeClock updateTimeClock(UserId userId, TimeClockUpdate timeClockUpdate) throws TimeClockNotStartedException { final TimeClock timeClock = getCurrentTimeClock(userId) .map(existingTimeClock -> prepareTimeClockUpdate(existingTimeClock, timeClockUpdate)) @@ -71,6 +111,11 @@ TimeClock updateTimeClock(UserId userId, TimeClockUpdate timeClockUpdate) throws return toTimeClock(timeClockRepository.save(timeClockEntity)); } + /** + * When there is a running {@linkplain TimeClock} it will be stopped, otherwise nothing is done. + * + * @param userId id of the user which timeClock will be stopped + */ void stopTimeClock(UserId userId) { timeClockRepository.findByOwnerAndStoppedAtIsNull(userId.value()) .map(entity -> timeClockEntityWithStoppedAt(entity, ZonedDateTime.now(userSettingsProvider.zoneId()))) diff --git a/src/test/java/de/focusshift/zeiterfassung/timeclock/TimeClockControllerTest.java b/src/test/java/de/focusshift/zeiterfassung/timeclock/TimeClockControllerTest.java index 72ec629b2..89e5d2535 100644 --- a/src/test/java/de/focusshift/zeiterfassung/timeclock/TimeClockControllerTest.java +++ b/src/test/java/de/focusshift/zeiterfassung/timeclock/TimeClockControllerTest.java @@ -10,6 +10,7 @@ import org.springframework.security.web.method.annotation.AuthenticationPrincipalArgumentResolver; import org.springframework.test.web.servlet.ResultActions; import org.springframework.test.web.servlet.request.MockHttpServletRequestBuilder; +import org.springframework.transaction.TransactionSystemException; import java.time.Instant; import java.time.LocalDate; @@ -25,6 +26,7 @@ import static org.hamcrest.core.AllOf.allOf; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyNoMoreInteractions; import static org.mockito.Mockito.when; @@ -63,7 +65,6 @@ void ensureStartTimeClock() throws Exception { .andExpect(status().is3xxRedirection()) .andExpect(redirectedUrl("referer-url")); - verify(timeClockService).getCurrentTimeClock(new UserId("batman")); verify(timeClockService).startTimeClock(new UserId("batman")); verifyNoMoreInteractions(timeClockService); } @@ -71,17 +72,17 @@ void ensureStartTimeClock() throws Exception { @Test void ensureStartTimeClockThrowsWhenClockIsRunningAlready() throws Exception { - final ZonedDateTime startedAt = ZonedDateTime.of(2023, 1, 11, 13, 37, 0, 0, ZONE_EUROPE_BERLIN); - final TimeClock timeClock = new TimeClock(1L, new UserId("batman"), startedAt, "awesome comment", false, Optional.empty()); - when(timeClockService.getCurrentTimeClock(new UserId("batman"))).thenReturn(Optional.of(timeClock)); + doThrow(new TransactionSystemException("")) + .when(timeClockService).startTimeClock(new UserId("batman")); perform( post("/timeclock/start") .with(oidcLogin().userInfoToken(builder -> builder.subject("batman"))) + .header("Referer", "referer-url") ) - .andExpect(status().isConflict()); + .andExpect(status().is3xxRedirection()) + .andExpect(redirectedUrl("referer-url")); - verify(timeClockService).getCurrentTimeClock(new UserId("batman")); verifyNoMoreInteractions(timeClockService); } diff --git a/src/test/java/de/focusshift/zeiterfassung/timeclock/TimeClockServiceTest.java b/src/test/java/de/focusshift/zeiterfassung/timeclock/TimeClockServiceTest.java index b3ac07d73..74cd41826 100644 --- a/src/test/java/de/focusshift/zeiterfassung/timeclock/TimeClockServiceTest.java +++ b/src/test/java/de/focusshift/zeiterfassung/timeclock/TimeClockServiceTest.java @@ -100,6 +100,17 @@ void ensureStartTimeClockPersistsNewEntity() { assertThat(persistedTimeClockEntity.getStoppedAt()).isNull(); } + @Test + void ensureStartTimeClockDoesNothingWhenThereIsAlreadyOne() { + + when(timeClockRepository.findByOwnerAndStoppedAtIsNull("batman")).thenReturn(Optional.of(new TimeClockEntity())); + + sut.startTimeClock(new UserId("batman")); + + verifyNoMoreInteractions(timeClockRepository); + verifyNoInteractions(userSettingsProvider); + } + @Test void ensureStopTimeClockDoesNothingWhenThereIsNothingRunningCurrently() {