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