From ef72f1db6fa1d14e623ad71f388a00f4bc78b8bf Mon Sep 17 00:00:00 2001
From: Robin Goodall <rjg21@cam.ac.uk>
Date: Mon, 29 Nov 2021 16:11:33 +0000
Subject: [PATCH 1/6] Fix Token endpoint and flatten 'api/' usage

Use '/api/Token' instead of '/Token'.
All endpoints now under '/api' so that can move to configuration and
not need explicitly specifying.
Also, it uses booker.api.email instead of username
---
 configuration-example.yaml          |  4 ++--
 essmsync/booker.py                  | 18 ++++++++++--------
 essmsync/state.py                   |  2 +-
 essmsync/tests/fixtures/config.yaml |  4 ++--
 essmsync/tests/test_booker.py       | 26 ++++++++++++++++----------
 secrets-example.yaml                |  2 +-
 6 files changed, 32 insertions(+), 24 deletions(-)

diff --git a/configuration-example.yaml b/configuration-example.yaml
index 38f8517..d85917e 100644
--- a/configuration-example.yaml
+++ b/configuration-example.yaml
@@ -27,9 +27,9 @@ termtime:
 booker:
   api:
     # Endpoint of Booker API (required)
-    url: "https://domain/"
+    url: "https://domain/api/"
     # Credentials for authenticating to Booker Token endpoint to get API key
-    username: "essm-sync@domain"
+    email: "essm-sync@domain"
     password: "SECRET"
 
   # Type of booking to create in Booker for events synced from TermTime (defaults
diff --git a/essmsync/booker.py b/essmsync/booker.py
index 8ccf60f..6be6f75 100644
--- a/essmsync/booker.py
+++ b/essmsync/booker.py
@@ -70,14 +70,16 @@ def auth():
     # Only request an access token if we don't already have one
     if not state.get('booker.api.access_token'):
         (token_resp, _) = booker_post(
+            # v1.21 moves endpoint to 'api/Token' so no need for 'api/' in all
+            # other endpoint usage, just append it to `booker.api.url` in configuration
             'Token',
             # Don't pass auth token as that's what we're trying to get
             with_auth=False,
             is_update=False,
             # Token endpoint only takes form-data not json!
             data={
-                'grant_type': 'password',
-                'username': state.get('booker.api.username'),
+                # v1.21 requires 'email' instead of 'username'
+                'email': state.get('booker.api.email'),
                 'password': state.get('booker.api.password')
             }
         )
@@ -91,7 +93,7 @@ def auth():
         LOG.info('Booker API access token already provided')
 
     # No ping to validate access_token (so use roomtypes as a low impact)
-    (roomtypes, _) = booker_get('api/roomtypes')
+    (roomtypes, _) = booker_get('roomtypes')
     # Should have a non-empty list back
     if not isinstance(roomtypes, list) or len(roomtypes) == 0:
         LOG.error('Booker API authentication failed')
@@ -105,7 +107,7 @@ def _op_get_list(endpoint, target, overwrite=True, **kwargs):
     `booker.<target>`
 
     """
-    (l, _) = booker_get(f"api/{endpoint}", **kwargs)
+    (l, _) = booker_get(f"{endpoint}", **kwargs)
     if not isinstance(l, list):
         LOG.error(f'Booker {endpoint} result is not a list')
         raise BookerAPIError()
@@ -405,7 +407,7 @@ def op_update_events():
         if params.get('BookedBy') is not None:
             LOG.info(f"   Booked By  : '{params['BookedBy']['Id']}'")
         try:
-            booker_put(f"api/booker/booking/{booking['OptimeIndex']}", json=params)
+            booker_put(f"booker/booking/{booking['OptimeIndex']}", json=params)
         except BookerAPIBookingConflict:
             # hopefully another update will remove the conflict and this will succeed next time
             LOG.warning(f"Failed to create event '{code}' due to conflict")
@@ -423,7 +425,7 @@ def op_update_events():
         LOG.info(f"   Building OI: '{params['BuildingId']}'")
         LOG.info(f"   Title      : '{params['Title']}'")
         try:
-            booker_post('api/booker/booking', json=params)
+            booker_post('booker/booking', json=params)
         except BookerAPIBookingConflict:
             # hopefully an update will remove the conflict and this will succeed next time
             LOG.warning(f"Failed to create event '{code}' due to conflict")
@@ -462,7 +464,7 @@ def op_cancel_events():
         LOG.info(f"   Building OI: '{params['BuildingId']}'")
         LOG.info(f"   Title      : '{params['Title']}'")
 
-        booker_put(f"api/booker/booking/{booking['OptimeIndex']}", json=params)
+        booker_put(f"booker/booking/{booking['OptimeIndex']}", json=params)
 
 
 def op_find_conflicts():
@@ -557,7 +559,7 @@ def _get_bookings(start=None, end=None, filter=None):
     # calendar/events endpoint needs a list of RoomIds (actually OptimeIndexes)
     room_ois = [r['OptimeIndex'] for r in state.get('booker.rooms', op_get_rooms)]
     (bookings, _) = booker_post(
-        'api/booker/calendars/events',
+        'booker/calendars/events',
         is_update=False,
         json={
             'start': start,
diff --git a/essmsync/state.py b/essmsync/state.py
index e6232cf..5604c71 100644
--- a/essmsync/state.py
+++ b/essmsync/state.py
@@ -21,7 +21,7 @@ REQUIRED_CONFIG_KEYS = [
     'termtime.api.url',
     'termtime.api.key',
     'booker.api.url',
-    'booker.api.username',
+    'booker.api.email',
     'booker.api.password',
 ]
 
diff --git a/essmsync/tests/fixtures/config.yaml b/essmsync/tests/fixtures/config.yaml
index 93c9c17..52f0619 100644
--- a/essmsync/tests/fixtures/config.yaml
+++ b/essmsync/tests/fixtures/config.yaml
@@ -7,7 +7,7 @@ termtime:
     database: "CamDatabase"
 booker:
   api:
-    url: "http://booker/"
-    username: "essm-user@booker"
+    url: "http://booker/api/"
+    email: "essm-user@booker"
     password: "BOOKER_PASSWORD"
     access_token: "MOCK_TOKEN"
diff --git a/essmsync/tests/test_booker.py b/essmsync/tests/test_booker.py
index 45e233e..ade9145 100644
--- a/essmsync/tests/test_booker.py
+++ b/essmsync/tests/test_booker.py
@@ -69,18 +69,21 @@ class BookerGetAllTests(TestCase):
 
         # Mock Token response
         def token_response(request, context):
-            # POST body should contain username and password from `config.json`
+            # POST body should contain email and password from `config.json`
             self.assertEqual(
-                request.text,
-                'grant_type=password&username=essm-user%40booker&password=BOOKER_PASSWORD'
+                urllib.parse.parse_qs(request.text),
+                {
+                    'email': ['essm-user@booker'],
+                    'password': ['BOOKER_PASSWORD'],
+                }
             )
             return '{"access_token": "NEW_TOKEN"}'
-        self.set_request_mock_func('POST', 'booker', 'Token', token_response)
+        self.set_request_mock_func('POST', 'booker/api', 'Token', token_response)
 
         booker.auth()
         # POST to token and GET from roomtypes made
         self.assertEqual(len(self.requests_mock.request_history), 2)
-        self.assertEqual(self.requests_mock.request_history[0].url, 'http://booker/Token')
+        self.assertEqual(self.requests_mock.request_history[0].url, 'http://booker/api/Token')
         self.assertEqual(self.requests_mock.request_history[0].method, 'POST')
         self.assertEqual(self.requests_mock.request_history[1].url, 'http://booker/api/roomtypes')
 
@@ -103,13 +106,16 @@ class BookerGetAllTests(TestCase):
 
         # Mock Token bad response
         def bad_token_response(request, context):
-            # POST body should contain username and password from `config.json`
+            # POST body should contain email and password from `config.json`
             self.assertEqual(
-                request.text,
-                'grant_type=password&username=essm-user%40booker&password=BOOKER_PASSWORD'
+                urllib.parse.parse_qs(request.text),
+                {
+                    'email': ['essm-user@booker'],
+                    'password': ['BOOKER_PASSWORD'],
+                }
             )
             return '{"not_token": "BLAH"}'
-        self.set_request_mock_func('POST', 'booker', 'Token', bad_token_response)
+        self.set_request_mock_func('POST', 'booker/api', 'Token', bad_token_response)
         with self.assertRaises(exceptions.BookerAPIError):
             booker.auth()
 
@@ -1000,7 +1006,7 @@ class BookerUpdateTests(TestCase):
         }
         for put in self.requests['PUT']:
             self.assertIsNotNone(put['data'].get('BookedBy'))
-            self.assertEquals(put['data']['BookedBy'], EXPECTED_BOOKED_BY)
+            self.assertEqual(put['data']['BookedBy'], EXPECTED_BOOKED_BY)
 
     def test_update_events_read_only(self):
         """
