Repository navigation
OpenConceptLab/ocl_issues#3010 | Collection version create asks for lock, evaluate from scratch or none - #96
snyaggarwal wants to merge 3 commits into
Conversation
… expansion to use
snyaggarwal
left a comment
There was a problem hiding this comment.
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.
|
The ACs on OpenConceptLab/ocl_issues#3010 changed after Jon's decision on Nov 5 (comment there). For TBv3:
|
…from scratch or none, only with the lock toggle on
|
Reworked in 1670100 for the new ACs:
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
left a comment
There was a problem hiding this comment.
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.
… expansion choice logic tested
|
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 ESLint passes; 🤖 Generated with Claude Code |
snyaggarwal
left a comment
There was a problem hiding this comment.
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.
Part of OpenConceptLab/ocl_issues#3010. Needs OpenConceptLab/oclapi2#980 for the
expansionfield. Reworked for the ACs Jon rewrote on Nov 5.expansion: lock,latestornone, withautoexpandmatching.autoexpandcheckbox, as on main.ESLint passes on
VersionForm.jsx;npm testpasses (122). Not yet clicked through in the browser, and no component test yet forVersionForm.🤖 Generated with Claude Code