diff --git a/plugins/employees/api/routes.py b/plugins/employees/api/routes.py index 2cbccf3..72162e1 100644 --- a/plugins/employees/api/routes.py +++ b/plugins/employees/api/routes.py @@ -147,7 +147,13 @@ def resolve_employee_display_name(sso): if name: return name except Exception: - pass + # Logged, not swallowed. This is the branch that fails on a live + # site - an unreachable HR host, a rotated credential, a renamed + # column - and a bare pass made all of those look identical to + # "no such employee": a lowercase SSO where a name should be, and + # no photo, with nothing written down anywhere. + current_app.logger.exception( + 'Employee directory lookup failed for SSO %s', sso) # 2. Fallback: a shopdb login account for this SSO (e.g. a user from another # location not carried in the directory). Their name lives on the User @@ -160,7 +166,8 @@ def resolve_employee_display_name(sso): if name: return name except Exception: - pass + current_app.logger.exception( + 'User-account name fallback failed for SSO %s', sso) return None @@ -189,6 +196,83 @@ def _with_photo_url(employee): return employee +@employees_bp.route('/resolve/', methods=['GET']) +@jwt_required() +def resolve_employee(sso): + """Resolve one SSO to a name and photo, and say HOW. + + Built because the failure it diagnoses is invisible. The shopfloor board + resolves names and photos through two functions that returned None on any + problem, so an unreachable HR host, a rotated credential and a genuinely + unknown SSO all produced the same thing: a lowercase SSO where a name + should be, and no photo. Nothing distinguished them and nothing was logged. + + This returns the same answer the board gets, plus which source produced it, + which mode the directory is in, and the error when a source failed. It is + the difference between "the board is broken" and "the HR host refused the + connection at 09:12". + """ + result = { + 'sso': sso, + 'mode': 'selfhosted' if _selfhosted() else 'external', + 'name': None, + 'photourl': None, + 'source': None, + 'error': None, + } + + if sso is None or not str(sso).isdigit(): + result['error'] = 'SSO must be digits' + return success_response(result) + + if _selfhosted(): + employee = db.session.get(DirectoryEmployee, int(sso)) + if employee: + name = '{} {}'.format(employee.firstname or '', + employee.lastname or '').strip() + if name: + result['name'] = name + result['source'] = 'directory' + else: + try: + conn = employee_connection() + with conn.cursor() as cur: + cur.execute( + 'SELECT First_Name, Last_Name FROM employees WHERE SSO = %s', + (int(sso),)) + row = cur.fetchone() + conn.close() + if row: + name = '{} {}'.format((row.get('First_Name') or '').strip(), + (row.get('Last_Name') or '').strip()).strip() + if name: + result['name'] = name + result['source'] = 'hrdirectory' + else: + result['error'] = 'No row in the HR directory for this SSO' + except Exception as exception: + # Reported, not hidden. This string is the whole point of the + # endpoint. + result['error'] = '{}: {}'.format(type(exception).__name__, exception) + current_app.logger.exception( + 'Employee directory lookup failed for SSO %s', sso) + + if not result['name']: + from shopdb.api import User + user = User.query.filter_by(username=str(sso)).first() + if user: + name = '{} {}'.format((user.firstname or '').strip(), + (user.lastname or '').strip()).strip() + if name: + result['name'] = name + result['source'] = 'useraccount' + + result['photourl'] = resolve_employee_photo_url(sso) + if result['name'] is None and result['error'] is None: + result['error'] = 'No name found in the directory or in a user account' + return success_response(result) + + @employees_bp.route('/search', methods=['GET']) def search_employees(): """ diff --git a/tests/test_plugins/test_employees_resolve.py b/tests/test_plugins/test_employees_resolve.py new file mode 100644 index 0000000..8f074a9 --- /dev/null +++ b/tests/test_plugins/test_employees_resolve.py @@ -0,0 +1,68 @@ +"""Resolving an SSO says HOW it resolved, or why it did not. + +Built after the shopfloor board lost every photo and showed lowercase SSOs +instead of names, and nothing in the system could say why. Both resolvers +returned None on any problem, so an unreachable HR host, a rotated credential +and a genuinely unknown SSO were indistinguishable - and none of them were +logged. This endpoint exists to tell those apart. +""" + +URL = '/api/employees/resolve/' + + +def _selfhosted(db): + from shopdb.core.models import Setting + row = Setting.query.filter_by(key='employee_directory_mode').first() + if row: + row.value = 'selfhosted' + else: + db.session.add(Setting(key='employee_directory_mode', value='selfhosted')) + db.session.commit() + + +def test_a_known_employee_resolves_and_names_its_source(client, db, auth_headers): + from plugins.employees.models import DirectoryEmployee + _selfhosted(db) + db.session.add(DirectoryEmployee(sso=502123, firstname='Ada', + lastname='Lovelace')) + db.session.commit() + + data = client.get(URL + '502123', headers=auth_headers).get_json()['data'] + assert data['name'] == 'Ada Lovelace' + assert data['source'] == 'directory' + assert data['mode'] == 'selfhosted' + assert data['error'] is None + + +def test_an_unknown_sso_says_so_rather_than_returning_nothing(client, db, + auth_headers): + """The board shows a lowercase SSO for this case AND for a broken + directory. The whole point is that they now read differently.""" + _selfhosted(db) + data = client.get(URL + '999999', headers=auth_headers).get_json()['data'] + assert data['name'] is None + assert data['error'] + + +def test_a_user_account_answers_when_the_directory_does_not(client, db, + auth_headers): + """Someone from another site has no directory row but does have a login. + That fallback exists; without this endpoint nothing showed it had fired.""" + from shopdb.core.models import User + _selfhosted(db) + db.session.add(User(username='701234', firstname='Grace', lastname='Hopper', + email='g@example.com', passwordhash='x')) + db.session.commit() + + data = client.get(URL + '701234', headers=auth_headers).get_json()['data'] + assert data['name'] == 'Grace Hopper' + assert data['source'] == 'useraccount' + + +def test_a_non_numeric_sso_is_rejected_clearly(client, db, auth_headers): + data = client.get(URL + 'abc', headers=auth_headers).get_json()['data'] + assert data['error'] == 'SSO must be digits' + + +def test_the_endpoint_needs_a_login(client, db): + assert client.get(URL + '502123').status_code == 401