Only allow taskTypeId as parameter in task creation (#6640)

* Only allow taskTypeId as parameter in task creation * fix sample orga task type id * changelog, migration guide
scalableminds · Nov 28, 2022 · 003b14d · 003b14d
1 parent cd52b50
commit 003b14d
Show file tree

Hide file tree

Showing 9 changed files with 40 additions and 50 deletions.
diff --git a/CHANGELOG.unreleased.md b/CHANGELOG.unreleased.md
@@ -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
 

diff --git a/MIGRATIONS.unreleased.md b/MIGRATIONS.unreleased.md
@@ -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:
diff --git a/app/controllers/InitialDataController.scala b/app/controllers/InitialDataController.scala
@@ -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!"

diff --git a/app/controllers/TaskController.scala b/app/controllers/TaskController.scala
@@ -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)
@@ -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

diff --git a/app/models/task/TaskCreationParameters.scala b/app/models/task/TaskCreationParameters.scala
@@ -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,

diff --git a/app/models/task/TaskCreationService.scala b/app/models/task/TaskCreationService.scala
@@ -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)
@@ -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(
@@ -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
@@ -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,
@@ -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(
@@ -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)
@@ -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))
@@ -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)
 }
diff --git a/frontend/javascripts/admin/task/task_create_bulk_view.tsx b/frontend/javascripts/admin/task/task_create_bulk_view.tsx
@@ -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?:
@@ -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) ||
@@ -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]);
@@ -150,7 +150,7 @@ function TaskCreateBulkView() {
           };
     return {
       dataSet,
-      taskTypeIdOrSummary,
+      taskTypeId,
       scriptId,
       openInstances,
       boundingBox,
@@ -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
@@ -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,
                 }}

diff --git a/frontend/javascripts/admin/task/task_create_form_view.tsx b/frontend/javascripts/admin/task/task_create_form_view.tsx
@@ -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"];
@@ -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={[

diff --git a/frontend/javascripts/test/backend-snapshot-tests/tasks.e2e.ts b/frontend/javascripts/test/backend-snapshot-tests/tasks.e2e.ts
@@ -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,
@@ -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