diff --git a/secrets-example.yaml b/secrets-example.yaml
index b317927..78b344a 100644
--- a/secrets-example.yaml
+++ b/secrets-example.yaml
@@ -14,5 +14,5 @@ booker:
     # if access_token is not given then a new token is retrieved from the token
     # endpoint every run
     access_token: ""
-    username: "essm-user@domain"
+    email: "essm-user@domain"
     password: "BOOKER_PASSWORD"
-- 
GitLab


From 26147f27143901951d5bf34487377af631653e8e Mon Sep 17 00:00:00 2001
From: Robin Goodall <rjg21@cam.ac.uk>
Date: Mon, 29 Nov 2021 16:45:41 +0000
Subject: [PATCH 2/6] Pass params for calendar events endpoint in form data

---
 essmsync/booker.py         |  3 ++-
 essmsync/tests/__init__.py | 12 ++++++++++--
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/essmsync/booker.py b/essmsync/booker.py
index 6be6f75..e9db48e 100644
--- a/essmsync/booker.py
+++ b/essmsync/booker.py
@@ -561,7 +561,8 @@ def _get_bookings(start=None, end=None, filter=None):
     (bookings, _) = booker_post(
         'booker/calendars/events',
         is_update=False,
-        json={
+        # As of v1.21 this endpoint only accepts the params in form data
+        data={
             'start': start,
             'end': end,
             'StatusList': ['Approved'],
diff --git a/essmsync/tests/__init__.py b/essmsync/tests/__init__.py
index 58736ab..620db12 100644
--- a/essmsync/tests/__init__.py
+++ b/essmsync/tests/__init__.py
@@ -1,8 +1,8 @@
 import unittest
 import os
 import re
+import urllib
 import requests_mock
-import json
 from .. import state
 
 
@@ -69,7 +69,15 @@ class TestCase(unittest.TestCase):
         self.bookings_post = None
 
         def bookings_callback(request, context):
-            self.bookings_post = json.loads(request.text)
+            self.assertEqual(
+                request.headers.get('Content-Type'),
+                'application/x-www-form-urlencoded')
+            # urllib.parse.parse_qs puts single valued params in a list, so flatten those
+            self.bookings_post = {
+                k: v if len(v) > 1 else v[0]
+                for k, v in urllib.parse.parse_qs(request.text).items()
+            }
+            self.assertTrue(set(self.bookings_post.keys()).issuperset(['start', 'end']))
             start = self.bookings_post.get('start')
             end = self.bookings_post.get('end')
             file = f'{self.fixtures_dir}/booker-bookings-{start}-{end}.json'
-- 
GitLab


From 49e83a6e412bf429b777039a7a1dd7cf5a23481f Mon Sep 17 00:00:00 2001
From: Robin Goodall <rjg21@cam.ac.uk>
Date: Mon, 29 Nov 2021 17:10:36 +0000
Subject: [PATCH 3/6] Booking descriptions can sometimes be None

---
 essmsync/booker.py                            |  3 ++
 .../booker-bookings-missing-description.json  | 22 ++++++++++
 .../booker-bookings-none-description.json     | 23 +++++++++++
 essmsync/tests/test_booker.py                 | 41 +++++++++++++++++++
 4 files changed, 89 insertions(+)
 create mode 100644 essmsync/tests/fixtures/booker-bookings-missing-description.json
 create mode 100644 essmsync/tests/fixtures/booker-bookings-none-description.json

diff --git a/essmsync/booker.py b/essmsync/booker.py
index e9db48e..9538bbf 100644
--- a/essmsync/booker.py
+++ b/essmsync/booker.py
@@ -573,10 +573,13 @@ def _get_bookings(start=None, end=None, filter=None):
     bookings = [b['booking'] for b in bookings if 'booking' in b]
     LOG.info(f'All bookings between {start} to {end}: {len(bookings)}')
     # all bookings need a ['BookedBy']['Id']
+    # also we assume ['Description'] is a string but appears to sometimes be None, so fix here
     for b in bookings:
         if b.get('BookedBy', {}).get('Id') is None:
             LOG.error('At least one booking missing BookedBy.Id property')
             raise BookerAPIError()
+        if b.get('Description') is None:
+            b['Description'] = ''
     # return filtered bookings, only including:
     # - those starting on or after current time (or 'now')
     # - those passing `filter` (if specified)
diff --git a/essmsync/tests/fixtures/booker-bookings-missing-description.json b/essmsync/tests/fixtures/booker-bookings-missing-description.json
new file mode 100644
index 0000000..520b879
--- /dev/null
+++ b/essmsync/tests/fixtures/booker-bookings-missing-description.json
@@ -0,0 +1,22 @@
+[
+  {
+    "booking": {
+      "BatchId": -1,
+      "BookedBy": {
+        "Id": "real-user"
+      },
+      "EndTime": "2021-05-20T15:00:00Z",
+      "ExpectedAttendees": 10,
+      "OptimeIndex": 1013,
+      "Room": {
+        "Id": "B002-00-0001"
+      },
+      "RoomId": 3,
+      "BuildingId": 2,
+      "StartTime": "2021-05-20T14:00:00Z",
+      "Status": "Approved",
+      "Title": "Booking missing Description",
+      "Type": "Teaching"
+    }
+  }
+]
diff --git a/essmsync/tests/fixtures/booker-bookings-none-description.json b/essmsync/tests/fixtures/booker-bookings-none-description.json
new file mode 100644
index 0000000..4849129
--- /dev/null
+++ b/essmsync/tests/fixtures/booker-bookings-none-description.json
@@ -0,0 +1,23 @@
+[
+  {
+    "booking": {
+      "BatchId": -1,
+      "BookedBy": {
+        "Id": "real-user"
+      },
+      "Description": null,
+      "EndTime": "2021-05-20T15:00:00Z",
+      "ExpectedAttendees": 10,
+      "OptimeIndex": 1013,
+      "Room": {
+        "Id": "B002-00-0001"
+      },
+      "RoomId": 3,
+      "BuildingId": 2,
+      "StartTime": "2021-05-20T14:00:00Z",
+      "Status": "Approved",
+      "Title": "Booking missing Description",
+      "Type": "Teaching"
+    }
+  }
+]
diff --git a/essmsync/tests/test_booker.py b/essmsync/tests/test_booker.py
index ade9145..5b0a8c1 100644
--- a/essmsync/tests/test_booker.py
+++ b/essmsync/tests/test_booker.py
@@ -489,6 +489,47 @@ class BookerGetBookingsTests(TestCase):
         self.assertIsNone(state.get('booker.roombookings'))
         self.assertRaises(exceptions.BookerAPIError, booker.op_get_room_bookings)
 
+    def test_room_bookings_with_no_description(self):
+        """
+        Booker get_room_bookings operation will convert bookings without a Description to having
+        an empty string.
+
+        """
+        # Override mock with response with a booking missing a Description field
+        def missing_description(request, context):
+            with open(f'{self.fixtures_dir}/booker-bookings-missing-description.json') as f:
+                text = f.read()
+            return text
+        self.set_request_mock_func(
+            'POST', 'booker/api', 'booker/calendars/events', missing_description)
+
+        self.assertIsNone(state.get('booker.roombookings'))
+
+        # Get bookings
+        bookings = state.get('booker.roombookings', booker.op_get_room_bookings)
+
+        self.assertEqual(bookings[0].get('Description'), '')
+
+    def test_room_bookings_with_none_description(self):
+        """
+        Booker get_room_bookings operation will convert bookings with a Description of None
+        to having an empty string.
+
+        """
+        # Override mock with response with a booking with a Description of None
+        def none_description(request, context):
+            with open(f'{self.fixtures_dir}/booker-bookings-none-description.json') as f:
+                text = f.read()
+            return text
+        self.set_request_mock_func(
+            'POST', 'booker/api', 'booker/calendars/events', none_description)
+
+        self.assertIsNone(state.get('booker.roombookings'))
+
+        bookings = state.get('booker.roombookings', booker.op_get_room_bookings)
+
+        self.assertEqual(bookings[0].get('Description'), '')
+
     def test_get_room_bookings(self):
         """
         Booker get_room_bookings operation retrieves bookings within `termtime.timeframe`
-- 
GitLab


From 3a9782ec522f4d92155802a816c9801c0468be3d Mon Sep 17 00:00:00 2001
From: Robin Goodall <rjg21@cam.ac.uk>
Date: Tue, 30 Nov 2021 12:42:14 +0000
Subject: [PATCH 4/6] Site property of building is now object not OptimeIndex

booker.rooms[].Building.Site moves to SiteId and Site contains site object
---
 essmsync/booker.py                            | 22 ++++----
 essmsync/comparison.py                        | 26 +++++-----
 essmsync/tests/fixtures/booker-buildings.json |  8 +--
 .../tests/fixtures/booker-rooms-new-room.json | 51 ++++++++++++++++---
 .../tests/fixtures/booker-rooms-none-tt.json  | 44 +++++++++++++---
 essmsync/tests/fixtures/booker-rooms.json     | 44 +++++++++++++---
 essmsync/tests/test_booker.py                 | 28 +---------
 essmsync/tests/test_comparison.py             | 46 +++++++++++------
 essmsync/tests/test_termtime.py               | 25 +++++++--
 operations.md                                 |  6 +--
 10 files changed, 198 insertions(+), 102 deletions(-)

diff --git a/essmsync/booker.py b/essmsync/booker.py
index 9538bbf..fa9b15b 100644
--- a/essmsync/booker.py
+++ b/essmsync/booker.py
@@ -292,21 +292,19 @@ def op_get_sites():
     in `booker.sites`
 
     """
-    # Get all (non -1) site OptimeIndexes for filtered buildings
-    site_ois = {b['Site'] for b in state.get('booker.buildings', op_get_buildings)
-                if b.get('Site', -1) != -1}
-    if len(site_ois) == 0:
+    # In v1.21 the 'Site' property of the 'Building' property of a room response changed from
+    # the OptimeIndex to the site object and 'SiteId' became the OptimeIndex
+    # Use this Site property of the buildings list (itself the Buildings property of the rooms
+    # list) to build the list of sites.
+    # Convert dict keyed on SiteId to list to remove duplicates
+    sites = list({
+        b['SiteId']: b['Site'] for b in state.get('booker.buildings', op_get_buildings)
+        if b.get('SiteId', -1) != -1 and b.get('Site')
+    }.values())
+    if len(sites) == 0:
         LOG.error('No sites in Booker related to TermTime rooms')
         raise BookerAPIError()
-    LOG.info(f'Booker filtered rooms OptimeIndexes: {len(site_ois)}')
-
-    # Now get all sites (no way to filter API get) and filter to those in the id list
-    sites = [s for s in state.get('booker.all_sites', op_get_all_sites)
-             if s['OptimeIndex'] in site_ois]
     LOG.info(f'Booker filtered sites: {len(sites)}')
-    if len(sites) != len(site_ois):
-        LOG.warning(f'Booker missing {len(site_ois) - len(sites)} sites')
-
     state.set('booker.sites', sites)
 
 
diff --git a/essmsync/comparison.py b/essmsync/comparison.py
index dcab8c4..bd9fc2e 100644
--- a/essmsync/comparison.py
+++ b/essmsync/comparison.py
@@ -30,7 +30,7 @@ def _compare_b_to_tt(data_type, booker_state, termtime_state, mapping, code='cod
                     - 'expected' - expected TermTime keys and values
                     - 'update' - keys and values need to create or update TermTime
                     Returns None - if entry is invalid
-    :type comparitor: callable[[dict], dict]
+    :type mapping: callable[[dict], dict]
     :param code: termtime field used for code (usually 'code', sometimes 'c'), defaults to 'code'
     :type code: str, optional
 
@@ -169,15 +169,15 @@ def op_compare_buildings():
         # Common 'Description' to 'n' mapping
         m = _mapping_n(booker_entry)
         id = booker_entry.get('Id')
-        # Validate Site (OptimeIndex) exists
-        site_oi = booker_entry.get('Site')
+        # Validate SiteId (OptimeIndex) exists
+        site_oi = booker_entry.get('SiteId')
         if site_oi is None:
-            LOG.warning(f"Booker building missing Site - '{id}'")
+            LOG.warning(f"Booker building missing SiteId - '{id}'")
             return None
         # Lookup site to get name and id
         site = sites_by_oi.get(site_oi)
         if site is None:
-            LOG.warning(f"Booker building has missing Site '{site_oi}' - '{id}'")
+            LOG.warning(f"Booker building has missing SiteId '{site_oi}' - '{id}'")
             return None
         # Add Campuses Name (for comparison)
         m['expected']['cn'] = site.get('Description')
@@ -246,15 +246,15 @@ def op_compare_rooms():
         if b_name is None:
             LOG.warning(f"Booker room missing Building.Description - '{id}'")
             return None
-        # Validate Building.Site (OptimeIndex) exists
-        site_oi = b.get('Site')
+        # Validate Building.SiteId (OptimeIndex) exists
+        site_oi = b.get('SiteId')
         if site_oi is None:
-            LOG.warning(f"Booker room missing Building.Site - '{id}'")
+            LOG.warning(f"Booker room missing Building.SiteId - '{id}'")
             return None
         # Lookup site to get name and id
         site = sites_by_oi.get(site_oi)
         if site is None:
-            LOG.warning(f"Booker room has missing Site '{site_oi}' - '{id}'")
+            LOG.warning(f"Booker room has missing SiteId '{site_oi}' - '{id}'")
             return None
         # Validate DepartmentId (OptimeIndex) exists
         dept_oi = booker_entry.get('DepartmentId')
@@ -346,16 +346,16 @@ def op_compare_floors():
             LOG.warning(f"Booker floor missing/invalid Building - '{floor.get('Id')}'")
             results['invalid'].append(floor)
             continue
-        # Validate Building.Site (OptimeIndex) exists
-        site_oi = b.get('Site')
+        # Validate Building.SiteId (OptimeIndex) exists
+        site_oi = b.get('SiteId')
         if site_oi is None:
-            LOG.warning(f"Booker floor missing Building.Site - '{floor.get('Id')}'")
+            LOG.warning(f"Booker floor missing Building.SiteId - '{floor.get('Id')}'")
             results['invalid'].append(floor)
             continue
         # Lookup site to get name and id
         site = sites_by_oi.get(site_oi)
         if site is None:
-            LOG.warning(f"Booker floor has missing Site '{site_oi}' - '{floor.get('Id')}'")
+            LOG.warning(f"Booker floor has missing SiteId '{site_oi}' - '{floor.get('Id')}'")
             results['invalid'].append(floor)
             continue
 
diff --git a/essmsync/tests/fixtures/booker-buildings.json b/essmsync/tests/fixtures/booker-buildings.json
index 9ebd2ad..0e49650 100644
--- a/essmsync/tests/fixtures/booker-buildings.json
+++ b/essmsync/tests/fixtures/booker-buildings.json
@@ -3,21 +3,21 @@
     "OptimeIndex": 1,
     "Description": "Building One",
     "Id": "B001",
-    "Site": 1
+    "SiteId": 1
   },{
     "OptimeIndex": 2,
     "Description": "Building Two",
     "Id": "B002",
-    "Site": 1
+    "SiteId": 1
   },{
     "OptimeIndex": 3,
     "Description": "Secondary Building",
     "Id": "B201",
-    "Site": 2
+    "SiteId": 2
   },{
     "OptimeIndex": 4,
     "Description": "Unused Building",
     "Id": "U001",
-    "Site": 3
+    "SiteId": 3
   }
 ]
diff --git a/essmsync/tests/fixtures/booker-rooms-new-room.json b/essmsync/tests/fixtures/booker-rooms-new-room.json
index 513573f..0ec0ad8 100644
--- a/essmsync/tests/fixtures/booker-rooms-new-room.json
+++ b/essmsync/tests/fixtures/booker-rooms-new-room.json
@@ -13,7 +13,12 @@
       "OptimeIndex": 1,
       "Description": "Building One",
       "Id": "B001",
-      "Site": 1
+      "SiteId": 1,
+      "Site": {
+        "OptimeIndex": 1,
+        "Id": "21000001",
+        "Description": "Main Site"
+      }
     },
     "Equipment": [
       1,
@@ -35,7 +40,12 @@
       "OptimeIndex": 1,
       "Description": "Building One",
       "Id": "B001",
-      "Site": 1
+      "SiteId": 1,
+      "Site": {
+        "OptimeIndex": 1,
+        "Id": "21000001",
+        "Description": "Main Site"
+      }
     },
     "Equipment": [
       1,
@@ -58,7 +68,12 @@
       "OptimeIndex": 2,
       "Description": "Building Two",
       "Id": "B002",
-      "Site": 1
+      "SiteId": 1,
+      "Site": {
+        "OptimeIndex": 1,
+        "Id": "21000001",
+        "Description": "Main Site"
+      }
     },
     "Equipment": [
       1,
@@ -81,7 +96,12 @@
       "OptimeIndex": 3,
       "Description": "Secondary Building",
       "Id": "B201",
-      "Site": 2
+      "SiteId": 2,
+      "Site": {
+        "OptimeIndex": 2,
+        "Id": "21000002",
+        "Description": "Second Site"
+      }
     },
     "Equipment": [
       1,
@@ -92,7 +112,7 @@
     "Inactive": false
   },{
     "OptimeIndex": 5,
-    "Description": "Usused Room C",
+    "Description": "Unused Room C",
     "Id": "B201-03-0012",
     "Capacity": 5,
     "DepartmentId": 1,
@@ -104,7 +124,12 @@
       "OptimeIndex": 3,
       "Description": "Secondary Building",
       "Id": "B201",
-      "Site": 2
+      "SiteId": 2,
+      "Site": {
+        "OptimeIndex": 2,
+        "Id": "21000002",
+        "Description": "Second Site"
+      }
     },
     "Equipment": [
       2
@@ -125,7 +150,12 @@
       "OptimeIndex": 4,
       "Description": "Unused Building",
       "Id": "U001",
-      "Site": 3
+      "SiteId": 3,
+      "Site": {
+        "OptimeIndex": 3,
+        "Id": "21000003",
+        "Description": "Unused Site"
+      }
     },
     "Equipment": [
     ],
@@ -145,7 +175,12 @@
       "OptimeIndex": 1,
       "Description": "Building One",
       "Id": "B001",
-      "Site": 1
+      "SiteId": 1,
+      "Site": {
+        "OptimeIndex": 1,
+        "Id": "21000001",
+        "Description": "Main Site"
+      }
     },
     "Equipment": [
       1,
diff --git a/essmsync/tests/fixtures/booker-rooms-none-tt.json b/essmsync/tests/fixtures/booker-rooms-none-tt.json
index 50bee3d..77d90ad 100644
--- a/essmsync/tests/fixtures/booker-rooms-none-tt.json
+++ b/essmsync/tests/fixtures/booker-rooms-none-tt.json
@@ -13,7 +13,12 @@
       "OptimeIndex": 1,
       "Description": "Building One",
       "Id": "B001",
-      "Site": 1
+      "SiteId": 1,
+      "Site": {
+        "OptimeIndex": 1,
+        "Id": "21000001",
+        "Description": "Main Site"
+      }
     },
     "Equipment": [
       3
@@ -34,7 +39,12 @@
       "OptimeIndex": 1,
       "Description": "Building One",
       "Id": "B001",
-      "Site": 1
+      "SiteId": 1,
+      "Site": {
+        "OptimeIndex": 1,
+        "Id": "21000001",
+        "Description": "Main Site"
+      }
     },
     "Equipment": [
       3,
@@ -56,7 +66,12 @@
       "OptimeIndex": 2,
       "Description": "Building Two",
       "Id": "B002",
-      "Site": 1
+      "SiteId": 1,
+      "Site": {
+        "OptimeIndex": 1,
+        "Id": "21000001",
+        "Description": "Main Site"
+      }
     },
     "Equipment": [
       3,
@@ -78,7 +93,12 @@
       "OptimeIndex": 3,
       "Description": "Secondary Building",
       "Id": "B201",
-      "Site": 2
+      "SiteId": 2,
+      "Site": {
+        "OptimeIndex": 2,
+        "Id": "21000002",
+        "Description": "Second Site"
+      }
     },
     "Equipment": [
       2,
@@ -88,7 +108,7 @@
     "Inactive": false
   },{
     "OptimeIndex": 5,
-    "Description": "Usused Room C",
+    "Description": "Unused Room C",
     "Id": "B201-03-0012",
     "Capacity": 5,
     "DepartmentId": 1,
@@ -100,7 +120,12 @@
       "OptimeIndex": 3,
       "Description": "Secondary Building",
       "Id": "B201",
-      "Site": 2
+      "SiteId": 2,
+      "Site": {
+        "OptimeIndex": 2,
+        "Id": "21000002",
+        "Description": "Second Site"
+      }
     },
     "Equipment": [
       2
@@ -121,7 +146,12 @@
       "OptimeIndex": 4,
       "Description": "Unused Building",
       "Id": "U001",
-      "Site": 3
+      "SiteId": 3,
+      "Site": {
+        "OptimeIndex": 3,
+        "Id": "21000003",
+        "Description": "Unused Site"
+      }
     },
     "Equipment": [
     ],
