From edac075f145556d9ee756a2ce120ac174effc55b Mon Sep 17 00:00:00 2001 From: ssedoudbgouv Date: Tue, 18 Feb 2025 09:00:49 +0100 Subject: [PATCH] TRELLO-2904 : add unicity constraint and remove list for review and engagement --- .../EngagementOrchestrator.scala | 31 +++++-------------- .../ReportConsumerReviewOrchestrator.scala | 25 ++++----------- .../ResponseConsumerReviewRepository.scala | 4 +-- ...nseConsumerReviewRepositoryInterface.scala | 2 +- .../ReportEngagementReviewRepository.scala | 5 +-- ...tEngagementReviewRepositoryInterface.scala | 2 +- build.sbt | 5 +++ ... => V54__unique_engagement_constraint.sql} | 0 .../default/V55__unique_review_constraint.sql | 12 +++++++ 9 files changed, 38 insertions(+), 48 deletions(-) rename conf/db/migration/default/{V54__unique_review_constraint.sql => V54__unique_engagement_constraint.sql} (100%) create mode 100644 conf/db/migration/default/V55__unique_review_constraint.sql diff --git a/app/orchestrators/EngagementOrchestrator.scala b/app/orchestrators/EngagementOrchestrator.scala index 15b7be154..35c405975 100644 --- 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 index eb9894544..01665af6d 100644 --- 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 index 63056191c..f1ec534ef 100644 --- 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 index 9cf789998..2ac5dd486 100644 --- 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 index 34eec9d27..2ce34184b 100644 --- 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 index 653dc1396..6ba5d725f 100644 --- 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 index 7172e7436..9c60b157d 100644 --- 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/conf/db/migration/default/V54__unique_review_constraint.sql b/conf/db/migration/default/V54__unique_engagement_constraint.sql similarity index 100% rename from conf/db/migration/default/V54__unique_review_constraint.sql rename to conf/db/migration/default/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 new file mode 100644 index 000000000..830212452 --- /dev/null +++ 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); \ No newline at end of file