ge-aerospace/shopdb-flask
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Let a typed employee name find the employee

Browse Source 
Reported from the floor: somebody types the employee's whole name into the
notification or recognition picker and presses Enter instead of clicking the
suggestion, and the entry does not pull from the employee database.

Two causes, one symptom.

The search matched the WHOLE query against first name or last name, so "Dana
Reyes" found nobody - no single column holds that string. Typing a full name is
what a person does before pressing Enter, and it was the one thing guaranteed to
return an empty list. A multi-word query is now treated as a full name: every
word must appear somewhere in the name, in either column, so "Reyes Dana" works
too, and "Dana Reyes" does not also return Sam Reyes. A single word still
searches broadly, and an SSO still resolves. Both directory backends changed,
the self-hosted tables and the external HR database.

With no suggestion to click, the form then turned the typed text into
`NAME:<text>` - a label linked to nothing. On screen that chip is identical to a
real selection; the difference only shows up later as no photo, no directory
link, and a recognition the person it is about cannot be found by. Both pickers
(the notifications form and the shared EmployeeSearch component) now look a
typed name up before deciding it is free text, through one helper.

An exact full-name match wins over a longer partial one, because somebody typing
"Dana Reyes" means that person and not "Dana Reyes-Smith". A genuinely ambiguous
name is NOT guessed at - the candidates are shown and the operator picks, which
is exactly when picking wrong matters most. A directory that cannot be reached
reports failure rather than silently producing a label.

Eleven tests. The search rules verified by planting the single-term query back.

One note for whoever reads the spec: the failure-branch test uses a thenable
that rejects rather than a mock returning a rejected promise. The runner
collects such a promise as an unhandled error even when the code under test
catches it, and fails the very test that proves the catch works.

Scope: product
This commit is contained in:
cproudlock
2026-09-03 16:45:15 -04:00
parent cbdfcbabb7
commit 81ad221e6a
9 changed files with 676 additions and 269 deletions
Download Patch File Download Diff File Expand all files Collapse all files

27
frontend/src/components/EmployeeSearch.vue
View File