diff --git a/essmsync/tests/fixtures/booker-rooms.json b/essmsync/tests/fixtures/booker-rooms.json
index 2426801..bbcb831 100644
--- a/essmsync/tests/fixtures/booker-rooms.json
+++ b/essmsync/tests/fixtures/booker-rooms.json
@@ -13,7 +13,12 @@
       "OptimeIndex": 1,
       "Description": "Building One",
       "Id": "B001",
-      "Site": 1
+      "SiteId": 1,
+      "Site": {
+        "OptimeIndex": 1,
+        "Id": "21000001",
+        "Description": "Main Site"
+      }
     },
     "Equipment": [
       1,
@@ -35,7 +40,12 @@
       "OptimeIndex": 1,
       "Description": "Building One",
       "Id": "B001",
-      "Site": 1
+      "SiteId": 1,
+      "Site": {
+        "OptimeIndex": 1,
+        "Id": "21000001",
+        "Description": "Main Site"
+      }
     },
     "Equipment": [
       1,
@@ -58,7 +68,12 @@
       "OptimeIndex": 2,
       "Description": "Building Two",
       "Id": "B002",
-      "Site": 1
+      "SiteId": 1,
+      "Site": {
+        "OptimeIndex": 1,
+        "Id": "21000001",
+        "Description": "Main Site"
+      }
     },
     "Equipment": [
       1,
@@ -81,7 +96,12 @@
       "OptimeIndex": 3,
       "Description": "Secondary Building",
       "Id": "B201",
-      "Site": 2
+      "SiteId": 2,
+      "Site": {
+        "OptimeIndex": 2,
+        "Id": "21000002",
+        "Description": "Second Site"
+      }
     },
     "Equipment": [
       1,
@@ -92,7 +112,7 @@
     "Inactive": false
   },{
     "OptimeIndex": 5,
-    "Description": "Usused Room C",
+    "Description": "Unused Room C",
     "Id": "B201-03-0012",
     "Capacity": 5,
     "DepartmentId": 1,
@@ -104,7 +124,12 @@
       "OptimeIndex": 3,
       "Description": "Secondary Building",
       "Id": "B201",
-      "Site": 2
+      "SiteId": 2,
+      "Site": {
+        "OptimeIndex": 2,
+        "Id": "21000002",
+        "Description": "Second Site"
+      }
     },
     "Equipment": [
       2
@@ -125,7 +150,12 @@
       "OptimeIndex": 4,
       "Description": "Unused Building",
       "Id": "U001",
-      "Site": 3
+      "SiteId": 3,
+      "Site": {
+        "OptimeIndex": 3,
+        "Id": "21000003",
+        "Description": "Unused Site"
+      }
     },
     "Equipment": [
     ],
