Introduce`/api/rules/csv-import` endpoint #477

simonbyford · 2024-07-23T15:39:27Z

What does this change?

Introduces a new endpoint /api/rules/csv-import which enables the bulk import of regex rules via a CSV file. In particular, this will provide a useful method to quickly populate typerighter with the names of incoming MPs after an election.

The endpoint accepts three parameters encoded as form-data:

file (required): the CSV file containing the rules data
tag (optional): a tag to be applied to all imported rules. The tag must already exist in typerighter.
category (optional): a category to be applied to all imported rules. Category is also known as "source" in the UI.

For example:

curl --location 'https://manager.typerighter.gutools.co.uk/api/rules/csv-import' \
--header 'accept: */*' \
--header 'content-type: application/json' \
--form 'file=@"/Users/Simon_Byford/Downloads/mps.csv"' \
--form 'tag="MP"'
--form 'category="Style guide and names"'

Note: this won't actually work for reasons I'll get to..

The CSV file should not contain headers, and the columns must appear in the following order: pattern,replacement,description

For example, the following CSV file is valid:

Diann?e Abbott?,Diane Abbott,"MP last elected in 2024: Labour, Hackney North and Stoke Newington"
Debbie Abrahams,Debbie Abrahams,"MP last elected in 2024: Labour, Oldham East and Saddleworth"
Rebecca Long-? ?Bailey,Rebecca Long-Bailey,"MP last elected in 2024: Labour, Salford"

If the operation is successful, the API will return the number of rules added.

How to test

For the first iteration, we decided not to expose this in the UI, instead the endpoint must be queried manually. This is a bit fiddly because it requires a cookie for authentication. The best way to get around this is to interact with the manager interface (manager.typerighter.gutools.co.uk) - for example, start creating a rule - and inspect any network request to /api/rules. You can then copy the request as cURL:

You can then either make the necessary edits and run it directly on the command line (scary), or import it into a client like Postman:

Images

Using the above method, I tested this on CODE with Max's spreadsheet of 2024 MPs. I used the tag "CSV import" and category "Imported from CSV".

jonathonherbert

Brilliant to see this! One drive-by comment.

apps/rule-manager/app/service/RuleManager.scala

apps/rule-manager/app/controllers/RulesController.scala

jonathonherbert · 2024-07-24T14:31:53Z

One other thing to mention while I'm here – there's a good testing story for DB operations in RuleManagerSpec that should let us test CSV data is written correctly to DB. Could we add a test? (See CAPIFixtures for an example of reading a file from our resources folder, we could add a proper CSV file to our test/resources folder, and be really sure this is doing the right thing ✨)

Very happy to pair!

simonbyford · 2024-07-29T11:46:35Z

One other thing to mention while I'm here – there's a good testing story for DB operations in RuleManagerSpec that should let us test CSV data is written correctly to DB. Could we add a test? (See CAPIFixtures for an example of reading a file from our resources folder, we could add a proper CSV file to our test/resources folder, and be really sure this is doing the right thing ✨)

Very happy to pair!

Hey @jonathonherbert, I've added a test (with your help 😄), does it look okay?

simonbyford · 2024-07-29T15:11:13Z

apps/rule-manager/app/controllers/RulesController.scala

@@ -279,4 +279,29 @@ class RulesController(
      case Left(error) => BadRequest(s"Invalid request: $error")
    }
  }
+
+  def csvImport() = Action { implicit request =>


I think this should be APIAuthAction

Agreed! 😅

jonathonherbert

Yes, that looks great!

Tested locally with the endpoint, all works as expected. Yes, please don't merge before the Action is an APIAuthAction 😀

Two very non-blocking things I spotted whilst re-reviewing.

One broader point – it looks like we can enter identical rules, e.g. spamming the endpoint with repeated requests results in duplicates, which might be an easy mistake to make. We could make this safer with a smoke test on import that checks to see that no rules exactly match the description or pattern of a rule. Perhaps one to follow up with.

(To be fair, arguably the pattern of a rule should have a unique constraint at the DB level. I can't think of a scenario where we'd want two rules to have the same pattern.)

jonathonherbert · 2024-07-29T16:18:18Z

apps/rule-manager/app/controllers/RulesController.scala

+    )
+
+    rules match {
+      case Right(int)    => Ok(Json.toJson(int))


Suggested change

case Right(int) => Ok(Json.toJson(int))

case Right(noOfRulesAdded) => Ok(Json.toJson(noOfRulesAdded))

jonathonherbert · 2024-07-29T16:21:26Z

apps/rule-manager/app/controllers/RulesController.scala

@@ -279,4 +279,29 @@ class RulesController(
      case Left(error) => BadRequest(s"Invalid request: $error")
    }
  }
+
+  def csvImport() = Action { implicit request =>


Agreed! 😅

jonathonherbert · 2024-07-29T16:22:14Z

apps/rule-manager/app/controllers/RulesController.scala

+      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
+      }


I think this can be even neater:

Suggested change

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

}

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").flatMap(_.headOption)

category = formData.dataParts.get("category").flatMap(_.headOption)

Lovely - thank you!

simonbyford · 2024-07-31T09:57:15Z

One broader point – it looks like we can enter identical rules, e.g. spamming the endpoint with repeated requests results in duplicates, which might be an easy mistake to make. We could make this safer with a smoke test on import that checks to see that no rules exactly match the description or pattern of a rule. Perhaps one to follow up with.

Good point, thank you for raising it. Some further work is needed before this can actually be used, namely the introduction of another endpoint to bulk-archive existing rules (based on a tag), so I'll leave this for now

prout-bot · 2024-07-31T10:07:08Z

Seen on Rule Manager (merged by @simonbyford 9 minutes and 30 seconds ago) Please check your changes!

prout-bot · 2024-07-31T10:12:40Z

Overdue on Checker (merged by @simonbyford 15 minutes and 3 seconds ago) What's gone wrong?

jonathonherbert · 2024-07-31T10:24:53Z

This is expected until #468 is merged – any reviews v. welcome 🙏

…

On Wed, 31 Jul 2024 at 11:13, Prout ***@***.***> wrote: Overdue on Checker <https://checker.typerighter.gutools.co.uk/healthcheck> (merged by @simonbyford <https://github.com/simonbyford> 15 minutes and 3 seconds ago) What's gone wrong? — Reply to this email directly, view it on GitHub <#477 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB3IMFYVXDFNZQ77QBNJJX3ZPC2C7AVCNFSM6AAAAABLKWL34GVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENRQGE3DCMZSGM> . You are receiving this because you were mentioned.Message ID: ***@***.***>

-- Jonathon Herbert · he/him Senior Developer, Guardian News and Media ***@***.*** ***@***.***>

----- Kings Place, 90 York Way, London N1 9GU theguardian.com

----- Download the Guardian app for Android <https://play.google.com/store/apps/details?id=com.guardian&hl=en_GB> and iOS <https://itunes.apple.com/gb/app/the-guardian/id409128287?mt=8>

-- This e-mail and all attachments are confidential and may also be privileged. If you are not the named recipient, please notify the sender and delete the e-mail and all attachments immediately. Do not disclose the contents to another person. You may not use the information for any purpose, or store, or copy, it in any way. Guardian News & Media Limited is not liable for any computer viruses or other material transmitted with or as part of this e-mail. You should employ virus checking software. Guardian News & Media Limited is a member of Guardian Media Group plc. Registered Office: PO Box 68164, Kings Place, 90 York Way, London, N1P 2AP. Registered in England Number 908396

simonbyford requested a review from a team as a code owner July 23, 2024 15:39

jonathonherbert reviewed Jul 24, 2024

View reviewed changes

apps/rule-manager/app/service/RuleManager.scala Show resolved Hide resolved

jonathonherbert reviewed Jul 24, 2024

View reviewed changes

apps/rule-manager/app/controllers/RulesController.scala Outdated Show resolved Hide resolved

Introduce /api/rules/csv-import endpoint

fb68260

simonbyford force-pushed the csv-import branch from eb257d5 to dd46453 Compare July 24, 2024 13:22

simonbyford added 4 commits July 24, 2024 16:55

Allow category to be specified in request

31db894

Close resource after use

0d18bdf

Avoid modelling an absent tag as an empty string

49329d2

Refactor nested match statements

24d0bf9

simonbyford force-pushed the csv-import branch from dd46453 to 24d0bf9 Compare July 24, 2024 15:55

Add test

4e5d197

simonbyford commented Jul 29, 2024

View reviewed changes

jonathonherbert approved these changes Jul 29, 2024

View reviewed changes

simonbyford added 3 commits July 31, 2024 10:27

Further refactoring

51e3dd6

Improve naming

4037bd4

Add auth to endpoint

29eec02

simonbyford merged commit cfd0bbf into main Jul 31, 2024
1 check passed

simonbyford deleted the csv-import branch July 31, 2024 09:57

prout-bot added Pending-on-Checker Pending-on-Rule Manager Seen-on-Rule Manager and removed Pending-on-Rule Manager labels Jul 31, 2024

prout-bot added Overdue-on-Checker and removed Pending-on-Checker labels Jul 31, 2024

simonbyford mentioned this pull request Aug 2, 2024

Introduce /api/rules/archive-by-tag endpoint #478

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce`/api/rules/csv-import` endpoint #477

Introduce`/api/rules/csv-import` endpoint #477

simonbyford commented Jul 23, 2024 •

edited

Loading

jonathonherbert left a comment

jonathonherbert commented Jul 24, 2024

simonbyford commented Jul 29, 2024

simonbyford Jul 29, 2024

jonathonherbert Jul 29, 2024

jonathonherbert left a comment

jonathonherbert Jul 29, 2024

jonathonherbert Jul 29, 2024

jonathonherbert Jul 29, 2024

simonbyford Jul 31, 2024

simonbyford commented Jul 31, 2024

prout-bot commented Jul 31, 2024

prout-bot commented Jul 31, 2024

jonathonherbert commented Jul 31, 2024 via email

	case Right(int) => Ok(Json.toJson(int))
	case Right(noOfRulesAdded) => Ok(Json.toJson(noOfRulesAdded))

Introduce/api/rules/csv-import endpoint #477

Introduce/api/rules/csv-import endpoint #477

Conversation

simonbyford commented Jul 23, 2024 • edited Loading

What does this change?

How to test

Images

jonathonherbert left a comment

Choose a reason for hiding this comment

jonathonherbert commented Jul 24, 2024

simonbyford commented Jul 29, 2024

simonbyford Jul 29, 2024

Choose a reason for hiding this comment

jonathonherbert Jul 29, 2024

Choose a reason for hiding this comment

jonathonherbert left a comment

Choose a reason for hiding this comment

jonathonherbert Jul 29, 2024

Choose a reason for hiding this comment

jonathonherbert Jul 29, 2024

Choose a reason for hiding this comment

jonathonherbert Jul 29, 2024

Choose a reason for hiding this comment

simonbyford Jul 31, 2024

Choose a reason for hiding this comment

simonbyford commented Jul 31, 2024

prout-bot commented Jul 31, 2024

prout-bot commented Jul 31, 2024

jonathonherbert commented Jul 31, 2024 via email

Introduce`/api/rules/csv-import` endpoint #477

Introduce`/api/rules/csv-import` endpoint #477

simonbyford commented Jul 23, 2024 •

edited

Loading