Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DMP-4216: Allow inactive tasks to be run manually #2238

Merged
merged 21 commits into from
Nov 12, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ void findsAutomatedTasksById() {
void findsAllAutomatedTasks() {
var persistedTasks = dartsDatabase.getAllAutomatedTasks();

var automatedTasks = adminAutomatedTaskService.getAllAutomatedTasks();
var automatedTasks = adminAutomatedTaskService.getAllAutomatedTasksSummaries();

assertThat(automatedTasks).extracting("id").isEqualTo(taskIdsOf(persistedTasks));
assertThat(automatedTasks).extracting("name").isEqualTo(taskNamesOf(persistedTasks));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ public class AutomatedTasksController implements TasksApi {
@Authorisation(contextId = ANY_ENTITY_ID, globalAccessSecurityRoles = {SUPER_ADMIN})
@Override
public ResponseEntity<List<AutomatedTaskSummary>> getAutomatedTasks() {
return new ResponseEntity<>(adminAutomatedTaskService.getAllAutomatedTasks(), HttpStatus.OK);
return new ResponseEntity<>(adminAutomatedTaskService.getAllAutomatedTasksSummaries(), HttpStatus.OK);
}

@SecurityRequirement(name = SECURITY_SCHEMES_BEARER_AUTH)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,12 @@ public interface AutomatedTask extends Runnable {

String getTaskName();

void run(boolean isManualRun);

default void run() {
run(false);
}

AutomatedTaskStatus getAutomatedTaskStatus();

String getLastCronExpression();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,12 +53,6 @@ public abstract class AbstractLockableAutomatedTask implements AutomatedTask, Au

private ThreadLocal<UUID> executionId;

private boolean isManualTask = false;

public void setManualTask() {
this.isManualTask = true;
}

protected AbstractLockableAutomatedTask(AutomatedTaskRepository automatedTaskRepository,
AutomatedTaskConfigurationProperties automatedTaskConfigurationProperties,
LogApi logApi, LockService lockService) {
Expand All @@ -69,17 +63,15 @@ protected AbstractLockableAutomatedTask(AutomatedTaskRepository automatedTaskRep
}

private void setupUserAuthentication() {

Jwt jwt = Jwt.withTokenValue("automated-task")
.header("alg", "RS256")
.claim("emails", List.of(automatedTaskConfigurationProperties.getSystemUserEmail()))
.build();
SecurityContextHolder.getContext().setAuthentication(new JwtAuthenticationToken(jwt));

}

@Override
public void run() {
public void run(boolean isManualRun) {
Copy link
Contributor

@jackmaloney jackmaloney Nov 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code is making my head hurt. Why are we passing this isManualRun in as a parameter? Can't we just use the isManualTask variable that is already used in this class? Or just get rid of the isManualTask class variable and use your new parameter.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have removed the ManualTaskService as this no longer provided any value and updated the code to reflect this change

executionId = ThreadLocal.withInitial(UUID::randomUUID);
preRunTask();
try {
Expand All @@ -89,8 +81,11 @@ public void run() {
AutomatedTaskEntity automatedTask = automatedTaskEntity.get();
String dbCronExpression = automatedTask.getCronExpression();
// Check the cron expression hasn't been changed in the database by another instance, if so skip this run
if (isManualTask || getLastCronExpression().equals(dbCronExpression)) {
if (TRUE.equals(automatedTask.getTaskEnabled())) {
if (isManualRun || getLastCronExpression().equals(dbCronExpression)) {
if (isManualRun || TRUE.equals(automatedTask.getTaskEnabled())) {
if (!TRUE.equals(automatedTask.getTaskEnabled())) {
log.info("Task: {} is inactive but has been run manually", getTaskName());
}
logApi.taskStarted(executionId.get(), this.getTaskName());
lockService.getLockingTaskExecutor().executeWithLock(new LockedTask(), getLockConfiguration());
} else {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@

public interface AdminAutomatedTaskService {

List<AutomatedTaskSummary> getAllAutomatedTasks();
List<AutomatedTaskSummary> getAllAutomatedTasksSummaries();

DetailedAutomatedTask getAutomatedTaskById(Integer taskId);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
import uk.gov.hmcts.darts.common.repository.AutomatedTaskRepository;
import uk.gov.hmcts.darts.task.api.AutomatedTaskName;
import uk.gov.hmcts.darts.task.exception.AutomatedTaskApiError;
import uk.gov.hmcts.darts.task.runner.AutomatedTask;
import uk.gov.hmcts.darts.task.service.AdminAutomatedTaskService;
import uk.gov.hmcts.darts.task.service.LockService;
import uk.gov.hmcts.darts.tasks.model.AutomatedTaskPatch;
Expand All @@ -34,22 +35,29 @@ public class AdminAutomatedTasksServiceImpl implements AdminAutomatedTaskService

private final AutomatedTaskRepository automatedTaskRepository;
private final AutomatedTasksMapper mapper;
private final ManualTaskService manualTaskService;
private final AutomatedTaskRunner automatedTaskRunner;
private final AuditApi auditApi;
private final LockService lockService;

private final ConfigurableBeanFactory configurableBeanFactory;
private final List<AutomatedTask> automatedTasks;

@Override
public List<AutomatedTaskSummary> getAllAutomatedTasks() {
var automatedTask = automatedTaskRepository.findAll()
public List<AutomatedTaskSummary> getAllAutomatedTasksSummaries() {
return mapper.mapEntitiesToModel(getAllAutomatedTasksEntities());
}

List<AutomatedTaskEntity> getAllAutomatedTasksEntities() {
return automatedTaskRepository.findAll()
.stream()
.filter(this::shouldIncludeAutomatedTask)
.toList();
return mapper.mapEntitiesToModel(automatedTask);
}

List<AutomatedTask> getAllAutomatedTasks() {
return automatedTasks;
}


@Override
public DetailedAutomatedTask getAutomatedTaskById(Integer taskId) {
return mapper.mapEntityToDetailedAutomatedTask(getAutomatedTaskEntityById(taskId));
Expand All @@ -65,7 +73,8 @@ public void runAutomatedTask(Integer taskId) {
throw new DartsApiException(AUTOMATED_TASK_ALREADY_RUNNING);
}

var automatedTask = manualTaskService.getAutomatedTasks().stream()
var automatedTask = getAllAutomatedTasks()
.stream()
.filter(task -> task.getTaskName().equals(automatedTaskEntity.getTaskName()))
.findFirst();

Expand All @@ -74,7 +83,7 @@ public void runAutomatedTask(Integer taskId) {
throw new DartsApiException(AutomatedTaskApiError.AUTOMATED_TASK_NOT_CONFIGURED_CORRECTLY);
}

automatedTaskRunner.run(automatedTask.get());
automatedTaskRunner.run(automatedTask.get(), true);

auditApi.record(RUN_JOB_MANUALLY, automatedTaskEntity.getTaskName());
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,15 +3,14 @@
import lombok.extern.slf4j.Slf4j;
import org.springframework.scheduling.annotation.Async;
import org.springframework.stereotype.Component;
import uk.gov.hmcts.darts.task.runner.impl.AbstractLockableAutomatedTask;
import uk.gov.hmcts.darts.task.runner.AutomatedTask;

@Component
@Slf4j
public class AutomatedTaskRunner {

@Async
public void run(AbstractLockableAutomatedTask task) {
log.info("Attempting manual run of {}", task.getTaskName());
task.run();
public void run(AutomatedTask task, boolean isManualRun) {
log.info("Attempting {} run of {}", isManualRun ? " manual " : " automated", task.getTaskName());
task.run(isManualRun);
}
}

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
import uk.gov.hmcts.darts.common.exception.DartsApiException;
import uk.gov.hmcts.darts.common.repository.AutomatedTaskRepository;
import uk.gov.hmcts.darts.task.exception.AutomatedTaskApiError;
import uk.gov.hmcts.darts.task.runner.AutomatedTask;
import uk.gov.hmcts.darts.task.runner.impl.AbstractLockableAutomatedTask;
import uk.gov.hmcts.darts.task.service.LockService;
import uk.gov.hmcts.darts.tasks.model.AutomatedTaskPatch;
Expand All @@ -27,8 +28,10 @@
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.Mockito.doReturn;
import static org.mockito.Mockito.lenient;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.spy;
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.verifyNoInteractions;
Expand All @@ -43,8 +46,6 @@ class AdminAutomatedTasksServiceImplTest {
@Mock
private AutomatedTasksMapper mapper;
@Mock
private ManualTaskService manualTaskService;
@Mock
private AutomatedTaskRunner automatedTaskRunner;
@Mock
private LockService lockService;
Expand All @@ -62,17 +63,19 @@ class AdminAutomatedTasksServiceImplTest {

@Test
void invokesTaskWhenTaskIsNotLocked() {
adminAutomatedTaskService = spy(adminAutomatedTaskService);
var automatedTask = anAutomatedTaskEntityWithName("some-task-name", null);
when(someAutomatedTask.getTaskName()).thenReturn("some-task-name");
when(lockService.isLocked(automatedTask)).thenReturn(false);

when(automatedTaskRepository.findById(1)).thenReturn(Optional.of(automatedTask));
when(manualTaskService.getAutomatedTasks()).thenReturn(List.of(someAutomatedTask));
doReturn(List.of((AutomatedTask) someAutomatedTask)).when(adminAutomatedTaskService).getAllAutomatedTasks();

adminAutomatedTaskService.runAutomatedTask(1);

verify(automatedTaskRunner, times(1)).run(someAutomatedTask);
verify(auditApi, times(1)).record(AuditActivity.RUN_JOB_MANUALLY,"some-task-name");
verify(automatedTaskRunner, times(1)).run(someAutomatedTask, true);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have set isManual to true but is there a test where its set to false

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the is manual run false should be covered by the existing tests for automated tasks. As when it is not manually run we default this value to be false

verify(auditApi, times(1)).record(AuditActivity.RUN_JOB_MANUALLY, "some-task-name");
verify(adminAutomatedTaskService, times(1)).getAllAutomatedTasks();
}

@Test
Expand All @@ -95,7 +98,7 @@ void updateAutomatedTask() {
assertFalse(automatedTaskEntity.getTaskEnabled());
assertEquals(100, automatedTaskEntity.getBatchSize());
assertEquals(expectedReturnTask, task);
verify(auditApi).record(AuditActivity.ENABLE_DISABLE_JOB,"some-task-name disabled");
verify(auditApi).record(AuditActivity.ENABLE_DISABLE_JOB, "some-task-name disabled");
verifyNoMoreInteractions(auditApi);
}

Expand All @@ -116,7 +119,7 @@ void updateAutomatedTaskDoesNotUpdateFieldsWithNullValueInPatchRequest() {


@Test
void positiveGetAllAutomatedTasks() {
void positiveGetAllAutomatedTasksSummaries() {
AutomatedTaskEntity automatedTaskEntity1 =
createAutomatedTaskEntity("task1", true);
AutomatedTaskEntity automatedTaskEntity2 =
Expand All @@ -126,7 +129,7 @@ void positiveGetAllAutomatedTasks() {

when(automatedTaskRepository.findAll()).thenReturn(List.of(automatedTaskEntity1, automatedTaskEntity2, automatedTaskEntity3));

adminAutomatedTaskService.getAllAutomatedTasks();
adminAutomatedTaskService.getAllAutomatedTasksSummaries();

verify(automatedTaskRepository, times(1))
.findAll();
Expand All @@ -136,7 +139,7 @@ void positiveGetAllAutomatedTasks() {


@Test
void positiveGetAllAutomatedTasksDisabledExcludeFlag() {
void positiveGetAllAutomatedTasksSummariesDisabledExcludeFlag() {
AutomatedTaskEntity automatedTaskEntity1 =
createAutomatedTaskEntity("task1", true);
AutomatedTaskEntity automatedTaskEntity2 =
Expand All @@ -146,7 +149,7 @@ void positiveGetAllAutomatedTasksDisabledExcludeFlag() {

when(automatedTaskRepository.findAll()).thenReturn(List.of(automatedTaskEntity1, automatedTaskEntity2, automatedTaskEntity3));

adminAutomatedTaskService.getAllAutomatedTasks();
adminAutomatedTaskService.getAllAutomatedTasksSummaries();

verify(automatedTaskRepository, times(1))
.findAll();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -513,7 +513,7 @@ public void setLastCronExpression(String cronExpression) {
}

@Override
public void run() {
public void run(boolean isManualRun) {
log.debug("Running test automated task");
}

Expand Down
Loading