diff --git a/essmsync/tests/test_booker.py b/essmsync/tests/test_booker.py
index 5b0a8c1..5bbafd6 100644
--- a/essmsync/tests/test_booker.py
+++ b/essmsync/tests/test_booker.py
@@ -326,37 +326,13 @@ class BookerGetFilteredTests(TestCase):
         # Load TermTime related buildings
         booker.op_get_buildings()
         self.assertIsInstance(state.get('booker.buildings'), list)
-        # Change all buildings have Site of -1 so that no sites are found and an
+        # Change all buildings have SiteId of -1 so that no sites are found and an
         # exception is raised
         state.set('booker.buildings', [
-            {**b, **{'Site': -1}} for b in state.get('booker.buildings')
+            {**b, **{'SiteId': -1}} for b in state.get('booker.buildings')
         ])
         self.assertRaises(exceptions.BookerAPIError, booker.op_get_sites)
 
-    def test_get_sites_missing_warning(self):
-        """
-        Booker get_sites operation warns if any of the sites that TermTime
-        rooms belong to are missing
-
-        """
-        self.assertIsNone(state.get('booker.all_sites'))
-        # get all sites
-        booker.op_get_all_sites()
-        # remove second (of three) departments
-        s = state.get('booker.all_sites')
-        self.assertEqual(len(s), 3)
-        s.pop(1)
-
-        # Now get filtered sites
-        with self.assertLogs(booker.LOG, "WARN") as log:
-            booker.op_get_sites()
-        # Only one site found
-        s = state.get('booker.sites')
-        self.assertIsInstance(s, list)
-        self.assertEqual(len(s), 1)
-        # And warning of one missing
-        self.assertIn("WARNING:essmsync.booker:Booker missing 1 sites", log.output)
-
     def test_get_buildings(self):
         """
         Booker get_buildings operation loads buildings list and filters to only
diff --git a/essmsync/tests/test_comparison.py b/essmsync/tests/test_comparison.py
index 1c8023f..6d9a2b6 100644
--- a/essmsync/tests/test_comparison.py
+++ b/essmsync/tests/test_comparison.py
@@ -398,12 +398,18 @@ class CompareBuildingsTests(ComparisonTests):
         OLD_SITE_OI = 1
         NEW_SITE_OI = 2
         NEW_SITE_CODE = '21000002'
+        NEW_SITE_DESC = 'Second Site'
         s = state.get('booker.buildings')
         self._assert_list_contains(s, 'Id', ID)
         for d in s:
             if d.get('Id') == ID:
-                self.assertEqual(d['Site'], OLD_SITE_OI)
-                d['Site'] = NEW_SITE_OI
+                self.assertEqual(d['SiteId'], OLD_SITE_OI)
+                d['SiteId'] = NEW_SITE_OI
+                d['Site'] = {
+                    'OptimeIndex': NEW_SITE_OI,
+                    'Id': NEW_SITE_CODE,
+                    'Description': NEW_SITE_DESC,
+                }
 
         # Compare buildings
         comparison.op_compare_buildings()
@@ -442,12 +448,18 @@ class CompareBuildingsTests(ComparisonTests):
         NEW_DESC = 'Shiny New Building'
         SITE_OI = 2
         SITE_ID = '21000002'
+        SITE_DESC = 'Second Site'
         s = state.get('booker.buildings')
         s.append({
             'OptimeIndex': 999,
             'Id': NEW_ID,
             'Description': NEW_DESC,
-            'Site': SITE_OI,
+            'SiteId': SITE_OI,
+            'Site': {
+                'OptimeIndex': SITE_OI,
+                'Id': SITE_ID,
+                'Description': SITE_DESC,
+            }
         })
 
         self._assert_list_contains(state.get('booker.buildings'), 'Id', NEW_ID)
@@ -482,22 +494,23 @@ class CompareBuildingsTests(ComparisonTests):
         self.assertIsNone(state.get('booker.buildings'))
         self.assertIsNone(state.get('termtime.buildings'))
 
-        # Preload the Booker buildings
+        # Preload the Booker buildings and sites
         booker.op_get_buildings()
+        booker.op_get_sites()
 
-        # Invalidate "Building One" (no Site) and "Building Two" (invalid Site)
+        # Invalidate "Building One" (no SiteId) and "Building Two" (invalid SiteId)
         for d in state.get('booker.buildings'):
             if d.get('Id') == 'B001':
-                del(d['Site'])
+                del(d['SiteId'])
             if d.get('Id') == 'B002':
-                d['Site'] = 666
+                d['SiteId'] = 666
 
         # Compare buildings causes warnings for both
         with self.assertLogs(comparison.LOG, "WARN") as log:
             comparison.op_compare_buildings()
         self.assertTrue(set(log.output).issuperset({
-            "WARNING:essmsync.comparison:Booker building missing Site - 'B001'",
-            "WARNING:essmsync.comparison:Booker building has missing Site '666' - 'B002'"
+            "WARNING:essmsync.comparison:Booker building missing SiteId - 'B001'",
+            "WARNING:essmsync.comparison:Booker building has missing SiteId '666' - 'B002'"
         }))
 
         # both buildings put in invalid results
@@ -806,23 +819,24 @@ class CompareRoomsTests(ComparisonTests):
         self.assertIsNone(state.get('booker.rooms'))
         self.assertIsNone(state.get('termtime.rooms'))
 
-        # Preload the Booker rooms
+        # Preload the Booker rooms and sites
         booker.op_get_rooms()
+        booker.op_get_sites()
 
         for d in state.get('booker.rooms'):
-            # Invalidate "Teaching Room 1" (no Building.Site)
+            # Invalidate "Teaching Room 1" (no Building.SiteId)
             if d.get('Id') == 'B001-01-0001':
-                del(d['Building']['Site'])
-            # Invalidate "Teaching Room 2" (invalid Building.Site)
+                del(d['Building']['SiteId'])
+            # Invalidate "Teaching Room 2" (invalid Building.SiteId)
             if d.get('Id') == 'B001-02-0001':
-                d['Building']['Site'] = 666
+                d['Building']['SiteId'] = 666
 
         # Compare rooms causes warnings for both
         with self.assertLogs(comparison.LOG, "WARN") as log:
             comparison.op_compare_rooms()
         self.assertTrue(set(log.output).issuperset({
-            "WARNING:essmsync.comparison:Booker room missing Building.Site - 'B001-01-0001'",
-            "WARNING:essmsync.comparison:Booker room has missing Site '666' - 'B001-02-0001'"
+            "WARNING:essmsync.comparison:Booker room missing Building.SiteId - 'B001-01-0001'",
+            "WARNING:essmsync.comparison:Booker room has missing SiteId '666' - 'B001-02-0001'"
         }))
 
         # rooms put in invalid results
diff --git a/essmsync/tests/test_termtime.py b/essmsync/tests/test_termtime.py
index 3bd9239..32540ee 100644
--- a/essmsync/tests/test_termtime.py
+++ b/essmsync/tests/test_termtime.py
@@ -832,11 +832,17 @@ class TermTimeUpdateTests(TestCase):
         NEW_DESC = 'New Building (gets url-parsed)'
         SITE_OI = 2
         SITE_ID = '21000002'
+        SITE_DESC = 'Second Site'
         state.get('booker.buildings').append({
             'OptimeIndex': NEW_OPTIMEINDEX,
             'Id': NEW_ID,
             'Description': NEW_DESC,
-            'Site': SITE_OI,
+            'SiteId': SITE_OI,
+            'Site': {
+                'OptimeIndex': SITE_OI,
+                'Id': SITE_ID,
+                'Description': SITE_DESC,
+            }
         })
 
         termtime.op_update_buildings()
@@ -871,12 +877,18 @@ class TermTimeUpdateTests(TestCase):
         OLD_SITE_OI = 1
         NEW_SITE_OI = 2
         NEW_SITE_ID = '21000002'
+        NEW_SITE_DESC = 'Second Site'
         for d in state.get('booker.buildings'):
             if d.get('Id') == ID:
                 self.assertEqual(d['Description'], OLD_DESC)
                 d['Description'] = NEW_DESC
-                self.assertEqual(d['Site'], OLD_SITE_OI)
-                d['Site'] = NEW_SITE_OI
+                self.assertEqual(d['SiteId'], OLD_SITE_OI)
+                d['SiteId'] = NEW_SITE_OI
+                d['Site'] = {
+                    'OptimeIndex': NEW_SITE_OI,
+                    'Id': NEW_SITE_ID,
+                    'Description': NEW_SITE_DESC,
+                }
 
         termtime.op_update_buildings()
 
@@ -909,7 +921,12 @@ class TermTimeUpdateTests(TestCase):
             'OptimeIndex': 999,
             'Id': 'B501',
             'Description': 'New Building (gets url-parsed)',
-            'Site': 2,
+            'SiteId': 2,
+            'Site': {
+                'OptimeIndex': 2,
+                'Id': '21000002',
+                'Description': 'Second Site',
+            }
         })
         # Add an update too
         for d in state.get('booker.buildings'):
diff --git a/operations.md b/operations.md
index f618666..1d71f38 100644
--- a/operations.md
+++ b/operations.md
@@ -100,11 +100,7 @@ Get a list of sites related to the filtered rooms list (see get_rooms)
 and put them in the `booker.sites` state key.
 
 The `get_buildings` operation is called, if needed, to obtain the filtered list
-of buildings. A list of the `Site` values of each buildings (an OptimeIndex) is
-compiled.
-
-The `get_all_sites` operation is then called, if needed, then filtered to only
-those sites with OptimeIndexes in the compiled list of `Site` values.
+of buildings. A list of the `Site` values of each buildings is compiled.
 
 #### get_room_bookings(start=None, end=None)
 - `start` - date at start of period in YYYY-MM-DD format (inclusive)
-- 
GitLab


From 14aae641ce113b5fa7538bbdf635582163cb376c Mon Sep 17 00:00:00 2001
From: Robin Goodall <rjg21@cam.ac.uk>
Date: Tue, 30 Nov 2021 17:24:31 +0000
Subject: [PATCH 5/6] Rooms no longer have a DepartmentId

---
 essmsync/booker.py                            | 16 +++++-
 essmsync/comparison.py                        | 13 ++---
 .../tests/fixtures/booker-rooms-new-room.json |  7 ---
 .../tests/fixtures/booker-rooms-none-tt.json  |  6 ---
 essmsync/tests/fixtures/booker-rooms.json     |  6 ---
 essmsync/tests/test_booker.py                 | 51 ++++++++++++++++++-
 essmsync/tests/test_comparison.py             | 15 +++---
 operations.md                                 |  5 +-
 8 files changed, 81 insertions(+), 38 deletions(-)

diff --git a/essmsync/booker.py b/essmsync/booker.py
index fa9b15b..822c8a3 100644
--- a/essmsync/booker.py
+++ b/essmsync/booker.py
@@ -249,9 +249,21 @@ def op_get_depts():
     in `booker.depts`
 
     """
