From 7042ed707140ff846bbb0d2f3d1798a78d5b5eed Mon Sep 17 00:00:00 2001 From: ssedoudbgouv <85867707+ssedoudbgouv@users.noreply.github.com> Date: Tue, 18 Feb 2025 08:57:30 +0100 Subject: [PATCH 1/2] TRELLO-2916: prevent other user than CCRF to use pro connect (#1892) --- app/orchestrators/UserOrchestrator.scala | 3 +++ 1 file changed, 3 insertions(+) diff --git a/app/orchestrators/UserOrchestrator.scala b/app/orchestrators/UserOrchestrator.scala index c641915d..a0a81f94 100644 --- a/app/orchestrators/UserOrchestrator.scala +++ b/app/orchestrators/UserOrchestrator.scala @@ -19,6 +19,7 @@ import java.time.OffsetDateTime import cats.syntax.option._ import models.AuthProvider.ProConnect import models.AuthProvider.SignalConso +import models.UserRole.DGCCRF import models.event.Event import models.event.Event.stringToDetailsJsValue import models.proconnect.ProConnectClaim @@ -88,11 +89,13 @@ class UserOrchestrator(userRepository: UserRepositoryInterface, eventRepository: override def getProConnectUser(claim: ProConnectClaim, role: UserRole): Future[User] = OptionT(userRepository.findByAuthProviderId(claim.sub)) .orElseF(userRepository.findByEmail(claim.email)) + .filter(_.userRole == DGCCRF) .semiflatMap { user => val updated = user.copy( email = EmailAddress(claim.email), firstName = claim.givenName, lastName = claim.usualName, + userRole = role, authProvider = ProConnect, authProviderId = claim.sub.some, lastEmailValidation = Some(OffsetDateTime.now()) From f9d30b4e13527f0bea75cef3d9401b079ee1a32a Mon Sep 17 00:00:00 2001 From: ssedoudbgouv <85867707+ssedoudbgouv@users.noreply.github.com> Date: Tue, 18 Feb 2025 09:23:04 +0100 Subject: [PATCH 2/2] =?UTF-8?q?TRELLO-2904=20:=20add=20unicity=20constrain?= =?UTF-8?q?t=20and=20remove=20list=20for=20review=20and=20e=E2=80=A6=20(#1?= =?UTF-8?q?893)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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 +++ .../V55__unique_engagement_constraint.sql | 12 +++++++ 8 files changed, 38 insertions(+), 48 deletions(-) create mode 100644 conf/db/migration/default/V55__unique_engagement_constraint.sql diff --git a/app/orchestrators/EngagementOrchestrator.scala b/app/orchestrators/EngagementOrchestrator.scala index 15b7be15..35c40597 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 eb989454..01665af6 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 63056191..f1ec534e 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 9cf78999..2ac5dd48 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 34eec9d2..2ce34184 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 653dc139..6ba5d725 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 7172e743..9c60b157 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/V55__unique_engagement_constraint.sql b/conf/db/migration/default/V55__unique_engagement_constraint.sql new file mode 100644 index 00000000..6b46ac0d --- /dev/null +++ b/conf/db/migration/default/V55__unique_engagement_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_engagement_report_id UNIQUE (report_id); \ No newline at end of file