Clear up typing and keep name not just USN for changes

.gitlab-ci.yml

@@ -20,7 +20,9 @@ variables:
 test:
   artifacts:
     reports:
-      cobertura: ./artefacts/**/coverage.xml
+      coverage_report:
+        coverage_format: cobertura
+        path: ./artefacts/**/coverage.xml
 
 sync:
   stage: deploy

lookupsync/__init__.py

@@ -105,7 +105,7 @@ def _student_inst_members(opts: dict, dry_run: bool):
 
     # Build a map of Lookup group name to sets of students within that institution with status
     # matching career.
-    students_by_group = get_students_by_group(session, inst_map)
+    students_by_group, student_names_by_id = get_students_by_group(session, inst_map)
 
     # Sanity check
     if len(students_by_group) == 0:
@@ -124,7 +124,7 @@ def _student_inst_members(opts: dict, dry_run: bool):
 
     # Calculate changes and find missing groups
     (missing_groups, group_changes) = compare_with_lookup_groups(
-            ibis_group_methods, students_by_group)
+            ibis_group_methods, students_by_group, student_names_by_id)
 
     if missing_groups:
         # Create groups that couldn't be found
@@ -140,4 +140,4 @@ def _student_inst_members(opts: dict, dry_run: bool):
                          previous_count - len(group_changes))
 
     # Make changes to Lookup groups
-    update_lookup_groups(ibis_group_methods, group_changes, dry_run)
+    update_lookup_groups(ibis_group_methods, group_changes, student_names_by_id, dry_run)

lookupsync/inst_mapping.py

-from typing import Dict
 import logging
 import os
 import sys
@@ -6,12 +5,14 @@ import requests_oauthlib
 
 from identitylib.identifiers import Identifier, IdentifierSchemes
 
+from .types import InstMapping
+
 INST_MAPPING_API_ROOT = 'https://api.apps.cam.ac.uk/institutions/mapping/v1/'
 
 LOG = logging.getLogger(os.path.basename(sys.argv[0]))
 
 
-def fetch_inst_mapping(session: requests_oauthlib.OAuth2Session) -> Dict[str, str]:
+def fetch_inst_mapping(session: requests_oauthlib.OAuth2Session) -> InstMapping:
     """
     Fetch institutional mapping dict from API Gateway. Use it to compile a mapping of Student
     Records Institution ids to Lookup instids
@@ -19,7 +20,7 @@ def fetch_inst_mapping(session: requests_oauthlib.OAuth2Session) -> Dict[str, st
     """
     r = session.get(INST_MAPPING_API_ROOT)
     r.raise_for_status()
-    inst_map = {}
+    inst_map: InstMapping = {}
     for datum in r.json().get('institutions', []):
         for i in datum.get('identifiers', []):
             try:

lookupsync/lookup.py

-from typing import Dict, List, Set, Tuple
+from typing import Dict, List, Tuple
 import logging
 import os
 import sys
 import ibisclient
 
-LOG = logging.getLogger(os.path.basename(sys.argv[0]))
+from .types import (GroupChange, GroupChanges, GroupName, GroupSet,
+                    InstIdSet, LookupInstId,
+                    StudentMapping, StudentsByGroup)
 