+    # Since v1.21 the room response has no 'DepartmentId' just the list of departments
+    # under 'Departments'. We'll assume that the first in the list is the room's department
+    # we want (as TermTime only has one per room) but warn if we see more than one or none.
+
     # Get all (non -1) department OptimeIndexes for filtered rooms
-    dept_ois = {d['DepartmentId'] for d in state.get('booker.rooms', op_get_rooms)
-                if d.get('DepartmentId', -1) != -1}
+    dept_ois = set()
+    for d in state.get('booker.rooms', op_get_rooms):
+        dl = d.get('Departments', [])
+        if len(dl) == 0:
+            LOG.warning(f"Room '{d['Id']}' has no department")
+            continue
+        if len(dl) > 1:
+            LOG.warning(f"Room '{d['Id']}' has {len(dl)} departments")
+        dept_ois.add(dl[0])
+
     if len(dept_ois) == 0:
         LOG.error('No departments in Booker related to TermTime rooms')
         raise BookerAPIError()
diff --git a/essmsync/comparison.py b/essmsync/comparison.py
index bd9fc2e..9d43094 100644
--- a/essmsync/comparison.py
+++ b/essmsync/comparison.py
@@ -202,7 +202,7 @@ def op_compare_rooms():
     # Need to be able to match building.site OptimeIndex to site Id
     sites_by_oi = {s['OptimeIndex']: s for s in state.get('booker.sites', booker.op_get_sites)}
 
