diff --git a/gi_service/contract/rest/organisation_api_contract.yaml b/gi_service/contract/rest/organisation_api_contract.yaml index c3d7990..a954227 100644 --- a/gi_service/contract/rest/organisation_api_contract.yaml +++ b/gi_service/contract/rest/organisation_api_contract.yaml @@ -425,6 +425,8 @@ paths: items: type: object properties: + department_name: + type: string ministry_id: type: string ministry_name: @@ -436,7 +438,14 @@ paths: period: type: string example: - - ministry_id: "min_01" + - department_name: "Department of Treasury Operations" + ministry_id: "min_01" + ministry_name: "Ministry of Finance" + minister_id: "pers_01" + minister_name: "John Doe" + period: "2021 Jan 01 - 2022 Jan 01" + - department_name: "Department of Treasury Operations" + ministry_id: "min_01" ministry_name: "Ministry of Finance" minister_id: "pers_01" minister_name: "John Doe" @@ -971,4 +980,4 @@ components: name: type: string id: - type: string \ No newline at end of file + type: string diff --git a/src/services/organisation_service.py b/src/services/organisation_service.py index 00bc260..c15f8bf 100644 --- a/src/services/organisation_service.py +++ b/src/services/organisation_service.py @@ -678,20 +678,38 @@ async def department_history_timeline(self, department_id: str): try: # 1. Get Lineage and Initial Relations department_ids = await self._get_renamed_lineage(department_id) + department_id_list = list(department_ids) - ministry_department_relation_map = await self._fetch_and_map_relations( - list(department_ids), - Relation(name="AS_DEPARTMENT", direction="INCOMING") + department_info_map, ministry_department_relation_map = await asyncio.gather( + self._fetch_and_map_entities(department_id_list), + self._fetch_and_map_relations( + department_id_list, + Relation(name="AS_DEPARTMENT", direction="INCOMING") + ) ) + + department_name_map = { + entity_id: Util.decode_protobuf_attribute_name(entity.name) + for entity_id, entity in department_info_map.items() + } # Filter out same startTime and endTime min-dep relations all_ministry_department_relations = [ - relation for relations in ministry_department_relation_map.values() for relation in relations + { + "department_id": current_department_id, + "department_name": department_name_map.get(current_department_id, ""), + "relation": relation, + } + for current_department_id, relations in ministry_department_relation_map.items() + for relation in relations if relation.startTime != relation.endTime ] # 2. Fetch all Ministry Info and Person Appointment relations in parallel - ministry_ids = list(set(relation.relatedEntityId for relation in all_ministry_department_relations)) + ministry_ids = list(set( + relation_item["relation"].relatedEntityId + for relation_item in all_ministry_department_relations + )) ministry_info_map, person_appointment_relation_map = await asyncio.gather( self._fetch_and_map_entities(ministry_ids), @@ -704,13 +722,16 @@ async def department_history_timeline(self, department_id: str): # 4. Filter person appointments that overlap with this specific ministry-department period enriched = [] - for ministry_department_relation in all_ministry_department_relations: + for ministry_department_item in all_ministry_department_relations: + ministry_department_relation = ministry_department_item["relation"] ministry_id = ministry_department_relation.relatedEntityId ministry_entity = ministry_info_map.get(ministry_id) if not ministry_entity: continue ministry_name = Util.decode_protobuf_attribute_name(ministry_entity.name) + current_department_name = ministry_department_item["department_name"] + current_department_id = ministry_department_item["department_id"] relevant_persons = [] for person_appointment in person_appointment_relation_map.get(ministry_id, []): @@ -723,6 +744,8 @@ async def department_history_timeline(self, department_id: str): if overlap_start < overlap_end: relevant_persons.append({ + "_department_source_id": current_department_id, + "department_name": current_department_name, "ministry_id": ministry_id, "ministry_name": ministry_name, "minister_id": person_entity.id, @@ -739,15 +762,21 @@ async def department_history_timeline(self, department_id: str): for person in relevant_persons: if current_time < person["startTime"]: enriched.append({ + "_department_source_id": current_department_id, + "department_name": current_department_name, "ministry_id": ministry_id, "ministry_name": ministry_name, - "minister_id": None, "startTime": current_time, "endTime": person["startTime"] + "minister_id": None, "minister_name": None, + "startTime": current_time, "endTime": person["startTime"] }) current_time = person["endTime"] if current_time < relation_end: enriched.append({ + "_department_source_id": current_department_id, + "department_name": current_department_name, "ministry_id": ministry_id, "ministry_name": ministry_name, - "minister_id": None, "startTime": current_time, "endTime": relation_end + "minister_id": None, "minister_name": None, + "startTime": current_time, "endTime": relation_end }) enriched.extend(relevant_persons) @@ -780,6 +809,7 @@ async def department_history_timeline(self, department_id: str): collapsed = [] for entry in enriched: if collapsed and (collapsed[-1]["minister_id"] == entry["minister_id"] and + collapsed[-1]["_department_source_id"] == entry["_department_source_id"] and collapsed[-1]["ministry_name"] == entry["ministry_name"] and collapsed[-1]["endTime"] >= entry["startTime"]): collapsed[-1]["endTime"] = max(collapsed[-1]["endTime"], entry["endTime"]) @@ -789,10 +819,12 @@ async def department_history_timeline(self, department_id: str): # 7. Final Sort and clean up collapsed.sort(key=lambda x: x["startTime"], reverse=True) for entry in collapsed: + # Keep raw timestamps until the last step because the gap-filling and + # collapse logic depends on overlap comparisons. actual_end = None if entry["endTime"] == FAR_FUTURE else entry["endTime"] entry["period"] = Util.term(entry["startTime"], actual_end, get_full_date=True) - for key in ["startTime", "endTime"]: + for key in ["startTime", "endTime", "_department_source_id"]: entry.pop(key, None) return collapsed @@ -800,4 +832,3 @@ async def department_history_timeline(self, department_id: str): except Exception as e: logger.error(f"Error in enrich_department_timeline: {e}") raise InternalServerError("An unexpected error occurred") from e - diff --git a/test/test_document_service.py b/test/test_document_service.py index 39dcdff..04c16df 100644 --- a/test/test_document_service.py +++ b/test/test_document_service.py @@ -64,8 +64,14 @@ async def test_get_gazette_data_points_empty(document_service, mock_opengin_serv async def test_get_gazette_data_points_internal_server_error( document_service, mock_opengin_service ): + mock_opengin_service.get_entities.side_effect = [ + [MagicMock(id="org_gazette_1", created="2023-01-15T10:00:00Z")], + [], + ] + with patch( - "src.services.document_service.asyncio.gather", side_effect=Exception("failure") + "src.services.document_service.Util.normalize_timestamp", + side_effect=Exception("failure"), ): with pytest.raises(InternalServerError): await document_service.get_gazette_data_points() @@ -142,4 +148,3 @@ async def test_get_gazette_data_points_robustness(document_service, mock_opengin assert result["data"][1]["year"] == 2024 assert result["data"][1]["values"][4] == 1 # May - diff --git a/test/test_organisation_service.py b/test/test_organisation_service.py index 9e84959..58e2baa 100644 --- a/test/test_organisation_service.py +++ b/test/test_organisation_service.py @@ -526,6 +526,8 @@ async def fetch_relation_handler(entityId, relation): async def get_entities_handler(entity): mapping = { + "dep_01": "4465706172746d656e74204f6e65", + "dep_02": "4465706172746d656e742054776f", "min_01": "4d696e6973747279204f6e65", "min_02": "4d696e69737472792054776f", "pers_01": "4d696e69737465722041", @@ -562,13 +564,17 @@ async def get_entities_handler(entity): assert len(result) == 6 assert result[0]["minister_name"] == "President X" + assert result[0]["department_name"] == "Department Two" assert result[1]["minister_name"] == "Minister A" assert result[1]["ministry_name"] == "Ministry Two" + assert result[1]["department_name"] == "Department Two" assert result[4]["minister_name"] == "Minister A" assert result[4]["ministry_name"] == "Ministry One" + assert result[4]["department_name"] == "Department One" assert "period" in result[0] assert "startTime" not in result[0] assert "endTime" not in result[0] + assert "_department_source_id" not in result[0] @pytest.mark.asyncio @@ -614,6 +620,13 @@ async def fetch_relation_handler(entityId, relation): return [] async def get_entities_handler(entity): + if entity.id == "dep_01": + return [ + Entity( + id="dep_01", + name='{"value": "4465706172746d656e74206f66204d65646961"}', + ) + ] if entity.id in ["min_01", "min_02"]: # "Ministry of Media" in hex return [ @@ -636,9 +649,75 @@ async def get_entities_handler(entity): # Should collapse into ONE entry because same name and same person across min_01 and min_02 assert len(result) == 1 assert result[0]["minister_name"] == "Ranil" + assert result[0]["department_name"] == "Department of Media" assert result[0]["period"] == "2020-01-01 - 2022-01-01" +@pytest.mark.asyncio +async def test_department_history_timeline_keeps_renamed_departments_separate( + organisation_service, mock_opengin_service +): + department_id = "dep_01" + + async def fetch_relation_handler(entityId, relation): + if relation.name == "RENAMED_TO": + return [Relation(relatedEntityId="dep_02")] if entityId == "dep_01" else [] + if relation.name == "AS_DEPARTMENT": + if entityId == "dep_01": + return [ + Relation( + relatedEntityId="min_01", + startTime="2020-01-01T00:00:00Z", + endTime="2021-01-01T00:00:00Z", + ) + ] + if entityId == "dep_02": + return [ + Relation( + relatedEntityId="min_02", + startTime="2021-01-01T00:00:00Z", + endTime="2022-01-01T00:00:00Z", + ) + ] + if relation.name == "AS_APPOINTED": + return [ + Relation( + relatedEntityId="pers_01", + startTime="2020-01-01T00:00:00Z", + endTime="2022-01-01T00:00:00Z", + ) + ] + return [] + + async def get_entities_handler(entity): + mapping = { + "dep_01": "4465706172746d656e74204f6e65", + "dep_02": "4465706172746d656e742054776f", + "min_01": "4d696e6973747279206f66204d65646961", + "min_02": "4d696e6973747279206f66204d65646961", + "pers_01": "52616e696c", + } + name_hex = mapping.get(entity.id) + return ( + [Entity(id=entity.id, name=f'{{"value": "{name_hex}"}}')] + if name_hex + else [] + ) + + mock_opengin_service.fetch_relation.side_effect = fetch_relation_handler + mock_opengin_service.get_entities.side_effect = get_entities_handler + + result = await organisation_service.department_history_timeline( + department_id=department_id + ) + + assert len(result) == 2 + assert result[0]["department_name"] == "Department Two" + assert result[0]["period"] == "2021-01-01 - 2022-01-01" + assert result[1]["department_name"] == "Department One" + assert result[1]["period"] == "2020-01-01 - 2021-01-01" + + @pytest.mark.asyncio async def test_get_renamed_lineage_chain(organisation_service, mock_opengin_service): # Chain: A -> B -> C diff --git a/test/test_person_service.py b/test/test_person_service.py index 130c7dc..ce28ecb 100644 --- a/test/test_person_service.py +++ b/test/test_person_service.py @@ -160,9 +160,35 @@ async def test_fetch_person_history_internal_error( person_service, mock_opengin_service ): person_id = "person_123" - with patch( - "src.services.person_service.asyncio.gather", - side_effect=Exception("Gather fail"), + mock_opengin_service.fetch_relation.side_effect = [ + [ + Relation( + relatedEntityId="min_1", + startTime="2020-01-01T00:00:00Z", + endTime="2021-01-01T00:00:00Z", + ) + ], + [], + ] + + with ( + patch.object( + person_service, + "enrich_history_item", + new_callable=AsyncMock, + return_value={ + "id": "min_1", + "name": "Ministry One", + "term": "2020-01-01 - 2021-01-01", + "is_president": False, + "start_time": "2020-01-01T00:00:00Z", + "end_time": "2021-01-01T00:00:00Z", + }, + ), + patch( + "src.services.person_service.Util.history_sort_key", + side_effect=Exception("Sort fail"), + ), ): with pytest.raises(InternalServerError): await person_service.fetch_person_history(person_id) @@ -699,4 +725,3 @@ async def test_fetch_all_presidents_internal_error(person_service, mock_opengin_ with pytest.raises(InternalServerError): await person_service.fetch_all_presidents() -