diff --git a/public/js/admin/clinicalAssistant.js b/public/js/admin/clinicalAssistant.js index d2e6c837..fa83b777 100644 --- a/public/js/admin/clinicalAssistant.js +++ b/public/js/admin/clinicalAssistant.js @@ -408,12 +408,17 @@ export function initClinicalAssistantAdmin(adminEscapeHtml) { putAssistantConfig('clinical_assistant.allowed_image_models', checkedAssistantModels('assistant-allowed-image-models').join(',')), putAssistantConfig('my_resources.review_model', getValue('mr-review-model')), saveImageSettings() - ]).then(function() { + ]).then(function(results) { var reviewer = getValue('mr-review-model'); + // The image models named back. "Saved" does not tell an administrator + // that the model they chose is the one that will draw — which is the + // whole question, and the reason this card was hard to trust. + var images = Array.isArray(results[results.length - 1]) ? results[results.length - 1] : []; if (status) { status.textContent = 'Saved ' + new Date().toLocaleTimeString() + '. ' + - (reviewer ? 'Decks will be reviewed by ' + reviewer + '.' : 'Slide review is off.') + - ' New image jobs use these settings; existing jobs are unchanged.'; + (images.length ? 'Images — ' + images.join('; ') + '. ' : '') + + (reviewer ? 'Decks reviewed by ' + reviewer + '. ' : 'Slide review is off. ') + + 'New image jobs use these settings; existing jobs are unchanged.'; } showToast('Availability saved', 'success'); }).catch(function(err) { diff --git a/public/js/admin/imageSettings.js b/public/js/admin/imageSettings.js index 07b82de7..ba856f5f 100644 --- a/public/js/admin/imageSettings.js +++ b/public/js/admin/imageSettings.js @@ -50,21 +50,18 @@ async function load() { group.appendChild(legend); // My Resources picks its model per request from the roster, so only the - // Only the Assistant names a primary here. My Resources takes its model - // from the request that asks for the picture, so a disabled, permanently - // empty dropdown labelled "set per request" was a control that could - // never do anything — it read as broken rather than as not applicable. - // Its fallbacks still belong here: those are what runs when the chosen - // model fails, whoever chose it. + // Both workflows name a model. My Resources' was saved and ignored for a + // while — the generator read the Assistant's setting instead — so the + // field was disabled and labelled "set per request", describing something + // that happens nowhere. It works now, and blank means "use the Clinical + // Assistant's", which is what every install did before it worked. const primary = makeSelect([saved.model], discovered); + group.appendChild(row('Model', primary)); if (workflow.key === 'my_resources') { const note = document.createElement('p'); - note.style.cssText = 'margin:2px 0 8px;font-size:12px;color:var(--g500);'; - note.textContent = 'The model is chosen per request, where the deck is generated. ' + - 'The fallbacks below apply to whichever model that request picked.'; + note.style.cssText = 'margin:2px 0 10px;font-size:12px;color:var(--g500);'; + note.textContent = 'Leave blank to use the Clinical Assistant\'s image model above.'; group.appendChild(note); - } else { - group.appendChild(row('Model', primary)); } const fallbacks = []; @@ -100,18 +97,26 @@ async function load() { // settings that were not sent. export async function saveImageSettings() { if (!loaded) throw new Error('Image settings were not loaded, so they were not saved.'); + // What was written, for the card's confirmation. "Saved" alone does not + // answer the question an admin actually has, which is whether the model they + // just picked is the one that will be used. + const written = []; for (const workflow of WORKFLOWS) { const control = controls[workflow.key]; const body = { budget: Number(control.budget.value), fallbacks: control.fallbacks.map(s => s.value).filter(Boolean) }; - // Only the workflows that own a primary send one; My Resources would - // be overwriting a per-request choice with a form field. - if (workflow.key !== 'my_resources') body.model = control.primary.value; + // Every workflow sends its model. My Resources' used to be withheld, + // which is why its field could never be set to anything. + body.model = control.primary.value; await imageJson('/api/admin/image-settings/' + workflow.key, { method: 'PUT', body: JSON.stringify(body) }); + written.push(workflow.label + ': ' + + (body.model || 'the Clinical Assistant\'s model') + + (body.fallbacks.length ? ', then ' + body.fallbacks.join(', ') : ', no fallback')); } + return written; } function row(labelText, control) { diff --git a/src/routes/myResources.js b/src/routes/myResources.js index 7e3a9ac9..e8f11b3c 100644 --- a/src/routes/myResources.js +++ b/src/routes/myResources.js @@ -209,6 +209,24 @@ router.get('/my-resources/image/jobs/:id', async function (req, res) { } }); +// Which model draws the figures in a resource. +// +// Its own setting, with the Clinical Assistant's as the fallback. This used to +// read the Assistant's and only the Assistant's, while the admin screen saved +// my_resources.image_model — so that field was stored, returned by the API, +// shown in the form, and never used. Somebody noticed the field did nothing and +// disabled it rather than finding out why, which left a control that could not +// be changed and a note saying the model was "chosen per request", which is not +// a thing that happens anywhere. +// +// Blank still means "use the Assistant's", so an install that only ever set one +// model keeps working without anyone touching the screen. +async function resourceImageModel() { + var own = String(await db.getSetting('my_resources.image_model', '') || ''); + if (own) return own; + return String(await db.getSetting('clinical_assistant.image_model', '') || ''); +} + router.get('/my-resources/options', async function (req, res) { try { var models = await allowedModels(); @@ -216,7 +234,7 @@ router.get('/my-resources/options', async function (req, res) { success: true, models: models.allowed, defaultModel: models.configured, - imagesAvailable: Boolean(await db.getSetting('clinical_assistant.image_model', '')), + imagesAvailable: Boolean(await resourceImageModel()), // Whether this person has somewhere to send a file. The button is hidden // rather than shown-and-refused: an action that always fails is worse // than an action that is not offered. @@ -275,7 +293,7 @@ async function gatherSources(subject, body, keywords) { // Asking for an illustration when no image model is configured is not an // error, it just cannot happen; the caller reports that rather than failing. - var imageModel = wantsImages ? String(await db.getSetting('clinical_assistant.image_model', '') || '') : ''; + var imageModel = wantsImages ? await resourceImageModel() : ''; return { corpus: corpus, searches: searches, literature: literature, webFindings: webFindings, // A search that was asked for and came back empty is the dangerous case: the diff --git a/test/frontend-prompt-env.test.js b/test/frontend-prompt-env.test.js index 44dd0197..2fd1a38a 100644 --- a/test/frontend-prompt-env.test.js +++ b/test/frontend-prompt-env.test.js @@ -421,9 +421,12 @@ test('image-model dropdowns keep saved selections through discovery failures and 'Learning Hub is gone; saving to it is what broke the card'); assert.deepEqual(puts().find(c => c.url.endsWith('clinical_assistant')).body, { model: 'saved-image', budget: 32000, fallbacks: ['saved-backup'] }); - // My Resources chooses its model per request, so the form must not send one. + // My Resources sends its model like every other workflow. It used to be + // withheld — the generator read the Clinical Assistant's setting instead, so + // my_resources.image_model was saved and ignored, and the field was disabled + // rather than fixed. Blank is meaningful: it means "use the Assistant's". assert.deepEqual(puts().find(c => c.url.endsWith('my_resources')).body, - { budget: 32000, fallbacks: [] }); + { model: '', budget: 32000, fallbacks: [] }); assert.equal(select.value, 'saved-image', 'save never reverts the selection'); }); @@ -513,3 +516,25 @@ test('catalogue loading failure always reaches a visible retry state, never an e assert.doesNotMatch(f.textContent, /Loading/, 'no eternal spinner text remains'); } }); + +test('saving names the image models back, so a choice can be seen to have taken', async t => { + // "Saved 8:31:59 PM" answered a different question from the one an admin has, + // which is whether the model they just picked is the one that will draw. + const ui = await browser(t, 'admin', url => { + if (url === '/api/admin/config') return json(assistantConfig()); + if (url === '/api/admin/image-settings') return json({ success: true, maxModels: 3, workflows: { + clinical_assistant: { model: 'draws-this', fallbacks: ['then-this'], budget: 32000 }, + my_resources: { model: '', fallbacks: [], budget: 32000 } } }); + if (url.endsWith('/image-models/discover')) return json({ models: [{ id: 'draws-this' }, { id: 'then-this' }] }); + if (url.includes('/api/admin/image-settings/')) return json({ success: true }); + }); + for (let i = 0; i < 6; i++) await tick(); + ui.document.getElementById('btn-save-availability').click(); + for (let i = 0; i < 10; i++) await tick(); + + const said = ui.document.getElementById('assistant-availability-status').textContent; + assert.match(said, /Clinical Assistant: draws-this, then then-this/); + // Blank is meaningful and is spelled out rather than shown as an empty gap. + assert.match(said, /My Resources: the Clinical Assistant's model/); + assert.match(said, /existing jobs are unchanged/); +});