-    # Need to be able to match DepartmentId OptimeIndex to department's Id
+    # Need to be able to match Departments[0] OptimeIndex to department's Id
     depts_by_oi = {d['OptimeIndex']: d for d in state.get('booker.depts', booker.op_get_depts)}
 
     # Get TermTime capabilities
@@ -256,12 +256,13 @@ def op_compare_rooms():
         if site is None:
             LOG.warning(f"Booker room has missing SiteId '{site_oi}' - '{id}'")
             return None
-        # Validate DepartmentId (OptimeIndex) exists
-        dept_oi = booker_entry.get('DepartmentId')
-        if dept_oi is None:
-            LOG.warning(f"Booker room missing DepartmentId - '{id}'")
+        # Validate Departments[0] (OptimeIndex) exists
+        try:
+            dept_oi = booker_entry.get('Departments', [])[0]
+        except IndexError:
+            LOG.warning(f"Booker room missing Department - '{id}'")
             return None
-        # Lookup site to get name and id
+        # Lookup department to get name and id
         dept = depts_by_oi.get(dept_oi)
         if dept is None:
             LOG.warning(f"Booker room has missing Department '{dept_oi}' - '{id}'")
diff --git a/essmsync/tests/fixtures/booker-rooms-new-room.json b/essmsync/tests/fixtures/booker-rooms-new-room.json
index 0ec0ad8..92128f7 100644
--- a/essmsync/tests/fixtures/booker-rooms-new-room.json
+++ b/essmsync/tests/fixtures/booker-rooms-new-room.json
@@ -4,7 +4,6 @@
     "Description": "Teaching Room 1",
     "Id": "B001-01-0001",
     "Capacity": 20,
