From ffc45fff486920557f55772aeff4a488fdca60ce Mon Sep 17 00:00:00 2001 From: CollinBeczak Date: Fri, 1 Mar 2024 17:59:37 -0600 Subject: [PATCH 01/26] add resetTaskBundle endpoint --- .../controller/TaskBundleController.scala | 33 ++++- .../repository/TaskBundleRepository.scala | 133 +++++++++++++++++- .../framework/service/TaskBundleService.scala | 26 +++- conf/v2_route/bundle.api | 29 +++- 4 files changed, 207 insertions(+), 14 deletions(-) diff --git a/app/org/maproulette/framework/controller/TaskBundleController.scala b/app/org/maproulette/framework/controller/TaskBundleController.scala index 579d1582..37f4fb7e 100644 --- a/app/org/maproulette/framework/controller/TaskBundleController.scala +++ b/app/org/maproulette/framework/controller/TaskBundleController.scala @@ -259,6 +259,24 @@ class TaskBundleController @Inject() ( } } + /** + * Resets the bundle to the tasks provided, and unlock all tasks removed from current bundle + * + * @param bundleId The id of the bundle + * @param taskIds The task ids the bundle will reset to + * @param primaryTaskId The primary task id of the bundle + */ + def resetTaskBundle( + id: Long, + primaryTaskId: Long, + taskIds: List[Long] + ): Action[AnyContent] = Action.async { implicit request => + this.sessionManager.authenticatedRequest { implicit user => + this.serviceManager.taskBundle.resetTaskBundle(user, id, primaryTaskId, taskIds) + Ok(Json.toJson(this.serviceManager.taskBundle.getTaskBundle(user, id))) + } + } + /** * Remove tasks from a bundle. * @@ -266,12 +284,15 @@ class TaskBundleController @Inject() ( * @param taskIds List of task ids to remove * @return Task Bundle */ - def unbundleTasks(id: Long, taskIds: List[Long]): Action[AnyContent] = Action.async { - implicit request => - this.sessionManager.authenticatedRequest { implicit user => - this.serviceManager.taskBundle.unbundleTasks(user, id, taskIds) - Ok(Json.toJson(this.serviceManager.taskBundle.getTaskBundle(user, id))) - } + def unbundleTasks( + id: Long, + taskIds: List[Long], + preventTaskIdUnlocks: List[Long] + ): Action[AnyContent] = Action.async { implicit request => + this.sessionManager.authenticatedRequest { implicit user => + this.serviceManager.taskBundle.unbundleTasks(user, id, taskIds, preventTaskIdUnlocks) + Ok(Json.toJson(this.serviceManager.taskBundle.getTaskBundle(user, id))) + } } /** diff --git a/app/org/maproulette/framework/repository/TaskBundleRepository.scala b/app/org/maproulette/framework/repository/TaskBundleRepository.scala index 8f457fcf..c6030ee3 100644 --- a/app/org/maproulette/framework/repository/TaskBundleRepository.scala +++ b/app/org/maproulette/framework/repository/TaskBundleRepository.scala @@ -16,6 +16,7 @@ import org.maproulette.framework.psql.Query import org.maproulette.framework.psql.filter.BaseParameter import org.maproulette.framework.model.{Task, TaskBundle, User} import org.maproulette.framework.mixins.{TaskParserMixin, Locking} +import org.maproulette.framework.model.Task.STATUS_CREATED import org.maproulette.data.TaskType import play.api.db.Database @@ -91,12 +92,121 @@ class TaskBundleRepository @Inject() ( } } + /** + * Resets the bundle to the tasks provided, and unlock all tasks removed from current bundle + * + * @param bundleId The id of the bundle + * @param taskIds The task ids the bundle will reset to + * @param primaryTaskId The primary task id of the bundle + */ + def resetTaskBundle( + user: User, + bundleId: Long, + primaryTaskId: Long, + taskIds: List[Long] + ): Unit = { + withMRTransaction { implicit c => + // Remove tasks from the bundle join table and unlock them if necessary + val tasks = + retrieveTasks(Query.simple(List(BaseParameter("bundle_id", bundleId, table = Some("tb"))))) + + // Unset bundle_id for tasks not in the taskids param(this param is the task ids that the bundle will be "resetting" to) + SQL( + """UPDATE tasks + SET bundle_id = NULL + WHERE bundle_id = {bundleId} AND id NOT IN ({taskIds}) + """ + ).on( + "bundleId" -> bundleId, + "taskIds" -> taskIds + ) + .executeUpdate() + + for (task <- tasks) { + if (!task.isBundlePrimary.getOrElse(false) && !taskIds.contains(task.id)) { + SQL( + s"""DELETE FROM task_bundles WHERE bundle_id = $bundleId AND task_id = ${task.id}""" + ).executeUpdate() + try { + unlockItem(user, task) + } catch { + case e: Exception => logger.warn(e.getMessage) + } + } + } + + // Add tasks to the bundle and lock them + val existingTasks = SQL( + """SELECT task_id FROM task_bundles WHERE bundle_id = {bundleId}""" + ).on( + "bundleId" -> bundleId + ) + .as(SqlParser.long("task_id").*) + + // Construct the SQL query to insert task-bundle associations + val sqlQuery = + s"""INSERT INTO task_bundles (bundle_id, task_id) VALUES ($bundleId, {taskId})""" + + val tasksToAdd = taskIds.filterNot(existingTasks.contains) + + // Construct parameters for the BatchSql operation + val parameters = tasksToAdd.map(taskId => Seq[NamedParameter]("taskId" -> taskId)) + + // Execute the BatchSql operation to insert task-bundle associations + BatchSql(sqlQuery, parameters.head, parameters.tail: _*).execute() + + SQL(s"""UPDATE tasks SET bundle_id = {bundleId} + WHERE bundle_id IS NULL + AND id IN ({inList})""") + .on( + "bundleId" -> bundleId, + "inList" -> tasksToAdd + ) + .executeUpdate() + tasksToAdd.foreach { taskId => + val taskToLock = retrieveTasks(Query.simple(List(BaseParameter("id", List(taskId))))) + taskToLock.foreach { task => + try { + this.lockItem(user, task) + } catch { + case e: Exception => this.logger.warn(e.getMessage) + } + } + } + + // Retrieve the status of the primary task + val primaryTaskStatus = SQL( + """SELECT status FROM tasks WHERE id = {primaryTaskId}""" + ).on( + "primaryTaskId" -> primaryTaskId + ) + .as(SqlParser.scalar[Int].single) + + // Update the status of all tasks in the bundle to match the status of the primary task + SQL( + """UPDATE tasks + SET status = {primaryTaskStatus} + WHERE bundle_id = {bundleId} + """ + ).on( + "primaryTaskStatus" -> primaryTaskStatus, + "bundleId" -> bundleId + ) + .executeUpdate() + } + } + /** * Removes tasks from a bundle. * * @param bundleId The id of the bundle */ - def unbundleTasks(user: User, bundleId: Long, taskIds: List[Long])(): Unit = { + def unbundleTasks( + user: User, + bundleId: Long, + taskIds: List[Long], + preventTaskIdUnlocks: List[Long] + ): Unit = { this.withMRConnection { implicit c => // Unset any bundle_id on individual tasks (this is set when task is completed) SQL(s"""UPDATE tasks SET bundle_id = NULL @@ -125,10 +235,23 @@ class TaskBundleRepository @Inject() ( SQL(s"""DELETE FROM task_bundles WHERE bundle_id = ${bundleId} AND task_id = ${task.id}""").executeUpdate() - try { - this.unlockItem(user, task) - } catch { - case e: Exception => this.logger.warn(e.getMessage) + if (!preventTaskIdUnlocks.contains(task.id)) { + try { + this.unlockItem(user, task) + } catch { + case e: Exception => this.logger.warn(e.getMessage) + } + } else { + SQL( + """UPDATE tasks + SET status = {status} + WHERE id = {taskId} + """ + ).on( + "taskId" -> task.id, + "status" -> STATUS_CREATED + ) + .executeUpdate() } case None => // do nothing } diff --git a/app/org/maproulette/framework/service/TaskBundleService.scala b/app/org/maproulette/framework/service/TaskBundleService.scala index 7bc2b84a..e83e45b6 100644 --- a/app/org/maproulette/framework/service/TaskBundleService.scala +++ b/app/org/maproulette/framework/service/TaskBundleService.scala @@ -83,12 +83,34 @@ class TaskBundleService @Inject() ( ) } + /** + * Resets the bundle to the tasks provided, and unlock all tasks removed from current bundle + * + * @param bundleId The id of the bundle + * @param taskIds The task ids the bundle will reset to + * @param primaryTaskId The primary task id of the bundle + */ + def resetTaskBundle( + user: User, + bundleId: Long, + primaryTaskId: Long, + taskIds: List[Long] + ): TaskBundle = { + this.repository.resetTaskBundle(user, bundleId, primaryTaskId, taskIds) + this.getTaskBundle(user, bundleId) + } + /** * Removes tasks from a bundle. * * @param bundleId The id of the bundle */ - def unbundleTasks(user: User, bundleId: Long, taskIds: List[Long])(): TaskBundle = { + def unbundleTasks( + user: User, + bundleId: Long, + taskIds: List[Long], + preventTaskIdUnlocks: List[Long] + )(): TaskBundle = { val bundle = this.getTaskBundle(user, bundleId) // Verify permissions to modify this bundle @@ -98,7 +120,7 @@ class TaskBundleService @Inject() ( ) } - this.repository.unbundleTasks(user, bundleId, taskIds) + this.repository.unbundleTasks(user, bundleId, taskIds, preventTaskIdUnlocks) this.getTaskBundle(user, bundleId) } diff --git a/conf/v2_route/bundle.api b/conf/v2_route/bundle.api index 389f8fb8..f68b7409 100644 --- a/conf/v2_route/bundle.api +++ b/conf/v2_route/bundle.api @@ -44,6 +44,29 @@ POST /taskBundle @org.maproulette.framework.c GET /taskBundle/:id @org.maproulette.framework.controller.TaskBundleController.getTaskBundle(id:Long) ### # tags: [ Bundle ] +# summary: Resets a Task Bundle +# description: Resets the bundle to the tasks provided, and unlock all tasks removed from current bundle +# responses: +# '200': +# description: Ok with empty body +# '401': +# description: The user is not authorized to make this request +# parameters: +# - name: id +# in: path +# description: The id of the Task Bundle +# required: true +# - name: primaryId +# in: query +# description: The task ids the bundle will reset to +# - name: taskIds +# in: query +# description: The primary task id of the bundle + +### +POST /taskBundle/:id/reset @org.maproulette.framework.controller.TaskBundleController.resetTaskBundle(id: Long, primaryId: Long, taskIds: List[Long]) +### +# tags: [ Bundle ] # summary: Deletes a Task Bundle # description: Deletes a task bundle based on the supplied id # responses: @@ -84,5 +107,9 @@ DELETE /taskBundle/:id @org.maproulette.framework.c # in: query # description: The list of task ids to remove from the bundle # required: true +# - name: preventTaskIdUnlocks +# in: query +# description: The list of task ids to keep locked when removed from bundle +# required: true ### -POST /taskBundle/:id/unbundle @org.maproulette.framework.controller.TaskBundleController.unbundleTasks(id:Long, taskIds:List[Long]) +POST /taskBundle/:id/unbundle @org.maproulette.framework.controller.TaskBundleController.unbundleTasks(id:Long, taskIds:List[Long], preventTaskIdUnlocks:List[Long]) From 49ac8321a9943fa5bb7eec96c143394e265cc9c2 Mon Sep 17 00:00:00 2001 From: CollinBeczak Date: Fri, 8 Mar 2024 18:02:02 -0600 Subject: [PATCH 02/26] add bundleTasks endpoint and fix bugs with bundle resets and simplify bundle creation endpoint --- .../controller/TaskBundleController.scala | 29 ++- .../mixins/SearchParametersMixin.scala | 17 ++ .../repository/TaskBundleRepository.scala | 205 +++++++++++++----- .../framework/service/TaskBundleService.scala | 40 ++-- .../session/SearchParameters.scala | 7 + conf/v2_route/bundle.api | 31 ++- .../service/TaskBundleServiceSpec.scala | 22 +- 7 files changed, 269 insertions(+), 82 deletions(-) diff --git a/app/org/maproulette/framework/controller/TaskBundleController.scala b/app/org/maproulette/framework/controller/TaskBundleController.scala index 37f4fb7e..addbcfd4 100644 --- a/app/org/maproulette/framework/controller/TaskBundleController.scala +++ b/app/org/maproulette/framework/controller/TaskBundleController.scala @@ -237,12 +237,13 @@ class TaskBundleController @Inject() ( */ def createTaskBundle(): Action[JsValue] = Action.async(bodyParsers.json) { implicit request => this.sessionManager.authenticatedRequest { implicit user => - val name = (request.body \ "name").asOpt[String].getOrElse("") + val name = (request.body \ "name").asOpt[String].getOrElse("") + val primaryId = (request.body \ "primaryId").asOpt[Long] val taskIds = (request.body \ "taskIds").asOpt[List[Long]] match { case Some(tasks) => tasks case None => throw new InvalidException("No task ids provided for task bundle") } - val bundle = this.serviceManager.taskBundle.createTaskBundle(user, name, taskIds) + val bundle = this.serviceManager.taskBundle.createTaskBundle(user, name, primaryId, taskIds) Created(Json.toJson(bundle)) } } @@ -253,10 +254,11 @@ class TaskBundleController @Inject() ( * @param id The id for the bundle * @return Task Bundle */ - def getTaskBundle(id: Long): Action[AnyContent] = Action.async { implicit request => - this.sessionManager.authenticatedRequest { implicit user => - Ok(Json.toJson(this.serviceManager.taskBundle.getTaskBundle(user, id))) - } + def getTaskBundle(id: Long, lockTasks: Boolean): Action[AnyContent] = Action.async { + implicit request => + this.sessionManager.authenticatedRequest { implicit user => + Ok(Json.toJson(this.serviceManager.taskBundle.getTaskBundle(user, id, lockTasks))) + } } /** @@ -295,6 +297,21 @@ class TaskBundleController @Inject() ( } } + /** + * Adds tasks to a bundle. + * + * @param id The id for the bundle + * @param taskIds List of task ids to remove + * @return Task Bundle + */ + def bundleTasks(id: Long, taskIds: List[Long]): Action[AnyContent] = Action.async { + implicit request => + this.sessionManager.authenticatedRequest { implicit user => + this.serviceManager.taskBundle.bundleTasks(user, id, taskIds) + Ok(Json.toJson(this.serviceManager.taskBundle.getTaskBundle(user, id))) + } + } + /** * Delete bundle. * diff --git a/app/org/maproulette/framework/mixins/SearchParametersMixin.scala b/app/org/maproulette/framework/mixins/SearchParametersMixin.scala index a3de8b53..938083f0 100644 --- a/app/org/maproulette/framework/mixins/SearchParametersMixin.scala +++ b/app/org/maproulette/framework/mixins/SearchParametersMixin.scala @@ -40,6 +40,7 @@ trait SearchParametersMixin { this.filterChallengeStatus(params), this.filterChallengeRequiresLocal(params), this.filterBoundingGeometries(params), + this.filterBundleId(params), // For efficiency can only query on task properties with a parent challenge id this.filterTaskProps(params), this.filterChallenges(params), @@ -324,6 +325,22 @@ trait SearchParametersMixin { } } + /** + * Filters by bundle id + * @param params with inverting on 'bid' + */ + def filterBundleId(params: SearchParameters): FilterGroup = { + params.taskParams.bundleId match { + case Some(bid) => + FilterGroup( + List( + CustomParameter(s"CAST(${Task.TABLE}.${Task.FIELD_BUNDLE_ID} AS TEXT) LIKE '${bid}%'") + ) + ) + case _ => FilterGroup(List()) + } + } + /** * Filters by tasks.priority * @param params with inverting on 'priorities' diff --git a/app/org/maproulette/framework/repository/TaskBundleRepository.scala b/app/org/maproulette/framework/repository/TaskBundleRepository.scala index c6030ee3..a2e8d1b3 100644 --- a/app/org/maproulette/framework/repository/TaskBundleRepository.scala +++ b/app/org/maproulette/framework/repository/TaskBundleRepository.scala @@ -8,6 +8,7 @@ package org.maproulette.framework.repository import org.slf4j.LoggerFactory import anorm.ToParameterValue +import org.maproulette.cache.CacheManager import anorm._, postgresql._ import javax.inject.{Inject, Singleton} import org.maproulette.exception.InvalidException @@ -32,9 +33,9 @@ class TaskBundleRepository @Inject() ( ) extends RepositoryMixin with TaskParserMixin with Locking[Task] { - protected val logger = LoggerFactory.getLogger(this.getClass) - implicit val baseTable: String = Task.TABLE - val cacheManager = this.taskRepository.cacheManager + protected val logger = LoggerFactory.getLogger(this.getClass) + implicit val baseTable: String = Task.TABLE + val cacheManager: CacheManager[Long, Task] = this.taskRepository.cacheManager /** * Inserts a new task bundle with the given tasks, assigning ownership of @@ -47,6 +48,7 @@ class TaskBundleRepository @Inject() ( def insert( user: User, name: String, + primaryId: Option[Long], taskIds: List[Long], verifyTasks: (List[Task]) => Unit ): TaskBundle = { @@ -64,24 +66,48 @@ class TaskBundleRepository @Inject() ( verifyTasks(lockedTasks) - for (task <- lockedTasks) { - try { - this.lockItem(user, task) - } catch { - case e: Exception => this.logger.warn(e.getMessage) - } - } - val rowId = SQL"""INSERT INTO bundles (owner_id, name) VALUES (${user.id}, ${name})""".executeInsert() + rowId match { - case Some(bundleId) => - val sqlQuery = + case Some(bundleId: Long) => + // Insert tasks into the task_bundles table + val sqlInsertTaskBundles = s"""INSERT INTO task_bundles (task_id, bundle_id) VALUES ({taskId}, $bundleId)""" - val parameters = lockedTasks.map(task => { - Seq[NamedParameter]("taskId" -> task.id) - }) - BatchSql(sqlQuery, parameters.head, parameters.tail: _*).execute() + + // Update tasks in the tasks table + SQL(s"""UPDATE tasks SET bundle_id = $bundleId + WHERE id IN ({inList})""") + .on( + "inList" -> taskIds + ) + .executeUpdate() + + primaryId.foreach { id => + SQL"""UPDATE tasks SET is_bundle_primary = true, bundle_id = $bundleId WHERE id = $id""" + .executeUpdate() + } + + val parameters = lockedTasks.map(task => Seq[NamedParameter]("taskId" -> task.id)) + BatchSql(sqlInsertTaskBundles, parameters.head, parameters.tail: _*).execute() + + // Lock each of the new tasks to indicate they are part of the bundle + for (task <- lockedTasks) { + try { + this.lockItem(user, task) + } catch { + case e: Exception => this.logger.warn(e.getMessage) + } + this.cacheManager.withOptionCaching { () => + Some( + task.copy( + bundleId = Some(bundleId), + isBundlePrimary = Some(primaryId == task.id) + ) + ) + } + } + TaskBundle(bundleId, user.id, lockedTasks.map(task => { task.id }), Some(lockedTasks)) @@ -108,7 +134,9 @@ class TaskBundleRepository @Inject() ( withMRTransaction { implicit c => // Remove tasks from the bundle join table and unlock them if necessary val tasks = - retrieveTasks(Query.simple(List(BaseParameter("bundle_id", bundleId, table = Some("tb"))))) + this.retrieveTasks( + Query.simple(List(BaseParameter("bundle_id", bundleId, table = Some("tb")))) + ) // Unset bundle_id for tasks not in the taskids param(this param is the task ids that the bundle will be "resetting" to) SQL( @@ -128,10 +156,15 @@ class TaskBundleRepository @Inject() ( s"""DELETE FROM task_bundles WHERE bundle_id = $bundleId AND task_id = ${task.id}""" ).executeUpdate() try { - unlockItem(user, task) + this.unlockItem(user, task) } catch { case e: Exception => logger.warn(e.getMessage) } + this.cacheManager.withOptionCaching { () => + Some( + task.copy(bundleId = None, isBundlePrimary = None) + ) + } } } @@ -149,28 +182,36 @@ class TaskBundleRepository @Inject() ( val tasksToAdd = taskIds.filterNot(existingTasks.contains) - // Construct parameters for the BatchSql operation - val parameters = tasksToAdd.map(taskId => Seq[NamedParameter]("taskId" -> taskId)) + if (tasksToAdd.nonEmpty) { + // Construct parameters for the BatchSql operation + val parameters = tasksToAdd.map(taskId => Seq[NamedParameter]("taskId" -> taskId)) - // Execute the BatchSql operation to insert task-bundle associations - BatchSql(sqlQuery, parameters.head, parameters.tail: _*).execute() + // Execute the BatchSql operation to insert task-bundle associations + BatchSql(sqlQuery, parameters.head, parameters.tail: _*).execute() - SQL(s"""UPDATE tasks SET bundle_id = {bundleId} + SQL(s"""UPDATE tasks SET bundle_id = {bundleId} WHERE bundle_id IS NULL AND id IN ({inList})""") - .on( - "bundleId" -> bundleId, - "inList" -> tasksToAdd - ) - .executeUpdate() - tasksToAdd.foreach { taskId => - val taskToLock = retrieveTasks(Query.simple(List(BaseParameter("id", List(taskId))))) - taskToLock.foreach { task => + .on( + "bundleId" -> bundleId, + "inList" -> tasksToAdd + ) + .executeUpdate() + + val lockedTasks = this.withListLocking(user, Some(TaskType())) { () => + this.taskDAL.retrieveListById(-1, 0)(tasksToAdd) + } + lockedTasks.foreach { task => try { this.lockItem(user, task) } catch { case e: Exception => this.logger.warn(e.getMessage) } + this.cacheManager.withOptionCaching { () => + Some( + task.copy(bundleId = Some(bundleId), isBundlePrimary = Some(primaryTaskId == task.id)) + ) + } } } @@ -233,7 +274,7 @@ class TaskBundleRepository @Inject() ( taskIds.find(id => id == task.id) match { case Some(_) => SQL(s"""DELETE FROM task_bundles - WHERE bundle_id = ${bundleId} AND task_id = ${task.id}""").executeUpdate() + WHERE bundle_id = $bundleId AND task_id = ${task.id}""").executeUpdate() if (!preventTaskIdUnlocks.contains(task.id)) { try { @@ -260,6 +301,59 @@ class TaskBundleRepository @Inject() ( } } + /** + * Adds tasks to a bundle. + * + * @param bundleId The id of the bundle + */ + def bundleTasks(user: User, bundleId: Long, taskIds: List[Long]): Unit = { + this.withMRConnection { implicit c => + // Update tasks to set their bundle_id to the provided bundleId + SQL(s"""UPDATE tasks SET bundle_id = {bundleId} + WHERE bundle_id IS NULL + AND id IN ({inList})""") + .on( + "bundleId" -> bundleId, + "inList" -> taskIds + ) + .executeUpdate() + + // Construct the SQL query to insert task-bundle associations + val sqlQuery = + s"""INSERT INTO task_bundles (bundle_id, task_id) VALUES ($bundleId, {taskId})""" + + // Construct parameters for the BatchSql operation + val parameters = taskIds.map(taskId => Seq[NamedParameter]("taskId" -> taskId)) + + // Execute the BatchSql operation to insert task-bundle associations + BatchSql(sqlQuery, parameters.head, parameters.tail: _*).execute() + + // Retrieve the tasks associated with the given bundleId + val tasks = this.retrieveTasks( + Query.simple( + List( + BaseParameter("bundle_id", bundleId, table = Some("tb")) + ) + ) + ) + // Lock each of the new tasks to indicate they are part of the bundle + for (task <- tasks) { + try { + this.lockItem(user, task) + } catch { + case e: Exception => this.logger.warn(e.getMessage) + } + this.cacheManager.withOptionCaching { () => + Some( + task.copy( + bundleId = Some(bundleId) + ) + ) + } + } + } + } + /** * Deletes a task bundle. * @@ -278,6 +372,9 @@ class TaskBundleRepository @Inject() ( ) .executeUpdate() + val inList = bundle.tasks.getOrElse(Nil).map(_.id).mkString(",") + SQL(s"UPDATE tasks SET bundle_id = NULL WHERE id IN ($inList)").executeUpdate() + if (primaryTaskId != None) { // unlock tasks (everything but the primary task id) val tasks = bundle.tasks match { @@ -290,28 +387,19 @@ class TaskBundleRepository @Inject() ( case e: Exception => this.logger.warn(e.getMessage) } } + this.cacheManager.withOptionCaching { () => + Some( + task.copy( + bundleId = None, + isBundlePrimary = None + ) + ) + } } case None => // no tasks in bundle } } - // Update cache for each task in the bundle - bundle.tasks match { - case Some(t) => - for (task <- t) { - this.cacheManager.withOptionCaching { () => - Some( - task.copy( - bundleId = None, - isBundlePrimary = None - ) - ) - } - } - - case None => // no tasks in bundle - } - // Delete from task_bundles which will also cascade delete from bundles SQL("DELETE FROM task_bundles WHERE bundle_id = {bundleId}") .on(Symbol("bundleId") -> bundle.bundleId) @@ -348,4 +436,21 @@ class TaskBundleRepository @Inject() ( """).as(this.getTaskParser(this.taskRepository.updateAndRetrieve).*) } } + + /** + * Fetches a list of tasks associated with the given bundle id. + * + * @param bundleId The id of the bundle + */ + def lockBundledTasks(user: User, tasks: List[Task]) = { + this.withMRConnection { implicit c => + for (task <- tasks) { + try { + this.lockItem(user, task) + } catch { + case e: Exception => this.logger.warn(e.getMessage) + } + } + } + } } diff --git a/app/org/maproulette/framework/service/TaskBundleService.scala b/app/org/maproulette/framework/service/TaskBundleService.scala index e83e45b6..73986020 100644 --- a/app/org/maproulette/framework/service/TaskBundleService.scala +++ b/app/org/maproulette/framework/service/TaskBundleService.scala @@ -39,11 +39,17 @@ class TaskBundleService @Inject() ( * @param name The name of the task bundle * @param taskIds The tasks to be added to the bundle */ - def createTaskBundle(user: User, name: String, taskIds: List[Long]): TaskBundle = { + def createTaskBundle( + user: User, + name: String, + bundlePrimary: Option[Long], + taskIds: List[Long] + ): TaskBundle = { this.repository.insert( user, name, + bundlePrimary, taskIds, (tasks: List[Task]) => { if (tasks.length < 1) { @@ -100,6 +106,18 @@ class TaskBundleService @Inject() ( this.getTaskBundle(user, bundleId) } + /** + * Removes tasks from a bundle. + * + * @param bundleId The id of the bundle + */ + def bundleTasks(user: User, bundleId: Long, taskIds: List[Long])(): TaskBundle = { + val bundle = this.getTaskBundle(user, bundleId) + + this.repository.bundleTasks(user, bundleId, taskIds) + this.getTaskBundle(user, bundleId) + } + /** * Removes tasks from a bundle. * @@ -113,13 +131,6 @@ class TaskBundleService @Inject() ( )(): TaskBundle = { val bundle = this.getTaskBundle(user, bundleId) - // Verify permissions to modify this bundle - if (!permission.isSuperUser(user) && bundle.ownerId != user.id) { - throw new IllegalAccessException( - "Only a super user or the original user can delete this bundle." - ) - } - this.repository.unbundleTasks(user, bundleId, taskIds, preventTaskIdUnlocks) this.getTaskBundle(user, bundleId) } @@ -147,7 +158,7 @@ class TaskBundleService @Inject() ( * * @param bundleId The id of the bundle */ - def getTaskBundle(user: User, bundleId: Long): TaskBundle = { + def getTaskBundle(user: User, bundleId: Long, lockTasks: Boolean = false): TaskBundle = { val filterQuery = Query.simple( List( @@ -158,12 +169,13 @@ class TaskBundleService @Inject() ( val ownerId = this.repository.retrieveOwner(filterQuery) val tasks = this.repository.retrieveTasks(filterQuery) - if (ownerId == None) { - throw new NotFoundException(s"Task Bundle not found with id ${bundleId}.") + if (ownerId.isEmpty) { + throw new NotFoundException(s"Task Bundle not found with id $bundleId.") + } + if (lockTasks) { + this.repository.lockBundledTasks(user, tasks) } - TaskBundle(bundleId, ownerId.get, tasks.map(task => { - task.id - }), Some(tasks)) + TaskBundle(bundleId, ownerId.get, tasks.map(_.id), Some(tasks)) } } diff --git a/app/org/maproulette/session/SearchParameters.scala b/app/org/maproulette/session/SearchParameters.scala index 4d5070f9..10ef98e3 100644 --- a/app/org/maproulette/session/SearchParameters.scala +++ b/app/org/maproulette/session/SearchParameters.scala @@ -50,6 +50,7 @@ case class SearchTaskParameters( taskStatus: Option[List[Int]] = None, taskId: Option[Long] = None, taskFeatureId: Option[String] = None, + bundleId: Option[Long] = None, taskReviewStatus: Option[List[Int]] = None, taskProperties: Option[Map[String, String]] = None, taskPropertySearchType: Option[String] = None, @@ -250,6 +251,10 @@ object SearchParameters { case Some(c) => Utils.insertIntoJson(updated, "taskFeatureId", c, true) case None => updated } + updated = o.taskParams.bundleId match { + case Some(c) => Utils.insertIntoJson(updated, "bundleId", c, true) + case None => updated + } updated = o.taskParams.taskReviewStatus match { case Some(c) => Utils.insertIntoJson(updated, "taskReviewStatus", c, true) case None => updated @@ -455,6 +460,8 @@ object SearchParameters { this.getLongParameter(request.getQueryString("tid"), params.taskParams.taskId), //taskFeatureId this.getStringParameter(request.getQueryString("fid"), params.taskParams.taskFeatureId), + //bundleId + this.getLongParameter(request.getQueryString("bid"), params.taskParams.bundleId), //taskReviewStatus request.getQueryString("trStatus") match { case Some(v) => Utils.toIntList(v) diff --git a/conf/v2_route/bundle.api b/conf/v2_route/bundle.api index f68b7409..807cb2d4 100644 --- a/conf/v2_route/bundle.api +++ b/conf/v2_route/bundle.api @@ -40,8 +40,11 @@ POST /taskBundle @org.maproulette.framework.c # in: path # description: The id of the Task Bundle # required: true +# - name: lockTasks +# in: query +# description: The tasks in the bundle will be locked by the user. ### -GET /taskBundle/:id @org.maproulette.framework.controller.TaskBundleController.getTaskBundle(id:Long) +GET /taskBundle/:id @org.maproulette.framework.controller.TaskBundleController.getTaskBundle(id:Long, lockTasks:Boolean ?= false) ### # tags: [ Bundle ] # summary: Resets a Task Bundle @@ -113,3 +116,29 @@ DELETE /taskBundle/:id @org.maproulette.framework.c # required: true ### POST /taskBundle/:id/unbundle @org.maproulette.framework.controller.TaskBundleController.unbundleTasks(id:Long, taskIds:List[Long], preventTaskIdUnlocks:List[Long]) +### +# tags: [ Bundle ] +# summary: Bundles extra tasks to Task Bundle +# description: Adds a list of tasks to a bundle of tasks +# responses: +# '200': +# description: The task bundle with the new increased set of tasks +# content: +# application/json: +# schema: +# $ref: '#/components/schemas/TaskBundle' +# '401': +# description: The user is not authorized to make this request +# '404': +# description: No Task Bundle with provided ID found +# parameters: +# - name: id +# in: path +# description: The id of the Task Bundle +# required: true +# - name: taskIds +# in: query +# description: The list of task ids to add from to bundle +# required: true +### +POST /taskBundle/:id/bundle @org.maproulette.framework.controller.TaskBundleController.bundleTasks(id:Long, taskIds:List[Long]) diff --git a/test/org/maproulette/framework/service/TaskBundleServiceSpec.scala b/test/org/maproulette/framework/service/TaskBundleServiceSpec.scala index 7529a851..202807f2 100644 --- a/test/org/maproulette/framework/service/TaskBundleServiceSpec.scala +++ b/test/org/maproulette/framework/service/TaskBundleServiceSpec.scala @@ -35,7 +35,7 @@ class TaskBundleServiceSpec(implicit val application: Application) extends Frame ) val response = - this.service.createTaskBundle(User.superUser, "my bundle", List(task1.id, task2.id)) + this.service.createTaskBundle(User.superUser, "my bundle",Some(0), List(task1.id, task2.id)) response.taskIds.length mustEqual 2 // tasks.bundle_id is NOT set until setTaskStatus is called!!! @@ -51,14 +51,14 @@ class TaskBundleServiceSpec(implicit val application: Application) extends Frame // Cannot create a bundle with tasks already assigned intercept[InvalidException] { - this.service.createTaskBundle(User.superUser, "my bundle again", List(task1.id, task2.id)) + this.service.createTaskBundle(User.superUser, "my bundle again", Some(0), List(task1.id, task2.id)) } } "not create a task Bundle with no tasks" taggedAs (TaskTag) in { // Cannot create a bundle with no tasks intercept[InvalidException] { - this.service.createTaskBundle(User.superUser, "my bundle again", List()) + this.service.createTaskBundle(User.superUser, "my bundle again",Some(0), List()) } } @@ -93,7 +93,7 @@ class TaskBundleServiceSpec(implicit val application: Application) extends Frame // Cannot create a bundle with tasks from different challenges intercept[InvalidException] { - this.service.createTaskBundle(User.superUser, "bad bundle", List(task1.id, task2.id)) + this.service.createTaskBundle(User.superUser, "bad bundle", Some(0), List(task1.id, task2.id)) } } @@ -110,7 +110,7 @@ class TaskBundleServiceSpec(implicit val application: Application) extends Frame ) val bundle = - this.service.createTaskBundle(User.superUser, "my bundle for get", List(task1.id, task2.id)) + this.service.createTaskBundle(User.superUser, "my bundle for get",Some(0), List(task1.id, task2.id)) val response = this.service.getTaskBundle(User.superUser, bundle.bundleId) response.bundleId mustEqual bundle.bundleId @@ -130,7 +130,7 @@ class TaskBundleServiceSpec(implicit val application: Application) extends Frame ) val bundle = this.service - .createTaskBundle(User.superUser, "my bundle for delete", List(task1.id, task2.id)) + .createTaskBundle(User.superUser, "my bundle for delete",Some(0), List(task1.id, task2.id)) // tasks.bundle_id is NOT set until setTaskStatus is called taskDAL.setTaskStatus( @@ -159,7 +159,7 @@ class TaskBundleServiceSpec(implicit val application: Application) extends Frame ) val bundle = this.service - .createTaskBundle(User.superUser, "my bundle for delete", List(task1.id, task2.id)) + .createTaskBundle(User.superUser, "my bundle for delete",Some(0), List(task1.id, task2.id)) // tasks.bundle_id is NOT set until setTaskStatus is called taskDAL.setTaskStatus( @@ -193,7 +193,7 @@ class TaskBundleServiceSpec(implicit val application: Application) extends Frame ) val bundle = this.service - .createTaskBundle(User.superUser, "my bundle for unbundle", List(task1.id, task2.id)) + .createTaskBundle(User.superUser, "my bundle for unbundle",Some(0), List(task1.id, task2.id)) // tasks.bundle_id is NOT set until setTaskStatus is called taskDAL.setTaskStatus( @@ -204,7 +204,7 @@ class TaskBundleServiceSpec(implicit val application: Application) extends Frame primaryTaskId = Some(task1.id) ) - this.service.unbundleTasks(User.superUser, bundle.bundleId, List(task2.id))() + this.service.unbundleTasks(User.superUser, bundle.bundleId, List(task2.id), List(task2.id))() val response = this.service.getTaskBundle(User.superUser, bundle.bundleId) response.taskIds.length mustEqual 1 response.taskIds.head mustEqual task1.id @@ -223,7 +223,7 @@ class TaskBundleServiceSpec(implicit val application: Application) extends Frame ) val bundle = this.service - .createTaskBundle(User.superUser, "my bundle for unbundle", List(task1.id, task2.id)) + .createTaskBundle(User.superUser, "my bundle for unbundle", Some(0), List(task1.id, task2.id)) // tasks.bundle_id is NOT set until setTaskStatus is called taskDAL.setTaskStatus( @@ -241,7 +241,7 @@ class TaskBundleServiceSpec(implicit val application: Application) extends Frame // Random user is not allowed to delete this bundle an[IllegalAccessException] should be thrownBy - this.service.unbundleTasks(randomUser, bundle.bundleId, List(task2.id))() + this.service.unbundleTasks(randomUser, bundle.bundleId, List(task2.id), List(task2.id))() } } From 6096fcc87fbd5a44d6e1c39062118cd02193a7ee Mon Sep 17 00:00:00 2001 From: CollinBeczak Date: Sun, 10 Mar 2024 17:02:27 -0500 Subject: [PATCH 03/26] remove bundleTasks endpoint --- .../controller/TaskBundleController.scala | 15 -- .../mixins/SearchParametersMixin.scala | 2 +- .../repository/TaskBundleRepository.scala | 137 +++++++----------- .../framework/service/TaskBundleService.scala | 12 -- conf/v2_route/bundle.api | 26 ---- 5 files changed, 50 insertions(+), 142 deletions(-) diff --git a/app/org/maproulette/framework/controller/TaskBundleController.scala b/app/org/maproulette/framework/controller/TaskBundleController.scala index addbcfd4..effd945a 100644 --- a/app/org/maproulette/framework/controller/TaskBundleController.scala +++ b/app/org/maproulette/framework/controller/TaskBundleController.scala @@ -297,21 +297,6 @@ class TaskBundleController @Inject() ( } } - /** - * Adds tasks to a bundle. - * - * @param id The id for the bundle - * @param taskIds List of task ids to remove - * @return Task Bundle - */ - def bundleTasks(id: Long, taskIds: List[Long]): Action[AnyContent] = Action.async { - implicit request => - this.sessionManager.authenticatedRequest { implicit user => - this.serviceManager.taskBundle.bundleTasks(user, id, taskIds) - Ok(Json.toJson(this.serviceManager.taskBundle.getTaskBundle(user, id))) - } - } - /** * Delete bundle. * diff --git a/app/org/maproulette/framework/mixins/SearchParametersMixin.scala b/app/org/maproulette/framework/mixins/SearchParametersMixin.scala index 938083f0..4e589143 100644 --- a/app/org/maproulette/framework/mixins/SearchParametersMixin.scala +++ b/app/org/maproulette/framework/mixins/SearchParametersMixin.scala @@ -334,7 +334,7 @@ trait SearchParametersMixin { case Some(bid) => FilterGroup( List( - CustomParameter(s"CAST(${Task.TABLE}.${Task.FIELD_BUNDLE_ID} AS TEXT) LIKE '${bid}%'") + CustomParameter(s"${Task.TABLE}.${Task.FIELD_BUNDLE_ID} = $bid") ) ) case _ => FilterGroup(List()) diff --git a/app/org/maproulette/framework/repository/TaskBundleRepository.scala b/app/org/maproulette/framework/repository/TaskBundleRepository.scala index a2e8d1b3..53552f14 100644 --- a/app/org/maproulette/framework/repository/TaskBundleRepository.scala +++ b/app/org/maproulette/framework/repository/TaskBundleRepository.scala @@ -82,10 +82,13 @@ class TaskBundleRepository @Inject() ( "inList" -> taskIds ) .executeUpdate() - - primaryId.foreach { id => - SQL"""UPDATE tasks SET is_bundle_primary = true, bundle_id = $bundleId WHERE id = $id""" - .executeUpdate() + primaryId match { + case Some(id) => + val sqlQuery = s"""UPDATE tasks SET is_bundle_primary = true WHERE id = $id""" + SQL(sqlQuery).executeUpdate() + case None => + // Handle the case where primaryId is None + println("primaryId is not defined") } val parameters = lockedTasks.map(task => Seq[NamedParameter]("taskId" -> task.id)) @@ -198,6 +201,14 @@ class TaskBundleRepository @Inject() ( ) .executeUpdate() + // Retrieve the status of the primary task + val primaryTaskStatus = SQL( + """SELECT status FROM tasks WHERE id = {primaryTaskId}""" + ).on( + "primaryTaskId" -> primaryTaskId + ) + .as(SqlParser.scalar[Int].single) + val lockedTasks = this.withListLocking(user, Some(TaskType())) { () => this.taskDAL.retrieveListById(-1, 0)(tasksToAdd) } @@ -207,33 +218,29 @@ class TaskBundleRepository @Inject() ( } catch { case e: Exception => this.logger.warn(e.getMessage) } + // Update the status of all tasks in the bundle to match the status of the primary task + SQL( + """UPDATE tasks + SET status = {primaryTaskStatus} + WHERE bundle_id = {bundleId} + """ + ).on( + "primaryTaskStatus" -> primaryTaskStatus, + "bundleId" -> bundleId + ) + .executeUpdate() + this.cacheManager.withOptionCaching { () => Some( - task.copy(bundleId = Some(bundleId), isBundlePrimary = Some(primaryTaskId == task.id)) + task.copy( + bundleId = Some(bundleId), + status = Some(primaryTaskStatus), + isBundlePrimary = Some(primaryTaskId == task.id) + ) ) } } } - - // Retrieve the status of the primary task - val primaryTaskStatus = SQL( - """SELECT status FROM tasks WHERE id = {primaryTaskId}""" - ).on( - "primaryTaskId" -> primaryTaskId - ) - .as(SqlParser.scalar[Int].single) - - // Update the status of all tasks in the bundle to match the status of the primary task - SQL( - """UPDATE tasks - SET status = {primaryTaskStatus} - WHERE bundle_id = {bundleId} - """ - ).on( - "primaryTaskStatus" -> primaryTaskStatus, - "bundleId" -> bundleId - ) - .executeUpdate() } } @@ -275,25 +282,32 @@ class TaskBundleRepository @Inject() ( case Some(_) => SQL(s"""DELETE FROM task_bundles WHERE bundle_id = $bundleId AND task_id = ${task.id}""").executeUpdate() - + this.cacheManager.withOptionCaching { () => + Some( + task.copy(bundleId = None, status = Some(STATUS_CREATED)) + ) + } if (!preventTaskIdUnlocks.contains(task.id)) { try { this.unlockItem(user, task) } catch { case e: Exception => this.logger.warn(e.getMessage) } - } else { - SQL( - """UPDATE tasks + } + // This is in order to pass the filters so the task is displayed as "available" in task searching and maps. + SQL(s"DELETE FROM task_review tr WHERE tr.task_id = ${task.id}").executeUpdate() + + SQL( + """UPDATE tasks SET status = {status} WHERE id = {taskId} """ - ).on( - "taskId" -> task.id, - "status" -> STATUS_CREATED - ) - .executeUpdate() - } + ).on( + "taskId" -> task.id, + "status" -> STATUS_CREATED + ) + .executeUpdate() + case None => // do nothing } } @@ -301,59 +315,6 @@ class TaskBundleRepository @Inject() ( } } - /** - * Adds tasks to a bundle. - * - * @param bundleId The id of the bundle - */ - def bundleTasks(user: User, bundleId: Long, taskIds: List[Long]): Unit = { - this.withMRConnection { implicit c => - // Update tasks to set their bundle_id to the provided bundleId - SQL(s"""UPDATE tasks SET bundle_id = {bundleId} - WHERE bundle_id IS NULL - AND id IN ({inList})""") - .on( - "bundleId" -> bundleId, - "inList" -> taskIds - ) - .executeUpdate() - - // Construct the SQL query to insert task-bundle associations - val sqlQuery = - s"""INSERT INTO task_bundles (bundle_id, task_id) VALUES ($bundleId, {taskId})""" - - // Construct parameters for the BatchSql operation - val parameters = taskIds.map(taskId => Seq[NamedParameter]("taskId" -> taskId)) - - // Execute the BatchSql operation to insert task-bundle associations - BatchSql(sqlQuery, parameters.head, parameters.tail: _*).execute() - - // Retrieve the tasks associated with the given bundleId - val tasks = this.retrieveTasks( - Query.simple( - List( - BaseParameter("bundle_id", bundleId, table = Some("tb")) - ) - ) - ) - // Lock each of the new tasks to indicate they are part of the bundle - for (task <- tasks) { - try { - this.lockItem(user, task) - } catch { - case e: Exception => this.logger.warn(e.getMessage) - } - this.cacheManager.withOptionCaching { () => - Some( - task.copy( - bundleId = Some(bundleId) - ) - ) - } - } - } - } - /** * Deletes a task bundle. * diff --git a/app/org/maproulette/framework/service/TaskBundleService.scala b/app/org/maproulette/framework/service/TaskBundleService.scala index 73986020..a5d746f2 100644 --- a/app/org/maproulette/framework/service/TaskBundleService.scala +++ b/app/org/maproulette/framework/service/TaskBundleService.scala @@ -106,18 +106,6 @@ class TaskBundleService @Inject() ( this.getTaskBundle(user, bundleId) } - /** - * Removes tasks from a bundle. - * - * @param bundleId The id of the bundle - */ - def bundleTasks(user: User, bundleId: Long, taskIds: List[Long])(): TaskBundle = { - val bundle = this.getTaskBundle(user, bundleId) - - this.repository.bundleTasks(user, bundleId, taskIds) - this.getTaskBundle(user, bundleId) - } - /** * Removes tasks from a bundle. * diff --git a/conf/v2_route/bundle.api b/conf/v2_route/bundle.api index 807cb2d4..e6841b55 100644 --- a/conf/v2_route/bundle.api +++ b/conf/v2_route/bundle.api @@ -116,29 +116,3 @@ DELETE /taskBundle/:id @org.maproulette.framework.c # required: true ### POST /taskBundle/:id/unbundle @org.maproulette.framework.controller.TaskBundleController.unbundleTasks(id:Long, taskIds:List[Long], preventTaskIdUnlocks:List[Long]) -### -# tags: [ Bundle ] -# summary: Bundles extra tasks to Task Bundle -# description: Adds a list of tasks to a bundle of tasks -# responses: -# '200': -# description: The task bundle with the new increased set of tasks -# content: -# application/json: -# schema: -# $ref: '#/components/schemas/TaskBundle' -# '401': -# description: The user is not authorized to make this request -# '404': -# description: No Task Bundle with provided ID found -# parameters: -# - name: id -# in: path -# description: The id of the Task Bundle -# required: true -# - name: taskIds -# in: query -# description: The list of task ids to add from to bundle -# required: true -### -POST /taskBundle/:id/bundle @org.maproulette.framework.controller.TaskBundleController.bundleTasks(id:Long, taskIds:List[Long]) From c99f843be85a46cef9a75df78e2cb78b1c71e4d8 Mon Sep 17 00:00:00 2001 From: CollinBeczak Date: Sun, 10 Mar 2024 21:02:25 -0500 Subject: [PATCH 04/26] fix task bundle reset to setup up tasks added back with the proper review_status needed for reviewing --- .../repository/TaskBundleRepository.scala | 30 ++++++++++++++++--- 1 file changed, 26 insertions(+), 4 deletions(-) diff --git a/app/org/maproulette/framework/repository/TaskBundleRepository.scala b/app/org/maproulette/framework/repository/TaskBundleRepository.scala index 53552f14..6a98910e 100644 --- a/app/org/maproulette/framework/repository/TaskBundleRepository.scala +++ b/app/org/maproulette/framework/repository/TaskBundleRepository.scala @@ -8,6 +8,9 @@ package org.maproulette.framework.repository import org.slf4j.LoggerFactory import anorm.ToParameterValue +import anorm._ +import anorm.SqlParser.scalar + import org.maproulette.cache.CacheManager import anorm._, postgresql._ import javax.inject.{Inject, Singleton} @@ -201,13 +204,29 @@ class TaskBundleRepository @Inject() ( ) .executeUpdate() + // Define the fallback value + val fallbackStatus: Int = 0 + // Retrieve the status of the primary task - val primaryTaskStatus = SQL( + val primaryTaskStatus: Int = SQL( """SELECT status FROM tasks WHERE id = {primaryTaskId}""" ).on( "primaryTaskId" -> primaryTaskId ) - .as(SqlParser.scalar[Int].single) + .as(scalar[Int].singleOpt) + .getOrElse(fallbackStatus) + + // Define the fallback value for review status + val fallbackReviewStatus: Int = 0 + + // Retrieve the review status of the primary task + val primaryTaskReviewStatus: Int = SQL( + """SELECT review_status FROM task_review WHERE id = {primaryTaskId}""" + ).on( + "primaryTaskId" -> primaryTaskId + ) + .as(scalar[Int].singleOpt) + .getOrElse(fallbackReviewStatus) val lockedTasks = this.withListLocking(user, Some(TaskType())) { () => this.taskDAL.retrieveListById(-1, 0)(tasksToAdd) @@ -222,14 +241,17 @@ class TaskBundleRepository @Inject() ( SQL( """UPDATE tasks SET status = {primaryTaskStatus} - WHERE bundle_id = {bundleId} + WHERE id = {taskId} """ ).on( "primaryTaskStatus" -> primaryTaskStatus, - "bundleId" -> bundleId + "taskId" -> task.id ) .executeUpdate() + SQL"""INSERT INTO task_review (task_id, review_status) + VALUES (${task.id}, ${primaryTaskReviewStatus})""".executeUpdate() + this.cacheManager.withOptionCaching { () => Some( task.copy( From 25e8c06cf43d388065ba8ec02fa1dd3516633346 Mon Sep 17 00:00:00 2001 From: CollinBeczak Date: Mon, 11 Mar 2024 13:49:13 -0500 Subject: [PATCH 05/26] run scalafmt --- .../maproulette/framework/repository/TaskBundleRepository.scala | 1 - 1 file changed, 1 deletion(-) diff --git a/app/org/maproulette/framework/repository/TaskBundleRepository.scala b/app/org/maproulette/framework/repository/TaskBundleRepository.scala index 6a98910e..76a2fac5 100644 --- a/app/org/maproulette/framework/repository/TaskBundleRepository.scala +++ b/app/org/maproulette/framework/repository/TaskBundleRepository.scala @@ -8,7 +8,6 @@ package org.maproulette.framework.repository import org.slf4j.LoggerFactory import anorm.ToParameterValue -import anorm._ import anorm.SqlParser.scalar import org.maproulette.cache.CacheManager From eac3b5bb1ec40871962f2ab38c0ae72a69e5483f Mon Sep 17 00:00:00 2001 From: CollinBeczak Date: Mon, 11 Mar 2024 13:57:26 -0500 Subject: [PATCH 06/26] formatting, and fix test to allow task in bundle to be unlocked --- .../service/TaskBundleServiceSpec.scala | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/test/org/maproulette/framework/service/TaskBundleServiceSpec.scala b/test/org/maproulette/framework/service/TaskBundleServiceSpec.scala index 202807f2..1b26d6e2 100644 --- a/test/org/maproulette/framework/service/TaskBundleServiceSpec.scala +++ b/test/org/maproulette/framework/service/TaskBundleServiceSpec.scala @@ -35,7 +35,7 @@ class TaskBundleServiceSpec(implicit val application: Application) extends Frame ) val response = - this.service.createTaskBundle(User.superUser, "my bundle",Some(0), List(task1.id, task2.id)) + this.service.createTaskBundle(User.superUser, "my bundle", Some(0), List(task1.id, task2.id)) response.taskIds.length mustEqual 2 // tasks.bundle_id is NOT set until setTaskStatus is called!!! @@ -58,7 +58,7 @@ class TaskBundleServiceSpec(implicit val application: Application) extends Frame "not create a task Bundle with no tasks" taggedAs (TaskTag) in { // Cannot create a bundle with no tasks intercept[InvalidException] { - this.service.createTaskBundle(User.superUser, "my bundle again",Some(0), List()) + this.service.createTaskBundle(User.superUser, "my bundle again", Some(0), List()) } } @@ -110,7 +110,7 @@ class TaskBundleServiceSpec(implicit val application: Application) extends Frame ) val bundle = - this.service.createTaskBundle(User.superUser, "my bundle for get",Some(0), List(task1.id, task2.id)) + this.service.createTaskBundle(User.superUser, "my bundle for get", Some(0), List(task1.id, task2.id)) val response = this.service.getTaskBundle(User.superUser, bundle.bundleId) response.bundleId mustEqual bundle.bundleId @@ -130,7 +130,7 @@ class TaskBundleServiceSpec(implicit val application: Application) extends Frame ) val bundle = this.service - .createTaskBundle(User.superUser, "my bundle for delete",Some(0), List(task1.id, task2.id)) + .createTaskBundle(User.superUser, "my bundle for delete", Some(0), List(task1.id, task2.id)) // tasks.bundle_id is NOT set until setTaskStatus is called taskDAL.setTaskStatus( @@ -159,7 +159,7 @@ class TaskBundleServiceSpec(implicit val application: Application) extends Frame ) val bundle = this.service - .createTaskBundle(User.superUser, "my bundle for delete",Some(0), List(task1.id, task2.id)) + .createTaskBundle(User.superUser, "my bundle for delete", Some(0), List(task1.id, task2.id)) // tasks.bundle_id is NOT set until setTaskStatus is called taskDAL.setTaskStatus( @@ -193,7 +193,7 @@ class TaskBundleServiceSpec(implicit val application: Application) extends Frame ) val bundle = this.service - .createTaskBundle(User.superUser, "my bundle for unbundle",Some(0), List(task1.id, task2.id)) + .createTaskBundle(User.superUser, "my bundle for unbundle", Some(0), List(task1.id, task2.id)) // tasks.bundle_id is NOT set until setTaskStatus is called taskDAL.setTaskStatus( @@ -204,7 +204,7 @@ class TaskBundleServiceSpec(implicit val application: Application) extends Frame primaryTaskId = Some(task1.id) ) - this.service.unbundleTasks(User.superUser, bundle.bundleId, List(task2.id), List(task2.id))() + this.service.unbundleTasks(User.superUser, bundle.bundleId, List(task1.id), List(task2.id))() val response = this.service.getTaskBundle(User.superUser, bundle.bundleId) response.taskIds.length mustEqual 1 response.taskIds.head mustEqual task1.id @@ -241,7 +241,7 @@ class TaskBundleServiceSpec(implicit val application: Application) extends Frame // Random user is not allowed to delete this bundle an[IllegalAccessException] should be thrownBy - this.service.unbundleTasks(randomUser, bundle.bundleId, List(task2.id), List(task2.id))() + this.service.unbundleTasks(randomUser, bundle.bundleId, List(task1.id), List(task2.id))() } } From 635949ac76f92a43e5650afeffdce2cd0b8270f7 Mon Sep 17 00:00:00 2001 From: CollinBeczak Date: Mon, 11 Mar 2024 15:02:11 -0500 Subject: [PATCH 07/26] fix some variables in the task bundling related tests --- .../service/TaskBundleServiceSpec.scala | 22 +++++++++---------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/test/org/maproulette/framework/service/TaskBundleServiceSpec.scala b/test/org/maproulette/framework/service/TaskBundleServiceSpec.scala index 1b26d6e2..49ca566d 100644 --- a/test/org/maproulette/framework/service/TaskBundleServiceSpec.scala +++ b/test/org/maproulette/framework/service/TaskBundleServiceSpec.scala @@ -35,7 +35,7 @@ class TaskBundleServiceSpec(implicit val application: Application) extends Frame ) val response = - this.service.createTaskBundle(User.superUser, "my bundle", Some(0), List(task1.id, task2.id)) + this.service.createTaskBundle(User.superUser, "my bundle", Some(task1.id), List(task1.id, task2.id)) response.taskIds.length mustEqual 2 // tasks.bundle_id is NOT set until setTaskStatus is called!!! @@ -51,14 +51,14 @@ class TaskBundleServiceSpec(implicit val application: Application) extends Frame // Cannot create a bundle with tasks already assigned intercept[InvalidException] { - this.service.createTaskBundle(User.superUser, "my bundle again", Some(0), List(task1.id, task2.id)) + this.service.createTaskBundle(User.superUser, "my bundle again", Some(task1.id), List(task1.id, task2.id)) } } "not create a task Bundle with no tasks" taggedAs (TaskTag) in { // Cannot create a bundle with no tasks intercept[InvalidException] { - this.service.createTaskBundle(User.superUser, "my bundle again", Some(0), List()) + this.service.createTaskBundle(User.superUser, "my bundle again", Some(0), List()) } } @@ -93,7 +93,7 @@ class TaskBundleServiceSpec(implicit val application: Application) extends Frame // Cannot create a bundle with tasks from different challenges intercept[InvalidException] { - this.service.createTaskBundle(User.superUser, "bad bundle", Some(0), List(task1.id, task2.id)) + this.service.createTaskBundle(User.superUser, "bad bundle", Some(task1.id), List(task1.id, task2.id)) } } @@ -110,7 +110,7 @@ class TaskBundleServiceSpec(implicit val application: Application) extends Frame ) val bundle = - this.service.createTaskBundle(User.superUser, "my bundle for get", Some(0), List(task1.id, task2.id)) + this.service.createTaskBundle(User.superUser, "my bundle for get", Some(task1.id), List(task1.id, task2.id)) val response = this.service.getTaskBundle(User.superUser, bundle.bundleId) response.bundleId mustEqual bundle.bundleId @@ -130,7 +130,7 @@ class TaskBundleServiceSpec(implicit val application: Application) extends Frame ) val bundle = this.service - .createTaskBundle(User.superUser, "my bundle for delete", Some(0), List(task1.id, task2.id)) + .createTaskBundle(User.superUser, "my bundle for delete", Some(task1.id), List(task1.id, task2.id)) // tasks.bundle_id is NOT set until setTaskStatus is called taskDAL.setTaskStatus( @@ -159,7 +159,7 @@ class TaskBundleServiceSpec(implicit val application: Application) extends Frame ) val bundle = this.service - .createTaskBundle(User.superUser, "my bundle for delete", Some(0), List(task1.id, task2.id)) + .createTaskBundle(User.superUser, "my bundle for delete", Some(task1.id), List(task1.id, task2.id)) // tasks.bundle_id is NOT set until setTaskStatus is called taskDAL.setTaskStatus( @@ -193,7 +193,7 @@ class TaskBundleServiceSpec(implicit val application: Application) extends Frame ) val bundle = this.service - .createTaskBundle(User.superUser, "my bundle for unbundle", Some(0), List(task1.id, task2.id)) + .createTaskBundle(User.superUser, "my bundle for unbundle", Some(task1.id), List(task1.id, task2.id)) // tasks.bundle_id is NOT set until setTaskStatus is called taskDAL.setTaskStatus( @@ -204,7 +204,7 @@ class TaskBundleServiceSpec(implicit val application: Application) extends Frame primaryTaskId = Some(task1.id) ) - this.service.unbundleTasks(User.superUser, bundle.bundleId, List(task1.id), List(task2.id))() + this.service.unbundleTasks(User.superUser, bundle.bundleId, List(task2.id), List(task1.id))() val response = this.service.getTaskBundle(User.superUser, bundle.bundleId) response.taskIds.length mustEqual 1 response.taskIds.head mustEqual task1.id @@ -223,7 +223,7 @@ class TaskBundleServiceSpec(implicit val application: Application) extends Frame ) val bundle = this.service - .createTaskBundle(User.superUser, "my bundle for unbundle", Some(0), List(task1.id, task2.id)) + .createTaskBundle(User.superUser, "my bundle for unbundle", Some(task1.id), List(task1.id, task2.id)) // tasks.bundle_id is NOT set until setTaskStatus is called taskDAL.setTaskStatus( @@ -241,7 +241,7 @@ class TaskBundleServiceSpec(implicit val application: Application) extends Frame // Random user is not allowed to delete this bundle an[IllegalAccessException] should be thrownBy - this.service.unbundleTasks(randomUser, bundle.bundleId, List(task1.id), List(task2.id))() + this.service.unbundleTasks(randomUser, bundle.bundleId, List(task2.id), List(task1.id))() } } From 759a8aa7e72143b010c5111fd3c41d23e504dd03 Mon Sep 17 00:00:00 2001 From: CollinBeczak Date: Mon, 11 Mar 2024 15:23:07 -0500 Subject: [PATCH 08/26] revert task locking on fetch --- .../controller/TaskBundleController.scala | 4 ++-- .../repository/TaskBundleRepository.scala | 17 ----------------- .../framework/service/TaskBundleService.scala | 13 ++++++------- conf/v2_route/bundle.api | 5 +---- 4 files changed, 9 insertions(+), 30 deletions(-) diff --git a/app/org/maproulette/framework/controller/TaskBundleController.scala b/app/org/maproulette/framework/controller/TaskBundleController.scala index effd945a..91e86f44 100644 --- a/app/org/maproulette/framework/controller/TaskBundleController.scala +++ b/app/org/maproulette/framework/controller/TaskBundleController.scala @@ -254,10 +254,10 @@ class TaskBundleController @Inject() ( * @param id The id for the bundle * @return Task Bundle */ - def getTaskBundle(id: Long, lockTasks: Boolean): Action[AnyContent] = Action.async { + def getTaskBundle(id: Long): Action[AnyContent] = Action.async { implicit request => this.sessionManager.authenticatedRequest { implicit user => - Ok(Json.toJson(this.serviceManager.taskBundle.getTaskBundle(user, id, lockTasks))) + Ok(Json.toJson(this.serviceManager.taskBundle.getTaskBundle(user, id))) } } diff --git a/app/org/maproulette/framework/repository/TaskBundleRepository.scala b/app/org/maproulette/framework/repository/TaskBundleRepository.scala index 76a2fac5..4166d2a8 100644 --- a/app/org/maproulette/framework/repository/TaskBundleRepository.scala +++ b/app/org/maproulette/framework/repository/TaskBundleRepository.scala @@ -418,21 +418,4 @@ class TaskBundleRepository @Inject() ( """).as(this.getTaskParser(this.taskRepository.updateAndRetrieve).*) } } - - /** - * Fetches a list of tasks associated with the given bundle id. - * - * @param bundleId The id of the bundle - */ - def lockBundledTasks(user: User, tasks: List[Task]) = { - this.withMRConnection { implicit c => - for (task <- tasks) { - try { - this.lockItem(user, task) - } catch { - case e: Exception => this.logger.warn(e.getMessage) - } - } - } - } } diff --git a/app/org/maproulette/framework/service/TaskBundleService.scala b/app/org/maproulette/framework/service/TaskBundleService.scala index a5d746f2..df6827a9 100644 --- a/app/org/maproulette/framework/service/TaskBundleService.scala +++ b/app/org/maproulette/framework/service/TaskBundleService.scala @@ -146,7 +146,7 @@ class TaskBundleService @Inject() ( * * @param bundleId The id of the bundle */ - def getTaskBundle(user: User, bundleId: Long, lockTasks: Boolean = false): TaskBundle = { + def getTaskBundle(user: User, bundleId: Long): TaskBundle = { val filterQuery = Query.simple( List( @@ -157,13 +157,12 @@ class TaskBundleService @Inject() ( val ownerId = this.repository.retrieveOwner(filterQuery) val tasks = this.repository.retrieveTasks(filterQuery) - if (ownerId.isEmpty) { - throw new NotFoundException(s"Task Bundle not found with id $bundleId.") - } - if (lockTasks) { - this.repository.lockBundledTasks(user, tasks) + if (ownerId == None) { + throw new NotFoundException(s"Task Bundle not found with id ${bundleId}.") } - TaskBundle(bundleId, ownerId.get, tasks.map(_.id), Some(tasks)) + TaskBundle(bundleId, ownerId.get, tasks.map(task => { + task.id + }), Some(tasks)) } } diff --git a/conf/v2_route/bundle.api b/conf/v2_route/bundle.api index e6841b55..f68b7409 100644 --- a/conf/v2_route/bundle.api +++ b/conf/v2_route/bundle.api @@ -40,11 +40,8 @@ POST /taskBundle @org.maproulette.framework.c # in: path # description: The id of the Task Bundle # required: true -# - name: lockTasks -# in: query -# description: The tasks in the bundle will be locked by the user. ### -GET /taskBundle/:id @org.maproulette.framework.controller.TaskBundleController.getTaskBundle(id:Long, lockTasks:Boolean ?= false) +GET /taskBundle/:id @org.maproulette.framework.controller.TaskBundleController.getTaskBundle(id:Long) ### # tags: [ Bundle ] # summary: Resets a Task Bundle From 3386a8d3e084ad6c9d30e9fb7c621c2b6a20fd9c Mon Sep 17 00:00:00 2001 From: CollinBeczak Date: Mon, 11 Mar 2024 15:47:05 -0500 Subject: [PATCH 09/26] remove test because it is no longer needed --- .../controller/TaskBundleController.scala | 9 +++-- .../framework/service/TaskBundleService.scala | 2 +- .../service/TaskBundleServiceSpec.scala | 34 ------------------- 3 files changed, 5 insertions(+), 40 deletions(-) diff --git a/app/org/maproulette/framework/controller/TaskBundleController.scala b/app/org/maproulette/framework/controller/TaskBundleController.scala index 91e86f44..9ce94031 100644 --- a/app/org/maproulette/framework/controller/TaskBundleController.scala +++ b/app/org/maproulette/framework/controller/TaskBundleController.scala @@ -254,11 +254,10 @@ class TaskBundleController @Inject() ( * @param id The id for the bundle * @return Task Bundle */ - def getTaskBundle(id: Long): Action[AnyContent] = Action.async { - implicit request => - this.sessionManager.authenticatedRequest { implicit user => - Ok(Json.toJson(this.serviceManager.taskBundle.getTaskBundle(user, id))) - } + def getTaskBundle(id: Long): Action[AnyContent] = Action.async { implicit request => + this.sessionManager.authenticatedRequest { implicit user => + Ok(Json.toJson(this.serviceManager.taskBundle.getTaskBundle(user, id))) + } } /** diff --git a/app/org/maproulette/framework/service/TaskBundleService.scala b/app/org/maproulette/framework/service/TaskBundleService.scala index df6827a9..5e9183b4 100644 --- a/app/org/maproulette/framework/service/TaskBundleService.scala +++ b/app/org/maproulette/framework/service/TaskBundleService.scala @@ -161,7 +161,7 @@ class TaskBundleService @Inject() ( throw new NotFoundException(s"Task Bundle not found with id ${bundleId}.") } - TaskBundle(bundleId, ownerId.get, tasks.map(task => { + TaskBundle(bundleId, ownerId.get, tasks.map(task => { task.id }), Some(tasks)) } diff --git a/test/org/maproulette/framework/service/TaskBundleServiceSpec.scala b/test/org/maproulette/framework/service/TaskBundleServiceSpec.scala index 49ca566d..691e35c3 100644 --- a/test/org/maproulette/framework/service/TaskBundleServiceSpec.scala +++ b/test/org/maproulette/framework/service/TaskBundleServiceSpec.scala @@ -210,40 +210,6 @@ class TaskBundleServiceSpec(implicit val application: Application) extends Frame response.taskIds.head mustEqual task1.id } - "unbundle a task with permission check" taggedAs (TaskTag) in { - val task1 = taskDAL - .insert( - getTestTask(UUID.randomUUID().toString, challenge.id), - User.superUser - ) - var task2 = taskDAL - .insert( - getTestTask(UUID.randomUUID().toString, challenge.id), - User.superUser - ) - - val bundle = this.service - .createTaskBundle(User.superUser, "my bundle for unbundle", Some(task1.id), List(task1.id, task2.id)) - - // tasks.bundle_id is NOT set until setTaskStatus is called - taskDAL.setTaskStatus( - List(task1, task2), - Task.STATUS_FIXED, - User.superUser, - bundleId = Some(bundle.bundleId), - primaryTaskId = Some(task1.id) - ) - - val randomUser = serviceManager.user.create( - this.getTestUser(1022345, "RandomOUser2"), - User.superUser - ) - - // Random user is not allowed to delete this bundle - an[IllegalAccessException] should be thrownBy - this.service.unbundleTasks(randomUser, bundle.bundleId, List(task2.id), List(task1.id))() - } - } override implicit val projectTestName: String = "TaskBundleSpecProject" From aecba903967f9b255b6725ddbeda21913208fd2b Mon Sep 17 00:00:00 2001 From: CollinBeczak Date: Mon, 11 Mar 2024 16:27:11 -0500 Subject: [PATCH 10/26] add task locking back --- .../controller/TaskBundleController.scala | 9 +++++---- .../repository/TaskBundleRepository.scala | 17 +++++++++++++++++ .../framework/service/TaskBundleService.scala | 13 +++++++------ conf/v2_route/bundle.api | 5 ++++- 4 files changed, 33 insertions(+), 11 deletions(-) diff --git a/app/org/maproulette/framework/controller/TaskBundleController.scala b/app/org/maproulette/framework/controller/TaskBundleController.scala index 9ce94031..effd945a 100644 --- a/app/org/maproulette/framework/controller/TaskBundleController.scala +++ b/app/org/maproulette/framework/controller/TaskBundleController.scala @@ -254,10 +254,11 @@ class TaskBundleController @Inject() ( * @param id The id for the bundle * @return Task Bundle */ - def getTaskBundle(id: Long): Action[AnyContent] = Action.async { implicit request => - this.sessionManager.authenticatedRequest { implicit user => - Ok(Json.toJson(this.serviceManager.taskBundle.getTaskBundle(user, id))) - } + def getTaskBundle(id: Long, lockTasks: Boolean): Action[AnyContent] = Action.async { + implicit request => + this.sessionManager.authenticatedRequest { implicit user => + Ok(Json.toJson(this.serviceManager.taskBundle.getTaskBundle(user, id, lockTasks))) + } } /** diff --git a/app/org/maproulette/framework/repository/TaskBundleRepository.scala b/app/org/maproulette/framework/repository/TaskBundleRepository.scala index 4166d2a8..65bc0e83 100644 --- a/app/org/maproulette/framework/repository/TaskBundleRepository.scala +++ b/app/org/maproulette/framework/repository/TaskBundleRepository.scala @@ -418,4 +418,21 @@ class TaskBundleRepository @Inject() ( """).as(this.getTaskParser(this.taskRepository.updateAndRetrieve).*) } } + + /** + * Locks tasks on bundle fetch if task is in an editable status + * + * @param bundleId The id of the bundle + */ + def lockBundledTasks(user: User, tasks: List[Task]) = { + this.withMRConnection { implicit c => + for (task <- tasks) { + try { + this.lockItem(user, task) + } catch { + case e: Exception => this.logger.warn(e.getMessage) + } + } + } + } } diff --git a/app/org/maproulette/framework/service/TaskBundleService.scala b/app/org/maproulette/framework/service/TaskBundleService.scala index 5e9183b4..a5d746f2 100644 --- a/app/org/maproulette/framework/service/TaskBundleService.scala +++ b/app/org/maproulette/framework/service/TaskBundleService.scala @@ -146,7 +146,7 @@ class TaskBundleService @Inject() ( * * @param bundleId The id of the bundle */ - def getTaskBundle(user: User, bundleId: Long): TaskBundle = { + def getTaskBundle(user: User, bundleId: Long, lockTasks: Boolean = false): TaskBundle = { val filterQuery = Query.simple( List( @@ -157,12 +157,13 @@ class TaskBundleService @Inject() ( val ownerId = this.repository.retrieveOwner(filterQuery) val tasks = this.repository.retrieveTasks(filterQuery) - if (ownerId == None) { - throw new NotFoundException(s"Task Bundle not found with id ${bundleId}.") + if (ownerId.isEmpty) { + throw new NotFoundException(s"Task Bundle not found with id $bundleId.") + } + if (lockTasks) { + this.repository.lockBundledTasks(user, tasks) } - TaskBundle(bundleId, ownerId.get, tasks.map(task => { - task.id - }), Some(tasks)) + TaskBundle(bundleId, ownerId.get, tasks.map(_.id), Some(tasks)) } } diff --git a/conf/v2_route/bundle.api b/conf/v2_route/bundle.api index f68b7409..e6841b55 100644 --- a/conf/v2_route/bundle.api +++ b/conf/v2_route/bundle.api @@ -40,8 +40,11 @@ POST /taskBundle @org.maproulette.framework.c # in: path # description: The id of the Task Bundle # required: true +# - name: lockTasks +# in: query +# description: The tasks in the bundle will be locked by the user. ### -GET /taskBundle/:id @org.maproulette.framework.controller.TaskBundleController.getTaskBundle(id:Long) +GET /taskBundle/:id @org.maproulette.framework.controller.TaskBundleController.getTaskBundle(id:Long, lockTasks:Boolean ?= false) ### # tags: [ Bundle ] # summary: Resets a Task Bundle From 6c3b8c9844bbf3544be141453b66cc54ffbbd2df Mon Sep 17 00:00:00 2001 From: CollinBeczak Date: Tue, 12 Mar 2024 15:41:52 -0500 Subject: [PATCH 11/26] updae comments --- .../repository/TaskBundleRepository.scala | 20 +++++++------------ 1 file changed, 7 insertions(+), 13 deletions(-) diff --git a/app/org/maproulette/framework/repository/TaskBundleRepository.scala b/app/org/maproulette/framework/repository/TaskBundleRepository.scala index 65bc0e83..ffe214f2 100644 --- a/app/org/maproulette/framework/repository/TaskBundleRepository.scala +++ b/app/org/maproulette/framework/repository/TaskBundleRepository.scala @@ -60,6 +60,7 @@ class TaskBundleRepository @Inject() ( } val failedTaskIds = taskIds.diff(lockedTasks.map(_.id)) + // Checks to see if there where any tasks that were locked when the user tried to bundle them. if (failedTaskIds.nonEmpty) { throw new InvalidException( s"Bundle creation failed because the following task IDs were locked: ${failedTaskIds.mkString(", ")}" @@ -73,17 +74,17 @@ class TaskBundleRepository @Inject() ( rowId match { case Some(bundleId: Long) => - // Insert tasks into the task_bundles table val sqlInsertTaskBundles = s"""INSERT INTO task_bundles (task_id, bundle_id) VALUES ({taskId}, $bundleId)""" - // Update tasks in the tasks table + // Update the task object to bind it to the bundle SQL(s"""UPDATE tasks SET bundle_id = $bundleId - WHERE id IN ({inList})""") + WHERE id IN ({inList})""") .on( "inList" -> taskIds ) .executeUpdate() + primaryId match { case Some(id) => val sqlQuery = s"""UPDATE tasks SET is_bundle_primary = true WHERE id = $id""" @@ -173,7 +174,7 @@ class TaskBundleRepository @Inject() ( } } - // Add tasks to the bundle and lock them + // Add tasks we are resetting to to the bundle, and lock them. val existingTasks = SQL( """SELECT task_id FROM task_bundles WHERE bundle_id = {bundleId}""" ).on( @@ -181,17 +182,14 @@ class TaskBundleRepository @Inject() ( ) .as(SqlParser.long("task_id").*) - // Construct the SQL query to insert task-bundle associations val sqlQuery = s"""INSERT INTO task_bundles (bundle_id, task_id) VALUES ($bundleId, {taskId})""" val tasksToAdd = taskIds.filterNot(existingTasks.contains) if (tasksToAdd.nonEmpty) { - // Construct parameters for the BatchSql operation val parameters = tasksToAdd.map(taskId => Seq[NamedParameter]("taskId" -> taskId)) - // Execute the BatchSql operation to insert task-bundle associations BatchSql(sqlQuery, parameters.head, parameters.tail: _*).execute() SQL(s"""UPDATE tasks SET bundle_id = {bundleId} @@ -203,10 +201,8 @@ class TaskBundleRepository @Inject() ( ) .executeUpdate() - // Define the fallback value + // This is the fallback if the primary task status isnt found val fallbackStatus: Int = 0 - - // Retrieve the status of the primary task val primaryTaskStatus: Int = SQL( """SELECT status FROM tasks WHERE id = {primaryTaskId}""" ).on( @@ -215,10 +211,8 @@ class TaskBundleRepository @Inject() ( .as(scalar[Int].singleOpt) .getOrElse(fallbackStatus) - // Define the fallback value for review status + // This is the fallback if the primary task review status isnt found val fallbackReviewStatus: Int = 0 - - // Retrieve the review status of the primary task val primaryTaskReviewStatus: Int = SQL( """SELECT review_status FROM task_review WHERE id = {primaryTaskId}""" ).on( From 5cc7b9554a42f64911d30e88897c0db2c090bb64 Mon Sep 17 00:00:00 2001 From: CollinBeczak Date: Tue, 12 Mar 2024 19:02:03 -0500 Subject: [PATCH 12/26] add needed value in tasks that were reset and added back into the task_review table --- .../repository/TaskBundleRepository.scala | 23 ++++++++++++------- 1 file changed, 15 insertions(+), 8 deletions(-) diff --git a/app/org/maproulette/framework/repository/TaskBundleRepository.scala b/app/org/maproulette/framework/repository/TaskBundleRepository.scala index ffe214f2..821d9f9e 100644 --- a/app/org/maproulette/framework/repository/TaskBundleRepository.scala +++ b/app/org/maproulette/framework/repository/TaskBundleRepository.scala @@ -201,25 +201,31 @@ class TaskBundleRepository @Inject() ( ) .executeUpdate() - // This is the fallback if the primary task status isnt found - val fallbackStatus: Int = 0 + // Fallback for missing integer values + val fallbackInteger: Int = 0 val primaryTaskStatus: Int = SQL( """SELECT status FROM tasks WHERE id = {primaryTaskId}""" ).on( "primaryTaskId" -> primaryTaskId ) .as(scalar[Int].singleOpt) - .getOrElse(fallbackStatus) + .getOrElse(fallbackInteger) - // This is the fallback if the primary task review status isnt found - val fallbackReviewStatus: Int = 0 val primaryTaskReviewStatus: Int = SQL( """SELECT review_status FROM task_review WHERE id = {primaryTaskId}""" ).on( "primaryTaskId" -> primaryTaskId ) .as(scalar[Int].singleOpt) - .getOrElse(fallbackReviewStatus) + .getOrElse(fallbackInteger) + + val primaryTaskReviewRequestedBy: Int = SQL( + """SELECT review_requested_by FROM task_review WHERE id = {primaryTaskId}""" + ).on( + "primaryTaskId" -> primaryTaskId + ) + .as(scalar[Int].singleOpt) + .getOrElse(fallbackInteger) val lockedTasks = this.withListLocking(user, Some(TaskType())) { () => this.taskDAL.retrieveListById(-1, 0)(tasksToAdd) @@ -242,8 +248,9 @@ class TaskBundleRepository @Inject() ( ) .executeUpdate() - SQL"""INSERT INTO task_review (task_id, review_status) - VALUES (${task.id}, ${primaryTaskReviewStatus})""".executeUpdate() + SQL"""INSERT INTO task_review (task_id, review_status, review_requested_by) + VALUES (${task.id}, ${primaryTaskReviewStatus}, ${primaryTaskReviewRequestedBy})""" + .executeUpdate() this.cacheManager.withOptionCaching { () => Some( From 33ef5800428dd596f2ff77e7d60913a205477bbc Mon Sep 17 00:00:00 2001 From: CollinBeczak Date: Wed, 20 Mar 2024 18:18:47 -0500 Subject: [PATCH 13/26] formatting --- .../service/TaskBundleServiceSpec.scala | 49 ++++++++++++++++--- 1 file changed, 42 insertions(+), 7 deletions(-) diff --git a/test/org/maproulette/framework/service/TaskBundleServiceSpec.scala b/test/org/maproulette/framework/service/TaskBundleServiceSpec.scala index 691e35c3..7ed94c51 100644 --- a/test/org/maproulette/framework/service/TaskBundleServiceSpec.scala +++ b/test/org/maproulette/framework/service/TaskBundleServiceSpec.scala @@ -35,7 +35,12 @@ class TaskBundleServiceSpec(implicit val application: Application) extends Frame ) val response = - this.service.createTaskBundle(User.superUser, "my bundle", Some(task1.id), List(task1.id, task2.id)) + this.service.createTaskBundle( + User.superUser, + "my bundle", + Some(task1.id), + List(task1.id, task2.id) + ) response.taskIds.length mustEqual 2 // tasks.bundle_id is NOT set until setTaskStatus is called!!! @@ -51,7 +56,12 @@ class TaskBundleServiceSpec(implicit val application: Application) extends Frame // Cannot create a bundle with tasks already assigned intercept[InvalidException] { - this.service.createTaskBundle(User.superUser, "my bundle again", Some(task1.id), List(task1.id, task2.id)) + this.service.createTaskBundle( + User.superUser, + "my bundle again", + Some(task1.id), + List(task1.id, task2.id) + ) } } @@ -93,7 +103,12 @@ class TaskBundleServiceSpec(implicit val application: Application) extends Frame // Cannot create a bundle with tasks from different challenges intercept[InvalidException] { - this.service.createTaskBundle(User.superUser, "bad bundle", Some(task1.id), List(task1.id, task2.id)) + this.service.createTaskBundle( + User.superUser, + "bad bundle", + Some(task1.id), + List(task1.id, task2.id) + ) } } @@ -110,7 +125,12 @@ class TaskBundleServiceSpec(implicit val application: Application) extends Frame ) val bundle = - this.service.createTaskBundle(User.superUser, "my bundle for get", Some(task1.id), List(task1.id, task2.id)) + this.service.createTaskBundle( + User.superUser, + "my bundle for get", + Some(task1.id), + List(task1.id, task2.id) + ) val response = this.service.getTaskBundle(User.superUser, bundle.bundleId) response.bundleId mustEqual bundle.bundleId @@ -130,7 +150,12 @@ class TaskBundleServiceSpec(implicit val application: Application) extends Frame ) val bundle = this.service - .createTaskBundle(User.superUser, "my bundle for delete", Some(task1.id), List(task1.id, task2.id)) + .createTaskBundle( + User.superUser, + "my bundle for delete", + Some(task1.id), + List(task1.id, task2.id) + ) // tasks.bundle_id is NOT set until setTaskStatus is called taskDAL.setTaskStatus( @@ -159,7 +184,12 @@ class TaskBundleServiceSpec(implicit val application: Application) extends Frame ) val bundle = this.service - .createTaskBundle(User.superUser, "my bundle for delete", Some(task1.id), List(task1.id, task2.id)) + .createTaskBundle( + User.superUser, + "my bundle for delete", + Some(task1.id), + List(task1.id, task2.id) + ) // tasks.bundle_id is NOT set until setTaskStatus is called taskDAL.setTaskStatus( @@ -193,7 +223,12 @@ class TaskBundleServiceSpec(implicit val application: Application) extends Frame ) val bundle = this.service - .createTaskBundle(User.superUser, "my bundle for unbundle", Some(task1.id), List(task1.id, task2.id)) + .createTaskBundle( + User.superUser, + "my bundle for unbundle", + Some(task1.id), + List(task1.id, task2.id) + ) // tasks.bundle_id is NOT set until setTaskStatus is called taskDAL.setTaskStatus( From 61bafed69865caa6773a057e8534b678cfee2b52 Mon Sep 17 00:00:00 2001 From: CollinBeczak Date: Sun, 21 Apr 2024 18:44:20 -0500 Subject: [PATCH 14/26] remove logger and reorganize code --- .../repository/TaskBundleRepository.scala | 59 +++++++------------ 1 file changed, 21 insertions(+), 38 deletions(-) diff --git a/app/org/maproulette/framework/repository/TaskBundleRepository.scala b/app/org/maproulette/framework/repository/TaskBundleRepository.scala index 821d9f9e..6434077b 100644 --- a/app/org/maproulette/framework/repository/TaskBundleRepository.scala +++ b/app/org/maproulette/framework/repository/TaskBundleRepository.scala @@ -89,9 +89,7 @@ class TaskBundleRepository @Inject() ( case Some(id) => val sqlQuery = s"""UPDATE tasks SET is_bundle_primary = true WHERE id = $id""" SQL(sqlQuery).executeUpdate() - case None => - // Handle the case where primaryId is None - println("primaryId is not defined") + case None => // Handle the case where primaryId is None } val parameters = lockedTasks.map(task => Seq[NamedParameter]("taskId" -> task.id)) @@ -138,13 +136,7 @@ class TaskBundleRepository @Inject() ( taskIds: List[Long] ): Unit = { withMRTransaction { implicit c => - // Remove tasks from the bundle join table and unlock them if necessary - val tasks = - this.retrieveTasks( - Query.simple(List(BaseParameter("bundle_id", bundleId, table = Some("tb")))) - ) - - // Unset bundle_id for tasks not in the taskids param(this param is the task ids that the bundle will be "resetting" to) + // Unset bundle_id for tasks not in the taskids param SQL( """UPDATE tasks SET bundle_id = NULL @@ -156,6 +148,11 @@ class TaskBundleRepository @Inject() ( ) .executeUpdate() + // Remove previous tasks from the bundle join table and unlock them if necessary + val tasks = this.retrieveTasks( + Query.simple(List(BaseParameter("bundle_id", bundleId, table = Some("tb")))) + ) + for (task <- tasks) { if (!task.isBundlePrimary.getOrElse(false) && !taskIds.contains(task.id)) { SQL( @@ -167,14 +164,12 @@ class TaskBundleRepository @Inject() ( case e: Exception => logger.warn(e.getMessage) } this.cacheManager.withOptionCaching { () => - Some( - task.copy(bundleId = None, isBundlePrimary = None) - ) + Some(task.copy(bundleId = None, isBundlePrimary = None)) } } } - // Add tasks we are resetting to to the bundle, and lock them. + // Add tasks we are resetting to the bundle, and lock them val existingTasks = SQL( """SELECT task_id FROM task_bundles WHERE bundle_id = {bundleId}""" ).on( @@ -182,20 +177,18 @@ class TaskBundleRepository @Inject() ( ) .as(SqlParser.long("task_id").*) - val sqlQuery = - s"""INSERT INTO task_bundles (bundle_id, task_id) VALUES ($bundleId, {taskId})""" - val tasksToAdd = taskIds.filterNot(existingTasks.contains) if (tasksToAdd.nonEmpty) { + val sqlQuery = + s"""INSERT INTO task_bundles (bundle_id, task_id) VALUES ($bundleId, {taskId})""" val parameters = tasksToAdd.map(taskId => Seq[NamedParameter]("taskId" -> taskId)) BatchSql(sqlQuery, parameters.head, parameters.tail: _*).execute() - SQL(s"""UPDATE tasks SET bundle_id = {bundleId} - WHERE bundle_id IS NULL - AND id IN ({inList})""") - .on( + SQL( + s"""UPDATE tasks SET bundle_id = {bundleId} WHERE bundle_id IS NULL AND id IN ({inList})""" + ).on( "bundleId" -> bundleId, "inList" -> tasksToAdd ) @@ -211,21 +204,13 @@ class TaskBundleRepository @Inject() ( .as(scalar[Int].singleOpt) .getOrElse(fallbackInteger) - val primaryTaskReviewStatus: Int = SQL( - """SELECT review_status FROM task_review WHERE id = {primaryTaskId}""" + val (primaryTaskReviewStatus: Int, primaryTaskReviewRequestedBy: Int) = SQL( + """SELECT review_status, review_requested_by FROM task_review WHERE id = {primaryTaskId}""" ).on( "primaryTaskId" -> primaryTaskId ) - .as(scalar[Int].singleOpt) - .getOrElse(fallbackInteger) - - val primaryTaskReviewRequestedBy: Int = SQL( - """SELECT review_requested_by FROM task_review WHERE id = {primaryTaskId}""" - ).on( - "primaryTaskId" -> primaryTaskId - ) - .as(scalar[Int].singleOpt) - .getOrElse(fallbackInteger) + .as((scalar[Int] ~ scalar[Int]).singleOpt) + .getOrElse((fallbackInteger, fallbackInteger)) val lockedTasks = this.withListLocking(user, Some(TaskType())) { () => this.taskDAL.retrieveListById(-1, 0)(tasksToAdd) @@ -236,12 +221,10 @@ class TaskBundleRepository @Inject() ( } catch { case e: Exception => this.logger.warn(e.getMessage) } + // Update the status of all tasks in the bundle to match the status of the primary task SQL( - """UPDATE tasks - SET status = {primaryTaskStatus} - WHERE id = {taskId} - """ + """UPDATE tasks SET status = {primaryTaskStatus} WHERE id = {taskId}""" ).on( "primaryTaskStatus" -> primaryTaskStatus, "taskId" -> task.id @@ -249,7 +232,7 @@ class TaskBundleRepository @Inject() ( .executeUpdate() SQL"""INSERT INTO task_review (task_id, review_status, review_requested_by) - VALUES (${task.id}, ${primaryTaskReviewStatus}, ${primaryTaskReviewRequestedBy})""" + VALUES (${task.id}, ${primaryTaskReviewStatus}, ${primaryTaskReviewRequestedBy})""" .executeUpdate() this.cacheManager.withOptionCaching { () => From 0b381b7ecf6762264e74912912f94ad4342216c5 Mon Sep 17 00:00:00 2001 From: CollinBeczak Date: Sun, 21 Apr 2024 19:15:58 -0500 Subject: [PATCH 15/26] change the fetch bundle endpoint from a GET to a POST --- conf/v2_route/bundle.api | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/conf/v2_route/bundle.api b/conf/v2_route/bundle.api index e6841b55..23966b64 100644 --- a/conf/v2_route/bundle.api +++ b/conf/v2_route/bundle.api @@ -44,7 +44,7 @@ POST /taskBundle @org.maproulette.framework.c # in: query # description: The tasks in the bundle will be locked by the user. ### -GET /taskBundle/:id @org.maproulette.framework.controller.TaskBundleController.getTaskBundle(id:Long, lockTasks:Boolean ?= false) +POST /taskBundle/:id @org.maproulette.framework.controller.TaskBundleController.getTaskBundle(id:Long, lockTasks:Boolean ?= false) ### # tags: [ Bundle ] # summary: Resets a Task Bundle From 8b0e10c292d7744da7771503f96bdbe7fd8a142f Mon Sep 17 00:00:00 2001 From: CollinBeczak Date: Sun, 21 Apr 2024 19:43:11 -0500 Subject: [PATCH 16/26] fix mixed up param descriptions --- conf/v2_route/bundle.api | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/conf/v2_route/bundle.api b/conf/v2_route/bundle.api index 23966b64..0f917f52 100644 --- a/conf/v2_route/bundle.api +++ b/conf/v2_route/bundle.api @@ -61,11 +61,10 @@ POST /taskBundle/:id @org.maproulette.framework. # required: true # - name: primaryId # in: query -# description: The task ids the bundle will reset to +# description: The primary task id of the bundle # - name: taskIds # in: query -# description: The primary task id of the bundle - +# description: The task ids the bundle will reset to ### POST /taskBundle/:id/reset @org.maproulette.framework.controller.TaskBundleController.resetTaskBundle(id: Long, primaryId: Long, taskIds: List[Long]) ### From 98a709f8d604dae78255c5e005c89196ba264ae8 Mon Sep 17 00:00:00 2001 From: CollinBeczak Date: Mon, 22 Apr 2024 11:50:31 -0500 Subject: [PATCH 17/26] simplify task removal in bundle reset --- .../repository/TaskBundleRepository.scala | 43 +++---------------- 1 file changed, 6 insertions(+), 37 deletions(-) diff --git a/app/org/maproulette/framework/repository/TaskBundleRepository.scala b/app/org/maproulette/framework/repository/TaskBundleRepository.scala index 6434077b..7f6ce517 100644 --- a/app/org/maproulette/framework/repository/TaskBundleRepository.scala +++ b/app/org/maproulette/framework/repository/TaskBundleRepository.scala @@ -136,48 +136,17 @@ class TaskBundleRepository @Inject() ( taskIds: List[Long] ): Unit = { withMRTransaction { implicit c => - // Unset bundle_id for tasks not in the taskids param - SQL( - """UPDATE tasks - SET bundle_id = NULL - WHERE bundle_id = {bundleId} AND id NOT IN ({taskIds}) - """ - ).on( - "bundleId" -> bundleId, - "taskIds" -> taskIds - ) - .executeUpdate() - // Remove previous tasks from the bundle join table and unlock them if necessary - val tasks = this.retrieveTasks( + val currentTaskIds = this.retrieveTasks( Query.simple(List(BaseParameter("bundle_id", bundleId, table = Some("tb")))) - ) + ).map(_.id) - for (task <- tasks) { - if (!task.isBundlePrimary.getOrElse(false) && !taskIds.contains(task.id)) { - SQL( - s"""DELETE FROM task_bundles WHERE bundle_id = $bundleId AND task_id = ${task.id}""" - ).executeUpdate() - try { - this.unlockItem(user, task) - } catch { - case e: Exception => logger.warn(e.getMessage) - } - this.cacheManager.withOptionCaching { () => - Some(task.copy(bundleId = None, isBundlePrimary = None)) - } - } + val removeTaskIds = currentTaskIds.filter(taskId => !taskIds.contains(taskId)) + if(removeTaskIds.nonEmpty) { + this.unbundleTasks(user, bundleId, removeTaskIds, List.empty) } - // Add tasks we are resetting to the bundle, and lock them - val existingTasks = SQL( - """SELECT task_id FROM task_bundles WHERE bundle_id = {bundleId}""" - ).on( - "bundleId" -> bundleId - ) - .as(SqlParser.long("task_id").*) - - val tasksToAdd = taskIds.filterNot(existingTasks.contains) + val tasksToAdd = taskIds.filterNot(currentTaskIds.contains) if (tasksToAdd.nonEmpty) { val sqlQuery = From fa28dc545cbc96cb202c830f9f93518e2919e945 Mon Sep 17 00:00:00 2001 From: CollinBeczak Date: Mon, 22 Apr 2024 13:47:05 -0500 Subject: [PATCH 18/26] remove primary task id from params and simplify endpoints --- .../controller/TaskBundleController.scala | 8 +- .../repository/TaskBundleRepository.scala | 138 +++++++++--------- .../framework/service/TaskBundleService.scala | 7 +- conf/v2_route/bundle.api | 9 +- 4 files changed, 81 insertions(+), 81 deletions(-) diff --git a/app/org/maproulette/framework/controller/TaskBundleController.scala b/app/org/maproulette/framework/controller/TaskBundleController.scala index effd945a..73383889 100644 --- a/app/org/maproulette/framework/controller/TaskBundleController.scala +++ b/app/org/maproulette/framework/controller/TaskBundleController.scala @@ -270,11 +270,10 @@ class TaskBundleController @Inject() ( */ def resetTaskBundle( id: Long, - primaryTaskId: Long, taskIds: List[Long] ): Action[AnyContent] = Action.async { implicit request => this.sessionManager.authenticatedRequest { implicit user => - this.serviceManager.taskBundle.resetTaskBundle(user, id, primaryTaskId, taskIds) + this.serviceManager.taskBundle.resetTaskBundle(user, id, taskIds) Ok(Json.toJson(this.serviceManager.taskBundle.getTaskBundle(user, id))) } } @@ -301,12 +300,11 @@ class TaskBundleController @Inject() ( * Delete bundle. * * @param id The id for the bundle - * @param primaryId optional task id to no unlock after deleting this bundle */ - def deleteTaskBundle(id: Long, primaryId: Option[Long] = None): Action[AnyContent] = + def deleteTaskBundle(id: Long): Action[AnyContent] = Action.async { implicit request => this.sessionManager.authenticatedRequest { implicit user => - this.serviceManager.taskBundle.deleteTaskBundle(user, id, primaryId) + this.serviceManager.taskBundle.deleteTaskBundle(user, id) Ok } } diff --git a/app/org/maproulette/framework/repository/TaskBundleRepository.scala b/app/org/maproulette/framework/repository/TaskBundleRepository.scala index 7f6ce517..4f953025 100644 --- a/app/org/maproulette/framework/repository/TaskBundleRepository.scala +++ b/app/org/maproulette/framework/repository/TaskBundleRepository.scala @@ -132,20 +132,24 @@ class TaskBundleRepository @Inject() ( def resetTaskBundle( user: User, bundleId: Long, - primaryTaskId: Long, taskIds: List[Long] ): Unit = { withMRTransaction { implicit c => - // Remove previous tasks from the bundle join table and unlock them if necessary - val currentTaskIds = this.retrieveTasks( - Query.simple(List(BaseParameter("bundle_id", bundleId, table = Some("tb")))) - ).map(_.id) + // Retrieve all the task ids currently in the bundle + val currentTaskIds = this + .retrieveTasks( + Query.simple(List(BaseParameter("bundle_id", bundleId, table = Some("tb")))) + ) + .map(_.id) + // Remove previous tasks from the bundle join table and unlock them if necessary val removeTaskIds = currentTaskIds.filter(taskId => !taskIds.contains(taskId)) - if(removeTaskIds.nonEmpty) { + + if (removeTaskIds.nonEmpty) { this.unbundleTasks(user, bundleId, removeTaskIds, List.empty) } + // Filter for tasks that need to be added back to the bundle. val tasksToAdd = taskIds.filterNot(currentTaskIds.contains) if (tasksToAdd.nonEmpty) { @@ -154,32 +158,42 @@ class TaskBundleRepository @Inject() ( val parameters = tasksToAdd.map(taskId => Seq[NamedParameter]("taskId" -> taskId)) BatchSql(sqlQuery, parameters.head, parameters.tail: _*).execute() + val primaryTaskId = SQL( + """SELECT id FROM tasks WHERE bundle_id = {bundleId} AND is_bundle_primary = true""" + ).on("bundleId" -> bundleId) + .as(scalar[Int].singleOpt) + .getOrElse(0) + + val primaryTaskStatus: Int = SQL("""SELECT status FROM tasks WHERE id = {primaryTaskId}""") + .on("primaryTaskId" -> primaryTaskId) + .as(scalar[Int].singleOpt) + .getOrElse(0) + + val (primaryTaskReviewStatus: Int, primaryTaskReviewRequestedBy: Int) = SQL( + """SELECT review_status, review_requested_by FROM task_review WHERE id = {primaryTaskId}""" + ).on("primaryTaskId" -> primaryTaskId) + .as((scalar[Int] ~ scalar[Int]).singleOpt) + .getOrElse((0, 0)) SQL( - s"""UPDATE tasks SET bundle_id = {bundleId} WHERE bundle_id IS NULL AND id IN ({inList})""" + s"""UPDATE tasks SET bundle_id = {bundleId}, status = $primaryTaskStatus + WHERE bundle_id IS NULL AND id IN ({inList})""" ).on( "bundleId" -> bundleId, "inList" -> tasksToAdd ) .executeUpdate() - // Fallback for missing integer values - val fallbackInteger: Int = 0 - val primaryTaskStatus: Int = SQL( - """SELECT status FROM tasks WHERE id = {primaryTaskId}""" - ).on( - "primaryTaskId" -> primaryTaskId - ) - .as(scalar[Int].singleOpt) - .getOrElse(fallbackInteger) - - val (primaryTaskReviewStatus: Int, primaryTaskReviewRequestedBy: Int) = SQL( - """SELECT review_status, review_requested_by FROM task_review WHERE id = {primaryTaskId}""" + SQL( + """INSERT INTO task_review (task_id, review_status, review_requested_by) + SELECT id, {reviewStatus}, {reviewRequestedBy} + FROM tasks WHERE id IN ({inList})""" ).on( - "primaryTaskId" -> primaryTaskId + "reviewStatus" -> primaryTaskReviewStatus, + "reviewRequestedBy" -> primaryTaskReviewRequestedBy, + "inList" -> tasksToAdd ) - .as((scalar[Int] ~ scalar[Int]).singleOpt) - .getOrElse((fallbackInteger, fallbackInteger)) + .executeUpdate() val lockedTasks = this.withListLocking(user, Some(TaskType())) { () => this.taskDAL.retrieveListById(-1, 0)(tasksToAdd) @@ -188,22 +202,9 @@ class TaskBundleRepository @Inject() ( try { this.lockItem(user, task) } catch { - case e: Exception => this.logger.warn(e.getMessage) + case e: Exception => + this.logger.warn(e.getMessage) } - - // Update the status of all tasks in the bundle to match the status of the primary task - SQL( - """UPDATE tasks SET status = {primaryTaskStatus} WHERE id = {taskId}""" - ).on( - "primaryTaskStatus" -> primaryTaskStatus, - "taskId" -> task.id - ) - .executeUpdate() - - SQL"""INSERT INTO task_review (task_id, review_status, review_requested_by) - VALUES (${task.id}, ${primaryTaskReviewStatus}, ${primaryTaskReviewRequestedBy})""" - .executeUpdate() - this.cacheManager.withOptionCaching { () => Some( task.copy( @@ -294,50 +295,57 @@ class TaskBundleRepository @Inject() ( * * @param bundleId The id of the bundle */ - def deleteTaskBundle(user: User, bundle: TaskBundle, primaryTaskId: Option[Long] = None): Unit = { + def deleteTaskBundle(user: User, bundleId: Long): Unit = { this.withMRConnection { implicit c => + val primaryTaskId = SQL( + """SELECT task_id FROM tasks WHERE bundle_id = {bundleId} AND is_bundle_primary = true""" + ).on("bundleId" -> bundleId) + .as(scalar[Long].singleOpt) + .getOrElse(0L) + SQL( """UPDATE tasks - SET bundle_id = NULL, - is_bundle_primary = NULL - WHERE bundle_id = {bundleId} OR id = {primaryTaskId}""" + SET bundle_id = NULL, + is_bundle_primary = NULL + WHERE bundle_id = {bundleId} OR id = {primaryTaskId}""" ).on( - Symbol("bundleId") -> bundle.bundleId, + Symbol("bundleId") -> bundleId, Symbol("primaryTaskId") -> primaryTaskId ) .executeUpdate() - val inList = bundle.tasks.getOrElse(Nil).map(_.id).mkString(",") + val tasks = this.retrieveTasks( + Query.simple(List(BaseParameter("bundle_id", bundleId, table = Some("tb")))) + ) + val inList = tasks.map(_.id).mkString(",") + SQL(s"UPDATE tasks SET bundle_id = NULL WHERE id IN ($inList)").executeUpdate() - if (primaryTaskId != None) { - // unlock tasks (everything but the primary task id) - val tasks = bundle.tasks match { - case Some(t) => - for (task <- t) { - if (task.id != primaryTaskId.getOrElse(0)) { - try { - this.unlockItem(user, task) - } catch { - case e: Exception => this.logger.warn(e.getMessage) - } - } - this.cacheManager.withOptionCaching { () => - Some( - task.copy( - bundleId = None, - isBundlePrimary = None - ) - ) - } + if (primaryTaskId != 0) { + // Unlock tasks (everything but the primary task id) + tasks.foreach { task => + if (task.id != primaryTaskId) { + try { + this.unlockItem(user, task) + } catch { + case e: Exception => this.logger.warn(e.getMessage) } - case None => // no tasks in bundle + } + this.cacheManager.withOptionCaching { () => + Some( + task.copy( + bundleId = None, + isBundlePrimary = None + ) + ) + + } } } // Delete from task_bundles which will also cascade delete from bundles SQL("DELETE FROM task_bundles WHERE bundle_id = {bundleId}") - .on(Symbol("bundleId") -> bundle.bundleId) + .on(Symbol("bundleId") -> bundleId) .executeUpdate() } } diff --git a/app/org/maproulette/framework/service/TaskBundleService.scala b/app/org/maproulette/framework/service/TaskBundleService.scala index a5d746f2..8dcc0f03 100644 --- a/app/org/maproulette/framework/service/TaskBundleService.scala +++ b/app/org/maproulette/framework/service/TaskBundleService.scala @@ -99,10 +99,9 @@ class TaskBundleService @Inject() ( def resetTaskBundle( user: User, bundleId: Long, - primaryTaskId: Long, taskIds: List[Long] ): TaskBundle = { - this.repository.resetTaskBundle(user, bundleId, primaryTaskId, taskIds) + this.repository.resetTaskBundle(user, bundleId, taskIds) this.getTaskBundle(user, bundleId) } @@ -128,7 +127,7 @@ class TaskBundleService @Inject() ( * * @param bundleId The id of the bundle */ - def deleteTaskBundle(user: User, bundleId: Long, primaryTaskId: Option[Long] = None): Unit = { + def deleteTaskBundle(user: User, bundleId: Long): Unit = { val bundle = this.getTaskBundle(user, bundleId) // Verify permissions to delete this bundle @@ -138,7 +137,7 @@ class TaskBundleService @Inject() ( this.permission.hasObjectWriteAccess(challenge.get, user) } - this.repository.deleteTaskBundle(user, bundle, primaryTaskId) + this.repository.deleteTaskBundle(user, bundle.bundleId) } /** diff --git a/conf/v2_route/bundle.api b/conf/v2_route/bundle.api index 0f917f52..567afd2d 100644 --- a/conf/v2_route/bundle.api +++ b/conf/v2_route/bundle.api @@ -59,14 +59,11 @@ POST /taskBundle/:id @org.maproulette.framework. # in: path # description: The id of the Task Bundle # required: true -# - name: primaryId -# in: query -# description: The primary task id of the bundle # - name: taskIds # in: query # description: The task ids the bundle will reset to ### -POST /taskBundle/:id/reset @org.maproulette.framework.controller.TaskBundleController.resetTaskBundle(id: Long, primaryId: Long, taskIds: List[Long]) +POST /taskBundle/:id/reset @org.maproulette.framework.controller.TaskBundleController.resetTaskBundle(id: Long, taskIds: List[Long]) ### # tags: [ Bundle ] # summary: Deletes a Task Bundle @@ -81,10 +78,8 @@ POST /taskBundle/:id/reset @org.maproulette.framewo # in: path # description: The id of the Task Bundle # required: true -# - name: primaryId -# in: query ### -DELETE /taskBundle/:id @org.maproulette.framework.controller.TaskBundleController.deleteTaskBundle(id:Long, primaryId:Option[Long]) +DELETE /taskBundle/:id @org.maproulette.framework.controller.TaskBundleController.deleteTaskBundle(id:Long) ### # tags: [ Bundle ] # summary: Unbundles tasks from Task Bundle From bc7b2202a50f306616f4cb502e08f5b084400dc0 Mon Sep 17 00:00:00 2001 From: CollinBeczak Date: Mon, 22 Apr 2024 13:48:24 -0500 Subject: [PATCH 19/26] remove primary task id descriptions --- .../maproulette/framework/controller/TaskBundleController.scala | 1 - .../maproulette/framework/repository/TaskBundleRepository.scala | 1 - app/org/maproulette/framework/service/TaskBundleService.scala | 1 - 3 files changed, 3 deletions(-) diff --git a/app/org/maproulette/framework/controller/TaskBundleController.scala b/app/org/maproulette/framework/controller/TaskBundleController.scala index 73383889..52221010 100644 --- a/app/org/maproulette/framework/controller/TaskBundleController.scala +++ b/app/org/maproulette/framework/controller/TaskBundleController.scala @@ -266,7 +266,6 @@ class TaskBundleController @Inject() ( * * @param bundleId The id of the bundle * @param taskIds The task ids the bundle will reset to - * @param primaryTaskId The primary task id of the bundle */ def resetTaskBundle( id: Long, diff --git a/app/org/maproulette/framework/repository/TaskBundleRepository.scala b/app/org/maproulette/framework/repository/TaskBundleRepository.scala index 4f953025..b78e1c33 100644 --- a/app/org/maproulette/framework/repository/TaskBundleRepository.scala +++ b/app/org/maproulette/framework/repository/TaskBundleRepository.scala @@ -127,7 +127,6 @@ class TaskBundleRepository @Inject() ( * * @param bundleId The id of the bundle * @param taskIds The task ids the bundle will reset to - * @param primaryTaskId The primary task id of the bundle */ def resetTaskBundle( user: User, diff --git a/app/org/maproulette/framework/service/TaskBundleService.scala b/app/org/maproulette/framework/service/TaskBundleService.scala index 8dcc0f03..bf6c81c6 100644 --- a/app/org/maproulette/framework/service/TaskBundleService.scala +++ b/app/org/maproulette/framework/service/TaskBundleService.scala @@ -94,7 +94,6 @@ class TaskBundleService @Inject() ( * * @param bundleId The id of the bundle * @param taskIds The task ids the bundle will reset to - * @param primaryTaskId The primary task id of the bundle */ def resetTaskBundle( user: User, From 1997cc38f6f38668e54af871738b000300f2f966 Mon Sep 17 00:00:00 2001 From: CollinBeczak Date: Mon, 22 Apr 2024 14:10:57 -0500 Subject: [PATCH 20/26] remove params from tests --- .../framework/service/TaskBundleServiceSpec.scala | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/test/org/maproulette/framework/service/TaskBundleServiceSpec.scala b/test/org/maproulette/framework/service/TaskBundleServiceSpec.scala index 7ed94c51..0504e608 100644 --- a/test/org/maproulette/framework/service/TaskBundleServiceSpec.scala +++ b/test/org/maproulette/framework/service/TaskBundleServiceSpec.scala @@ -38,7 +38,6 @@ class TaskBundleServiceSpec(implicit val application: Application) extends Frame this.service.createTaskBundle( User.superUser, "my bundle", - Some(task1.id), List(task1.id, task2.id) ) response.taskIds.length mustEqual 2 @@ -59,7 +58,6 @@ class TaskBundleServiceSpec(implicit val application: Application) extends Frame this.service.createTaskBundle( User.superUser, "my bundle again", - Some(task1.id), List(task1.id, task2.id) ) } @@ -68,7 +66,7 @@ class TaskBundleServiceSpec(implicit val application: Application) extends Frame "not create a task Bundle with no tasks" taggedAs (TaskTag) in { // Cannot create a bundle with no tasks intercept[InvalidException] { - this.service.createTaskBundle(User.superUser, "my bundle again", Some(0), List()) + this.service.createTaskBundle(User.superUser, "my bundle again", List()) } } @@ -106,7 +104,6 @@ class TaskBundleServiceSpec(implicit val application: Application) extends Frame this.service.createTaskBundle( User.superUser, "bad bundle", - Some(task1.id), List(task1.id, task2.id) ) } @@ -128,7 +125,6 @@ class TaskBundleServiceSpec(implicit val application: Application) extends Frame this.service.createTaskBundle( User.superUser, "my bundle for get", - Some(task1.id), List(task1.id, task2.id) ) @@ -153,7 +149,6 @@ class TaskBundleServiceSpec(implicit val application: Application) extends Frame .createTaskBundle( User.superUser, "my bundle for delete", - Some(task1.id), List(task1.id, task2.id) ) @@ -187,7 +182,6 @@ class TaskBundleServiceSpec(implicit val application: Application) extends Frame .createTaskBundle( User.superUser, "my bundle for delete", - Some(task1.id), List(task1.id, task2.id) ) @@ -226,7 +220,6 @@ class TaskBundleServiceSpec(implicit val application: Application) extends Frame .createTaskBundle( User.superUser, "my bundle for unbundle", - Some(task1.id), List(task1.id, task2.id) ) From 7c6d0c14ebee56957537810c0547e129234671b2 Mon Sep 17 00:00:00 2001 From: CollinBeczak Date: Mon, 22 Apr 2024 15:02:46 -0500 Subject: [PATCH 21/26] fix tests --- .../framework/service/TaskBundleServiceSpec.scala | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/test/org/maproulette/framework/service/TaskBundleServiceSpec.scala b/test/org/maproulette/framework/service/TaskBundleServiceSpec.scala index 0504e608..7ed94c51 100644 --- a/test/org/maproulette/framework/service/TaskBundleServiceSpec.scala +++ b/test/org/maproulette/framework/service/TaskBundleServiceSpec.scala @@ -38,6 +38,7 @@ class TaskBundleServiceSpec(implicit val application: Application) extends Frame this.service.createTaskBundle( User.superUser, "my bundle", + Some(task1.id), List(task1.id, task2.id) ) response.taskIds.length mustEqual 2 @@ -58,6 +59,7 @@ class TaskBundleServiceSpec(implicit val application: Application) extends Frame this.service.createTaskBundle( User.superUser, "my bundle again", + Some(task1.id), List(task1.id, task2.id) ) } @@ -66,7 +68,7 @@ class TaskBundleServiceSpec(implicit val application: Application) extends Frame "not create a task Bundle with no tasks" taggedAs (TaskTag) in { // Cannot create a bundle with no tasks intercept[InvalidException] { - this.service.createTaskBundle(User.superUser, "my bundle again", List()) + this.service.createTaskBundle(User.superUser, "my bundle again", Some(0), List()) } } @@ -104,6 +106,7 @@ class TaskBundleServiceSpec(implicit val application: Application) extends Frame this.service.createTaskBundle( User.superUser, "bad bundle", + Some(task1.id), List(task1.id, task2.id) ) } @@ -125,6 +128,7 @@ class TaskBundleServiceSpec(implicit val application: Application) extends Frame this.service.createTaskBundle( User.superUser, "my bundle for get", + Some(task1.id), List(task1.id, task2.id) ) @@ -149,6 +153,7 @@ class TaskBundleServiceSpec(implicit val application: Application) extends Frame .createTaskBundle( User.superUser, "my bundle for delete", + Some(task1.id), List(task1.id, task2.id) ) @@ -182,6 +187,7 @@ class TaskBundleServiceSpec(implicit val application: Application) extends Frame .createTaskBundle( User.superUser, "my bundle for delete", + Some(task1.id), List(task1.id, task2.id) ) @@ -220,6 +226,7 @@ class TaskBundleServiceSpec(implicit val application: Application) extends Frame .createTaskBundle( User.superUser, "my bundle for unbundle", + Some(task1.id), List(task1.id, task2.id) ) From ff3f658365899b732bb10ae711f41f0a673c33fa Mon Sep 17 00:00:00 2001 From: CollinBeczak Date: Mon, 22 Apr 2024 15:12:46 -0500 Subject: [PATCH 22/26] fix wrong column name issue --- .../maproulette/framework/repository/TaskBundleRepository.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/org/maproulette/framework/repository/TaskBundleRepository.scala b/app/org/maproulette/framework/repository/TaskBundleRepository.scala index b78e1c33..cad7b889 100644 --- a/app/org/maproulette/framework/repository/TaskBundleRepository.scala +++ b/app/org/maproulette/framework/repository/TaskBundleRepository.scala @@ -297,7 +297,7 @@ class TaskBundleRepository @Inject() ( def deleteTaskBundle(user: User, bundleId: Long): Unit = { this.withMRConnection { implicit c => val primaryTaskId = SQL( - """SELECT task_id FROM tasks WHERE bundle_id = {bundleId} AND is_bundle_primary = true""" + """SELECT id FROM tasks WHERE bundle_id = {bundleId} AND is_bundle_primary = true""" ).on("bundleId" -> bundleId) .as(scalar[Long].singleOpt) .getOrElse(0L) From e9463d774917f7e5effafbbcd25d57f9e7013f7b Mon Sep 17 00:00:00 2001 From: CollinBeczak Date: Mon, 22 Apr 2024 16:07:21 -0500 Subject: [PATCH 23/26] split bundle tasks functionality into its own function --- .../repository/TaskBundleRepository.scala | 138 ++++++++++-------- 1 file changed, 78 insertions(+), 60 deletions(-) diff --git a/app/org/maproulette/framework/repository/TaskBundleRepository.scala b/app/org/maproulette/framework/repository/TaskBundleRepository.scala index cad7b889..1a5a4d26 100644 --- a/app/org/maproulette/framework/repository/TaskBundleRepository.scala +++ b/app/org/maproulette/framework/repository/TaskBundleRepository.scala @@ -142,77 +142,95 @@ class TaskBundleRepository @Inject() ( .map(_.id) // Remove previous tasks from the bundle join table and unlock them if necessary - val removeTaskIds = currentTaskIds.filter(taskId => !taskIds.contains(taskId)) + val tasksToRemove = currentTaskIds.filter(taskId => !taskIds.contains(taskId)) - if (removeTaskIds.nonEmpty) { - this.unbundleTasks(user, bundleId, removeTaskIds, List.empty) + if (tasksToRemove.nonEmpty) { + this.unbundleTasks(user, bundleId, tasksToRemove, List.empty) } // Filter for tasks that need to be added back to the bundle. val tasksToAdd = taskIds.filterNot(currentTaskIds.contains) if (tasksToAdd.nonEmpty) { - val sqlQuery = - s"""INSERT INTO task_bundles (bundle_id, task_id) VALUES ($bundleId, {taskId})""" - val parameters = tasksToAdd.map(taskId => Seq[NamedParameter]("taskId" -> taskId)) - - BatchSql(sqlQuery, parameters.head, parameters.tail: _*).execute() - val primaryTaskId = SQL( - """SELECT id FROM tasks WHERE bundle_id = {bundleId} AND is_bundle_primary = true""" - ).on("bundleId" -> bundleId) - .as(scalar[Int].singleOpt) - .getOrElse(0) - - val primaryTaskStatus: Int = SQL("""SELECT status FROM tasks WHERE id = {primaryTaskId}""") - .on("primaryTaskId" -> primaryTaskId) - .as(scalar[Int].singleOpt) - .getOrElse(0) - - val (primaryTaskReviewStatus: Int, primaryTaskReviewRequestedBy: Int) = SQL( - """SELECT review_status, review_requested_by FROM task_review WHERE id = {primaryTaskId}""" - ).on("primaryTaskId" -> primaryTaskId) - .as((scalar[Int] ~ scalar[Int]).singleOpt) - .getOrElse((0, 0)) - - SQL( - s"""UPDATE tasks SET bundle_id = {bundleId}, status = $primaryTaskStatus + this.bundleTasks(user, bundleId, tasksToAdd) + } + } + } + + /** + * Adds tasks to a bundle. + * + * @param bundleId The id of the bundle + */ + def bundleTasks( + user: User, + bundleId: Long, + taskIds: List[Long] + ): Unit = { + this.withMRConnection { implicit c => + val sqlQuery = + s"""INSERT INTO task_bundles (bundle_id, task_id) VALUES ($bundleId, {taskId})""" + val parameters = taskIds.map(taskId => Seq[NamedParameter]("taskId" -> taskId)) + + BatchSql(sqlQuery, parameters.head, parameters.tail: _*).execute() + val primaryTaskId = SQL( + """SELECT id FROM tasks WHERE bundle_id = {bundleId} AND is_bundle_primary = true""" + ).on("bundleId" -> bundleId) + .as(scalar[Int].singleOpt) + .getOrElse(0) + + val primaryTaskStatus: Int = SQL("""SELECT status FROM tasks WHERE id = {primaryTaskId}""") + .on("primaryTaskId" -> primaryTaskId) + .as(scalar[Int].singleOpt) + .getOrElse(0) + + SQL( + s"""UPDATE tasks SET bundle_id = {bundleId}, status = $primaryTaskStatus WHERE bundle_id IS NULL AND id IN ({inList})""" - ).on( - "bundleId" -> bundleId, - "inList" -> tasksToAdd - ) - .executeUpdate() - - SQL( - """INSERT INTO task_review (task_id, review_status, review_requested_by) - SELECT id, {reviewStatus}, {reviewRequestedBy} - FROM tasks WHERE id IN ({inList})""" - ).on( - "reviewStatus" -> primaryTaskReviewStatus, - "reviewRequestedBy" -> primaryTaskReviewRequestedBy, - "inList" -> tasksToAdd - ) - .executeUpdate() + ).on( + "bundleId" -> bundleId, + "inList" -> taskIds + ) + .executeUpdate() - val lockedTasks = this.withListLocking(user, Some(TaskType())) { () => - this.taskDAL.retrieveListById(-1, 0)(tasksToAdd) + val taskReviewQuery = SQL( + """SELECT review_status, review_requested_by FROM task_review WHERE task_id = {primaryTaskId}""" + ).on("primaryTaskId" -> primaryTaskId) + .as((scalar[Int] ~ scalar[Int]).singleOpt) + + taskReviewQuery match { + case Some((primaryTaskReviewStatus ~ primaryTaskReviewRequestedBy)) => + SQL( + """INSERT INTO task_review (task_id, review_status, review_requested_by) + SELECT id, {reviewStatus}, {reviewRequestedBy} + FROM tasks WHERE id IN ({inList})""" + ).on( + "reviewStatus" -> primaryTaskReviewStatus, + "reviewRequestedBy" -> primaryTaskReviewRequestedBy, + "inList" -> taskIds + ) + .executeUpdate() + case None => + } + + val lockedTasks = this.withListLocking(user, Some(TaskType())) { () => + this.taskDAL.retrieveListById(-1, 0)(taskIds) + } + lockedTasks.foreach { task => + try { + this.lockItem(user, task) + } catch { + case e: Exception => + this.logger.warn(e.getMessage) } - lockedTasks.foreach { task => - try { - this.lockItem(user, task) - } catch { - case e: Exception => - this.logger.warn(e.getMessage) - } - this.cacheManager.withOptionCaching { () => - Some( - task.copy( - bundleId = Some(bundleId), - status = Some(primaryTaskStatus), - isBundlePrimary = Some(primaryTaskId == task.id) - ) + this.cacheManager.withOptionCaching { () => + Some( + task.copy( + bundleId = Some(bundleId), + status = Some(primaryTaskStatus), + isBundlePrimary = Some(primaryTaskId == task.id) ) - } + ) } } } From 0f6f4ebdaa2892771a14e296dbea437d31aa587c Mon Sep 17 00:00:00 2001 From: CollinBeczak Date: Tue, 23 Apr 2024 12:35:00 -0500 Subject: [PATCH 24/26] add bundling user restrictions and test back --- .../framework/service/TaskBundleService.scala | 15 +++++++ .../service/TaskBundleServiceSpec.scala | 39 +++++++++++++++++++ 2 files changed, 54 insertions(+) diff --git a/app/org/maproulette/framework/service/TaskBundleService.scala b/app/org/maproulette/framework/service/TaskBundleService.scala index bf6c81c6..318a6c54 100644 --- a/app/org/maproulette/framework/service/TaskBundleService.scala +++ b/app/org/maproulette/framework/service/TaskBundleService.scala @@ -100,6 +100,14 @@ class TaskBundleService @Inject() ( bundleId: Long, taskIds: List[Long] ): TaskBundle = { + val bundle = this.getTaskBundle(user, bundleId) + + if (!permission.isSuperUser(user) && bundle.ownerId != user.id) { + throw new IllegalAccessException( + "Only a super user or the original user can reset this bundle." + ) + } + this.repository.resetTaskBundle(user, bundleId, taskIds) this.getTaskBundle(user, bundleId) } @@ -117,6 +125,13 @@ class TaskBundleService @Inject() ( )(): TaskBundle = { val bundle = this.getTaskBundle(user, bundleId) + // Verify permissions to modify this bundle + if (!permission.isSuperUser(user) && bundle.ownerId != user.id) { + throw new IllegalAccessException( + "Only a super user or the original user can delete this bundle." + ) + } + this.repository.unbundleTasks(user, bundleId, taskIds, preventTaskIdUnlocks) this.getTaskBundle(user, bundleId) } diff --git a/test/org/maproulette/framework/service/TaskBundleServiceSpec.scala b/test/org/maproulette/framework/service/TaskBundleServiceSpec.scala index 7ed94c51..fbf07da4 100644 --- a/test/org/maproulette/framework/service/TaskBundleServiceSpec.scala +++ b/test/org/maproulette/framework/service/TaskBundleServiceSpec.scala @@ -245,6 +245,45 @@ class TaskBundleServiceSpec(implicit val application: Application) extends Frame response.taskIds.head mustEqual task1.id } + "unbundle a task with permission check" taggedAs (TaskTag) in { + val task1 = taskDAL + .insert( + getTestTask(UUID.randomUUID().toString, challenge.id), + User.superUser + ) + var task2 = taskDAL + .insert( + getTestTask(UUID.randomUUID().toString, challenge.id), + User.superUser + ) + + val bundle = this.service + .createTaskBundle( + User.superUser, + "my bundle for unbundle", + Some(task1.id), + List(task1.id, task2.id) + ) + + // tasks.bundle_id is NOT set until setTaskStatus is called + taskDAL.setTaskStatus( + List(task1, task2), + Task.STATUS_FIXED, + User.superUser, + bundleId = Some(bundle.bundleId), + primaryTaskId = Some(task1.id) + ) + + val randomUser = serviceManager.user.create( + this.getTestUser(1022345, "RandomOUser2"), + User.superUser + ) + + // Random user is not allowed to delete this bundle + an[IllegalAccessException] should be thrownBy + this.service.unbundleTasks(randomUser, bundle.bundleId, List(task2.id))() + } + } override implicit val projectTestName: String = "TaskBundleSpecProject" From b3a291101a3966b3c1b6887866359f6e6bf51a11 Mon Sep 17 00:00:00 2001 From: CollinBeczak Date: Tue, 23 Apr 2024 13:57:48 -0500 Subject: [PATCH 25/26] add missing param to test --- .../maproulette/framework/service/TaskBundleServiceSpec.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/org/maproulette/framework/service/TaskBundleServiceSpec.scala b/test/org/maproulette/framework/service/TaskBundleServiceSpec.scala index fbf07da4..f3eb038b 100644 --- a/test/org/maproulette/framework/service/TaskBundleServiceSpec.scala +++ b/test/org/maproulette/framework/service/TaskBundleServiceSpec.scala @@ -281,7 +281,7 @@ class TaskBundleServiceSpec(implicit val application: Application) extends Frame // Random user is not allowed to delete this bundle an[IllegalAccessException] should be thrownBy - this.service.unbundleTasks(randomUser, bundle.bundleId, List(task2.id))() + this.service.unbundleTasks(randomUser, bundle.bundleId, List(task2.id), List())() } } From 27195059d9a21d4354fada64daa451c49d971f03 Mon Sep 17 00:00:00 2001 From: CollinBeczak Date: Fri, 26 Apr 2024 12:56:33 -0500 Subject: [PATCH 26/26] remove caching manager --- .../repository/TaskBundleRepository.scala | 97 ++++++------------- 1 file changed, 28 insertions(+), 69 deletions(-) diff --git a/app/org/maproulette/framework/repository/TaskBundleRepository.scala b/app/org/maproulette/framework/repository/TaskBundleRepository.scala index 1a5a4d26..3789546d 100644 --- a/app/org/maproulette/framework/repository/TaskBundleRepository.scala +++ b/app/org/maproulette/framework/repository/TaskBundleRepository.scala @@ -9,8 +9,6 @@ import org.slf4j.LoggerFactory import anorm.ToParameterValue import anorm.SqlParser.scalar - -import org.maproulette.cache.CacheManager import anorm._, postgresql._ import javax.inject.{Inject, Singleton} import org.maproulette.exception.InvalidException @@ -35,9 +33,8 @@ class TaskBundleRepository @Inject() ( ) extends RepositoryMixin with TaskParserMixin with Locking[Task] { - protected val logger = LoggerFactory.getLogger(this.getClass) - implicit val baseTable: String = Task.TABLE - val cacheManager: CacheManager[Long, Task] = this.taskRepository.cacheManager + protected val logger = LoggerFactory.getLogger(this.getClass) + implicit val baseTable: String = Task.TABLE /** * Inserts a new task bundle with the given tasks, assigning ownership of @@ -74,9 +71,6 @@ class TaskBundleRepository @Inject() ( rowId match { case Some(bundleId: Long) => - val sqlInsertTaskBundles = - s"""INSERT INTO task_bundles (task_id, bundle_id) VALUES ({taskId}, $bundleId)""" - // Update the task object to bind it to the bundle SQL(s"""UPDATE tasks SET bundle_id = $bundleId WHERE id IN ({inList})""") @@ -92,6 +86,8 @@ class TaskBundleRepository @Inject() ( case None => // Handle the case where primaryId is None } + val sqlInsertTaskBundles = + s"""INSERT INTO task_bundles (task_id, bundle_id) VALUES ({taskId}, $bundleId)""" val parameters = lockedTasks.map(task => Seq[NamedParameter]("taskId" -> task.id)) BatchSql(sqlInsertTaskBundles, parameters.head, parameters.tail: _*).execute() @@ -102,14 +98,6 @@ class TaskBundleRepository @Inject() ( } catch { case e: Exception => this.logger.warn(e.getMessage) } - this.cacheManager.withOptionCaching { () => - Some( - task.copy( - bundleId = Some(bundleId), - isBundlePrimary = Some(primaryId == task.id) - ) - ) - } } TaskBundle(bundleId, user.id, lockedTasks.map(task => { @@ -223,15 +211,6 @@ class TaskBundleRepository @Inject() ( case e: Exception => this.logger.warn(e.getMessage) } - this.cacheManager.withOptionCaching { () => - Some( - task.copy( - bundleId = Some(bundleId), - status = Some(primaryTaskStatus), - isBundlePrimary = Some(primaryTaskId == task.id) - ) - ) - } } } } @@ -274,11 +253,6 @@ class TaskBundleRepository @Inject() ( case Some(_) => SQL(s"""DELETE FROM task_bundles WHERE bundle_id = $bundleId AND task_id = ${task.id}""").executeUpdate() - this.cacheManager.withOptionCaching { () => - Some( - task.copy(bundleId = None, status = Some(STATUS_CREATED)) - ) - } if (!preventTaskIdUnlocks.contains(task.id)) { try { this.unlockItem(user, task) @@ -314,56 +288,41 @@ class TaskBundleRepository @Inject() ( */ def deleteTaskBundle(user: User, bundleId: Long): Unit = { this.withMRConnection { implicit c => - val primaryTaskId = SQL( - """SELECT id FROM tasks WHERE bundle_id = {bundleId} AND is_bundle_primary = true""" - ).on("bundleId" -> bundleId) - .as(scalar[Long].singleOpt) - .getOrElse(0L) - + // Update tasks to set bundle_id and is_bundle_primary to NULL SQL( """UPDATE tasks - SET bundle_id = NULL, - is_bundle_primary = NULL - WHERE bundle_id = {bundleId} OR id = {primaryTaskId}""" - ).on( - Symbol("bundleId") -> bundleId, - Symbol("primaryTaskId") -> primaryTaskId - ) + SET bundle_id = NULL, + is_bundle_primary = NULL + WHERE bundle_id = {bundleId}""" + ).on("bundleId" -> bundleId) .executeUpdate() + // Delete from task_bundles which will also cascade delete from bundles + SQL("DELETE FROM task_bundles WHERE bundle_id = {bundleId}") + .on("bundleId" -> bundleId) + .executeUpdate() + + // Get the primary task ID + val primaryTaskId = SQL( + """SELECT id FROM tasks WHERE bundle_id = {bundleId} AND is_bundle_primary = true""" + ).on("bundleId" -> bundleId) + .as(scalar[Option[Long]].singleOpt) + .getOrElse(0) + + // Retrieve tasks val tasks = this.retrieveTasks( Query.simple(List(BaseParameter("bundle_id", bundleId, table = Some("tb")))) ) - val inList = tasks.map(_.id).mkString(",") - - SQL(s"UPDATE tasks SET bundle_id = NULL WHERE id IN ($inList)").executeUpdate() - - if (primaryTaskId != 0) { - // Unlock tasks (everything but the primary task id) - tasks.foreach { task => - if (task.id != primaryTaskId) { - try { - this.unlockItem(user, task) - } catch { - case e: Exception => this.logger.warn(e.getMessage) - } - } - this.cacheManager.withOptionCaching { () => - Some( - task.copy( - bundleId = None, - isBundlePrimary = None - ) - ) + tasks.foreach { task => + if (task.id != primaryTaskId) { + try { + this.unlockItem(user, task) + } catch { + case e: Exception => this.logger.warn(e.getMessage) } } } - - // Delete from task_bundles which will also cascade delete from bundles - SQL("DELETE FROM task_bundles WHERE bundle_id = {bundleId}") - .on(Symbol("bundleId") -> bundleId) - .executeUpdate() } }