Skip to content

Commit a20d029

Browse files
stefan-ernststefan-ernst
committed
Permission fix to give Testers comment permission and updates to CustomFieldRenderer tests, fixing more linting issues
1 parent 49416fc commit a20d029

47 files changed

Lines changed: 494 additions & 36 deletions

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

‎frontend/src/lib/features/items/CustomFieldRenderer.edge.test.js‎

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -105,16 +105,20 @@ describe('malformed field.options', () => {
105105
});
106106

107107
describe('type-mismatched values (data drift)', () => {
108-
test('checkbox with the string "false" renders as common.yes (truthy coercion)', () => {
109-
// Subtle quirk: the renderer uses `v ? yes : no`. The string "false"
110-
// is truthy in JS, so a checkbox field with stale string data shows
111-
// as "yes". Pinning to flag this if the renderer adds smarter
112-
// boolean coercion later.
108+
test('checkbox with the string "false" renders as common.no', () => {
113109
renderReadonly({
114110
field: { field_type: 'checkbox', name: 'Done' },
115111
value: 'false',
116112
});
117-
expect(screen.getByText('common.yes')).toBeInTheDocument();
113+
expect(screen.getByText('common.no')).toBeInTheDocument();
114+
});
115+
116+
test('checkbox with the string "0" renders as common.no', () => {
117+
renderReadonly({
118+
field: { field_type: 'checkbox', name: 'Done' },
119+
value: '0',
120+
});
121+
expect(screen.getByText('common.no')).toBeInTheDocument();
118122
});
119123

120124
test('checkbox with the number 0 renders as common.no (falsy)', () => {

‎frontend/src/lib/features/items/CustomFieldRenderer.svelte‎

Lines changed: 15 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -98,6 +98,17 @@
9898
return inputValue || '';
9999
}
100100
101+
function coerceCheckboxValue(raw) {
102+
if (typeof raw === 'boolean') return raw;
103+
if (typeof raw === 'number') return raw !== 0;
104+
if (typeof raw === 'string') {
105+
const normalized = raw.trim().toLowerCase();
106+
if (['false', '0', 'no', 'off'].includes(normalized)) return false;
107+
if (['true', '1', 'yes', 'on'].includes(normalized)) return true;
108+
}
109+
return Boolean(raw);
110+
}
111+
101112
// Helper to render value text for display
102113
function renderDisplayValue() {
103114
if (value === null || value === undefined || value === '') {
@@ -149,7 +160,7 @@
149160
}
150161
return v;
151162
case 'checkbox':
152-
return v ? t('common.yes') : t('common.no');
163+
return coerceCheckboxValue(v) ? t('common.yes') : t('common.no');
153164
case 'number':
154165
const num = parseFloat(v);
155166
return isNaN(num) ? v : num.toString();
@@ -290,7 +301,7 @@
290301
</div>
291302
{:else if field.field_type === 'checkbox'}
292303
<CheckSquare class="w-4 h-4 flex-shrink-0" style="color: var(--ds-text-subtle);" />
293-
<span style="color: var(--ds-text);">{value ? t('common.yes') : t('common.no')}</span>
304+
<span style="color: var(--ds-text);">{coerceCheckboxValue(value) ? t('common.yes') : t('common.no')}</span>
294305
{:else if field.field_type === 'email'}
295306
<Mail class="w-4 h-4 flex-shrink-0" style="color: var(--ds-text-subtle);" />
296307
<span style="color: var(--ds-text);">{value}</span>
@@ -401,7 +412,7 @@
401412
{:else if field.field_type === 'checkbox'}
402413
<div class="flex items-center gap-2">
403414
<CheckSquare class="w-4 h-4" style="color: var(--ds-text-subtle);" />
404-
<span style="color: var(--ds-text);">{value ? t('common.yes') : t('common.no')}</span>
415+
<span style="color: var(--ds-text);">{coerceCheckboxValue(value) ? t('common.yes') : t('common.no')}</span>
405416
</div>
406417
{:else if field.field_type === 'email'}
407418
<div class="flex items-center gap-2">
@@ -623,7 +634,7 @@
623634
{:else if field.field_type === 'checkbox'}
624635
<div use:clickOutside onclickOutside={() => onCancel?.()} class="px-3 py-2">
625636
<Checkbox
626-
checked={!!value}
637+
checked={coerceCheckboxValue(value)}
627638
{disabled}
628639
onchange={(checked) => onChange(checked)}
629640
/>

‎frontend/src/lib/features/items/CustomFieldRenderer.test.js‎

Lines changed: 74 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { render, screen, waitFor } from '@testing-library/svelte';
1+
import { fireEvent, render, screen, waitFor } from '@testing-library/svelte';
22
import { afterEach, beforeAll, describe, expect, test, vi } from 'vitest';
33

44
// Mock the api module — the renderer calls api.getUsers() for the user
@@ -64,6 +64,18 @@ function renderStatic(props) {
6464
});
6565
}
6666

67+
function renderEdit(props) {
68+
return render(CustomFieldRenderer, {
69+
props: {
70+
readonly: false,
71+
onChange: vi.fn(),
72+
onCancel: vi.fn(),
73+
field: { field_type: 'text', name: 'Label' },
74+
...props,
75+
},
76+
});
77+
}
78+
6779
// Standard option set used by select/multiselect tests.
6880
const SELECT_OPTIONS = JSON.stringify({
6981
next_id: 4,
@@ -476,6 +488,67 @@ describe('linking field', () => {
476488
});
477489
});
478490

491+
describe('edit mode — scalar inputs', () => {
492+
test.each([
493+
['text', 'input[type="text"]', 'Changed text'],
494+
['textarea', 'textarea', 'Changed\ntext'],
495+
['number', 'input[type="number"]', '42.5'],
496+
['date', 'input[type="date"]', '2026-05-15'],
497+
['email', 'input[type="email"]', 'new@example.com'],
498+
['url', 'input[type="url"]', 'https://example.com/new'],
499+
])('%s calls onChange with edited value', async (fieldType, selector, editedValue) => {
500+
const onChange = vi.fn();
501+
const { container } = renderEdit({
502+
field: { field_type: fieldType, name: 'Label' },
503+
value: '',
504+
onChange,
505+
});
506+
507+
const input = container.querySelector(selector);
508+
expect(input).not.toBeNull();
509+
await fireEvent.input(input, { target: { value: editedValue } });
510+
511+
expect(onChange).toHaveBeenCalledWith(editedValue);
512+
});
513+
514+
test('date input strips time-like persisted values to YYYY-MM-DD', () => {
515+
const { container } = renderEdit({
516+
field: { field_type: 'date', name: 'Due' },
517+
value: '2026-05-15T12:34:56Z',
518+
});
519+
520+
const input = container.querySelector('input[type="date"]');
521+
expect(input).not.toBeNull();
522+
expect(input.value).toBe('2026-05-15');
523+
});
524+
525+
test('checkbox edit mode coerces string "false" to unchecked', () => {
526+
const { container } = renderEdit({
527+
field: { field_type: 'checkbox', name: 'Done' },
528+
value: 'false',
529+
});
530+
531+
const input = container.querySelector('input[type="checkbox"]');
532+
expect(input).not.toBeNull();
533+
expect(input.checked).toBe(false);
534+
});
535+
536+
test('checkbox calls onChange with boolean when toggled', async () => {
537+
const onChange = vi.fn();
538+
const { container } = renderEdit({
539+
field: { field_type: 'checkbox', name: 'Done' },
540+
value: false,
541+
onChange,
542+
});
543+
544+
const input = container.querySelector('input[type="checkbox"]');
545+
expect(input).not.toBeNull();
546+
await fireEvent.click(input);
547+
548+
expect(onChange).toHaveBeenCalledWith(true);
549+
});
550+
});
551+
479552
describe('unset / placeholder behavior', () => {
480553
test.each([
481554
['text', null],

‎frontend/src/lib/features/items/ItemDetailSidebar.svelte‎

Lines changed: 44 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@
2424
import { workspacePermissions } from '../../stores';
2525
import { t } from '../../stores/i18n.svelte.js';
2626
import { formatDateShort, formatCustomFieldDate } from '../../utils/dateFormatter.js';
27+
import { resolveOptionLabel, resolveOptionLabels } from '../../utils/optionUtils.js';
2728
import StatusBadge from '../../components/StatusBadge.svelte';
2829
import Badge from '../../components/Badge.svelte';
2930
import ApprovalsTimeline from './ApprovalsTimeline.svelte';
@@ -214,10 +215,13 @@
214215
? localStorage.getItem(SCHEDULING_COLLAPSED_KEY) === 'true'
215216
: false
216217
);
218+
let hasDateCustomFields = $derived(
219+
!!(workspaceScreenFields && workspaceScreenFields.some(f => f.field_type === 'custom' && getCustomFieldDefinition(f.field_identifier)?.field_type === 'date'))
220+
);
217221
let schedulingExpanded = $derived(
218-
(item?.due_date || item?.end_date) ? true : schedulingUserPref
222+
(item?.due_date || item?.end_date || hasDateCustomFields) ? true : schedulingUserPref
219223
);
220-
let schedulingForcedOpen = $derived(!!(item?.due_date || item?.end_date));
224+
let schedulingForcedOpen = $derived(!!(item?.due_date || item?.end_date || hasDateCustomFields));
221225

222226
function toggleScheduling() {
223227
if (schedulingForcedOpen) return;
@@ -278,6 +282,42 @@
278282
return customFieldDefinitions.find(field => field.id === parseInt(fieldId));
279283
}
280284

285+
function coerceCheckboxValue(raw) {
286+
if (typeof raw === 'boolean') return raw;
287+
if (typeof raw === 'number') return raw !== 0;
288+
if (typeof raw === 'string') {
289+
const normalized = raw.trim().toLowerCase();
290+
if (['false', '0', 'no', 'off'].includes(normalized)) return false;
291+
if (['true', '1', 'yes', 'on'].includes(normalized)) return true;
292+
}
293+
return Boolean(raw);
294+
}
295+
296+
function formatCustomFieldValue(fieldDef, value) {
297+
if (fieldDef.field_type === 'checkbox') {
298+
return coerceCheckboxValue(value) ? t('common.yes') : t('common.no');
299+
}
300+
if (fieldDef.field_type === 'select') {
301+
return resolveOptionLabel(fieldDef.options, value);
302+
}
303+
if (fieldDef.field_type === 'multiselect') {
304+
return resolveOptionLabels(fieldDef.options, Array.isArray(value) ? value : []).join(', ');
305+
}
306+
if (fieldDef.field_type === 'date') {
307+
return formatCustomFieldDate(value);
308+
}
309+
if (fieldDef.field_type === 'user' && typeof value === 'object') {
310+
return value.name || t('common.selected');
311+
}
312+
if (Array.isArray(value)) {
313+
return value.map(v => typeof v === 'object' ? v.title || v.name || v.label || v.value : v).join(', ');
314+
}
315+
if (typeof value === 'object') {
316+
return value.title || value.name || value.label || value.value || JSON.stringify(value);
317+
}
318+
return value;
319+
}
320+
281321
function formatVirtualFieldValue(field, value) {
282322
if (field.virtual_field_type === 'checkbox') {
283323
return value ? t('common.yes') : t('common.no');
@@ -969,7 +1009,7 @@
9691009
{/if}
9701010

9711011
<!-- Scheduling Section (collapsible) -->
972-
{#if shouldShowSystemField('due_date') || shouldShowSystemField('start_date') || shouldShowSystemField('end_date')}
1012+
{#if shouldShowSystemField('due_date') || shouldShowSystemField('start_date') || shouldShowSystemField('end_date') || hasDateCustomFields}
9731013
<div class="border-t my-4" style="border-color: var(--ds-border);"></div>
9741014

9751015
<!-- Scheduling Header -->
@@ -1233,19 +1273,7 @@
12331273
<Text variant="subtle" size="sm">{fieldDef.name}</Text>
12341274
<span style="color: {currentValue ? 'var(--ds-text)' : 'var(--ds-text-subtle)'};">
12351275
{#if currentValue !== null && currentValue !== undefined && currentValue !== ''}
1236-
{#if fieldDef.field_type === 'checkbox'}
1237-
{currentValue ? t('common.yes') : t('common.no')}
1238-
{:else if fieldDef.field_type === 'date'}
1239-
{formatCustomFieldDate(currentValue)}
1240-
{:else if fieldDef.field_type === 'user' && typeof currentValue === 'object'}
1241-
{currentValue.name || t('common.selected')}
1242-
{:else if Array.isArray(currentValue)}
1243-
{currentValue.map(v => typeof v === 'object' ? v.title || v.name || v.label || v.value : v).join(', ')}
1244-
{:else if typeof currentValue === 'object'}
1245-
{currentValue.title || currentValue.name || currentValue.label || currentValue.value || JSON.stringify(currentValue)}
1246-
{:else}
1247-
{currentValue}
1248-
{/if}
1276+
{formatCustomFieldValue(fieldDef, currentValue)}
12491277
{:else}
12501278
{t('common.none')}
12511279
{/if}

‎internal/aitools/approvals.go‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -202,5 +202,8 @@ func resolveUserNames(db database.Database, ids map[int]struct{}) map[int]string
202202
}
203203
names[id] = name
204204
}
205+
if err := rows.Err(); err != nil {
206+
return names
207+
}
205208
return names
206209
}

‎internal/aitools/items.go‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -678,6 +678,9 @@ func workspaceLookupMap(db database.Database) map[string]int {
678678
out[strings.ToLower(name)] = id
679679
out[strings.ToLower(key)] = id
680680
}
681+
if err := rows.Err(); err != nil {
682+
return out
683+
}
681684
return out
682685
}
683686

‎internal/aitools/labels.go‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,9 @@ func init() {
6161
}
6262
out.Labels = append(out.Labels, l)
6363
}
64+
if err := rows.Err(); err != nil {
65+
return nil, err
66+
}
6467
return out, nil
6568
},
6669
})

‎internal/aitools/misc.go‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -179,6 +179,9 @@ func init() {
179179
}
180180
out.Milestones = append(out.Milestones, m)
181181
}
182+
if err := rows.Err(); err != nil {
183+
return nil, err
184+
}
182185
return out, nil
183186
},
184187
})
@@ -245,6 +248,9 @@ func init() {
245248
}
246249
out.Iterations = append(out.Iterations, it)
247250
}
251+
if err := rows.Err(); err != nil {
252+
return nil, err
253+
}
248254
return out, nil
249255
},
250256
})
@@ -268,6 +274,9 @@ func init() {
268274
}
269275
out.CustomFields = append(out.CustomFields, cf)
270276
}
277+
if err := rows.Err(); err != nil {
278+
return nil, err
279+
}
271280
return out, nil
272281
},
273282
})
@@ -335,6 +344,9 @@ func init() {
335344
c.ChangedAt = changedAt.Format(time.RFC3339)
336345
out.Changes = append(out.Changes, c)
337346
}
347+
if err := rows.Err(); err != nil {
348+
return nil, err
349+
}
338350