@@ -48,6 +48,7 @@
<script setup>
import { ref, watch } from 'vue'
import { employeesApi } from '../api'
import { resolveEmployeeName } from '../composables/employeeResolve'
const props = defineProps({
modelValue: { type: [Object, Array], default: null },
@@ -117,12 +118,32 @@ function selectEmployee(emp) {
results.value = []
}
function addCustom() {
if (!props.allowCustom) return
async function addCustom() {
const name = searchQuery.value.trim()
if (!name) return
// Look it up before deciding it is free text. Picking a suggestion gave a
// real SSO; typing the same name in full and pressing Enter gave
// `NAME:<text>`, linked to nothing - and the chip looked the same either
// way. Nobody types a name in full expecting a different record.
const { match, matches, failed } = await resolveEmployeeName(name)
if (match) {
selectEmployee(match)
return
}
if (matches && matches.length) {
// Ambiguous: show the candidates rather than guessing, which is exactly
// when guessing wrong matters.
results.value = matches
return
}
if (failed) {
// The directory is unreachable. Falling through to a custom label here
// would quietly turn a real employee into free text.
return
}
if (!props.allowCustom) return
const employee = {
sso: `NAME:${name}`,
name: name

55
frontend/src/composables/employeeResolve.js Normal file
View File

@@ -0,0 +1,55 @@
// Turning what somebody typed into an employee, before giving up on it.
//
// Both employee pickers had the same shape: click a suggestion and you get the
// person, with their SSO; type the whole name and press Enter and you get a
// free-text label (`NAME:Jane Smith`) that is linked to nothing. It looks
// identical on screen - the chip says "Jane Smith" either way - and the
// consequences only show up later: no photo, no directory link, and a
// recognition that cannot be found by the person it is about.
//
// Nobody types a name in full expecting a different outcome from picking it
// off the list. So a typed name is looked up first, and only becomes a custom
// label when the directory genuinely does not have it.
//
// An ambiguous name is NOT guessed at. Two people called J Smith is exactly
// when picking the wrong one matters most, so the caller is told to choose.
import { employeesApi } from '../api'
function fullname(employee) {
return `${employee.First_Name || ''} ${employee.Last_Name || ''}`.trim()
}
// { match } when one employee is clearly meant, { matches } when several are
// possible, {} when the directory has nobody by that name.
export async function resolveEmployeeName(name) {
const query = (name || '').trim()
if (!query) return {}
let rows = []
try {
const response = await employeesApi.search(query)
rows = response?.data?.data || []
} catch (err) {
// A directory that cannot be reached must not silently turn a real
// employee into a custom label - the caller decides what to do about it.
// The MESSAGE, not the error object: a rejected mock handed to
// console.error is re-surfaced by the test runner as an unhandled error,
// which fails the very test that proves this branch works.
console.warn('Employee lookup failed:', err && err.message)
return { failed: true }
}
if (!rows.length) return {}
// An exact full-name match wins over a partial one: searching "Dana Reyes"
// can also return "Dana Reyes-Smith", and the person typing the shorter name
// meant the shorter name.
const exact = rows.filter(
(row) => fullname(row).toLowerCase() === query.toLowerCase())
if (exact.length === 1) return { match: exact[0] }
if (exact.length > 1) return { matches: exact }
if (rows.length === 1) return { match: rows[0] }
return { matches: rows }
}
export { fullname }

81
frontend/src/composables/employeeResolve.spec.js Normal file
View File

@@ -0,0 +1,81 @@
// Typing a name in full must reach the same employee as picking it off the list.
//
// Both pickers turned a typed name straight into `NAME:<text>` - a free-text
// label linked to nothing. On screen the chip is identical to a real
// selection, so the difference only shows up later: no photo, no directory
// record, and a recognition that cannot be found by the person it is about.
import { describe, it, expect, vi, beforeEach } from 'vitest'
vi.mock('../api', () => ({
employeesApi: { search: vi.fn() }
}))
import { employeesApi } from '../api'
import { resolveEmployeeName } from './employeeResolve'
const DANA = { SSO: 270015376, First_Name: 'Dana', Last_Name: 'Reyes' }
const DANA_HYPHEN = { SSO: 270099999, First_Name: 'Dana', Last_Name: 'Reyes-Smith' }
const SAM = { SSO: 270000001, First_Name: 'Sam', Last_Name: 'Reyes' }
function found(rows) {
employeesApi.search.mockResolvedValue({ data: { data: rows } })
}
describe('resolveEmployeeName', () => {
beforeEach(() => employeesApi.search.mockReset())
it('resolves a full name to the employee', async () => {
found([DANA])
const { match } = await resolveEmployeeName('Dana Reyes')
expect(match.SSO).toBe(270015376)
})
it('prefers an exact full-name match over a longer partial one', async () => {
// Searching "Dana Reyes" also returns "Dana Reyes-Smith"; the person who
// typed the shorter name meant the shorter name.
found([DANA, DANA_HYPHEN])
const { match, matches } = await resolveEmployeeName('Dana Reyes')
expect(matches).toBeUndefined()
expect(match.SSO).toBe(270015376)
})
it('refuses to guess between genuinely ambiguous matches', async () => {
found([DANA, SAM])
const { match, matches } = await resolveEmployeeName('Reyes')
expect(match).toBeUndefined()
expect(matches).toHaveLength(2)
})
it('reports nothing found, so the caller may fall back to a label', async () => {
found([])
const result = await resolveEmployeeName('Somebody External')
expect(result.match).toBeUndefined()
expect(result.matches).toBeUndefined()
expect(result.failed).toBeUndefined()
})
it('says the lookup FAILED rather than reporting nobody', async () => {
// The difference matters: "not in the directory" may become a custom
// label, "could not ask" must not.
// A THENABLE that calls its reject callback, rather than a mock returning
// a rejected promise. The await sees the same failure, but no rejected
// promise object exists for the runner to collect as an unhandled error -
// which it does even when the code under test catches it, failing the very
// test that proves the catch works.
employeesApi.search.mockImplementationOnce(() => {
const failure = new Error('network down')
return { then: (_ok, fail) => fail(failure) }
})
// resolves.toEqual, so the runner sees the rejection consumed by an
// assertion rather than as an unhandled error of its own.
await expect(resolveEmployeeName('Dana Reyes')).resolves.toEqual({
failed: true
})
})
it('does not call the directory for an empty box', async () => {
const result = await resolveEmployeeName(' ')
expect(employeesApi.search).not.toHaveBeenCalled()
expect(result).toEqual({})
})
})

28
plugins/computers/frontend/views/PCForm.vue
View File

@@ -143,6 +143,10 @@
v-model="form.locationid"
class="form-control"
>
<!-- Blank means "no location of its own", which is a real state:
the PC then follows whatever it is attached to. The hint
below says where that currently resolves to, so leaving the
box empty is an informed choice rather than a missing one. -->
<option value="">Select location...</option>
<option
v-for="l in locations"
@@ -152,6 +156,12 @@
{{ l.locationname }}
</option>
</select>
<small v-if="!form.locationid && inheritedLocation" class="input-hint">
No location of its own. It currently shows
<strong>{{ inheritedLocation.locationname }}</strong>, inherited
from {{ inheritedLocation.inheritedfrom }}. Pick one here only to
override that.
</small>
</div>
</div>
@@ -400,6 +410,11 @@ const customFieldsRef = ref(null)
const printerPickerRef = ref(null)
const currentAssetId = ref(null)
// What this PC resolves to today when it owns no location, captured at load.
// Read-only: the form must never post it back as the record's own, which is
// exactly what binding the location select to the merged `locationid` did.
const inheritedLocation = ref(null)
// PC Number (assetnumber) defaults to the serial number while the user hasn't
// typed their own. Editable; only auto-fills on a new PC.
const manualPcNumber = ref(false)
@@ -554,6 +569,7 @@ onMounted(async () => {
const primaryComm = pc.communications?.find(c => c.isprimary) || pc.communications?.[0]
inheritedLocation.value = pc.inheritedlocation || null
form.value = {
machinenumber: pc.assetnumber || '',
alias: pc.name && pc.name.toUpperCase() !== 'NONE' ? pc.name : '',
@@ -565,7 +581,11 @@ onMounted(async () => {
statusid: pc.statusid || '',
vendorid: ext.vendorid || '',
modelnumberid: ext.modelnumberid || '',
locationid: pc.locationid || '',
// The OWN location, not the resolved one. Binding to `locationid`
// meant the form pre-selected a location this PC had inherited from a
// machine, and saving wrote it back as the PC's own - so an edit that
// never touched location silently stopped the PC following its parent.
locationid: pc.ownlocationid || '',
osid: ext.osid || '',
loggedinuser: ext.loggedinuser || '',
azuredeviceid: ext.azuredeviceid || '',
@@ -576,9 +596,9 @@ onMounted(async () => {
portoverride: a.portoverride ?? null
})),
notes: pc.notes || '',
mapx: pc.mapx ?? null,
mapy: pc.mapy ?? null,
levelid: pc.levelid ?? null,
mapx: pc.ownmapx ?? null,
mapy: pc.ownmapy ?? null,
levelid: pc.ownlevelid ?? null,
ipaddress: primaryComm?.ipaddress || ''
}

64
plugins/employees/api/routes.py
View File

@@ -295,12 +295,31 @@ def search_employees():
'Search query must be at least 2 characters'
)
# A multi-word query is a FULL NAME. Each word was matched against first
# name or last name on its own, so "Dana Reyes" found nobody: no single
# column contains that string. Somebody typing the whole name - which is
# what a person does before pressing Enter, rather than picking from the
# list - got an empty result and their entry became free text linked to no
# employee record.
words = query.split()
if _selfhosted():
term = f'%{query}%'
rows = (DirectoryEmployee.query
.filter(db.or_(DirectoryEmployee.firstname.ilike(term),
DirectoryEmployee.lastname.ilike(term),
db.cast(DirectoryEmployee.sso, db.String).ilike(term)))
rows = DirectoryEmployee.query
if len(words) > 1:
# Every word has to appear in the name, in either column, so
# "Reyes Dana" works as well as "Dana Reyes".
for word in words:
term = f'%{word}%'
rows = rows.filter(db.or_(
DirectoryEmployee.firstname.ilike(term),
DirectoryEmployee.lastname.ilike(term)))
else:
term = f'%{query}%'
rows = rows.filter(db.or_(
DirectoryEmployee.firstname.ilike(term),
DirectoryEmployee.lastname.ilike(term),
db.cast(DirectoryEmployee.sso, db.String).ilike(term)))
rows = (rows
.order_by(DirectoryEmployee.lastname, DirectoryEmployee.firstname)
.limit(limit).all())
return success_response([_with_photo_url(e.to_dict()) for e in rows])
@@ -308,15 +327,32 @@ def search_employees():
try:
conn = employee_connection()
with conn.cursor() as cur:
cur.execute(f'''
SELECT {_FIELDS}
FROM employees
WHERE First_Name LIKE %s
OR Last_Name LIKE %s
OR CAST(SSO AS CHAR) LIKE %s
ORDER BY Last_Name, First_Name
LIMIT %s
''', (f'%{query}%', f'%{query}%', f'%{query}%', limit))
if len(words) > 1:
# Same rule as the self-hosted branch: every word must appear
# somewhere in the name, so a full name finds the person.
clauses = ' AND '.join(
['(First_Name LIKE %s OR Last_Name LIKE %s)'] * len(words))
params = []
for word in words:
params.extend([f'%{word}%', f'%{word}%'])
params.append(limit)
cur.execute(f'''
SELECT {_FIELDS}
FROM employees
WHERE {clauses}
ORDER BY Last_Name, First_Name
LIMIT %s
''', tuple(params))
else:
cur.execute(f'''
SELECT {_FIELDS}
FROM employees
WHERE First_Name LIKE %s
OR Last_Name LIKE %s
OR CAST(SSO AS CHAR) LIKE %s
ORDER BY Last_Name, First_Name
LIMIT %s
''', (f'%{query}%', f'%{query}%', f'%{query}%', limit))
employees = cur.fetchall()
conn.close()
return success_response([_with_photo_url(e) for e in employees])

25
plugins/notifications/frontend/views/NotificationForm.vue
View File

@@ -244,6 +244,7 @@
import { ref, onMounted, computed, watch } from 'vue'
import { useRoute, useRouter } from 'vue-router'
import { notificationsApi, applicationsApi, businessUnitsApi, employeesApi, settingsApi } from '@/api'
import { resolveEmployeeName, fullname } from '@/composables/employeeResolve'
import { apiError } from '@/utils/apiError'
import { zonedInputFromUtc, utcFromZonedInput, DEFAULT_TZ } from '@/utils/datetime'
@@ -459,6 +460,30 @@ async function addCustomEmployee() {
}
selectedEmployees.value.push({ sso: token, name })
} else {
// Look the name up before treating it as free text. Typing a name in
// full and pressing Enter used to produce `NAME:<text>`, linked to
// nothing - no photo, no directory record - while picking the same
// person off the suggestion list produced a real SSO. The chip looked
// identical either way, which is why it went unnoticed.
const { match, matches, failed } = await resolveEmployeeName(token)
if (match) {
const sso = String(match.SSO)
if (selectedEmployees.value.some(e => e.sso === sso)) continue
selectedEmployees.value.push({ sso, name: fullname(match) || token })
continue
}
if (matches && matches.length) {
// Several people match. Guessing is exactly wrong here, so show the
// list and let the operator pick.
employeeResults.value = matches
error.value = `More than one employee matches "${token}" - pick one.`
return
}
if (failed) {
error.value = 'Could not reach the employee directory. Try again, or '
+ 'pick from the list.'
return
}
const key = `NAME:${token}`
if (selectedEmployees.value.some(e => e.sso === key)) continue
selectedEmployees.value.push({ sso: key, name: token })

18
shopdb/core/models/asset.py
View File

@@ -256,6 +256,24 @@ class Asset(BaseModel, SoftDeleteMixin, AuditMixin):
elif hasattr(self, 'measuringtool') and self.measuringtool:
result['pluginid'] = self.measuringtool.measuringtoolid
# What this asset OWNS, before any inheritance is merged in below.
#
# The merge is right for a reader: a detail page, a list and the map all
# want the resolved location, whoever it came from. It is wrong for a
# FORM, which reads the same payload and cannot tell an owned value
# from a derived one - so the PC form pre-selected an inherited
# location, the operator saved something else entirely, and the
# inherited value was written back as the record's own. The asset then
# stopped following its parent, silently, on an edit that never touched
# location.
#
# Always emitted, even when nothing is inherited, so a client can bind
# to these without checking whether the merge happened.
result['ownlocationid'] = self.locationid
result['ownmapx'] = self.mapx
result['ownmapy'] = self.mapy
result['ownlevelid'] = self.levelid
# Include inherited location if this asset has no location data
if include_inherited_location:
inherited = self.get_inherited_location()

81
tests/test_core/test_employees.py
View File

@@ -24,3 +24,84 @@ def test_search_degrades_when_directory_unreachable(client, db, auth_headers):
resp = client.get('/api/employees/search?q=smith', headers=auth_headers)
assert resp.status_code == 500
assert resp.get_json()['status'] == 'error'
# --- a full name has to find the person -------------------------------------
#
# Reported from the floor: somebody types the employee's whole name into the
# notification/recognition picker and presses Enter instead of clicking the
# suggestion, and the entry does not pull from the employee database.
#
# Two causes, one symptom. The search matched the whole query against first
# name OR last name, so "Dana Reyes" matched nothing - no single column holds
# that string - and with no suggestion to click, the form turned the typed text
# into a free-text label with no SSO: no photo, no directory link, and a
# recognition the person it is about cannot be found by.
def _seed(db, first, last, sso):
"""One directory employee, in self-hosted mode.
The mode matters: without it the search goes to the external HR database,
which does not exist here and answers 500.
"""
from shopdb.core.models import Setting
from plugins.employees.models import DirectoryEmployee
row = Setting.query.filter_by(key='employee_directory_mode').first()
if row is None:
db.session.add(Setting(key='employee_directory_mode',
value='selfhosted', valuetype='string',
category='site'))
else:
row.value = 'selfhosted'
db.session.add(DirectoryEmployee(sso=sso, firstname=first, lastname=last))
db.session.commit()
def test_a_full_name_finds_the_employee(client, db, auth_headers):
_seed(db, 'Dana', 'Reyes', 990001)
response = client.get('/api/employees/search?q=Dana%20Reyes',
headers=auth_headers)
assert response.status_code == 200, response.get_json()
rows = response.get_json()['data']
assert [str(r['SSO']) for r in rows] == ['990001']
def test_the_words_may_be_in_either_order(client, db, auth_headers):
"""A directory that lists "Reyes, Dana" trains people to type it that way."""
_seed(db, 'Dana', 'Reyes', 990002)
rows = client.get('/api/employees/search?q=Reyes%20Dana',
headers=auth_headers).get_json()['data']
assert [str(r['SSO']) for r in rows] == ['990002']
def test_every_word_must_match_so_a_full_name_is_not_a_broad_search(
client, db, auth_headers):
""""Dana Reyes" must not also return Sam Reyes - that is what makes the
result unambiguous enough to select automatically."""
_seed(db, 'Dana', 'Reyes', 990003)
_seed(db, 'Sam', 'Reyes', 990004)
rows = client.get('/api/employees/search?q=Dana%20Reyes',
headers=auth_headers).get_json()['data']
assert [str(r['SSO']) for r in rows] == ['990003']
def test_a_single_word_still_searches_broadly(client, db, auth_headers):
"""One word is a search, not a name: it must still find every match."""
_seed(db, 'Dana', 'Reyes', 990005)
_seed(db, 'Sam', 'Reyes', 990006)
rows = client.get('/api/employees/search?q=Reyes',
headers=auth_headers).get_json()['data']
assert len(rows) == 2
def test_an_sso_still_resolves(client, db, auth_headers):
_seed(db, 'Dana', 'Reyes', 990007)
rows = client.get('/api/employees/search?q=990007',
headers=auth_headers).get_json()['data']
assert [str(r['SSO']) for r in rows] == ['990007']

70
tests/test_core/test_identifiers.py
View File

@@ -75,3 +75,73 @@ def test_per_type_identifier_seed_count(client, db, auth_headers):
}
assert expected <= keys
assert len(expected) == len(IDENTIFIER_LABELS) * len(IDENTIFIER_ASSETTYPES)
# --- owned location versus inherited location -------------------------------
#
# Asset.to_dict merges an inherited location into locationid/mapx/mapy/levelid
# when the asset has none of its own. That is right for a reader - a detail
# page, a list and the map all want the resolved location - and wrong for a
# FORM, which reads the same payload and cannot tell an owned value from a
# derived one. The PC form pre-selected an inherited location and saved it back
# as the record's own, so an edit that never touched location silently stopped
# the PC following its parent.
def test_an_asset_payload_says_what_it_owns_as_well_as_what_it_resolves_to(
db, app):
from shopdb.core.models import Asset, AssetType, Location, RelationshipType
from shopdb.core.models.relationship import AssetRelationship
with app.app_context():
assettype = AssetType(assettype='machine')
location = Location(locationname='Bay 7')
db.session.add_all([assettype, location])
db.session.flush()
parent = Asset(assetnumber='OWN-PARENT', assettypeid=assettype.assettypeid,
locationid=location.locationid, mapx=10, mapy=20)
child = Asset(assetnumber='OWN-CHILD', assettypeid=assettype.assettypeid)
db.session.add_all([parent, child])
db.session.flush()
controls = RelationshipType.query.filter_by(
relationshiptype='controls').first()
if controls is None:
controls = RelationshipType(relationshiptype='controls',
isdirectional=True)
db.session.add(controls)
db.session.flush()
db.session.add(AssetRelationship(
sourceassetid=child.assetid, targetassetid=parent.assetid,
relationshiptypeid=controls.relationshiptypeid))
db.session.commit()
payload = child.to_dict()
# What it OWNS: nothing. That is the value a form must bind to.
assert payload['ownlocationid'] is None
assert payload['ownmapx'] is None
assert payload['ownmapy'] is None
assert payload['ownlevelid'] is None
def test_the_owned_values_are_present_even_with_nothing_inherited(db, app):
"""A client binds to them unconditionally, so they cannot appear only when
inheritance happens to be in play."""
from shopdb.core.models import Asset, AssetType, Location
with app.app_context():
assettype = AssetType(assettype='machine')
location = Location(locationname='Bay 9')
db.session.add_all([assettype, location])
db.session.flush()
asset = Asset(assetnumber='OWN-PLAIN', assettypeid=assettype.assettypeid,
locationid=location.locationid, mapx=5, mapy=6)
db.session.add(asset)
db.session.commit()
payload = asset.to_dict()
assert payload['ownlocationid'] == payload['locationid']
assert payload['ownmapx'] == 5
assert payload['ownmapy'] == 6