Skip to content

add documentation and tests for form builder functionality DEV-2268#7155

Draft
jamesrkiger wants to merge 1 commit into
mainfrom
DEV-2268-form-builder-spec-tests
Draft

add documentation and tests for form builder functionality DEV-2268#7155
jamesrkiger wants to merge 1 commit into
mainfrom
DEV-2268-form-builder-spec-tests

Conversation

@jamesrkiger

Copy link
Copy Markdown
Contributor

🗒️ Checklist

  1. run linter locally
  2. update developer docs (API, README, inline, etc.), if any
  3. for user-facing doc changes create a Zulip thread at #Support Docs Updates, if any
  4. draft PR with a title <type>(<scope>)<!>: <title> DEV-1234
  5. assign yourself, tag PR: at least Front end and/or Back end or workflow
  6. fill in the template below and delete template comments
  7. review thyself: read the diff and repro the preview as written
  8. open PR & confirm that CI passes & request reviewers, if needed
  9. act on any greptile review below a 5/5 score or leave comment explaining why you won't
  10. delete this checklist section from the final squash commit before merging

📣 Summary

TODO

📖 Description

TODO

👷 Description for instance maintainers

TODO

💭 Notes

TODO

👀 Preview steps

  1. ℹ️ have an account and a project
  2. do this
  3. do that
  4. 🔴 [on main] notice that this isn't anywhere
  5. 🟢 [on PR] notice that this is here
  6. do that another thing
  7. 🟢 notice that this changed like that

@jamesrkiger jamesrkiger self-assigned this Jun 15, 2026
@jamesrkiger jamesrkiger marked this pull request as draft June 15, 2026 13:55
@greptile-apps

greptile-apps Bot commented Jun 15, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR introduces test infrastructure and design documentation for the new React-based form builder, adding XlformAdapter as a thin wrapper around the legacy xlform serialization path, plus a comprehensive suite of unit tests covering question types, editing mutations, skip logic, translations, and structural nesting. The tsconfig.json gains resolveJsonModule: true to allow fixture JSON imports in tests.

  • adapter.ts wraps dkobo_xlform.model.Survey with a typed load/serialize interface; its serialize() logic is also reimplemented in __tests__/helpers.ts, so most tests bypass the adapter class entirely.
  • DESIGN_REFERENCE.md is a detailed architecture document covering the API contract, xlform internals, serialization quirks, and open design questions for the replacement implementation.
  • The PR description still contains only the template-generated "TODO" placeholders in every section; the checklist is unchecked and the title does not follow the required <type>(<scope>): <title> DEV-XXXX format.

Confidence Score: 4/5

Safe to merge; changes are additive (docs and tests only) and do not touch any production runtime path.

The new test suite and design doc are purely additive — no production code is modified beyond adding adapter.ts and the tsconfig resolveJsonModule flag. The main concern is that tests/helpers.ts re-implements the XlformAdapter serialization logic independently, meaning most tests exercise the helper copy rather than the adapter class itself. The PR description also retains all TODO placeholders and an unconventional title, suggesting it is still a draft.

jsapp/js/formBuilder/tests/helpers.ts — its load/serialize logic is a copy of adapter.ts rather than a delegation, which quietly orphans the adapter from test coverage.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    subgraph TestInfra["Test Infrastructure"]
        H["__tests__/helpers.ts\n(loadSurvey, serialize, addRow, ...)"]
        A["formBuilder/adapter.ts\nXlformAdapter"]
    end

    subgraph FeatureTests["Feature Tests"]
        E["editing.tests.ts"]
        SL["skip-logic.tests.ts"]
        TR["translations.tests.ts"]
        N["nesting.tests.ts"]
        R["required.tests.ts"]
        C["constraint.tests.ts"]
    end

    subgraph QTypeTests["Question-Type Tests"]
        TXT["text.tests.ts"]
        SEL["select-one.tests.ts"]
        SOFF["select-one-from-file.tests.ts"]
        IMG["image.tests.ts"]
        RNG["range.tests.ts"]
        CALC["calculate.tests.ts"]
        DL["default-labels.tests.ts"]
        GRP["group.tests.ts"]
    end

    subgraph Core["xlform Core (CoffeeScript)"]
        XF["dkobo_xlform.model.Survey\n(loadDict / toJSON)"]
        FBU["formBuilderUtils.ts\n(surveyToValidJson,\nunnullifyTranslations)"]
    end

    H -->|uses directly| XF
    H -->|uses| FBU
    A -->|wraps| XF
    A -->|uses| FBU

    E --> H
    SL --> H
    TR --> H
    C --> H
    TXT --> H
    SEL --> H
    SOFF --> H
    IMG --> H
    RNG --> H
    CALC --> H
    DL --> H
    GRP --> H

    N --> A
    R --> A
    R --> H
