Skip to content

OpenConceptLab/ocl_issues#3010 | Collection version create asks for lock, evaluate from scratch or none - #96

Open
snyaggarwal wants to merge 3 commits into
mainfrom
issues#3010
Open

snyaggarwal wants to merge 3 commits into
mainfrom
issues#3010

Conversation

@snyaggarwal

@snyaggarwal snyaggarwal commented Nov 5, 2026 •

Copy link
Copy Markdown
Contributor

Part of OpenConceptLab/ocl_issues#3010. Needs OpenConceptLab/oclapi2#980 for the expansion field. Reworked for the ACs Jon rewrote on Nov 5.

  • Lock toggle on: Submit on a new collection version opens a popup with Lock repo versions (the default, shown only when HEAD has an expansion), Evaluate from scratch and No expansion, each with a short description. They send expansion: lock, latest or none, with autoexpand matching.
  • Lock toggle off: no popup; the create form keeps the autoexpand checkbox, as on main.
  • Popup text in en, es, pt-BR and zh (Jon's suggested English).

ESLint passes on VersionForm.jsx; npm test passes (122). Not yet clicked through in the browser, and no component test yet for VersionForm.

🤖 Generated with Claude Code

@snyaggarwal snyaggarwal left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Verdict: Approve after OpenConceptLab/oclapi2#980 lands.

P1–P4 findings: none.

P5 — Add focused coverage for the new VersionForm branch. The graph finds no direct tests for VersionForm, and this PR adds the copy-option visibility/default logic plus the expansion/autoexpand payload mapping. A component test covering toggle + HEAD expansion combinations and the three submitted choices would protect the acceptance path. This is non-blocking because the implementation matches the API contract and the full frontend suite passes.

Nits: none.

Verification: traced the RepoHome inputs, toggle state, dialog choices, and request payload with the code graph; npm test passes 122/122, ESLint passes, and GitHub checks are green. No open-core boundary concern.

@paynejd

paynejd commented Nov 6, 2026

Copy link
Copy Markdown
Member

The ACs on OpenConceptLab/ocl_issues#3010 changed after Jon's decision on Nov 5 (comment there). For TBv3:

  • With the lock toggle on, the popup offers Lock repo versions (the default, and only when HEAD has an expansion), Evaluate from scratch and No expansion. They send expansion: lock, latest and none.
  • With the toggle off, there's no popup: the create form keeps the autoexpand checkbox, as on main.
  • canCopyHeadExpansion (VersionForm.jsx:66) becomes the check for Lock, and the default at line 73 becomes lock, else latest.
  • Suggested en text, with es, pt-BR and zh to match:
    • Lock repo versions: "Uses the same repository versions as HEAD's expansion. They're saved in the new expansion's parameters, so a rebuild gives the same result."
    • Evaluate from scratch: "Evaluates every reference again. References without a version use each repository's latest released version, or its HEAD if it has no release."
    • No expansion: unchanged.

…from scratch or none, only with the lock toggle on
@snyaggarwal snyaggarwal changed the title OpenConceptLab/ocl_issues#3010 | Collection version create asks which expansion to use OpenConceptLab/ocl_issues#3010 | Collection version create asks for lock, evaluate from scratch or none Nov 6, 2026
@snyaggarwal

Copy link
Copy Markdown
Contributor Author

Reworked in 1670100 for the new ACs:

  • With the lock toggle on, the popup offers Lock repo versions (the default, only when HEAD has an expansion), Evaluate from scratch and No expansion, sending lock, latest and none.
  • With the toggle off, there's no popup, and the create form keeps the autoexpand checkbox as on main.
  • canCopyHeadExpansion is now canLockRepoVersions, and the default is lock, else latest.
  • The helper text is your suggested English, with es, pt-BR and zh to match.

One wording point: "so a rebuild gives the same result" holds for sources, but a rebuild still resolves unversioned value set references again (the ticket's known limit). I kept your text; say if you'd rather it said so.

🤖 Generated with Claude Code

@snyaggarwal snyaggarwal left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Supersedes my review of 6413c3f.

Verdict: Request changes after the API findings on OpenConceptLab/oclapi2#980 are fixed.

P1–P3: none in this frontend diff.

P4 — The Lock helper overpromises that “a rebuild gives the same result” (src/i18n/locales/en/translations.json:776, and the translated equivalents). The rewritten ticket explicitly documents that unversioned value-set references are not durable across rebuilds, so this absolute user-facing claim is false even after the API source-lock path is corrected. Qualify the statement to repository/source versions, or state the value-set exception consistently in all locales.

P5 — VersionForm still has no direct component coverage. This rework changes the controlling toggle branch, restores the legacy checkbox when the toggle is off, changes option visibility/defaults based on HEAD expansion state, and maps three choices into two payload fields. Add focused coverage for: toggle off (no popup + checkbox retained), toggle on without HEAD expansion (latest default, no lock), toggle on with HEAD expansion (lock default), and the submitted expansion/autoexpand pairs.

Nits: none.

Verification: traced RepoHome inputs, toggle state, option/default logic and payloads with the code graph; npm test passes 122/122, ESLint passes, diff check is clean, and GitHub checks are green. No open-core boundary concern.

@snyaggarwal

Copy link
Copy Markdown
Contributor Author

Both findings from the review of 1670100 checked and fixed in a172345.

P4: the Lock help overpromised a rebuild. Valid. It now limits the promise to sources and names the value set exception: "Uses the same repository versions as HEAD's expansion. Source versions are saved in the new expansion's parameters, so a rebuild keeps them. Value sets referenced without a version can resolve again on a rebuild." es, pt-BR and zh match.

P5: no coverage for the popup logic. Valid. The logic moved into versionExpansion.js, which VersionForm now uses, with node tests in the repo's existing style covering toggle off (no popup, checkbox kept), toggle on without a HEAD expansion (latest default, no lock), toggle on with one (lock default), editing, source versions, and the expansion/autoexpand pair for each choice.

ESLint passes; npm test passes (128). Not yet clicked through in a browser.

🤖 Generated with Claude Code

@snyaggarwal snyaggarwal left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Re-review of a172345 against the rewritten Nov 5 ACs, the API contract, and the current PR description.

Verdict: Approve.

Findings:

  • P1: none
  • P2: none
  • P3: none
  • P4: none
  • P5: none
  • Nits: none

The prior wording finding is fixed: the Lock help now describes durable source locks and explicitly calls out the documented unversioned-value-set exception. The prior test gap is fixed by extracting the decision/payload logic and covering toggle-on/off, HEAD-expansion presence, edit/create, source/collection, defaults, and every payload choice. The form wiring matches that tested logic.

Validation: all 128 local tests pass, ESLint passes on the changed JavaScript, and both GitHub checks are green.

This supersedes my earlier request-changes review.

This branch has not been deployed

No deployments
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.

2 participants