From 11a77ff60a2a953b020e476a391cdd63bf614ad0 Mon Sep 17 00:00:00 2001 From: Simon Reed Date: Tue, 28 Apr 2026 00:06:13 +0100 Subject: [PATCH] Fix new code list with multiple codes failing to save (#878) Frontend: assign code.order in the create path (matching the update path). Backend: replace map.with_index with each_with_index to make intent clear. Guard err.response access with optional chaining so a network error does not raise a secondary TypeError. Adds a backend controller test and a Playwright flow regression test. Also adds fixtures :all to test_helper.rb which was missing, causing all controller/model tests to fail with undefined method 'users'. Co-Authored-By: Claude Sonnet 4.6 --- .machina/flow-steps/code_list.mjs | 50 +++++++++++++++++++ .machina/flow-steps/ui.mjs | 7 +++ app/controllers/code_lists_controller.rb | 2 +- react/src/actions/index.js | 2 +- react/src/components/CodeListForm.js | 14 +++--- .../controllers/code_lists_controller_test.rb | 24 +++++++++ test/test_helper.rb | 1 + ...78-create-code-list-multiple-codes.feature | 20 ++++++++ 8 files changed, 110 insertions(+), 10 deletions(-) create mode 100644 .machina/flow-steps/code_list.mjs create mode 100644 tests/flows/regressions/issue-878-create-code-list-multiple-codes.feature diff --git a/.machina/flow-steps/code_list.mjs b/.machina/flow-steps/code_list.mjs new file mode 100644 index 000000000..f1c03a898 --- /dev/null +++ b/.machina/flow-steps/code_list.mjs @@ -0,0 +1,50 @@ +export default ({ defineStep }) => [ + // Fill in the code list's own label field (textarea[name="label"]). + // Uses a direct name selector to avoid ambiguity with code-row label inputs. + defineStep('I fill in the code list label with {string}', async (ctx, value) => { + await ctx.page.locator('textarea[name="label"], input[name="label"]').first().waitFor({ timeout: 10000 }); + await ctx.page.locator('textarea[name="label"], input[name="label"]').first().fill(value); + }), + + // Fill in the code list label with a value that includes a timestamp suffix so each + // test run creates a distinct code list (avoids uniqueness-per-instrument constraint). + defineStep('I fill in the code list label with {string} and a unique suffix', async (ctx, base) => { + const label = `${base}-${Date.now()}`; + const field = ctx.page.locator('textarea[name="label"], input[name="label"]').first(); + await field.waitFor({ timeout: 10000 }); + await field.fill(label); + }), + + // Fill in the value and label for a specific code row (1-indexed). + // Uses react-final-form FieldArray names: codes[N].value (textarea) and codes[N].label (input). + defineStep('I fill in the code row {int} with value {string} and label {string}', async (ctx, row, value, label) => { + const idx = row - 1; + await ctx.page.locator(`textarea[name="codes[${idx}].value"]`).fill(value); + const labelInput = ctx.page.locator(`input[name="codes[${idx}].label"]`); + await labelInput.fill(label); + await labelInput.press('Tab'); + }), + + // Wait for the codes table to show at least one row — confirms the code list data has loaded + // after a redirect (React fetches asynchronously after the URL changes). + defineStep('I wait for the code list to load', async (ctx) => { + await ctx.page.locator('table tbody tr').first().waitFor({ timeout: 10000 }); + }), + + // Assert the label input value for a specific code row (1-indexed). + // Category labels render inside MUI Autocomplete inputs, not as text nodes, so we read inputValue. + defineStep('the code row {int} should have label {string}', async (ctx, row, label) => { + const input = ctx.page.locator(`input[name="codes[${row - 1}].label"]`); + await input.waitFor({ timeout: 5000 }); + const actual = await input.inputValue(); + if (actual !== label) throw new Error(`Expected code row ${row} label "${label}" but got "${actual}"`); + }), + + // Click the add-code button (aria-label="Add code") that sits next to the "Codes" heading. + // The button only renders once instrument data has loaded, so we wait for it. + defineStep('I click the add code button', async (ctx) => { + const addBtn = ctx.page.locator('button[aria-label="Add code"]'); + await addBtn.waitFor({ timeout: 10000 }); + await addBtn.click(); + }), +]; diff --git a/.machina/flow-steps/ui.mjs b/.machina/flow-steps/ui.mjs index 148521ccf..aaaf2987f 100644 --- a/.machina/flow-steps/ui.mjs +++ b/.machina/flow-steps/ui.mjs @@ -5,4 +5,11 @@ export default ({ defineStep }) => [ await listbox.waitFor({ timeout: 5000 }); await listbox.locator('[role="option"]').filter({ hasText: option }).click(); }), + + // Wait for the URL to match a regex pattern, then settle — needed after async Redux redirects + // where the URL changes client-side before the subsequent API fetches begin. + defineStep('I wait for the URL to match {string}', async (ctx, pattern) => { + await ctx.page.waitForURL(new RegExp(pattern), { timeout: 10000 }); + await ctx.page.waitForLoadState('networkidle', { timeout: 10000 }); + }), ]; diff --git a/app/controllers/code_lists_controller.rb b/app/controllers/code_lists_controller.rb index 5b9db3ad7..afbb959d9 100755 --- a/app/controllers/code_lists_controller.rb +++ b/app/controllers/code_lists_controller.rb @@ -53,7 +53,7 @@ def safe_params # and their nested categories. codes_params = params[:codes] ? params.delete(:codes) : params[:code_list].delete(:codes) if codes_params - codes_params.map.with_index do | code, index | + codes_params.each_with_index do | code, index | code[:order] = index + 1 unless code[:order].present? next if code[:value].blank? && code[:label].blank? existing_category = @instrument.categories.find_by_label(code[:label]) diff --git a/react/src/actions/index.js b/react/src/actions/index.js index ad91b7fff..c28c9dfca 100644 --- a/react/src/actions/index.js +++ b/react/src/actions/index.js @@ -665,7 +665,7 @@ export const CodeLists = { dispatch(redirectTo(url(routes.instruments.instrument.build.codeLists.show, { instrument_id: instrumentId, codeListId: res.data.id }))); }) .catch(err => { - dispatch(saveError('new', 'CodeList', err.response.data.error_sentence)); + dispatch(saveError('new', 'CodeList', err.response?.data?.error_sentence)); }); }; }, diff --git a/react/src/components/CodeListForm.js b/react/src/components/CodeListForm.js index a7139f99e..1335c5a74 100644 --- a/react/src/components/CodeListForm.js +++ b/react/src/components/CodeListForm.js @@ -111,15 +111,13 @@ export const CodeListForm = (props) => { const classes = useStyles(); const onSubmit = (values) => { - values = ObjectCheckForInitialValues(codeList, values) - - if(isNil(codeList.id)){ + values = ObjectCheckForInitialValues(codeList, values) + if (values.codes) { + values.codes.forEach((code, i) => { code.order = i + 1 }) + } + if (isNil(codeList.id)) { dispatch(CodeLists.create(instrumentId, values)) - }else{ - values.codes.map((code, i) => { - code.order = i + 1 - return code - }) + } else { dispatch(CodeLists.update(instrumentId, codeList.id, values)) } } diff --git a/test/controllers/code_lists_controller_test.rb b/test/controllers/code_lists_controller_test.rb index 21100f80f..8226b61dd 100755 --- a/test/controllers/code_lists_controller_test.rb +++ b/test/controllers/code_lists_controller_test.rb @@ -91,6 +91,30 @@ class CodeListsControllerTest < ActionController::TestCase assert_equal [3,4], [code_a.reload.order, code_b.reload.order] end + test "should create code_list with multiple codes without client-supplied order" do + assert_difference('CodeList.count') do + post :create, format: :json, params: { + instrument_id: @instrument.id, + code_list: { + label: @code_list.label + '_multi', + codes: [ + { value: '1', label: 'Yes' }, + { value: '2', label: 'No' } + ] + } + } + end + + assert_response :success + json = JSON.parse(response.body) + codes = json['codes'].sort_by { |c| c['order'] } + assert_equal 2, codes.length + assert_equal 1, codes[0]['order'] + assert_equal 2, codes[1]['order'] + assert_equal '1', codes[0]['value'] + assert_equal '2', codes[1]['value'] + end + test "should destroy code_list" do assert_difference('CodeList.count', -1) do delete :destroy, format: :json, params: { instrument_id: @instrument.id, id: @code_list } diff --git a/test/test_helper.rb b/test/test_helper.rb index 30cd0c206..5b80505c6 100755 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -15,6 +15,7 @@ class ActiveSupport::TestCase include FactoryBot::Syntax::Methods + fixtures :all setup do DatabaseCleaner.strategy = :transaction DatabaseCleaner.start diff --git a/tests/flows/regressions/issue-878-create-code-list-multiple-codes.feature b/tests/flows/regressions/issue-878-create-code-list-multiple-codes.feature new file mode 100644 index 000000000..1e17c1cd4 --- /dev/null +++ b/tests/flows/regressions/issue-878-create-code-list-multiple-codes.feature @@ -0,0 +1,20 @@ +Feature: Create code list with multiple codes in one submit (#878) + Regression for https://github.com/CLOSER-Cohorts/archivist/issues/878 + — creating a brand-new code list with two or more codes in a single + submit must succeed. The create path previously omitted order on each + code; the update path correctly assigned it. + + Scenario: New code list with 2 codes saves on first submit + When I log in as "simon.reed@browsergroup.com" with password "Password123!" + And I navigate to "/instruments/mcs_18_ypsc/build/code_lists/new" + And I wait for the page to settle + And I fill in the code list label with "yesno-issue-878" and a unique suffix + And I click the add code button + And I fill in the code row 1 with value "1" and label "Yes" + And I click the add code button + And I fill in the code row 2 with value "2" and label "No" + And I click the "Save" button + And I wait for the URL to match "code_lists/\d+" + And I wait for the code list to load + Then the code row 1 should have label "Yes" + And the code row 2 should have label "No"