Loading

Reviews (1): Last reviewed commit: "Initial work" | Re-trigger Greptile


/**
* `required` behaves differently on question rows vs group rows.
* See SERIALIZATION_QUIRKS.md for context.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Dangling reference to non-existent file

The comment references SERIALIZATION_QUIRKS.md, but that file does not exist anywhere in the repository — only DESIGN_REFERENCE.md was added in this PR. Any developer following the cross-reference will hit a dead end. Either the file needs to be created, or the comment should point to the relevant section of DESIGN_REFERENCE.md instead.

Comment on lines +1 to +44
import clonedeep from 'lodash.clonedeep'
import dkobo_xlform from '../../../xlform/src/_xlform.init'
import { surveyToValidJson, unnullifyTranslations } from '#/components/formBuilder/formBuilderUtils'
import type { AssetContent } from '#/dataInterface'

export function createSurvey() {
return dkobo_xlform.model.Survey.create()
}

export function loadSurvey(content: AssetContent) {
return dkobo_xlform.model.Survey.loadDict(clonedeep(content))
}

export function serialize(survey: any): Record<string, any> {
let json = surveyToValidJson(survey)
if (survey._initialParams?.translations_0) {
json = unnullifyTranslations(json, survey._initialParams)
}
return JSON.parse(json)
}

export function addRow(survey: any, attrs: Record<string, any>) {
survey.addRow(attrs)
return survey.rows.last()
}

export function surveyRow(survey: any, type: string) {
return serialize(survey).survey.find((r: any) => r.type === type)
}

// Find a row in the live survey model by its $autoname (as loaded from a fixture)
export function findRowByAutoname(survey: any, autoname: string): any {
let found: any = null
survey.forEachRow((row: any) => {
const detail = row.get?.('$autoname')
if (detail?.get?.('value') === autoname) found = row
}, { includeGroups: true })
return found
}

// Set a label on a live row
export function setLabel(row: any, value: string) {
row.get('label').set('value', value)
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Test helpers duplicate XlformAdapter logic, leaving the adapter uncovered

helpers.ts re-implements loadSurvey and serialize independently of XlformAdapter. Most tests (editing, skip-logic, translations, constraint, and all question-type tests) import from helpers, so they exercise this copy-pasted path rather than the actual XlformAdapter.serialize() in adapter.ts. If the adapter's serialization logic diverges in a future change, all those tests will continue to pass while the real adapter silently breaks. Only nesting.tests.ts and required.tests.ts use XlformAdapter directly. Consider having the helpers delegate to XlformAdapter to keep a single source of truth.

@@ -0,0 +1,716 @@
# Form Builder — Design Reference

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 PR description and title do not follow the repository template

Every section of the PR description (📣 Summary, 📖 Description, 👷 Description for instance maintainers, 💭 Notes, 👀 Preview steps) still contains the placeholder "TODO", the checklist has no items checked, and the PR title (add documentation and tests for form builder functionality DEV-2268) does not follow the required <type>(<scope>): <title> DEV-XXXX format. Please fill in or delete each section before merging, as required by the team's template.

Rule Used: What: PR descriptions must follow the repository's... (source)

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant