From ba5c9d2c8b757cd7de97b3697120c51686e3793b Mon Sep 17 00:00:00 2001
From: Richard Towers <richard.towers@digital.cabinet-office.gov.uk>
Date: Thu, 30 May 2024 11:03:24 +0100
Subject: [PATCH] Update / test remove MP letters task

There were a few edge cases which came up when I ran this task last night:

1) MySQL LIKE statements are case insensitive, so the query found things like "Francis Implementation Team" (as it's got a lower case mp in).
2) Even if it was case sensitive, %MP% would match people with MPhil degrees.
3) The `puts` statement being outside of the if statement caused some confusion - it would log things as updated that matched the query, but not the ruby gsub, even though they hadn't been updated.

I considered making the MySQL query stricter, but then we might actually miss things (like someone with MP in their letters, but separated with a comma, or lower case). Instead, I've just made it clear which ones are being skipped in the logs.

While writing tests for this, I also noticed an edge case where it would replace letters like "CBE MP VC" with "CBEVC" (i.e. it would remove both surrounding spaces from MP). Better to do it with split / join to avoid this I think.
---
 lib/tasks/election.rake                       | 10 +++---
 .../tasks/election_remove_mp_letters_test.rb  | 34 +++++++++++++++++++
 2 files changed, 40 insertions(+), 4 deletions(-)
 create mode 100644 test/unit/lib/tasks/election_remove_mp_letters_test.rb

diff --git a/lib/tasks/election.rake b/lib/tasks/election.rake
index 590c09084ab..5138fc5d63a 100644
--- a/lib/tasks/election.rake
+++ b/lib/tasks/election.rake
@@ -3,12 +3,14 @@ namespace :election do
   task remove_mp_letters: :environment do
     puts "Removing MP from MP's letters:"
     Person.where('letters LIKE "%MP%"').find_each do |person|
-      puts "updating #{person.name}"
-      new_letters = person.letters.gsub(/(^|\s)MP(\s|$)/, "")
-      if person.letters != new_letters
+      new_letters = person.letters.split(" ").reject { _1 == "MP" }.join(" ")
+      if person.letters == new_letters
+        puts "skipped #{person.name} - includes MP (case insensitive), but doesn't match exactly"
+      else
+        old_name = person.name
         person.update!(letters: new_letters)
+        puts "updated #{old_name} to #{person.name}"
       end
-      puts "changed to #{person.name}"
     end
   end
 
diff --git a/test/unit/lib/tasks/election_remove_mp_letters_test.rb b/test/unit/lib/tasks/election_remove_mp_letters_test.rb
new file mode 100644
index 00000000000..d0a03f0af31
--- /dev/null
+++ b/test/unit/lib/tasks/election_remove_mp_letters_test.rb
@@ -0,0 +1,34 @@
+require "test_helper"
+require "rake"
+
+class RemoveMpLettersRake < ActiveSupport::TestCase
+  extend Minitest::Spec::DSL
+
+  teardown { task.reenable }
+
+  describe "#remove_mp_letters" do
+    let(:task) { Rake::Task["election:remove_mp_letters"] }
+
+    it "removes the letters MP from people names" do
+      member_a = create(:person, forename: "A", surname: "Member", letters: "MP")
+      member_b = create(:person, forename: "B", surname: "Member", letters: "CBE MP")
+      member_c = create(:person, forename: "C", surname: "Member", letters: "MP MEng")
+      member_d = create(:person, forename: "D", surname: "Member", letters: "CBE MP MPhil")
+      non_member_a = create(:person, forename: "A", surname: "Non-Member", letters: "CBE MPhil VC")
+
+      out, _err = capture_io { task.invoke }
+
+      assert_match(/updated A Member MP to A Member/, out)
+      assert_match(/updated B Member CBE MP to B Member CBE/, out)
+      assert_match(/updated C Member MP MEng to C Member MEng/, out)
+      assert_match(/updated D Member CBE MP MPhil to D Member CBE MPhil/, out)
+      assert_match(/skipped A Non-Member/, out)
+
+      assert_equal "A Member", member_a.reload.name
+      assert_equal "B Member CBE", member_b.reload.name
+      assert_equal "C Member MEng", member_c.reload.name
+      assert_equal "D Member CBE MPhil", member_d.reload.name
+      assert_equal "A Non-Member CBE MPhil VC", non_member_a.reload.name
+    end
+  end
+end