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

Only allow taskTypeId as parameter in task creation #6640

Merged
merged 6 commits into from
Nov 28, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions CHANGELOG.unreleased.md
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ For upgrade instructions, please check the [migration guide](MIGRATIONS.released
### Added

### Changed
- Bulk task creation now needs the taskTypeId, the task type summary will no longer be accepted. [#6640](https://github.com/scalableminds/webknossos/pull/6640)

### Fixed

Expand Down
2 changes: 2 additions & 0 deletions MIGRATIONS.unreleased.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,4 +8,6 @@ User-facing changes are documented in the [changelog](CHANGELOG.released.md).
## Unreleased
[Commits](https://github.com/scalableminds/webknossos/compare/22.12.0...HEAD)

- Bulk task creation now needs the taskTypeId, the task type summary will no longer be accepted. If you have scripts generating CSVs for bulk task creation, they should not output task type summaries. [#6640](https://github.com/scalableminds/webknossos/pull/6640)

### Postgres Evolutions:
2 changes: 1 addition & 1 deletion app/controllers/InitialDataController.scala
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,7 @@ Samplecountry
taskTypeDAO.findAll.flatMap { types =>
if (types.isEmpty) {
val taskType = TaskType(
ObjectId.generate,
ObjectId("63721e2cef0100470266c485"),
organizationTeam._id,
"sampleTaskType",
"Check those cells out!"
Expand Down
11 changes: 4 additions & 7 deletions app/controllers/TaskController.scala
Original file line number Diff line number Diff line change
Expand Up @@ -62,11 +62,8 @@ class TaskController @Inject()(taskCreationService: TaskCreationService,
def create: Action[List[TaskParameters]] = sil.SecuredAction.async(validateJson[List[TaskParameters]]) {
implicit request =>
for {
taskParameters <- Fox.serialCombined(request.body) { tp =>
taskCreationService.normalizeTaskTypeId(tp, request.identity._organization)
}
_ <- taskCreationService.assertBatchLimit(request.body.length, taskParameters.map(_.taskTypeIdOrSummary))
taskParameters <- taskCreationService.createTracingsFromBaseAnnotations(taskParameters,
_ <- taskCreationService.assertBatchLimit(request.body.length, request.body.map(_.taskTypeId))
taskParameters <- taskCreationService.createTracingsFromBaseAnnotations(request.body,
request.identity._organization)
skeletonBaseOpts: List[Option[SkeletonTracing]] <- taskCreationService.createTaskSkeletonTracingBases(
taskParameters)
Expand Down Expand Up @@ -109,8 +106,8 @@ Expects:
_ <- bool2Fox(inputFiles.nonEmpty) ?~> "nml.file.notFound"
jsonString <- body.dataParts.get("formJSON").flatMap(_.headOption) ?~> "format.json.missing"
params <- JsonHelper.parseJsonToFox[NmlTaskParameters](jsonString) ?~> "task.create.failed"
_ <- taskCreationService.assertBatchLimit(inputFiles.length, List(params.taskTypeIdOrSummary))
taskTypeIdValidated <- ObjectId.fromString(params.taskTypeIdOrSummary) ?~> "taskType.id.invalid"
_ <- taskCreationService.assertBatchLimit(inputFiles.length, List(params.taskTypeId))
taskTypeIdValidated <- ObjectId.fromString(params.taskTypeId) ?~> "taskType.id.invalid"
taskType <- taskTypeDAO.findOne(taskTypeIdValidated) ?~> "taskType.notFound" ~> NOT_FOUND
project <- projectDAO
.findOneByNameAndOrganization(params.projectName, request.identity._organization) ?~> "project.notFound" ~> NOT_FOUND
Expand Down
31 changes: 13 additions & 18 deletions app/models/task/TaskCreationParameters.scala
Original file line number Diff line number Diff line change
Expand Up @@ -4,29 +4,24 @@ import com.scalableminds.util.geometry.{BoundingBox, Vec3Int, Vec3Double}
import models.user.Experience
import play.api.libs.json.{Format, Json}

case class TaskParameters(
taskTypeIdOrSummary: String,
// The taskTypeIdOrSummary parameter may be an id or a summary. At the beginning of TaskController>create
// this is normalized so only the task id is stored in this field.
neededExperience: Experience,
openInstances: Int,
projectName: String,
scriptId: Option[String],
boundingBox: Option[BoundingBox],
dataSet: String,
editPosition: Vec3Int,
editRotation: Vec3Double,
creationInfo: Option[String],
description: Option[String],
baseAnnotation: Option[BaseAnnotation]
)
case class TaskParameters(taskTypeId: String,
neededExperience: Experience,
openInstances: Int,
projectName: String,
scriptId: Option[String],
boundingBox: Option[BoundingBox],
dataSet: String,
editPosition: Vec3Int,
editRotation: Vec3Double,
creationInfo: Option[String],
description: Option[String],
baseAnnotation: Option[BaseAnnotation])

object TaskParameters {
implicit val taskParametersFormat: Format[TaskParameters] = Json.format[TaskParameters]
}

case class NmlTaskParameters(taskTypeIdOrSummary: String,
// taskTypeIdOrSummary named like this for compatibility, note that in NML case only ids are valid.
case class NmlTaskParameters(taskTypeId: String,
neededExperience: Experience,
openInstances: Int,
projectName: String,
Expand Down
19 changes: 7 additions & 12 deletions app/models/task/TaskCreationService.scala
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ class TaskCreationService @Inject()(taskTypeService: TaskTypeService,
taskParameters: TaskParameters,
organizationId: ObjectId)(implicit ctx: DBAccessContext, m: MessagesProvider): Fox[BaseAnnotation] =
for {
taskTypeIdValidated <- ObjectId.fromString(taskParameters.taskTypeIdOrSummary) ?~> "taskType.id.invalid"
taskTypeIdValidated <- ObjectId.fromString(taskParameters.taskTypeId) ?~> "taskType.id.invalid"
taskType <- taskTypeDAO.findOne(taskTypeIdValidated) ?~> "taskType.notFound"
dataSet <- dataSetDAO.findOneByNameAndOrganization(taskParameters.dataSet, organizationId)
baseAnnotationIdValidated <- ObjectId.fromString(baseAnnotation.baseId)
Expand Down Expand Up @@ -169,7 +169,7 @@ class TaskCreationService @Inject()(taskTypeService: TaskTypeService,
implicit ctx: DBAccessContext): Fox[List[Option[SkeletonTracing]]] =
Fox.serialCombined(paramsList) { params =>
for {
taskTypeIdValidated <- ObjectId.fromString(params.taskTypeIdOrSummary) ?~> "taskType.id.invalid"
taskTypeIdValidated <- ObjectId.fromString(params.taskTypeId) ?~> "taskType.id.invalid"
taskType <- taskTypeDAO.findOne(taskTypeIdValidated) ?~> "taskType.notFound"
skeletonTracingOpt = if ((taskType.tracingType == TracingType.skeleton || taskType.tracingType == TracingType.hybrid) && params.baseAnnotation.isEmpty) {
Some(
Expand All @@ -189,7 +189,7 @@ class TaskCreationService @Inject()(taskTypeService: TaskTypeService,
m: MessagesProvider): Fox[List[Option[(VolumeTracing, Option[File])]]] =
Fox.serialCombined(paramsList) { params =>
for {
taskTypeIdValidated <- ObjectId.fromString(params.taskTypeIdOrSummary) ?~> "taskType.id.invalid"
taskTypeIdValidated <- ObjectId.fromString(params.taskTypeId) ?~> "taskType.id.invalid"
taskType <- taskTypeDAO.findOne(taskTypeIdValidated) ?~> "taskType.notFound"
volumeTracingOpt <- if ((taskType.tracingType == TracingType.volume || taskType.tracingType == TracingType.hybrid) && params.baseAnnotation.isEmpty) {
annotationService
Expand Down Expand Up @@ -261,7 +261,7 @@ class TaskCreationService @Inject()(taskTypeService: TaskTypeService,
val parsedNmlTracingBoundingBox = params._1.map(b => BoundingBox(b.topLeft, b.width, b.height, b.depth))
val bbox = if (nmlFormParams.boundingBox.isDefined) nmlFormParams.boundingBox else parsedNmlTracingBoundingBox
TaskParameters(
nmlFormParams.taskTypeIdOrSummary,
nmlFormParams.taskTypeId,
nmlFormParams.neededExperience,
nmlFormParams.openInstances,
nmlFormParams.projectName,
Expand Down Expand Up @@ -390,7 +390,7 @@ class TaskCreationService @Inject()(taskTypeService: TaskTypeService,
"dataSet.notFound",
firstDatasetName)
_ = if (fullTasks.exists(task => task._1.baseAnnotation.isDefined))
slackNotificationService.noticeBaseAnnotationTaskCreation(fullTasks.map(_._1.taskTypeIdOrSummary).distinct,
slackNotificationService.noticeBaseAnnotationTaskCreation(fullTasks.map(_._1.taskTypeId).distinct,
fullTasks.count(_._1.baseAnnotation.isDefined))
tracingStoreClient <- tracingStoreService.clientFor(dataSet)
savedSkeletonTracingIds: List[Box[Option[String]]] <- tracingStoreClient.saveSkeletonTracings(
Expand Down Expand Up @@ -475,7 +475,7 @@ class TaskCreationService @Inject()(taskTypeService: TaskTypeService,
} match {
case Full((params: TaskParameters, Some((tracing, initialFile)))) =>
for {
taskTypeIdValidated <- ObjectId.fromString(params.taskTypeIdOrSummary) ?~> "taskType.id.invalid"
taskTypeIdValidated <- ObjectId.fromString(params.taskTypeId) ?~> "taskType.id.invalid"
taskType <- taskTypeDAO.findOne(taskTypeIdValidated) ?~> "taskType.notFound"
saveResult <- tracingStoreClient
.saveVolumeTracing(tracing, initialFile, resolutionRestrictions = taskType.settings.resolutionRestrictions)
Expand Down Expand Up @@ -528,7 +528,7 @@ class TaskCreationService @Inject()(taskTypeService: TaskTypeService,
skeletonIdOpt <- skeletonTracingIdBox.toFox
volumeIdOpt <- volumeTracingIdBox.toFox
_ <- bool2Fox(skeletonIdOpt.isDefined || volumeIdOpt.isDefined) ?~> "task.create.needsEitherSkeletonOrVolume"
taskTypeIdValidated <- ObjectId.fromString(params.taskTypeIdOrSummary)
taskTypeIdValidated <- ObjectId.fromString(params.taskTypeId)
project <- projectDAO.findOneByNameAndOrganization(params.projectName, requestingUser._organization) ?~> "project.notFound"
_ <- validateScript(params.scriptId) ?~> "script.invalid"
_ <- Fox.assertTrue(userService.isTeamManagerOrAdminOf(requestingUser, project._team))
Expand Down Expand Up @@ -561,9 +561,4 @@ class TaskCreationService @Inject()(taskTypeService: TaskTypeService,
js <- taskService.publicWrites(task)
} yield js

def normalizeTaskTypeId(taskParameters: TaskParameters, organization: ObjectId)(
implicit ctx: DBAccessContext): Fox[TaskParameters] =
for {
taskTypeIdString <- taskTypeService.idOrSummaryToId(taskParameters.taskTypeIdOrSummary, organization)
} yield taskParameters.copy(taskTypeIdOrSummary = taskTypeIdString)
}
16 changes: 8 additions & 8 deletions frontend/javascripts/admin/task/task_create_bulk_view.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ export type NewTask = {
readonly projectName: string;
readonly scriptId: string | null | undefined;
readonly openInstances: number;
readonly taskTypeIdOrSummary: string;
readonly taskTypeId: string;
readonly csvFile?: File;
readonly nmlFiles?: File;
readonly baseAnnotation?:
Expand Down Expand Up @@ -69,7 +69,7 @@ function TaskCreateBulkView() {
if (
!_.isString(task.neededExperience.domain) ||
!_.isString(task.dataSet) ||
!_.isString(task.taskTypeIdOrSummary) ||
!_.isString(task.taskTypeId) ||
!_.isString(task.projectName) ||
task.editPosition.some(Number.isNaN) ||
task.editRotation.some(Number.isNaN) ||
Expand Down Expand Up @@ -111,7 +111,7 @@ function TaskCreateBulkView() {
function parseLine(line: string): NewTask {
const words = splitToWords(line);
const dataSet = words[0];
const taskTypeIdOrSummary = words[1];
const taskTypeId = words[1];
const experienceDomain = words[2];
const minExperience = parseInt(words[3]);
const x = parseInt(words[4]);
Expand Down Expand Up @@ -150,7 +150,7 @@ function TaskCreateBulkView() {
};
return {
dataSet,
taskTypeIdOrSummary,
taskTypeId,
scriptId,
openInstances,
boundingBox,
Expand Down Expand Up @@ -257,9 +257,9 @@ function TaskCreateBulkView() {
Specify each new task on a separate line as comma seperated values (CSV) in the
following format:
<br />
<a href="/dashboard">dataSet</a>, <a href="/taskTypes">taskType (ID or summary)</a>,
experienceDomain, minExperience, x, y, z, rotX, rotY, rotZ, instances, minX, minY, minZ,
width, height, depth, <a href="/projects">project</a> [, <a href="/scripts">scriptId</a>
<a href="/dashboard">dataSet</a>, <a href="/taskTypes">taskTypeId</a>, experienceDomain,
minExperience, x, y, z, rotX, rotY, rotZ, instances, minX, minY, minZ, width, height,
depth, <a href="/projects">project</a> [, <a href="/scripts">scriptId</a>
, baseAnnotationId]
<br />
If you want to define some (but not all) of the optional values, please list all
Expand Down Expand Up @@ -299,7 +299,7 @@ function TaskCreateBulkView() {
>
<TextArea
className="input-monospace"
placeholder="dataset, taskType (ID or summary), experienceDomain, minExperience, x, y, z, rotX, rotY, rotZ, instances, minX, minY, minZ, width, height, depth, project[, scriptId, baseAnnotationId]"
placeholder="dataset, taskTypeId, experienceDomain, minExperience, x, y, z, rotX, rotY, rotZ, instances, minX, minY, minZ, width, height, depth, project[, scriptId, baseAnnotationId]"
autoSize={{
minRows: 6,
}}
Expand Down
4 changes: 2 additions & 2 deletions frontend/javascripts/admin/task/task_create_form_view.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ const fullWidth = {
};
const maxDisplayedTasksCount = 50;
const TASK_CSV_HEADER =
"taskId,dataSet,taskType,experienceDomain,minExperience,x,y,z,rotX,rotY,rotZ,instances,minX,minY,minZ,width,height,depth,project,scriptId,creationInfo";
"taskId,dataSet,taskTypeId,experienceDomain,minExperience,x,y,z,rotX,rotY,rotZ,instances,minX,minY,minZ,width,height,depth,project,scriptId,creationInfo";
type Props = {
taskId: string | null | undefined;
history: RouteComponentProps["history"];
Expand Down Expand Up @@ -611,7 +611,7 @@ class TaskCreateFormView extends React.PureComponent<Props, State> {
<Row gutter={8} align="middle" wrap={false}>
<Col flex="auto">
<FormItem
name="taskTypeIdOrSummary"
name="taskTypeId"
label="Task Type"
hasFeedback
rules={[
Expand Down
4 changes: 2 additions & 2 deletions frontend/javascripts/test/backend-snapshot-tests/tasks.e2e.ts
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ test.serial("updateTask()", async (t) => {

const task = _.omitBy(
Object.assign({}, taskBase, {
taskTypeIdOrSummary: taskBase.type.id,
taskTypeId: taskBase.type.id,
boundingBox: taskBase.boundingBox ? taskBase.boundingBoxVec6 : null,
scriptId: taskBase.script ? taskBase.script.id : null,
openInstances: taskBase.status.open,
Expand Down Expand Up @@ -94,7 +94,7 @@ const newTask = {
projectName: "Test_Project4",
scriptId: null,
openInstances: 3,
taskTypeIdOrSummary: "570b9f4c2a7c0e4c008da6ee",
taskTypeId: "570b9f4c2a7c0e4c008da6ee",
};
test.serial("createTasks() and deleteTask()", async (t) => {
// @ts-expect-error ts-migrate(2322) FIXME: Type '{ boundingBox: null; dataSet: string; editPo... Remove this comment to see the full error message
Expand Down