From fb68260f589dd6b158e1a3d7e402fc4b1378d306 Mon Sep 17 00:00:00 2001 From: Simon Byford Date: Tue, 23 Jul 2024 14:18:13 +0100 Subject: [PATCH 1/9] Introduce /api/rules/csv-import endpoint --- .../app/controllers/RulesController.scala | 20 +++++++++ .../app/service/RuleManager.scala | 45 +++++++++++++++++++ apps/rule-manager/conf/routes | 2 + build.sbt | 1 + 4 files changed, 68 insertions(+) diff --git a/apps/rule-manager/app/controllers/RulesController.scala b/apps/rule-manager/app/controllers/RulesController.scala index 58f3998fb..927a614cb 100644 --- a/apps/rule-manager/app/controllers/RulesController.scala +++ b/apps/rule-manager/app/controllers/RulesController.scala @@ -279,4 +279,24 @@ class RulesController( case Left(error) => BadRequest(s"Invalid request: $error") } } + + def csvImport() = Action { implicit request => + val rules = for { + formData <- request.body.asMultipartFormData.toRight("No form data found in request") + file <- formData.file("file").toRight("No file found in request") + tag = formData.dataParts.get("tag") match { + case Some(tag) => tag.head + case None => "" + } + } yield RuleManager.csvImport( + file.ref.path.toFile, + tag, + bucketRuleResource + ) + + rules match { + case Right(int) => Ok(Json.toJson(int)) + case Left(message) => BadRequest(message) + } + } } diff --git a/apps/rule-manager/app/service/RuleManager.scala b/apps/rule-manager/app/service/RuleManager.scala index f74a74316..9731c4934 100644 --- a/apps/rule-manager/app/service/RuleManager.scala +++ b/apps/rule-manager/app/service/RuleManager.scala @@ -18,6 +18,8 @@ import model.{DictionaryForm, LTRuleCoreForm, LTRuleXMLForm, PaginatedResponse, import play.api.data.FormError import play.api.libs.json.{Json, OWrites} import scalikejdbc.DBSession +import com.github.tototoshi.csv.CSVReader +import java.io.File object AllRuleData { implicit val writes: OWrites[AllRuleData] = Json.writes[AllRuleData] @@ -32,6 +34,49 @@ case class AllRuleData( ) object RuleManager extends Loggable { + def csvImport(toFile: File, tagName: String, bucketRuleResource: BucketRuleResource) = { + val reader = CSVReader.open(toFile) + val rules = reader.all() + + val initialRuleOrder = DbRuleDraft.getLatestRuleOrder() + 1 + + val draftRules = rules.zipWithIndex.map { case (rule, index) => + val pattern = rule(0) + val replacement = rule(1) + val description = rule(2) + + DbRuleDraft.withUser( + id = None, + ruleType = RuleType.regex, + pattern = Some(pattern), + category = Some("Imported from CSV"), + description = Some(description), + ignore = false, + replacement = Some(replacement), + user = "CSV Import", + ruleOrder = initialRuleOrder + index + ) + } + val ruleIds = DbRuleDraft.batchInsert(draftRules, true) + + // Apply tag + val allTags = Tags.findAll() + allTags.find(tag => tag.name == tagName).map(_.id) match { + case Some(tagId) => + RuleTagDraft.batchInsert(ruleIds.map(id => RuleTagDraft(id, tagId.getOrElse(-1)))) + case None => log.error(s"Tag $tagName not found") + } + + val rulesWithIds = DbRuleDraft.findRules(ruleIds) + val liveRulesWithIds = rulesWithIds.map(_.toLive("Imported from CSV", true)) + + DbRuleLive.batchInsert(liveRulesWithIds) + + publishLiveRules(bucketRuleResource) + + liveRulesWithIds.size + } + object RuleType { val regex = "regex" val languageToolXML = "languageToolXML" diff --git a/apps/rule-manager/conf/routes b/apps/rule-manager/conf/routes index 4a8206c08..545d23d8c 100644 --- a/apps/rule-manager/conf/routes +++ b/apps/rule-manager/conf/routes @@ -21,6 +21,8 @@ GET /api/rules/batch/:ids controllers.RulesController.getRules(ids +nocsrf POST /api/rules/batch controllers.RulesController.batchUpdate() +nocsrf +POST /api/rules/csv-import controllers.RulesController.csvImport() ++nocsrf GET /api/rules/:id controllers.RulesController.get(id: Int) +nocsrf POST /api/rules/:id controllers.RulesController.update(id: Int) diff --git a/build.sbt b/build.sbt index 988cdc3d1..4446b6c62 100644 --- a/build.sbt +++ b/build.sbt @@ -172,6 +172,7 @@ val ruleManager = playProject( "org.scalikejdbc" %% "scalikejdbc-test" % scalikejdbcVersion % Test, "org.scalikejdbc" %% "scalikejdbc-syntax-support-macro" % scalikejdbcVersion, "com.gu" %% "editorial-permissions-client" % "2.14", + "com.github.tototoshi" %% "scala-csv" % "2.0.0", ), libraryDependencySchemes += "org.scala-lang.modules" %% "scala-xml" % VersionScheme.Always ) From 31db89460bf40e45f21381ca89bc7c28485f390c Mon Sep 17 00:00:00 2001 From: Simon Byford Date: Wed, 24 Jul 2024 11:48:02 +0100 Subject: [PATCH 2/9] Allow `category` to be specified in request --- apps/rule-manager/app/controllers/RulesController.scala | 5 +++++ apps/rule-manager/app/service/RuleManager.scala | 9 +++++++-- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/apps/rule-manager/app/controllers/RulesController.scala b/apps/rule-manager/app/controllers/RulesController.scala index 927a614cb..0622d93d6 100644 --- a/apps/rule-manager/app/controllers/RulesController.scala +++ b/apps/rule-manager/app/controllers/RulesController.scala @@ -288,9 +288,14 @@ class RulesController( case Some(tag) => tag.head case None => "" } + category = formData.dataParts.get("category") match { + case Some(category) => category.head + case None => "" + } } yield RuleManager.csvImport( file.ref.path.toFile, tag, + category, bucketRuleResource ) diff --git a/apps/rule-manager/app/service/RuleManager.scala b/apps/rule-manager/app/service/RuleManager.scala index 9731c4934..ddddd7498 100644 --- a/apps/rule-manager/app/service/RuleManager.scala +++ b/apps/rule-manager/app/service/RuleManager.scala @@ -34,7 +34,12 @@ case class AllRuleData( ) object RuleManager extends Loggable { - def csvImport(toFile: File, tagName: String, bucketRuleResource: BucketRuleResource) = { + def csvImport( + toFile: File, + tagName: String, + category: String, + bucketRuleResource: BucketRuleResource + ) = { val reader = CSVReader.open(toFile) val rules = reader.all() @@ -49,7 +54,7 @@ object RuleManager extends Loggable { id = None, ruleType = RuleType.regex, pattern = Some(pattern), - category = Some("Imported from CSV"), + category = Some(category), description = Some(description), ignore = false, replacement = Some(replacement), From 0d18bdf6958b834879c9380382c10d334c0deeb3 Mon Sep 17 00:00:00 2001 From: Simon Byford Date: Wed, 24 Jul 2024 11:58:10 +0100 Subject: [PATCH 3/9] Close resource after use --- apps/rule-manager/app/service/RuleManager.scala | 1 + 1 file changed, 1 insertion(+) diff --git a/apps/rule-manager/app/service/RuleManager.scala b/apps/rule-manager/app/service/RuleManager.scala index ddddd7498..b3909eaa8 100644 --- a/apps/rule-manager/app/service/RuleManager.scala +++ b/apps/rule-manager/app/service/RuleManager.scala @@ -42,6 +42,7 @@ object RuleManager extends Loggable { ) = { val reader = CSVReader.open(toFile) val rules = reader.all() + reader.close() val initialRuleOrder = DbRuleDraft.getLatestRuleOrder() + 1 From 49329d287f7bc72719ef5493a9fa51259b9159b6 Mon Sep 17 00:00:00 2001 From: Simon Byford Date: Wed, 24 Jul 2024 14:18:18 +0100 Subject: [PATCH 4/9] Avoid modelling an absent tag as an empty string --- .../app/controllers/RulesController.scala | 8 +++--- .../app/service/RuleManager.scala | 25 ++++++++++++------- 2 files changed, 20 insertions(+), 13 deletions(-) diff --git a/apps/rule-manager/app/controllers/RulesController.scala b/apps/rule-manager/app/controllers/RulesController.scala index 0622d93d6..c7c77df8e 100644 --- a/apps/rule-manager/app/controllers/RulesController.scala +++ b/apps/rule-manager/app/controllers/RulesController.scala @@ -285,12 +285,12 @@ class RulesController( formData <- request.body.asMultipartFormData.toRight("No form data found in request") file <- formData.file("file").toRight("No file found in request") tag = formData.dataParts.get("tag") match { - case Some(tag) => tag.head - case None => "" + case Some(tag) => Some(tag.head) + case None => None } category = formData.dataParts.get("category") match { - case Some(category) => category.head - case None => "" + case Some(category) => Some(category.head) + case None => None } } yield RuleManager.csvImport( file.ref.path.toFile, diff --git a/apps/rule-manager/app/service/RuleManager.scala b/apps/rule-manager/app/service/RuleManager.scala index b3909eaa8..0b56dd62a 100644 --- a/apps/rule-manager/app/service/RuleManager.scala +++ b/apps/rule-manager/app/service/RuleManager.scala @@ -36,8 +36,8 @@ case class AllRuleData( object RuleManager extends Loggable { def csvImport( toFile: File, - tagName: String, - category: String, + maybeTagName: Option[String], + category: Option[String], bucketRuleResource: BucketRuleResource ) = { val reader = CSVReader.open(toFile) @@ -55,7 +55,7 @@ object RuleManager extends Loggable { id = None, ruleType = RuleType.regex, pattern = Some(pattern), - category = Some(category), + category = category, description = Some(description), ignore = false, replacement = Some(replacement), @@ -65,12 +65,19 @@ object RuleManager extends Loggable { } val ruleIds = DbRuleDraft.batchInsert(draftRules, true) - // Apply tag - val allTags = Tags.findAll() - allTags.find(tag => tag.name == tagName).map(_.id) match { - case Some(tagId) => - RuleTagDraft.batchInsert(ruleIds.map(id => RuleTagDraft(id, tagId.getOrElse(-1)))) - case None => log.error(s"Tag $tagName not found") + maybeTagName match { + case Some(tagName) => + val allTags = Tags.findAll() + allTags.find(tag => tag.name == tagName).map(_.id) match { + case Some(maybeTagId) => + maybeTagId match { + case Some(tagId) => + RuleTagDraft.batchInsert(ruleIds.map(id => RuleTagDraft(id, tagId))) + case None => log.error(s"Tag ${tagName} has no ID") + } + case None => log.error(s"Tag ${tagName} not found") + } + case None => // No tag specified. Do nothing } val rulesWithIds = DbRuleDraft.findRules(ruleIds) From 24d0bf9c30b329dfa9309eccdad81fea98216fd8 Mon Sep 17 00:00:00 2001 From: Simon Byford Date: Wed, 24 Jul 2024 16:18:20 +0100 Subject: [PATCH 5/9] Refactor nested match statements --- .../app/service/RuleManager.scala | 25 +++++++++---------- 1 file changed, 12 insertions(+), 13 deletions(-) diff --git a/apps/rule-manager/app/service/RuleManager.scala b/apps/rule-manager/app/service/RuleManager.scala index 0b56dd62a..9e7363d1d 100644 --- a/apps/rule-manager/app/service/RuleManager.scala +++ b/apps/rule-manager/app/service/RuleManager.scala @@ -65,19 +65,18 @@ object RuleManager extends Loggable { } val ruleIds = DbRuleDraft.batchInsert(draftRules, true) - maybeTagName match { - case Some(tagName) => - val allTags = Tags.findAll() - allTags.find(tag => tag.name == tagName).map(_.id) match { - case Some(maybeTagId) => - maybeTagId match { - case Some(tagId) => - RuleTagDraft.batchInsert(ruleIds.map(id => RuleTagDraft(id, tagId))) - case None => log.error(s"Tag ${tagName} has no ID") - } - case None => log.error(s"Tag ${tagName} not found") - } - case None => // No tag specified. Do nothing + for { + tagName <- maybeTagName + tag <- Tags.findAll().find(_.name == tagName).orElse { + log.error(s"Tag $tagName not found") + None + } + tagId <- tag.id.orElse { + log.error(s"Tag $tagName has no ID") + None + } + } yield { + RuleTagDraft.batchInsert(ruleIds.map(RuleTagDraft(_, tagId))) } val rulesWithIds = DbRuleDraft.findRules(ruleIds) From 4e5d19738273477923ae1100a13657dacbea77db Mon Sep 17 00:00:00 2001 From: Simon Byford Date: Mon, 29 Jul 2024 12:41:57 +0100 Subject: [PATCH 6/9] Add test --- .../test/db/RuleManagerSpec.scala | 44 ++++++++++++++++++- apps/rule-manager/test/resources/csv/mps.csv | 3 ++ 2 files changed, 46 insertions(+), 1 deletion(-) create mode 100644 apps/rule-manager/test/resources/csv/mps.csv diff --git a/apps/rule-manager/test/db/RuleManagerSpec.scala b/apps/rule-manager/test/db/RuleManagerSpec.scala index 114fd134c..1776a7e53 100644 --- a/apps/rule-manager/test/db/RuleManagerSpec.scala +++ b/apps/rule-manager/test/db/RuleManagerSpec.scala @@ -11,7 +11,7 @@ import com.gu.typerighter.model.{ TextSuggestion } import com.gu.typerighter.rules.BucketRuleResource -import db.{DBTest, DbRuleDraft, DbRuleLive} +import db.{DBTest, DbRuleDraft, DbRuleLive, Tags} import org.scalatest.flatspec.FixtureAnyFlatSpec import org.scalatest.matchers.should.Matchers import scalikejdbc.scalatest.AutoRollback @@ -20,6 +20,8 @@ import com.softwaremill.diffx.scalatest.DiffShouldMatcher._ import fixtures.RuleFixtures import play.api.data.FormError import utils.LocalStack + +import java.io.File import java.time.OffsetDateTime class RuleManagerSpec extends FixtureAnyFlatSpec with Matchers with AutoRollback with DBTest { @@ -558,4 +560,44 @@ class RuleManagerSpec extends FixtureAnyFlatSpec with Matchers with AutoRollback .copy(updatedAt = mockUpdatedAt) } } + + "csvImport" should "create draft and live rules based on the contents of the csv file passed in, ensuring the appropriate tag and category are set" in { + () => + val tagToApply = Tags.create(name = "testTag") + + val file = new File(getClass.getResource("/csv/mps.csv").toURI) + + RuleManager.csvImport( + file, + Some("testTag"), + Some("Style guide and names"), + bucketRuleResource + ) + + val draftRules = DbRuleDraft.findAll() + + draftRules.length shouldBe 3 + + val draftRule1 = draftRules(0) + draftRule1.pattern shouldBe Some("Dami(e|a)n Egan") + draftRule1.replacement shouldBe Some("Damien Egan") + draftRule1.description shouldBe Some("MP last elected in 2024: Labour, Bristol North East") + draftRule1.category shouldBe Some("Style guide and names") + + draftRule1.tags.length shouldBe 1 + draftRule1.tags(0) shouldBe tagToApply.get.id.get + + val liveRules = DbRuleLive.findAll() + + liveRules.length shouldBe 3 + + val liveRule1 = liveRules(0) + liveRule1.pattern shouldBe Some("Dami(e|a)n Egan") + liveRule1.replacement shouldBe Some("Damien Egan") + liveRule1.description shouldBe Some("MP last elected in 2024: Labour, Bristol North East") + liveRule1.category shouldBe Some("Style guide and names") + + liveRule1.tags.length shouldBe 1 + liveRule1.tags(0) shouldBe tagToApply.get.id.get + } } diff --git a/apps/rule-manager/test/resources/csv/mps.csv b/apps/rule-manager/test/resources/csv/mps.csv new file mode 100644 index 000000000..4370e87ad --- /dev/null +++ b/apps/rule-manager/test/resources/csv/mps.csv @@ -0,0 +1,3 @@ +Dami(e|a)n Egan,Damien Egan,"MP last elected in 2024: Labour, Bristol North East" +Clive Efford,Clive Efford,"MP last elected in 2024: Labour, Eltham and Chislehurst" +Maya Ellis,Maya Ellis,"MP last elected in 2024: Labour, Ribble Valley" \ No newline at end of file From 51e3dd60612bd8ef2c7fb2f3f11128f398091c77 Mon Sep 17 00:00:00 2001 From: Simon Byford Date: Wed, 31 Jul 2024 10:27:21 +0100 Subject: [PATCH 7/9] Further refactoring --- .../rule-manager/app/controllers/RulesController.scala | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/apps/rule-manager/app/controllers/RulesController.scala b/apps/rule-manager/app/controllers/RulesController.scala index c7c77df8e..c98039d31 100644 --- a/apps/rule-manager/app/controllers/RulesController.scala +++ b/apps/rule-manager/app/controllers/RulesController.scala @@ -284,14 +284,8 @@ class RulesController( val rules = for { formData <- request.body.asMultipartFormData.toRight("No form data found in request") file <- formData.file("file").toRight("No file found in request") - tag = formData.dataParts.get("tag") match { - case Some(tag) => Some(tag.head) - case None => None - } - category = formData.dataParts.get("category") match { - case Some(category) => Some(category.head) - case None => None - } + tag = formData.dataParts.get("tag").flatMap(_.headOption) + category = formData.dataParts.get("category").flatMap(_.headOption) } yield RuleManager.csvImport( file.ref.path.toFile, tag, From 4037bd4b5bfd49dc59b3aef62491d1685648423d Mon Sep 17 00:00:00 2001 From: Simon Byford Date: Wed, 31 Jul 2024 10:34:28 +0100 Subject: [PATCH 8/9] Improve naming --- apps/rule-manager/app/controllers/RulesController.scala | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/apps/rule-manager/app/controllers/RulesController.scala b/apps/rule-manager/app/controllers/RulesController.scala index c98039d31..323d4903a 100644 --- a/apps/rule-manager/app/controllers/RulesController.scala +++ b/apps/rule-manager/app/controllers/RulesController.scala @@ -294,8 +294,8 @@ class RulesController( ) rules match { - case Right(int) => Ok(Json.toJson(int)) - case Left(message) => BadRequest(message) + case Right(noOfRulesAdded) => Ok(Json.toJson(noOfRulesAdded)) + case Left(message) => BadRequest(message) } } } From 29eec024b1f53c51aeb336f26f8aa73dd32d60e0 Mon Sep 17 00:00:00 2001 From: Simon Byford Date: Wed, 31 Jul 2024 10:35:56 +0100 Subject: [PATCH 9/9] Add auth to endpoint --- apps/rule-manager/app/controllers/RulesController.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/rule-manager/app/controllers/RulesController.scala b/apps/rule-manager/app/controllers/RulesController.scala index 323d4903a..78f3a4e90 100644 --- a/apps/rule-manager/app/controllers/RulesController.scala +++ b/apps/rule-manager/app/controllers/RulesController.scala @@ -280,7 +280,7 @@ class RulesController( } } - def csvImport() = Action { implicit request => + def csvImport() = APIAuthAction { implicit request => val rules = for { formData <- request.body.asMultipartFormData.toRight("No form data found in request") file <- formData.file("file").toRight("No file found in request")