-# Convenient type definition for group changes dict
-GroupChanges = Dict[str, Dict[str, Set[str]]]
+LOG = logging.getLogger(os.path.basename(sys.argv[0]))
 
 # Make group 'career' suffix to group title suffix
 GROUP_TITLE_MAPPING = {
@@ -50,7 +51,7 @@ def create_lookup_connection(opts: dict) -> ibisclient.IbisClientConnection:
     return ibis_conn
 
 
-def group_name(instid: str, career: str):
+def group_name(instid: LookupInstId, career: str) -> GroupName:
     """
     Create Lookup group name from institution id and academic career mapping
 
@@ -65,7 +66,8 @@ def group_name(instid: str, career: str):
 
 def compare_with_lookup_groups(
         ibis_group_methods: ibisclient.GroupMethods,
-        students_by_group: Dict[str, Set[str]]) -> Tuple[Set[str], GroupChanges]:
+        students_by_group: StudentsByGroup,
+        student_names_by_id: StudentMapping) -> Tuple[GroupSet, GroupChanges]:
     """
     Check each lookup group exists and compare its membership to supplied set.
     Provide set of missing Lookup groups, and necessary changes as a dict mapping
@@ -73,14 +75,14 @@ def compare_with_lookup_groups(
 
     """
     # calculate changes
-    missing_groups = set()
-    group_changes = dict()
+    missing_groups: GroupSet = set()
+    group_changes: GroupChanges = dict()
     for group, students in sorted(students_by_group.items()):
         # Check that group exists
         if ibis_group_methods.getGroup(group) is None:
             missing_groups.add(group)
             # Will want to add everyone after creating the group
-            group_changes[group] = {'add': students, 'remove': set()}
+            group_changes[group] = GroupChange(add=students, remove=set())
             LOG.info('Group "%s" needs creating with %s students', group, len(students))
             continue
 
@@ -89,7 +91,6 @@ def compare_with_lookup_groups(
         # Get lookup direct membership
         members: List[ibisclient.IbisPerson] = ibis_group_methods.getDirectMembers(
             group, 'all_identifiers')
-        LOG.info('- Lookup has %s member(s)', len(members))
 
         # Get set of USNs from membership
         group_usns = {
@@ -98,18 +99,27 @@ def compare_with_lookup_groups(
             for id in person.identifiers if id.scheme == 'usn'
         }
         LOG.info(
-            '- with %s USNs%s', len(group_usns),
+            '- Lookup has %s member(s) with %s USNs%s',
+            len(members), len(group_usns),
             ' - mismatch with membership' if len(group_usns) != len(members) else ''
         )
 
         # Determine who to add and/or remove
         to_add = students - group_usns
-        LOG.info('- %s need adding', len(to_add))
+        if to_add:
+            LOG.info('- %s need adding', len(to_add))
         to_remove = group_usns - students
-        LOG.info('- %s need removing', len(to_remove))
+        if to_remove:
+            LOG.info('- %s need removing', len(to_remove))
+            # Add their names to our id to name mapping
+            student_names_by_id.update({
+                id.value: person.visibleName
+                for person in members if person.identifiers is not None
+                for id in person.identifiers if id.scheme == 'usn' and id.value in to_remove
+            })
 
         if to_add | to_remove:
-            group_changes[group] = {'add': to_add, 'remove': to_remove}
+            group_changes[group] = GroupChange(add=to_add, remove=to_remove)
 
     LOG.info('%s group(s) need creating', len(missing_groups))
     LOG.info('%s group(s) need changes', len(group_changes))
@@ -119,7 +129,8 @@ def compare_with_lookup_groups(
 def update_lookup_groups(
         ibis_group_methods: ibisclient.GroupMethods,
         group_changes: GroupChanges,
-        dry_run: bool = True):
+        student_names_by_id: StudentMapping,
+        dry_run: bool = True) -> None:
     """
     Log and update (if not a dry-run) group memberships
 
@@ -127,9 +138,9 @@ def update_lookup_groups(
     for group, changes in group_changes.items():
         LOG.info('Updating %s:', group)
         for usn in changes['add']:
-            LOG.info('- adding usn/%s', usn)
+            LOG.info('- adding usn/%s (%s)', usn, student_names_by_id.get(usn, 'Unknown'))
         for usn in changes['remove']:
-            LOG.info('- removing usn/%s', usn)
+            LOG.info('- removing usn/%s (%s)', usn, student_names_by_id.get(usn, 'Unknown'))
         if dry_run:
             LOG.info('- skipping update in dry-run mode')
         else:
@@ -143,8 +154,8 @@ def update_lookup_groups(
 
 def create_lookup_groups(
         ibis_inst_methods: ibisclient.InstitutionMethods,
-        groups_to_create: Set[str],
-        dry_run: bool = True) -> Set[str]:
+        groups_to_create: GroupSet,
+        dry_run: bool = True) -> InstIdSet:
     """
     Log and create (if not a dry-run) lookup groups with names, titles, descriptions
     and the current lookup account as the only manager.
@@ -154,9 +165,9 @@ def create_lookup_groups(
     # Get current authenticated lookup account
     managed_by = ibis_inst_methods.conn.username
     # Cache institution details so we can name, title and describe groups appropriately
-    institutions: Dict[str, ibisclient.IbisInstitution] = {}
+    institutions: Dict[LookupInstId, ibisclient.IbisInstitution] = {}
     # Compile a set of institutions we couldn't find
-    missing_insts = set()
+    missing_insts: InstIdSet = set()
 
     for group in sorted(groups_to_create):
         # split group name into instid and career (ug or pg)
@@ -188,7 +199,7 @@ def create_lookup_groups(
 
 
 def strip_groups_missing_insts(
-        group_changes: GroupChanges, missing_insts: Set[str]) -> GroupChanges:
+        group_changes: GroupChanges, missing_insts: InstIdSet) -> GroupChanges:
     """
     Remove group changes that would belong to institutions that couldn't be found.
 

lookupsync/student_api.py

-from typing import Dict, Generator, List, Optional, Set
+from typing import Generator, List, Optional, Tuple
 import logging
 import os
 import sys
@@ -9,6 +9,7 @@ import urllib.parse
 import requests_oauthlib
 from identitylib.identifiers import IdentifierSchemes
 
+from .types import InstMapping, StudentMapping, StudentsByGroup
 from .lookup import group_name
 
 STUDENT_API_ROOT = 'https://api.apps.cam.ac.uk/university-student/v1alpha2/'
@@ -22,7 +23,8 @@ LOG = logging.getLogger(os.path.basename(sys.argv[0]))
 
 
 def get_students_by_group(
-        session: requests_oauthlib.OAuth2Session, inst_map: Dict[str, str]) -> Dict[str, Set[str]]:
+        session: requests_oauthlib.OAuth2Session,
+        inst_map: InstMapping) -> Tuple[StudentsByGroup, StudentMapping]:
     """
     Create a map from Lookup group name to sets of students within that institution with status
     matching career. Group names are formed from the Lookup instids and student identifiers
@@ -31,8 +33,11 @@ def get_students_by_group(
     Note that since students can be members of more than one institution, the sum of the lengths
     of each sets may not equal the length of the union of all of the sets.
 
+    Also create mapping for student ids to names for logging.
+
     """
-    students_by_group = {}
+    students_by_group: StudentsByGroup = {}
+    student_names_by_id: StudentMapping = {}
 
     # Capture ignored affiliations and careers
     ignored_affiliations = set()
@@ -85,8 +90,9 @@ def get_students_by_group(
                 if id_scheme != IdentifierSchemes.USN:
                     continue
 
-                # Add USN to group
+                # Add USN and remember name for USN
                 student_ids.add(i.value)
+                student_names_by_id[i.value] = f'{s.forenames} {s.surname}'
 
     # Report ignored values (possibly missing from inst mapping or career mapping above)
     if ignored_affiliations:
@@ -98,7 +104,7 @@ def get_students_by_group(
         for a in sorted(ignored_careers):
             LOG.info(f'- {a}')
 
-    return students_by_group
+    return students_by_group, student_names_by_id
 
 
 # Student API schema

lookupsync/tests/test_lookup.py

+import logging
 from typing import Dict, List
 from unittest.mock import MagicMock, patch
 import pytest
@@ -105,7 +106,21 @@ def mock_students_by_group():
 
 
 @pytest.fixture
-def mock_lookup_group_members(mock_students_by_group):
+def mock_student_names_by_id():
+    """
+    An example dict of student ids to their visible names
+    """
+    return {
+        '123456789': 'Emil Tatton',
+        '234567890': 'Pen Goddard',
+        '345678901': 'Lucia Bennington',
+        '456789012': 'Lynette Massey',
+        '567890123': 'Dorthy Adams',
+    }
+
+
+@pytest.fixture
+def mock_lookup_group_members(mock_students_by_group, mock_student_names_by_id):
     """
     An example dict of Lookup groups to list of ibisclient.IbisPerson records matching
     students_by_group mock
@@ -116,6 +131,7 @@ def mock_lookup_group_members(mock_students_by_group):
         setattr(identifier, 'scheme', 'usn')
         setattr(identifier, 'value', usn)
         setattr(person, 'identifiers', [identifier])
+        setattr(person, 'visibleName', mock_student_names_by_id.get(usn))
         return person
 
     return {
@@ -149,7 +165,7 @@ def test_compare_with_lookup_groups(mock_students_by_group, mock_lookup_group_me
     # Perform comparison
     mock_group_methods = MockGroupMethods(mock_lookup_group_members)
     missing_groups, group_changes = compare_with_lookup_groups(
-        mock_group_methods, mock_students_by_group)
+        mock_group_methods, mock_students_by_group, {})
 
     # Empty results
     assert missing_groups == set()
@@ -172,7 +188,7 @@ def test_compare_with_lookup_groups_missing_group(
     # Perform comparison
     mock_group_methods = MockGroupMethods(mock_lookup_group_members)
     missing_groups, group_changes = compare_with_lookup_groups(
-        mock_group_methods, mock_students_by_group)
+        mock_group_methods, mock_students_by_group, {})
 
     # Just new group reported missing and all members appear in changes to be added
     assert missing_groups == {NEW_GROUP}
@@ -180,7 +196,7 @@ def test_compare_with_lookup_groups_missing_group(
 
 
 def test_compare_with_lookup_groups_changes(
-        mock_students_by_group, mock_lookup_group_members):
+        mock_students_by_group, mock_lookup_group_members, mock_student_names_by_id):
     """
     Group changes to add and/or remove members are reported
 
@@ -189,31 +205,35 @@ def test_compare_with_lookup_groups_changes(
     GROUP_TO_REMOVE_ONLY_FROM = 'foo-sis-pg'
     GROUP_TO_ADD_TO_AND_REMOVE_FROM = 'bar-sis-ug'
     USN_TO_ADD = '999999999'
+    NAME_TO_ADD = 'Kacie Elwyn'
     USN_TO_REMOVE = '456789012'
+    NAME_TO_REMOVE = 'Lynette Massey'
 
     # Assert groups exist
     assert GROUP_TO_ADD_ONLY_TO in mock_students_by_group
     assert GROUP_TO_REMOVE_ONLY_FROM in mock_students_by_group
     assert GROUP_TO_ADD_TO_AND_REMOVE_FROM in mock_students_by_group
 
-    # Assert USNs to add don't already exist in appropriate groups
+    # Assert USN student to add don't already exist in appropriate groups
     assert USN_TO_ADD not in mock_students_by_group[GROUP_TO_ADD_ONLY_TO]
     assert USN_TO_ADD not in mock_students_by_group[GROUP_TO_ADD_TO_AND_REMOVE_FROM]
 
-    # Assert USNs to remove do already exist in appropriate groups
+    # Assert USN of student to remove does already exist in appropriate groups
     assert USN_TO_REMOVE in mock_students_by_group[GROUP_TO_REMOVE_ONLY_FROM]
     assert USN_TO_REMOVE in mock_students_by_group[GROUP_TO_ADD_TO_AND_REMOVE_FROM]
 
     # Make changes
+    mock_student_names_by_id[USN_TO_ADD] = NAME_TO_ADD
     mock_students_by_group[GROUP_TO_ADD_ONLY_TO].add(USN_TO_ADD)
     mock_students_by_group[GROUP_TO_ADD_TO_AND_REMOVE_FROM].add(USN_TO_ADD)
+    del mock_student_names_by_id[USN_TO_REMOVE]
     mock_students_by_group[GROUP_TO_REMOVE_ONLY_FROM].remove(USN_TO_REMOVE)
     mock_students_by_group[GROUP_TO_ADD_TO_AND_REMOVE_FROM].remove(USN_TO_REMOVE)
 
     # Perform comparison
     mock_group_methods = MockGroupMethods(mock_lookup_group_members)
     missing_groups, group_changes = compare_with_lookup_groups(
-        mock_group_methods, mock_students_by_group)
+        mock_group_methods, mock_students_by_group, mock_student_names_by_id)
 
     # No missing groups but appropriate changes reported
     assert missing_groups == set()
@@ -222,9 +242,11 @@ def test_compare_with_lookup_groups_changes(
         GROUP_TO_REMOVE_ONLY_FROM: {'add': set(), 'remove': {USN_TO_REMOVE}},
         GROUP_TO_ADD_TO_AND_REMOVE_FROM: {'add': {USN_TO_ADD}, 'remove': {USN_TO_REMOVE}},
     }
+    # Student to remove has their name mapped from their USN
+    assert mock_student_names_by_id.get(USN_TO_REMOVE) == NAME_TO_REMOVE
 
 
-def test_update_lookup_groups():
+def test_update_lookup_groups(caplog):
     """
     update_lookup_groups calls updateDirectMembers method of ibisclient.GroupMethods instance
     for each group with matching changes
@@ -236,18 +258,20 @@ def test_update_lookup_groups():
         'foo-sis-pg': {'add': set(), 'remove': {'333', '444'}},
         'bar-sis-ug': {'add': {'555', '666'}, 'remove': {'777', '888'}},
     }
+    NAMES_BY_ID = {str(x)*3: f'Name {x}' for x in range(1, 9)}
 
     mock_group_methods = MockGroupMethods()
 
     # Make update calls (dry-run)
-    update_lookup_groups(mock_group_methods, CHANGES, True)
+    update_lookup_groups(mock_group_methods, CHANGES, NAMES_BY_ID, True)
 
     # No updateDirectMembers calls made
     assert mock_group_methods.updates == {}
 
     # Make update calls (not dry-run)
     mock_group_methods = MockGroupMethods()
-    update_lookup_groups(mock_group_methods, CHANGES, False)
+    with caplog.at_level(logging.INFO):
+        update_lookup_groups(mock_group_methods, CHANGES, NAMES_BY_ID, False)
 
     # Each group was updated
     assert set(CHANGES.keys()) == set(mock_group_methods.updates.keys())
@@ -263,6 +287,14 @@ def test_update_lookup_groups():
         assert expected_to_add == set(mock_group_methods.updates[group]['to_add'])
         assert expected_to_remove == set(mock_group_methods.updates[group]['to_remove'])
 
+    # additions and removals logged with names
+    all_expected_to_add = {usn for usn in changes['add'] for _, changes in CHANGES.items()}
+    for usn in all_expected_to_add:
+        assert f'adding usn/{usn} ({NAMES_BY_ID.get(usn)})' in caplog.text
+    all_expected_to_remove = {usn for usn in changes['remove'] for _, changes in CHANGES.items()}
+    for usn in all_expected_to_remove:
+        assert f'removing usn/{usn} ({NAMES_BY_ID.get(usn)})' in caplog.text
+
 
 # Mock ibisclient.InstitutionMethods
 class MockInstitutionMethods:

lookupsync/tests/test_student_api.py

@@ -142,7 +142,7 @@ class StudentAPITest(TestCase):
 
         # get students by groups with list of fake students in mock response
         session = MockSession(self._students_to_responses(fake_students))
-        students_by_group = get_students_by_group(session, INST_MAPPING)
+        students_by_group, student_names_by_id = get_students_by_group(session, INST_MAPPING)
 
         # Expect groups for all status at single lookup inst match keys of result
         lookup_inst = INST_MAPPING.get(SR_INST_VALUE)
@@ -155,6 +155,7 @@ class StudentAPITest(TestCase):
         # Expect all students to be returned (being only in a single group)
         count = sum([len(s) for s in students_by_group.values()])
         self.assertEqual(count, NUMBER_STUDENTS)
+        self.assertEqual(len(student_names_by_id), NUMBER_STUDENTS)
 
     def test_get_students_by_group_multiple_affiliations(self):
         """
@@ -163,6 +164,8 @@ class StudentAPITest(TestCase):
         """
         # Single student
         STUDENT_USN = '123456789'
+        FORENAMES = 'John'
+        SURNAME = 'Smith'
         identifier_id = f'{STUDENT_USN}@{IdentifierSchemes.USN}'
         # in multiple distinct lookup institutions
         AFFILIATION_ID_VALUES = {'SRID1', 'SRID3', 'SRID4'}
@@ -176,12 +179,14 @@ class StudentAPITest(TestCase):
         ]
         fake_student = self.fake.student(
             identifier_id=identifier_id,
-            affiliations=affiliations
+            affiliations=affiliations,
+            surname=SURNAME,
+            forenames=FORENAMES,
         )
 
         # get students by groups with just fake student in mock response
         session = MockSession(self._students_to_responses([fake_student]))
-        students_by_group = get_students_by_group(session, INST_MAPPING)
+        students_by_group, student_names_by_id = get_students_by_group(session, INST_MAPPING)
 
         # Expect groups for all status at single lookup inst match keys of result
         expected_groups = {
@@ -191,9 +196,11 @@ class StudentAPITest(TestCase):
         self.assertEqual(expected_groups, set(students_by_group.keys()))
 
         # The student is the single member of each list
-        for group, students in students_by_group.items():
+        for _, students in students_by_group.items():
             self.assertEqual(len(students), 1)
             self.assertEqual(students, {STUDENT_USN})
+        # and their name is mapped to USN
+        self.assertEqual(student_names_by_id.get(STUDENT_USN), f'{FORENAMES} {SURNAME}')
 
     def test_get_students_by_group_depreciated(self):
         """
@@ -203,6 +210,8 @@ class StudentAPITest(TestCase):
         """
         # Single student with depreciated USN scheme
         STUDENT_USN = '123456789'
+        FORENAMES = 'John'
+        SURNAME = 'Smith'
         identifier_id = f'{STUDENT_USN}@person.camsis.identifiers.admin.cam.ac.uk'
         # and affiliation with depreciated institution scheme
         AFFILIATION_VALUE = 'SRID3'
@@ -212,16 +221,20 @@ class StudentAPITest(TestCase):
         fake_student = self.fake.student(
             identifier_id=identifier_id,
             affiliation_id=affiliation_id,
+            surname=SURNAME,
+            forenames=FORENAMES,
             status='PGRD',  # as a postgraduate
         )
 
         # get students by groups with just fake student in mock response
         session = MockSession(self._students_to_responses([fake_student]))
-        students_by_group = get_students_by_group(session, INST_MAPPING)
+        students_by_group, student_names_by_id = get_students_by_group(session, INST_MAPPING)
 
         # Expect just single lookup group with single matching student
         expected_group = group_name(lookup_id, 'pg')
         self.assertEqual({expected_group: {STUDENT_USN}}, students_by_group)
+        # and their name is mapped to USN
+        self.assertEqual(student_names_by_id.get(STUDENT_USN), f'{FORENAMES} {SURNAME}')
 
     def test_get_students_by_group_no_aff(self):
         """
@@ -238,10 +251,11 @@ class StudentAPITest(TestCase):
 
         # get students by groups with just fake student in mock response
         session = MockSession(self._students_to_responses([fake_student]))
-        students_by_group = get_students_by_group(session, INST_MAPPING)
+        students_by_group, student_names_by_id = get_students_by_group(session, INST_MAPPING)
 
         # Expect empty result
         self.assertEqual({}, students_by_group)
+        self.assertEqual({}, student_names_by_id)
 
     def test_get_students_by_group_invalid_aff(self):
         """
@@ -258,10 +272,11 @@ class StudentAPITest(TestCase):
 
         # get students by groups with just fake student in mock response
         session = MockSession(self._students_to_responses([fake_student]))
-        students_by_group = get_students_by_group(session, INST_MAPPING)
+        students_by_group, student_names_by_id = get_students_by_group(session, INST_MAPPING)
 
         # Expect empty result
         self.assertEqual({}, students_by_group)
+        self.assertEqual({}, student_names_by_id)
 
     def test_get_students_by_group_no_usn(self):
         """
@@ -280,11 +295,12 @@ class StudentAPITest(TestCase):
 
         # get students by groups with just fake student in mock response
         session = MockSession(self._students_to_responses([fake_student]))
-        students_by_group = get_students_by_group(session, INST_MAPPING)
+        students_by_group, student_names_by_id = get_students_by_group(session, INST_MAPPING)
 
         # Expect result with single group but no students
         expected_group = group_name(lookup_id, 'pg')
         self.assertEqual({expected_group: set()}, students_by_group)
+        self.assertEqual({}, student_names_by_id)
 
     def test_get_students_by_group_invalid_usn(self):
         """
@@ -304,11 +320,12 @@ class StudentAPITest(TestCase):
 
         # get students by groups with just fake student in mock response
         session = MockSession(self._students_to_responses([fake_student]))
-        students_by_group = get_students_by_group(session, INST_MAPPING)
+        students_by_group, student_names_by_id = get_students_by_group(session, INST_MAPPING)
 
         # Expect result with single group but no students
         expected_group = group_name(lookup_id, 'pg')
         self.assertEqual({expected_group: set()}, students_by_group)
+        self.assertEqual({}, student_names_by_id)
 
     def test_get_students_by_group_periods(self):
         """
@@ -318,6 +335,8 @@ class StudentAPITest(TestCase):
         """
         # Student with multiple affiliation
         STUDENT_USN = '123456789'
+        FORENAMES = 'John'
+        SURNAME = 'Smith'
         identifier_id = f'{STUDENT_USN}@{IdentifierSchemes.USN}'
         AFFILIATION_ID_VALUES = ['SRID1', 'SRID3', 'SRID4']
         lookup_id_values = [INST_MAPPING.get(v) for v in AFFILIATION_ID_VALUES]
@@ -337,16 +356,21 @@ class StudentAPITest(TestCase):
         ]
         fake_student = self.fake.student(
             identifier_id=identifier_id,
-            affiliations=affiliations
+            affiliations=affiliations,
+            surname=SURNAME,
+            forenames=FORENAMES,
         )
+        print(fake_student)
+        print(self._students_to_responses([fake_student]))
 
         # get students by groups with just fake student in mock response
         session = MockSession(self._students_to_responses([fake_student]))
-        students_by_group = get_students_by_group(session, INST_MAPPING)
-
+        students_by_group, student_names_by_id = get_students_by_group(session, INST_MAPPING)
         # Expect just one group (for current affiliation) with the student
         expected_group = group_name(lookup_id_values[1], 'ug')
         self.assertEqual({expected_group: {STUDENT_USN}}, students_by_group)
+        # and their name is mapped to USN
+        self.assertEqual(student_names_by_id.get(STUDENT_USN), f'{FORENAMES} {SURNAME}')
 
     def test_get_students_by_group_ignored(self):
         """
@@ -355,6 +379,8 @@ class StudentAPITest(TestCase):
         """
         # Student with multiple affiliation (last one unmappable)
         STUDENT_USN = '123456789'
+        FORENAMES = 'John'
+        SURNAME = 'Smith'
         identifier_id = f'{STUDENT_USN}@{IdentifierSchemes.USN}'
         AFFILIATION_ID_VALUES = ['SRID1', 'SRID3', 'BADAFF']
         lookup_id_values = [INST_MAPPING.get(v) for v in AFFILIATION_ID_VALUES]
@@ -370,17 +396,21 @@ class StudentAPITest(TestCase):
         ]
         fake_student = self.fake.student(
             identifier_id=identifier_id,
-            affiliations=affiliations
+            affiliations=affiliations,
+            surname=SURNAME,
+            forenames=FORENAMES,
         )
 
         # get students by groups with just fake student in mock response
         session = MockSession(self._students_to_responses([fake_student]))
         with self.assertLogs(level=logging.INFO) as captured:
-            students_by_group = get_students_by_group(session, INST_MAPPING)
+            students_by_group, student_names_by_id = get_students_by_group(session, INST_MAPPING)
 
         # Result contains just mappable affiliation (the first)
         expected_group = group_name(lookup_id_values[0], career_mappings[0])
         self.assertEqual({expected_group: {STUDENT_USN}}, students_by_group)
+        # and their name is mapped to USN
+        self.assertEqual(student_names_by_id.get(STUDENT_USN), f'{FORENAMES} {SURNAME}')