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 2 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 @@ -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 @@ -79,7 +79,7 @@ private void setupUserAuthentication() {
}

@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 @@ -90,7 +90,7 @@ public void run() {
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 || TRUE.equals(automatedTask.getTaskEnabled())) {
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 @@ -74,7 +74,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 @@ -8,10 +8,9 @@
@Component
@Slf4j
public class AutomatedTaskRunner {

@Async
public void run(AbstractLockableAutomatedTask task) {
log.info("Attempting manual run of {}", task.getTaskName());
task.run();
public void run(AbstractLockableAutomatedTask task, boolean isManualRun) {
log.info("Attempting {} run of {}", isManualRun ? " manual " : " automated", task.getTaskName());
task.run(isManualRun);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -71,8 +71,8 @@ void invokesTaskWhenTaskIsNotLocked() {

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");
}

@Test
Expand All @@ -95,7 +95,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 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