TRELLO-2904 : add unicity constraint and remove list for review and e…

…ngagement
betagouv · Feb 18, 2025 · edac075 · edac075
1 parent fb25b21
commit edac075
Show file tree

Hide file tree

Showing 9 changed files with 38 additions and 48 deletions.
diff --git a/app/orchestrators/EngagementOrchestrator.scala b/app/orchestrators/EngagementOrchestrator.scala
@@ -1,7 +1,6 @@
 package orchestrators
 
 import cats.implicits.catsSyntaxOption
-import cats.implicits.catsSyntaxOptionId
 import controllers.error.AppError.CannotReviewReportResponse
 import controllers.error.AppError.EngagementNotFound
 import controllers.error.AppError.ReportNotFound
@@ -11,8 +10,8 @@ import models.engagement.EngagementId
 import models.event.Event
 import models.report.ExistingReportResponse
 import models.report.ReportStatus.hasResponse
-import models.report.review.EngagementReview
 import models.report.review.ConsumerReviewApi
+import models.report.review.EngagementReview
 import models.report.review.ResponseConsumerReviewId
 import play.api.Logger
 import play.api.libs.json.Json
@@ -108,7 +107,7 @@ class EngagementOrchestrator(
 
   def removeEngagement(reportId: UUID): Future[Unit] =
     for {
-      _ <- getEngagementReview(reportId).flatMap {
+      _ <- reportEngagementReviewRepository.findByReportId(reportId).flatMap {
         case Some(engagementReview) =>
           reportEngagementReviewRepository.delete(engagementReview.id).map(_ => ())
         case None => Future.unit
@@ -119,24 +118,9 @@ class EngagementOrchestrator(
   def getVisibleEngagementReview(reportId: UUID, user: User): Future[Option[EngagementReview]] =
     for {
       _           <- visibleReportOrchestrator.checkReportIsVisible(reportId, user)
-      maybeReview <- getEngagementReview(reportId)
+      maybeReview <- reportEngagementReviewRepository.findByReportId(reportId)
     } yield maybeReview
 
-  private def getEngagementReview(reportId: UUID): Future[Option[EngagementReview]] =
-    reportEngagementReviewRepository.findByReportId(reportId) map {
-      case Nil =>
-        logger.info(s"No engagement review found for report $reportId")
-        None
-      case review :: Nil => Some(review)
-      case engagementReviews =>
-        io.sentry.Sentry.captureException(
-          new Exception(
-            s"More than one engagement review for report id $reportId, this is not and expected behavior it should be investigated"
-          )
-        )
-        engagementReviews.maxBy(_.creationDate).some
-    }
-
   def getEngagementReviews(reportIds: Seq[UUID]): Future[Map[UUID, Option[EngagementReview]]] =
     reportEngagementReviewRepository.findByReportIds(reportIds)
 
@@ -160,7 +144,7 @@ class EngagementOrchestrator(
       }
       _ = logger.debug(s"Report validated")
       reviews <- reportEngagementReviewRepository.findByReportId(reportId)
-      _ <- reviews.headOption match {
+      _ <- reviews match {
         case Some(review) =>
           updateEngagementReview(review.copy(evaluation = reviewApi.evaluation, details = reviewApi.details))
         case None =>
@@ -172,14 +156,14 @@ class EngagementOrchestrator(
   def deleteDetails(reportId: UUID): Future[Unit] = for {
     reviews <- reportEngagementReviewRepository.findByReportId(reportId)
     _ <- reviews match {
-      case review :: _ => reportEngagementReviewRepository.update(review.id, review.copy(details = Some("")))
-      case _           => Future.unit
+      case Some(review) => reportEngagementReviewRepository.update(review.id, review.copy(details = Some("")))
+      case None         => Future.unit
     }
   } yield ()
 
   def doesEngagementReviewExists(reportId: UUID): Future[Boolean] =
     for {
-      maybeReview <- getEngagementReview(reportId)
+      maybeReview <- reportEngagementReviewRepository.findByReportId(reportId)
       hasNonEmptyReview = maybeReview.exists(_.details.nonEmpty)
     } yield hasNonEmptyReview
 
@@ -191,6 +175,7 @@ class EngagementOrchestrator(
         details = review.details
       )
     )
+
   private def createEngagementReview(reportId: UUID, review: ConsumerReviewApi): Future[Event] =
     reportEngagementReviewRepository
       .createOrUpdate(

diff --git a/app/orchestrators/ReportConsumerReviewOrchestrator.scala b/app/orchestrators/ReportConsumerReviewOrchestrator.scala
@@ -2,7 +2,6 @@ package orchestrators
 
 import org.apache.pekko.Done
 import controllers.error.AppError.CannotReviewReportResponse
-import controllers.error.AppError.ServerError
 import models.User
 import models.report.ReportStatus.hasResponse
 import models.report.review.ConsumerReviewApi
@@ -32,7 +31,7 @@ class ReportConsumerReviewOrchestrator(
   val logger = Logger(this.getClass)
 
   def remove(reportId: UUID): Future[Done] =
-    getReview(reportId).flatMap {
+    responseConsumerReviewRepository.findByReportId(reportId).flatMap {
       case Some(responseConsumerReview) =>
         responseConsumerReviewRepository.delete(responseConsumerReview.id).map(_ => Done)
       case None => Future.successful(Done)
@@ -41,27 +40,15 @@ class ReportConsumerReviewOrchestrator(
   def getVisibleReview(reportId: UUID, user: User): Future[Option[ResponseConsumerReview]] =
     for {
       _           <- visibleReportOrchestrator.checkReportIsVisible(reportId, user)
-      maybeReview <- getReview(reportId)
+      maybeReview <- responseConsumerReviewRepository.findByReportId(reportId)
     } yield maybeReview
 
   def doesReviewExists(reportId: UUID): Future[Boolean] =
     for {
-      maybeReview <- getReview(reportId)
+      maybeReview <- responseConsumerReviewRepository.findByReportId(reportId)
       hasNonEmptyReview = maybeReview.exists(_.details.nonEmpty)
     } yield hasNonEmptyReview
 
-  private def getReview(reportId: UUID): Future[Option[ResponseConsumerReview]] =
-    for {
-      reviews <- responseConsumerReviewRepository.findByReportId(reportId)
-      maybeReview = reviews match {
-        case Nil =>
-          logger.info(s"No review found for report $reportId")
-          None
-        case review :: Nil => Some(review)
-        case _             => throw ServerError(s"More than one consumer review for report id $reportId")
-      }
-    } yield maybeReview
-
   def getReviews(reportIds: List[UUID]): Future[Map[UUID, Option[ResponseConsumerReview]]] =
     responseConsumerReviewRepository.findByReportIds(reportIds)
 
@@ -87,7 +74,7 @@ class ReportConsumerReviewOrchestrator(
       }
       _ = logger.debug(s"Report validated")
       reviews <- responseConsumerReviewRepository.findByReportId(reportId)
-      _ <- reviews.headOption match {
+      _ <- reviews match {
         case Some(review) =>
           updateReview(review.copy(evaluation = reviewApi.evaluation, details = reviewApi.details))
         case None =>
@@ -99,8 +86,8 @@ class ReportConsumerReviewOrchestrator(
   def deleteDetails(reportId: UUID): Future[Unit] = for {
     reviews <- responseConsumerReviewRepository.findByReportId(reportId)
     _ <- reviews match {
-      case review :: _ => responseConsumerReviewRepository.update(review.id, review.copy(details = Some("")))
-      case _           => Future.unit
+      case Some(review) => responseConsumerReviewRepository.update(review.id, review.copy(details = Some("")))
+      case None         => Future.unit
     }
   } yield ()
 

diff --git a/app/repositories/reportconsumerreview/ResponseConsumerReviewRepository.scala b/app/repositories/reportconsumerreview/ResponseConsumerReviewRepository.scala
@@ -25,8 +25,8 @@ class ResponseConsumerReviewRepository(
 
   import dbConfig._
 
-  override def findByReportId(reportId: UUID): Future[List[ResponseConsumerReview]] =
-    db.run(table.filter(_.reportId === reportId).to[List].result)
+  override def findByReportId(reportId: UUID): Future[Option[ResponseConsumerReview]] =
+    db.run(table.filter(_.reportId === reportId).to[List].result.headOption)
 
   override def findByReportIds(reportIds: List[UUID]): Future[Map[UUID, Option[ResponseConsumerReview]]] =
     db.run(

diff --git a/app/repositories/reportconsumerreview/ResponseConsumerReviewRepositoryInterface.scala b/app/repositories/reportconsumerreview/ResponseConsumerReviewRepositoryInterface.scala
@@ -10,7 +10,7 @@ import scala.concurrent.Future
 trait ResponseConsumerReviewRepositoryInterface
     extends TypedCRUDRepositoryInterface[ResponseConsumerReview, ResponseConsumerReviewId] {
 
-  def findByReportId(reportId: UUID): Future[List[ResponseConsumerReview]]
+  def findByReportId(reportId: UUID): Future[Option[ResponseConsumerReview]]
 
   def findByReportIds(reportIds: List[UUID]): Future[Map[UUID, Option[ResponseConsumerReview]]]
 

diff --git a/app/repositories/reportengagementreview/ReportEngagementReviewRepository.scala b/app/repositories/reportengagementreview/ReportEngagementReviewRepository.scala
@@ -23,8 +23,9 @@ class ReportEngagementReviewRepository(
 
   import dbConfig._
 
-  override def findByReportId(reportId: UUID): Future[List[EngagementReview]] =
-    db.run(table.filter(_.reportId === reportId).to[List].result)
+  override def findByReportId(reportId: UUID): Future[Option[EngagementReview]] =
+    db.run(table.filter(_.reportId === reportId).to[List].result.headOption)
+
   override def findByReportIds(reportIds: Seq[UUID]): Future[Map[UUID, Option[EngagementReview]]] =
     db.run(
       table

diff --git a/app/repositories/reportengagementreview/ReportEngagementReviewRepositoryInterface.scala b/app/repositories/reportengagementreview/ReportEngagementReviewRepositoryInterface.scala
@@ -9,7 +9,7 @@ import scala.concurrent.Future
 
 trait ReportEngagementReviewRepositoryInterface
     extends TypedCRUDRepositoryInterface[EngagementReview, ResponseConsumerReviewId] {
-  def findByReportId(reportId: UUID): Future[List[EngagementReview]]
+  def findByReportId(reportId: UUID): Future[Option[EngagementReview]]
   def findByReportIds(reportIds: Seq[UUID]): Future[Map[UUID, Option[EngagementReview]]]
 
   def findByCompany(companyId: Option[UUID]): Future[List[EngagementReview]]

diff --git a/build.sbt b/build.sbt
@@ -58,6 +58,11 @@ Test / tpolecatExcludeOptions += ScalacOptions.lintMissingInterpolator
 
 resolvers += "Atlassian Releases" at "https://packages.atlassian.com/maven-public/"
 
+def doLint(cmd: String) = s"scalafmt;$cmd;"
+
+addCommandAlias("lint", doLint("scalafmt;compile;scalafix"))
+addCommandAlias("lintTest", doLint("scalafmtAll;Test / compile;scalafixAll"))
+
 Universal / mappings ++=
   (baseDirectory.value / "appfiles" * "*" get) map
     (x => x -> ("appfiles/" + x.getName))

diff --git a/...default/V54__unique_review_constraint.sql → ...ult/V54__unique_engagement_constraint.sql b/...default/V54__unique_review_constraint.sql → ...ult/V54__unique_engagement_constraint.sql
diff --git a/conf/db/migration/default/V55__unique_review_constraint.sql b/conf/db/migration/default/V55__unique_review_constraint.sql
@@ -0,0 +1,12 @@
+-- Removing duplicates before applying constraint
+DELETE
+FROM engagement_reviews r
+    USING (SELECT rr.report_id, MAX(rr.creation_date) AS max_creation
+           FROM engagement_reviews rr
+           GROUP BY rr.report_id
+           HAVING COUNT(1) > 1) agg
+WHERE agg.report_id = r.report_id
+  AND agg.max_creation <> r.creation_date;
+
+ALTER TABLE engagement_reviews
+    ADD CONSTRAINT unique_report_id UNIQUE (report_id);