339351
commentQuery := fmt.Sprintf(`SELECT c.content, c.created_at,
340352
w.key || '-' || CAST(i.workspace_item_number AS TEXT) as item_key, i.title,
@@ -363,6 +375,9 @@ func init() {
363375
}
364376
out.Comments = append(out.Comments, cm)
365377
}
378+
if err := cRows.Err(); err != nil {
379+
return nil, err
380+
}
366381
return out, nil
367382
},
368383
})

‎internal/aitools/time.go‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -164,6 +164,9 @@ func init() {
164164
}
165165
out.Projects = append(out.Projects, p)
166166
}
167+
if err := rows.Err(); err != nil {
168+
return nil, err
169+
}
167170
return out, nil
168171
},
169172
})
@@ -232,6 +235,9 @@ func init() {
232235
}
233236
out.Worklogs = append(out.Worklogs, w)
234237
}
238+
if err := rows.Err(); err != nil {
239+
return nil, err
240+
}
235241
return out, nil
236242
},
237243
})

‎internal/database/schema/permissions.sql‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -317,6 +317,17 @@ FROM workspace_roles r
317317
JOIN permissions p ON p.permission_key = 'test.manage'
318318
WHERE r.name = 'Tester';
319319

320+
-- Tester also gets item.comment so testers can follow up on bugs they file
321+
-- (reproduction steps, attachments, status updates). Deliberately NOT granted
322+
-- item.edit — that would collapse the Editor/Tester role distinction. Editor
323+
-- owns broad item editing; Tester owns test.execute / test.manage. Both share
324+
-- view/create/comment.
325+
INSERT OR IGNORE INTO role_permissions (role_id, permission_id)
326+
SELECT r.id, p.id
327+
FROM workspace_roles r
328+
JOIN permissions p ON p.permission_key = 'item.comment'
329+
WHERE r.name = 'Tester';
330+
320331
-- Add item.create to Editor role
321332
INSERT OR IGNORE INTO role_permissions (role_id, permission_id)
322333
SELECT r.id, p.id

0 commit comments

Comments
 (0)