[TRELLO-2037] Remove userRole from Subscription (useless, there is th…

…e userId)
betagouv · Oct 18, 2023 · d440f36 · d440f36
1 parent 1803a42
commit d440f36
Show file tree

Hide file tree

Showing 13 changed files with 114 additions and 106 deletions.
diff --git a/app/controllers/AdminController.scala b/app/controllers/AdminController.scala
@@ -189,8 +189,7 @@ class AdminController(
     withoutTags = Nil,
     categories = Nil,
     sirets = Nil,
-    frequency = java.time.Period.ofDays(1),
-    userRole = Some(UserRole.DGCCRF)
+    frequency = java.time.Period.ofDays(1)
   )
 
   case class EmailContent(subject: String, body: play.twirl.api.Html)

diff --git a/app/controllers/SubscriptionController.scala b/app/controllers/SubscriptionController.scala
@@ -45,8 +45,7 @@ class SubscriptionController(
                   withoutTags = draftSubscription.withoutTags,
                   countries = draftSubscription.countries.map(Country.fromCode),
                   sirets = draftSubscription.sirets,
-                  frequency = draftSubscription.frequency,
-                  userRole = Some(request.identity.userRole)
+                  frequency = draftSubscription.frequency
                 )
               )
               .map(subscription => Ok(Json.toJson(subscription)))

