fix: county field (#2229)

* fix: school.country is optional

* remove postcode and make name unique

* all uk counties

* add choices

* make county field work

* add county options

* remove postcode

* hide/show county field

* show/hide county field pt.2

* merge from master

* remove redundant test

* fix: create postcode

* fix assertion

* feedback

* Merge branch 'master' into county_field

* ignore .venv

.gitignore

@@ -37,3 +37,5 @@ node_modules
 
 # tunnel software
 ngrok
+
+.venv

cfl_common/common/migrations/0047_school_location.py

@@ -0,0 +1,36 @@
+# Generated by Django 3.2.20 on 2023-11-06 18:56
+
+from django.apps.registry import Apps
+from django.db import migrations, models
+
+
+def unique_school_names(apps: Apps, *args):
+    School = apps.get_model("common", "School")
+
+    schools = School.objects.values("name").annotate(name_count=models.Count("name")).filter(name_count__gt=1)
+    for school in schools:
+        schools = list(School.objects.filter(name=school["name"]).order_by("id"))
+        for index in range(school["name_count"]):
+            if index > 0:
+                school = schools[index]
+                school.name += f" {index + 1}"
+                school.save()
+
+
+class Migration(migrations.Migration):
+    dependencies = [
+        ("common", "0046_alter_school_country"),
+    ]
+
+    operations = [
+        migrations.RemoveField(
+            model_name="school",
+            name="postcode",
+        ),
+        migrations.RunPython(code=unique_school_names),
+        migrations.AlterField(
+            model_name="school",
+            name="name",
+            field=models.CharField(max_length=200, unique=True),
+        ),
+    ]

cfl_common/common/models.py

@@ -8,8 +8,6 @@
 from django.utils import timezone
 from django_countries.fields import CountryField
 
-from .helpers.organisation import sanitise_uk_postcode
-
 
 class UserProfile(models.Model):
     user = models.OneToOneField(User, on_delete=models.CASCADE)
@@ -40,8 +38,7 @@ def get_queryset(self):
 
 
 class School(models.Model):
-    name = models.CharField(max_length=200)
-    postcode = models.CharField(max_length=10, null=True)
+    name = models.CharField(max_length=200, unique=True)
     country = CountryField(blank_label="(select country)", null=True, blank=True)
     # TODO: Create an Address model to house address details
     county = models.CharField(max_length=50, blank=True, null=True)
@@ -69,22 +66,9 @@ def admins(self):
 
     def anonymise(self):
         self.name = uuid4().hex
-        self.postcode = ""
         self.is_active = False
         self.save()
 
-    def save(self, **kwargs):
-        self.county = ""
-
-        if self.country == "GB":
-            if self.postcode.replace(" ", "") == "":
-                self.county = "nan"
-            else:
-                nomi = pgeocode.Nominatim("GB")
-                self.county = nomi.query_postal_code(sanitise_uk_postcode(self.postcode)).county_name
-
-        super(School, self).save(**kwargs)
-
 
 class TeacherModelManager(models.Manager):
     def factory(self, first_name, last_name, email, password):

cfl_common/common/tests/test_models.py

@@ -1,12 +1,12 @@
-from common.models import Student, Teacher, DailyActivity
+from common.models import DailyActivity, Student, Teacher
 from django.test import TestCase
 from django.utils import timezone
 
+from ..helpers.organisation import sanitise_uk_postcode
 from .utils.classes import create_class_directly
 from .utils.organisation import create_organisation_directly, join_teacher_to_organisation
 from .utils.student import create_independent_student_directly
 from .utils.teacher import signup_teacher_directly
-from ..helpers.organisation import sanitise_uk_postcode
 
 
 class TestModels(TestCase):
@@ -57,8 +57,8 @@ def test_school_admins(self):
         email2, password2 = signup_teacher_directly()
         email3, password3 = signup_teacher_directly()
         school = create_organisation_directly(email1)
-        join_teacher_to_organisation(email2, school.name, school.postcode)
-        join_teacher_to_organisation(email3, school.name, school.postcode, is_admin=True)
+        join_teacher_to_organisation(email2, school.name)
+        join_teacher_to_organisation(email3, school.name, is_admin=True)
 
         teacher1 = Teacher.objects.get(new_user__username=email1)
         teacher2 = Teacher.objects.get(new_user__username=email2)
@@ -69,28 +69,6 @@ def test_school_admins(self):
         assert teacher2 not in school.admins()
         assert teacher3 in school.admins()
 
-    def test_school_postcode(self):
-        """
-        Test that the school's county field gets populated according to country and postcode.
-        """
-        # Check that the county is found correctly (create school helper function uses Herts postcode)
-        teacher_email, _ = signup_teacher_directly()
-        school = create_organisation_directly(teacher_email)
-
-        assert school.county == "Hertfordshire"
-
-        # Check that an empty postcode outputs "nan" as county and doesn't throw an exception
-        school.postcode = " "
-        school.save()
-
-        assert school.county == "nan"
-
-        # Check that changing the country to something other than "GB" clears the county field
-        school.country = "FR"
-        school.save()
-
-        assert school.county == ""
-
     def test_sanitise_uk_postcode(self):
         postcode_with_space = "AL10 9NE"
         postcode_without_space = "AL109UL"

cfl_common/common/tests/utils/organisation.py

@@ -1,22 +1,21 @@
-from common.models import Teacher, School
+from common.models import School, Teacher
 
 
 def generate_details(**kwargs):
     name = kwargs.get("name", "School %d" % generate_details.next_id)
-    postcode = kwargs.get("postcode", "Al10 9NE")
 
     generate_details.next_id += 1
 
-    return name, postcode
+    return name
 
 
 generate_details.next_id = 1
 
 
 def create_organisation_directly(teacher_email, **kwargs):
-    name, postcode = generate_details(**kwargs)
+    name = generate_details(**kwargs)
 
-    school = School.objects.create(name=name, postcode=postcode, country="GB")
+    school = School.objects.create(name=name, country="GB")
 
     teacher = Teacher.objects.get(new_user__email=teacher_email)
     teacher.school = school
@@ -26,18 +25,17 @@ def create_organisation_directly(teacher_email, **kwargs):
     return school
 
 
-def join_teacher_to_organisation(teacher_email, org_name, postcode, is_admin=False):
+def join_teacher_to_organisation(teacher_email, org_name, is_admin=False):
     teacher = Teacher.objects.get(new_user__email=teacher_email)
-    school = School.objects.get(name=org_name, postcode=postcode)
+    school = School.objects.get(name=org_name)
 
     teacher.school = school
     teacher.is_admin = is_admin
     teacher.save()
 
 
 def create_organisation(page, password):
+    name = generate_details()
+    page = page.create_organisation(name, password)
 
-    name, postcode = generate_details()
-    page = page.create_organisation(name, password, postcode)
-
-    return page, name, postcode
+    return page, name

portal/admin.py

@@ -2,14 +2,14 @@
 
 from common.models import (
     Class,
+    DailyActivity,
+    DynamicElement,
     School,
     SchoolTeacherInvitation,
     Student,
     Teacher,
-    UserProfile,
-    DynamicElement,
-    DailyActivity,
     TotalActivity,
+    UserProfile,
 )
 from django.contrib import admin
 from django.contrib.auth.admin import UserAdmin
@@ -31,9 +31,9 @@ def teacher_school(self, obj):
 
 
 class SchoolAdmin(admin.ModelAdmin, ExportActionMixin):
-    search_fields = ["name", "country", "postcode", "county"]
-    list_filter = ["county", "postcode", "country"]
-    list_display = ["__str__", "country", "county", "postcode", "number_of_teachers", "number_of_classes"]
+    search_fields = ["name", "country", "county"]
+    list_filter = ["county", "country"]
+    list_display = ["__str__", "country", "county", "number_of_teachers", "number_of_classes"]
 
     def number_of_teachers(self, obj):
         return len(obj.teacher_school.all())