-    "DepartmentId": 1,
     "Departments": [
       1
     ],
@@ -31,7 +30,6 @@
     "Description": "Teaching Room 2",
     "Id": "B001-02-0001",
     "Capacity": 10,
-    "DepartmentId": 1,
     "Departments": [
       1
     ],
@@ -59,7 +57,6 @@
     "Description": "Seminar Room A",
     "Id": "B002-00-0001",
     "Capacity": 5,
-    "DepartmentId": 2,
     "Departments": [
       2
     ],
@@ -87,7 +84,6 @@
     "Description": "Seminar Room B",
     "Id": "B201-03-0011",
     "Capacity": 10,
-    "DepartmentId": 1,
     "Departments": [
       1
     ],
@@ -115,7 +111,6 @@
     "Description": "Unused Room C",
     "Id": "B201-03-0012",
     "Capacity": 5,
-    "DepartmentId": 1,
     "Departments": [
       1
     ],
@@ -141,7 +136,6 @@
     "Description": "Unused Room",
     "Id": "U001-04-0002",
     "Capacity": 30,
-    "DepartmentId": 3,
     "Departments": [
       3
     ],
@@ -166,7 +160,6 @@
     "Description": "Teaching Room 3",
     "Id": "B001-03-0001",
     "Capacity": 20,
-    "DepartmentId": 1,
     "Departments": [
       1
     ],
diff --git a/essmsync/tests/fixtures/booker-rooms-none-tt.json b/essmsync/tests/fixtures/booker-rooms-none-tt.json
index 77d90ad..72141c5 100644
--- a/essmsync/tests/fixtures/booker-rooms-none-tt.json
+++ b/essmsync/tests/fixtures/booker-rooms-none-tt.json
@@ -4,7 +4,6 @@
     "Description": "Teaching Room 1",
     "Id": "B001-01-0001",
     "Capacity": 20,
-    "DepartmentId": 1,
     "Departments": [
       1
     ],
@@ -30,7 +29,6 @@
     "Description": "Teaching Room 2",
     "Id": "B001-02-0001",
     "Capacity": 10,
-    "DepartmentId": 1,
     "Departments": [
       1
     ],
@@ -57,7 +55,6 @@
     "Description": "Seminar Room A",
     "Id": "B002-00-0001",
     "Capacity": 5,
-    "DepartmentId": 2,
     "Departments": [
       2
     ],
@@ -84,7 +81,6 @@
     "Description": "Seminar Room B",
     "Id": "B201-03-0011",
     "Capacity": 10,
-    "DepartmentId": 1,
     "Departments": [
       1
     ],
@@ -111,7 +107,6 @@
     "Description": "Unused Room C",
     "Id": "B201-03-0012",
     "Capacity": 5,
-    "DepartmentId": 1,
     "Departments": [
       1
     ],
@@ -137,7 +132,6 @@
     "Description": "Unused Room",
     "Id": "U001-04-0002",
     "Capacity": 30,
-    "DepartmentId": 3,
     "Departments": [
       3
     ],
diff --git a/essmsync/tests/fixtures/booker-rooms.json b/essmsync/tests/fixtures/booker-rooms.json
index bbcb831..e045bbf 100644
--- a/essmsync/tests/fixtures/booker-rooms.json
+++ b/essmsync/tests/fixtures/booker-rooms.json
@@ -4,7 +4,6 @@
     "Description": "Teaching Room 1",
     "Id": "B001-01-0001",
     "Capacity": 20,
-    "DepartmentId": 1,
     "Departments": [
       1
     ],
@@ -31,7 +30,6 @@
     "Description": "Teaching Room 2",
     "Id": "B001-02-0001",
     "Capacity": 10,
-    "DepartmentId": 1,
     "Departments": [
       1
     ],
@@ -59,7 +57,6 @@
     "Description": "Seminar Room A",
     "Id": "B002-00-0001",
     "Capacity": 5,
-    "DepartmentId": 2,
     "Departments": [
       2
     ],
@@ -87,7 +84,6 @@
     "Description": "Seminar Room B",
     "Id": "B201-03-0011",
     "Capacity": 10,
-    "DepartmentId": 1,
     "Departments": [
       1
     ],
@@ -115,7 +111,6 @@
     "Description": "Unused Room C",
     "Id": "B201-03-0012",
     "Capacity": 5,
-    "DepartmentId": 1,
     "Departments": [
       1
     ],
@@ -141,7 +136,6 @@
     "Description": "Unused Room",
     "Id": "U001-04-0002",
     "Capacity": 30,
-    "DepartmentId": 3,
     "Departments": [
       3
     ],
diff --git a/essmsync/tests/test_booker.py b/essmsync/tests/test_booker.py
index 5bbafd6..60464d6 100644
--- a/essmsync/tests/test_booker.py
+++ b/essmsync/tests/test_booker.py
@@ -270,10 +270,10 @@ class BookerGetFilteredTests(TestCase):
         # Load TermTime related rooms
         booker.op_get_rooms()
         self.assertIsInstance(state.get('booker.rooms'), list)
-        # Change all rooms to have DepartmentId of -1 so that no departments are
+        # Change all rooms to have Departments list to be empty so that no departments are
         # found and an exception is raised
         state.set('booker.rooms', [
-            {**r, **{'DepartmentId': -1}} for r in state.get('booker.rooms')
+            {**r, **{'Departments': list()}} for r in state.get('booker.rooms')
         ])
         self.assertRaises(exceptions.BookerAPIError, booker.op_get_depts)
 
@@ -301,6 +301,53 @@ class BookerGetFilteredTests(TestCase):
         # And warning of one missing
         self.assertIn("WARNING:essmsync.booker:Booker missing 1 departments", log.output)
 