diff --git a/app/loader/SignalConsoApplicationLoader.scala b/app/loader/SignalConsoApplicationLoader.scala
@@ -476,7 +476,14 @@ class SignalConsoComponents(
   logger.debug("Starting App and sending sentry alert")
 
   val reportNotificationTask =
-    new ReportNotificationTask(actorSystem, reportRepository, subscriptionRepository, mailService, taskConfiguration)
+    new ReportNotificationTask(
+      actorSystem,
+      reportRepository,
+      subscriptionRepository,
+      userRepository,
+      mailService,
+      taskConfiguration
+    )
 
   val inactiveDgccrfAccountRemoveTask =
     new InactiveDgccrfAccountRemoveTask(userRepository, subscriptionRepository, asyncFileRepository)

diff --git a/app/models/Subscription.scala b/app/models/Subscription.scala
@@ -51,8 +51,7 @@ case class Subscription(
     withoutTags: List[ReportTag] = List.empty,
     countries: List[Country] = List.empty,
     sirets: List[SIRET] = List.empty,
-    frequency: Period,
-    userRole: Option[UserRole]
+    frequency: Period
 )
 
 object Subscription {

diff --git a/app/repositories/subscription/SubscriptionTable.scala b/app/repositories/subscription/SubscriptionTable.scala
@@ -1,7 +1,6 @@
 package repositories.subscription
 
 import models.Subscription
-import models.UserRole
 import models.report.ReportCategory
 import models.report.ReportTag
 import repositories.DatabaseTable
@@ -14,14 +13,9 @@ import java.time.OffsetDateTime
 import java.time.Period
 import java.util.UUID
 import repositories.report.ReportColumnType.ReportTagListColumnType
-import slick.ast.BaseTypedType
-import slick.jdbc.JdbcType
 
 class SubscriptionTable(tag: Tag) extends DatabaseTable[Subscription](tag, "subscriptions") {
 
-  implicit val userRoleColumnType: JdbcType[UserRole] with BaseTypedType[UserRole] =
-    MappedColumnType.base[UserRole, String](_.entryName, UserRole.withName)
-
   def creationDate = column[OffsetDateTime]("creation_date")
   def userId       = column[Option[UUID]]("user_id")
   def email        = column[Option[EmailAddress]]("email")
@@ -32,7 +26,6 @@ class SubscriptionTable(tag: Tag) extends DatabaseTable[Subscription](tag, "subs
   def countries    = column[List[Country]]("countries")
   def sirets       = column[List[SIRET]]("sirets")
   def frequency    = column[Period]("frequency")
-  def userRole     = column[Option[UserRole]]("user_role")
 
   type SubscriptionData = (
       UUID,
@@ -45,8 +38,7 @@ class SubscriptionTable(tag: Tag) extends DatabaseTable[Subscription](tag, "subs
       List[ReportTag],
       List[Country],
       List[SIRET],
-      Period,
-      Option[UserRole]
+      Period
   )
 
   def constructSubscription: SubscriptionData => Subscription = {
@@ -61,8 +53,7 @@ class SubscriptionTable(tag: Tag) extends DatabaseTable[Subscription](tag, "subs
           withoutTags,
           countries,
           sirets,
-          frequency,
-          userRole
+          frequency
         ) =>
       Subscription(
         id = id,
@@ -75,8 +66,7 @@ class SubscriptionTable(tag: Tag) extends DatabaseTable[Subscription](tag, "subs
         withoutTags = withoutTags,
         countries = countries,
         sirets = sirets,
-        frequency = frequency,
-        userRole = userRole
+        frequency = frequency
       )
   }
 
@@ -92,8 +82,7 @@ class SubscriptionTable(tag: Tag) extends DatabaseTable[Subscription](tag, "subs
           withoutTags,
           countries,
           sirets,
-          frequency,
-          userRole
+          frequency
         ) =>
       (
         id,
@@ -106,8 +95,7 @@ class SubscriptionTable(tag: Tag) extends DatabaseTable[Subscription](tag, "subs
         withoutTags,
         countries,
         sirets,
-        frequency,
-        userRole
+        frequency
       )
   }
 
@@ -123,8 +111,7 @@ class SubscriptionTable(tag: Tag) extends DatabaseTable[Subscription](tag, "subs
       withoutTags,
       countries,
       sirets,
-      frequency,
-      userRole
+      frequency
     ) <> (constructSubscription, extractSubscription.lift)
 }
 

diff --git a/app/tasks/report/ReportNotificationTask.scala b/app/tasks/report/ReportNotificationTask.scala
@@ -4,6 +4,7 @@ import akka.actor.ActorSystem
 import cats.implicits.toTraverseOps
 import config.TaskConfiguration
 import models.Subscription
+import models.User
 import models.UserRole
 import models.report.PreFilter
 import models.report.Report
@@ -12,6 +13,7 @@ import models.report.ReportFilter
 import play.api.Logger
 import repositories.report.ReportRepositoryInterface
 import repositories.subscription.SubscriptionRepositoryInterface
+import repositories.user.UserRepositoryInterface
 import services.Email.DgccrfReportNotification
 import services.MailService
 import tasks.report.ReportNotificationTask.refineReportBasedOnSubscriptionFilters
@@ -32,6 +34,7 @@ class ReportNotificationTask(
     actorSystem: ActorSystem,
     reportRepository: ReportRepositoryInterface,
     subscriptionRepository: SubscriptionRepositoryInterface,
+    userRepository: UserRepositoryInterface,
     mailService: MailService,
     taskConfiguration: TaskConfiguration
 )(implicit executionContext: ExecutionContext) {
@@ -76,10 +79,11 @@ class ReportNotificationTask(
         )
       )
       _ = logger.debug(s"Found ${reportsWithFiles.size} reports for this period ($period)")
-      subscriptionsEmailAndReports = subscriptionsWithEmails.map { case (subscription, emailAddress) =>
-        val filteredReport = refineReportBasedOnSubscriptionFilters(reportsWithFiles, subscription)
-        (subscription, emailAddress, filteredReport)
-      }
+      subscriptionsEmailAndReports <- Future.sequence(subscriptionsWithEmails.map { case (subscription, emailAddress) =>
+        refineReportBasedOnSubscriptionFilters(userRepository, reportsWithFiles, subscription).map { filteredReport =>
+          (subscription, emailAddress, filteredReport)
+        }
+      })
       subscriptionEmailAndNonEmptyReports = subscriptionsEmailAndReports.filter(_._3.nonEmpty)
       _ = logger.debug(
         s"We have ${subscriptionEmailAndNonEmptyReports.size} emails of notifications to send (period $period)"
@@ -115,47 +119,58 @@ class ReportNotificationTask(
 
 object ReportNotificationTask {
 
+  private def userFromSubscription(
+      userRepository: UserRepositoryInterface,
+      subscription: Subscription
+  ): Future[Option[User]] = subscription.userId match {
+    case Some(userId) => userRepository.get(userId)
+    case None         => Future.successful(None)
+  }
+
   def refineReportBasedOnSubscriptionFilters(
+      userRepository: UserRepositoryInterface,
       reportsWithFiles: SortedMap[Report, List[ReportFile]],
       subscription: Subscription
-  ): SortedMap[Report, List[ReportFile]] = reportsWithFiles
-    .filter { case (report, _) =>
-      subscription.userRole match {
-        case Some(UserRole.DGAL) =>
-          PreFilter.DGALFilter.category.forall(_.entryName == report.category) || (if (
-                                                                                     PreFilter.DGALFilter.tags.isEmpty
-                                                                                   ) true
-                                                                                   else
-                                                                                     PreFilter.DGALFilter.tags
-                                                                                       .intersect(report.tags)
-                                                                                       .nonEmpty)
-        case Some(UserRole.DGCCRF)        => true
-        case Some(UserRole.Admin)         => true
-        case Some(UserRole.Professionnel) => true
-        case None                         => true
-      }
-    }
-    .filter { case (report, _) =>
-      subscription.departments.isEmpty || (report.companyAddress.country.isEmpty && subscription.departments
-        .map(Some(_))
-        .contains(report.companyAddress.postalCode.flatMap(Departments.fromPostalCode)))
-    }
-    .filter { case (report, _) =>
-      subscription.categories.isEmpty || subscription.categories.map(_.entryName).contains(report.category)
-    }
-    .filter { case (report, _) =>
-      subscription.sirets.isEmpty || subscription.sirets.map(Some(_)).contains(report.companySiret)
-    }
-    .filter { case (report, _) =>
-      subscription.countries.isEmpty || subscription.countries
-        .map(Some(_))
-        .contains(report.companyAddress.country)
-    }
-    .filter { case (report, _) =>
-      subscription.withTags.isEmpty || subscription.withTags.intersect(report.tags).nonEmpty
-    }
-    .filter { case (report, _) =>
-      subscription.withoutTags.isEmpty || subscription.withoutTags.intersect(report.tags).isEmpty
+  )(implicit ec: ExecutionContext): Future[SortedMap[Report, List[ReportFile]]] =
+    userFromSubscription(userRepository, subscription).map { maybeUser =>
+      reportsWithFiles
+        .filter { case (report, _) =>
+          maybeUser.map(_.userRole) match {
+            case Some(UserRole.DGAL) =>
+              PreFilter.DGALFilter.category
+                .forall(_.entryName == report.category) || (if (PreFilter.DGALFilter.tags.isEmpty) true
+                                                            else
+                                                              PreFilter.DGALFilter.tags
+                                                                .intersect(report.tags)
+                                                                .nonEmpty)
+            case Some(UserRole.DGCCRF)        => true
+            case Some(UserRole.Admin)         => true
+            case Some(UserRole.Professionnel) => true
+            case None                         => true
+          }
+        }
+        .filter { case (report, _) =>
+          subscription.departments.isEmpty || (report.companyAddress.country.isEmpty && subscription.departments
+            .map(Some(_))
+            .contains(report.companyAddress.postalCode.flatMap(Departments.fromPostalCode)))
+        }
+        .filter { case (report, _) =>
+          subscription.categories.isEmpty || subscription.categories.map(_.entryName).contains(report.category)
+        }
+        .filter { case (report, _) =>
+          subscription.sirets.isEmpty || subscription.sirets.map(Some(_)).contains(report.companySiret)
+        }
+        .filter { case (report, _) =>
+          subscription.countries.isEmpty || subscription.countries
+            .map(Some(_))
+            .contains(report.companyAddress.country)
+        }
+        .filter { case (report, _) =>
+          subscription.withTags.isEmpty || subscription.withTags.intersect(report.tags).nonEmpty
+        }
+        .filter { case (report, _) =>
+          subscription.withoutTags.isEmpty || subscription.withoutTags.intersect(report.tags).isEmpty
+        }
     }
 
 }
diff --git a/conf/db/migration/default/V14__add_user_role_to_subscriptions.sql b/conf/db/migration/default/V14__add_user_role_to_subscriptions.sql
diff --git a/test/tasks/account/InactiveAccountTaskSpec.scala b/test/tasks/account/InactiveAccountTaskSpec.scala
@@ -5,7 +5,6 @@ import models.AsyncFile
 import models.AsyncFileKind
 import models.Subscription
 import models.User
-import models.UserRole
 import org.specs2.concurrent.ExecutionEnv
 import org.specs2.matcher.FutureMatchers
 import play.api.mvc.Results
@@ -84,8 +83,7 @@ class InactiveAccountTaskSpec(implicit ee: ExecutionEnv)
             email = None,
             creationDate = OffsetDateTime.now().truncatedTo(ChronoUnit.MILLIS),
             userId = Some(inactiveDGCCRFUser.id),
-            frequency = Period.ofDays(1),
-            userRole = Some(UserRole.Admin)
+            frequency = Period.ofDays(1)
           )
 
         // Subscriptions that should be kept
@@ -94,8 +92,7 @@ class InactiveAccountTaskSpec(implicit ee: ExecutionEnv)
             email = None,
             creationDate = OffsetDateTime.now().truncatedTo(ChronoUnit.MILLIS),
             userId = Some(activeDGCCRFUser.id),
-            frequency = Period.ofDays(1),
-            userRole = Some(UserRole.Admin)
+            frequency = Period.ofDays(1)
           )
 
         val (userList, deletedUsersList, activeSubscriptionList, inactiveSubscriptionList, inactivefiles, activefiles) =

diff --git a/test/tasks/report/DailyReportNotificationTaskSpec.scala b/test/tasks/report/DailyReportNotificationTaskSpec.scala
@@ -83,25 +83,22 @@ abstract class DailyReportNotificationTaskSpec(implicit ee: ExecutionEnv)
     email = Some(covidEmail),
     departments = List(covidDept),
     categories = List(ReportCategory.Coronavirus),
-    frequency = Period.ofDays(1),
-    userRole = Some(UserRole.Admin)
+    frequency = Period.ofDays(1)
   )
 
   val tagSubscription = Subscription(
     userId = None,
     email = Some(tagEmail),
     departments = List(tagDept),
     withTags = List(ReportTag.ProduitDangereux),
-    frequency = Period.ofDays(1),
-    userRole = Some(UserRole.Admin)
+    frequency = Period.ofDays(1)
   )
 
   val countrySubscription = Subscription(
     userId = None,
     email = Some(countryEmail),
     countries = List(Country.Suisse),
-    frequency = Period.ofDays(1),
-    userRole = Some(UserRole.Admin)
+    frequency = Period.ofDays(1)
   )
 
   val company = Fixtures.genCompany.sample.get

diff --git a/test/tasks/report/NoReportNotificationTaskSpec.scala b/test/tasks/report/NoReportNotificationTaskSpec.scala
@@ -62,25 +62,22 @@ abstract class NoReportNotificationTaskSpec(implicit ee: ExecutionEnv)
     email = Some(covidEmail),
     departments = List(covidDept),
     categories = List(ReportCategory.Coronavirus),
-    frequency = Period.ofDays(1),
-    userRole = Some(UserRole.Admin)
+    frequency = Period.ofDays(1)
   )
 
   val tagSubscription = Subscription(
     userId = None,
     email = Some(tagEmail),
     departments = List(tagDept),
     withTags = List(ReportTag.ProduitDangereux),
-    frequency = Period.ofDays(1),
-    userRole = Some(UserRole.Admin)
+    frequency = Period.ofDays(1)
   )
 
   val countrySubscription = Subscription(
     userId = None,
     email = Some(countryEmail),
     countries = List(Country.Suisse),
-    frequency = Period.ofDays(1),
-    userRole = Some(UserRole.Admin)
+    frequency = Period.ofDays(1)
   )
 
   val company = Fixtures.genCompany.sample.get