+    def test_get_depts_room_with_no_dept(self):
+        """
+        Booker get_depts operation warns if any of the Booker rooms matching the TermTime
+        rooms are missing a department
+
+        """
+        self.assertIsNone(state.get('booker.rooms'))
+        self.assertIsNone(state.get('booker.depts'))
+        # remove the departments field from Teaching Room 1 (B001-01-0001) and
+        # empty the departments field from Teaching Room 2 (B001-02-0001)
+        ROOM_ID_DEL = 'B001-01-0001'
+        ROOM_ID_EMPTY = 'B001-02-0001'
+        for r in state.get('booker.rooms', booker.op_get_rooms):
+            if r['Id'] == ROOM_ID_DEL:
+                del(r['Departments'])
+            if r['Id'] == ROOM_ID_EMPTY:
+                r['Departments'] = list()
+
+        # Now get filtered departments
+        with self.assertLogs(booker.LOG, "WARN") as log:
+            booker.op_get_depts()
+        # warnings of two missing
+        self.assertTrue(set(log.output).issuperset({
+            f"WARNING:essmsync.booker:Room '{ROOM_ID_DEL}' has no department",
+            f"WARNING:essmsync.booker:Room '{ROOM_ID_EMPTY}' has no department",
+        }))
+
+    def test_get_depts_room_with_multiple_depts(self):
+        """
+        Booker get_depts operation warns if any of the Booker rooms matching the TermTime
+        rooms have more than one department
+
+        """
+        self.assertIsNone(state.get('booker.rooms'))
+        self.assertIsNone(state.get('booker.depts'))
+        # add another department to Teaching Room 1 (B001-01-0001)
+        ROOM_ID = 'B001-01-0001'
+        for r in state.get('booker.rooms', booker.op_get_rooms):
+            if r['Id'] == ROOM_ID:
+                r['Departments'].append(666)
+
+        # Now get filtered departments
+        with self.assertLogs(booker.LOG, "WARN") as log:
+            booker.op_get_depts()
+        # warnings of one room with two departments
+        self.assertIn(f"WARNING:essmsync.booker:Room '{ROOM_ID}' has 2 departments", log.output)
+
     def test_get_sites(self):
         """
         Booker get_sites operation loads sites list and filters to only those
diff --git a/essmsync/tests/test_comparison.py b/essmsync/tests/test_comparison.py
index 6d9a2b6..c8b7dba 100644
--- a/essmsync/tests/test_comparison.py
+++ b/essmsync/tests/test_comparison.py
@@ -610,8 +610,8 @@ class CompareRoomsTests(ComparisonTests):
         self._assert_list_contains(s, 'Id', ID)
         for d in s:
             if d.get('Id') == ID:
-                self.assertEqual(d['DepartmentId'], OLD_DEPT_OI)
-                d['DepartmentId'] = NEW_DEPT_OI
+                self.assertEqual(d['Departments'][0], OLD_DEPT_OI)
+                d['Departments'] = [NEW_DEPT_OI]
 
         # Compare rooms
         comparison.op_compare_rooms()
@@ -858,18 +858,19 @@ class CompareRoomsTests(ComparisonTests):
         booker.op_get_rooms()
 
         for d in state.get('booker.rooms'):
-            # Invalidate "Teaching Room 1" (no Building.DepartmentId)
+            # Invalidate "Teaching Room 1" (no Building.Departments)
             if d.get('Id') == 'B001-01-0001':
-                del(d['DepartmentId'])
-            # Invalidate "Teaching Room 2" (invalid Building.DepartmentId)
+                del(d['Departments'])
+            # Invalidate "Teaching Room 2" (invalid Building.Departments[0])
             if d.get('Id') == 'B001-02-0001':
-                d['DepartmentId'] = 666
+                d['Departments'] = [666]
 
         # Compare rooms causes warnings for both
         with self.assertLogs(comparison.LOG, "WARN") as log:
             comparison.op_compare_rooms()
+        print(log.output)
         self.assertTrue(set(log.output).issuperset({
-            "WARNING:essmsync.comparison:Booker room missing DepartmentId - 'B001-01-0001'",
+            "WARNING:essmsync.comparison:Booker room missing Department - 'B001-01-0001'",
             "WARNING:essmsync.comparison:Booker room has missing Department '666' - 'B001-02-0001'"
         }))
 
diff --git a/operations.md b/operations.md
index 1d71f38..cad5109 100644
--- a/operations.md
+++ b/operations.md
@@ -72,10 +72,11 @@ Get a list of departments related to the filtered rooms list (see get_rooms)
 and put them in the `booker.depts` state key.
 
 The `get_rooms` operation is called, if needed, to obtain the filtered list of
-rooms. A list of the `DepartmentId` of each room (an OptimeIndex) is compiled.
+rooms. A list of department OptimeIndexes is compiled from the first member of
+the room's `Departments` list.
 
 The `get_all_depts` operation is then called, if needed, then filtered to only
-those departments with OptimeIndexes in the compiled list of `DepartmentId`s.
+those departments with OptimeIndexes in the above compiled list.
 
 #### get_floors
 Get list of floors related to the filtered rooms list (see get_rooms) and put
-- 
GitLab


From c8efd93bf900933ecb0a84e9a48bfaad8ebfe0d4 Mon Sep 17 00:00:00 2001
From: Robin Goodall <rjg21@cam.ac.uk>
Date: Wed, 1 Dec 2021 13:15:44 +0000
Subject: [PATCH 6/6] Must include BookedBy when updating even if not changing

---
 essmsync/booker.py            | 3 +--
 essmsync/comparison.py        | 7 -------
 essmsync/tests/test_booker.py | 3 ---
 3 files changed, 1 insertion(+), 12 deletions(-)

diff --git a/essmsync/booker.py b/essmsync/booker.py
index 822c8a3..7f62750 100644
--- a/essmsync/booker.py
+++ b/essmsync/booker.py
@@ -414,8 +414,7 @@ def op_update_events():
         LOG.info(f"   Room Id/OI : '{room_id}'/'{params['RoomId']}'")
         LOG.info(f"   Building OI: '{params['BuildingId']}'")
         LOG.info(f"   Title      : '{params['Title']}'")
-        if params.get('BookedBy') is not None:
-            LOG.info(f"   Booked By  : '{params['BookedBy']['Id']}'")
+        LOG.info(f"   Booked By  : '{params['BookedBy']['Id']}'")
         try:
             booker_put(f"booker/booking/{booking['OptimeIndex']}", json=params)
         except BookerAPIBookingConflict:
diff --git a/essmsync/comparison.py b/essmsync/comparison.py
index 9d43094..b5366ce 100644
--- a/essmsync/comparison.py
+++ b/essmsync/comparison.py
@@ -622,7 +622,6 @@ def op_compare_events():
             match = matched[0]
             matched_ois.append(match.get('OptimeIndex'))
 
-            explicit_booked_by = False
             # If TermTime event has activity with CRSid user field then we expect
             # sufficient parts of BookedBy to match
             act = acts_by_code.get(act_code)
@@ -652,7 +651,6 @@ def op_compare_events():
                         'UserType': 'Staff',
                         'UserOptimeIndex': staff['OptimeIndex'],
                     }
-                    explicit_booked_by = True
 
             # Compare expection with matched booking
             if _compare_expected(expected, match, 'event', code):
@@ -672,11 +670,6 @@ def op_compare_events():
                     'RoomId': room_oi,  # Booker room OptimeIndex not actually Id
                     'BuildingId': building_oi_by_room_oi.get(room_oi),
                 }
-                # Remove the BookedBy key from the update if we weren't expected a specific crsid
-                # or it already matches
-                if not explicit_booked_by or match['BookedBy']['Id'] == expected['BookedBy']['Id']:
-                    del updated_event['BookedBy']
-
                 results['update'].append(updated_event)
 
     # Find those in Booker but not TermTime
diff --git a/essmsync/tests/test_booker.py b/essmsync/tests/test_booker.py
index 60464d6..ecbae63 100644
--- a/essmsync/tests/test_booker.py
+++ b/essmsync/tests/test_booker.py
@@ -1030,9 +1030,6 @@ class BookerUpdateTests(TestCase):
         }
         self.assertDictIncludes(put['data'], EXPECTED_TO_INCLUDE)
 
-        # also BookedBy is not sent if activity requestor hasn't changed
-        self.assertIsNone(put['data'].get('BookedBy'))
-
     def test_update_events_update_just_requestor(self):
         """
         update_events operation will send request to update an event's BookedBy
-- 
GitLab