diff --git a/src/mechcomp/web/app.py b/src/mechcomp/web/app.py index 744e491..38e0377 100644 --- a/src/mechcomp/web/app.py +++ b/src/mechcomp/web/app.py @@ -41,15 +41,44 @@ from mechcomp import svg # as root:mechcomp 0640, and the service user can read it. ENV_FILE = "/etc/mechcomp/mechcomp.env" +# Parameters whose value is one of a fixed set rather than a number. +# +# WHY THIS EXISTS RATHER THAN A SPECIAL CASE FOR length_view +# A free-text box for a parameter with two valid values is a trap. The +# type coercion below falls through to the raw string for anything that is +# not a bool, int or float, and ``_common.model_length_mm`` compares +# against the literal "Full Length" and silently falls back to the preview +# length for anything else. So "Full Length " with a trailing space would +# produce a 100 mm model with no rejection and no error -- a file that +# looks right and is the wrong object. +# +# Declaring the choices here does three things at once: the browser renders +# a select instead of an input, a value outside the set never reaches the +# build, and the next enumerated parameter needs no new machinery. +# +# NOT MIRRORED AS A CHECK IN THE FAMILIES, DELIBERATELY +# ``geometry_checks`` is shared with the oracle, and the reference accepted +# any string here and fell through to preview. Adding a check there would +# put the 123 frozen cases at risk to fix a user-interface problem. The +# guard belongs in the composer, which is not oracle-bearing. +ENUM_PARAMS: Dict[str, List[str]] = { + "length_view": ["Preview", "Full Length"], +} + # Parameters that apply to every profile, in the order they make sense to a # person: what the stock is, how it fits, then how thick the printed walls are. +# +# ``length_view`` leads the Model group because it decides which of the two +# lengths below it actually applies. Without it the Full Length branch was +# unreachable from the composer and every model was a 100 mm preview whether +# or not ``member_length_ft`` had been set. COMMON_GROUPS: List[Tuple[str, List[str]]] = [ ("Stock", ["strap_width_mm", "strap_thickness_mm", "bundle_count"]), ("Fit", ["fit_clearance_mm"]), ("Walls", ["inside_wall_thickness_mm", "outside_wall_thickness_mm", "edge_wall_thickness_mm", "min_wall_mm"]), - ("Model", ["preview_length_mm", "member_length_ft", "material_density_g_cm3", - "facets"]), + ("Model", ["length_view", "preview_length_mm", "member_length_ft", + "material_density_g_cm3", "facets"]), ] # Which parameter prefixes belong to which profile, so a person is not shown @@ -110,6 +139,16 @@ def build_payload(family_name: str, profile: str, for key, raw in overrides.items(): if key not in family.defaults: continue + + # An enumerated parameter takes its value or nothing. Dropping an + # unrecognised value leaves the family default in place, which is a + # valid model, rather than passing a string the geometry will ignore + # without saying so. + if key in ENUM_PARAMS: + if raw in ENUM_PARAMS[key]: + typed[key] = raw + continue + default = family.defaults[key] try: if isinstance(default, bool): @@ -127,7 +166,7 @@ def build_payload(family_name: str, profile: str, "family": family_name, "profile": profile, "groups": [[title, keys] for title, keys in profile_params(family, profile)], - "defaults": {k: family.defaults[k] for k in family.defaults}, + "choices": {k: v for k, v in ENUM_PARAMS.items() if k in family.defaults}, "values": {**family.defaults, **typed}, "profiles": sorted(family.catalogue), } @@ -255,6 +294,7 @@ async function refresh(sendValues) { prof.onchange = () => { state.profile = prof.value; refresh(true); }; state.values = {}; + const choices = data.choices || {}; const box = document.getElementById("controls"); box.innerHTML = ""; for (const [title, keys] of data.groups) { @@ -267,9 +307,16 @@ async function refresh(sendValues) { const name = document.createElement("span"); name.textContent = k.replace(/_mm$|_deg$|_ft$/, "").replace(/_/g, " "); name.title = k; - const inp = document.createElement("input"); - inp.value = v; - if (typeof v === "number") { inp.type = "number"; inp.step = "any"; } + let inp; + if (choices[k]) { + inp = document.createElement("select"); + for (const c of choices[k]) inp.add(new Option(c, c)); + inp.value = v; + } else { + inp = document.createElement("input"); + inp.value = v; + if (typeof v === "number") { inp.type = "number"; inp.step = "any"; } + } inp.onchange = () => { state.values[k] = inp.value; refresh(true); }; lab.append(name, inp); fs.append(lab); @@ -341,7 +388,8 @@ class Handler(BaseHTTPRequestHandler): except Exception as exc: # noqa: BLE001 payload = {"ok": False, "message": "%s: %s" % (type(exc).__name__, exc), - "profiles": [], "groups": [], "values": {}} + "profiles": [], "groups": [], "values": {}, + "choices": {}} self._send(200, json.dumps(payload).encode("utf-8"), "application/json; charset=utf-8") return diff --git a/tests/test_enum_params.py b/tests/test_enum_params.py new file mode 100644 index 0000000..f2bf88a --- /dev/null +++ b/tests/test_enum_params.py @@ -0,0 +1,159 @@ +""" +Enumerated parameters, and the length control that motivated them. + +WHAT THIS IS ACTUALLY GUARDING + ``model_length_mm`` compares ``length_view`` against the literal + "Full Length" and falls through to the preview length for anything else -- + no rejection, no error. The reference behaved the same way, so the port is + right to keep it. + + That makes the composer the only place the mistake can be caught. Before + this, ``length_view`` was not in COMMON_GROUPS at all, so the Full Length + branch was unreachable and every model was a 100 mm preview whatever + ``member_length_ft`` said. Exposing it as free text would have replaced an + unreachable control with a silent one: "Full Length " with a trailing space + builds a preview and says nothing. + + So the claims under test are: the branch is reachable, it moves the length + it is supposed to move, and a value outside the declared set cannot reach + the build. +""" + +from __future__ import annotations + +import pytest + +app = pytest.importorskip("mechcomp.web.app") + + +def payload(**overrides): + return app.build_payload("3x", "Y", dict(overrides)) + + +def length_of(**overrides): + p = payload(**overrides) + assert p["ok"], p.get("message") + return p["report"]["LENGTH_MM"] + + +# --------------------------------------------------------------------------- +# The declaration +# --------------------------------------------------------------------------- + +def test_length_view_is_declared_with_exactly_its_two_values(): + assert app.ENUM_PARAMS["length_view"] == ["Preview", "Full Length"] + + +def test_every_enumerated_parameter_is_a_real_family_parameter(): + """ + A declared choice for a parameter no family has would render a control + that does nothing -- the failure COMMON_GROUPS has a comment about. + """ + for family in app.families().values(): + for key in app.ENUM_PARAMS: + assert key in family.defaults, key + + +def test_every_declared_default_is_one_of_its_own_choices(): + """ + If a family's default were outside the list, the select would open showing + a value it cannot represent and the first interaction would silently move + the model. + """ + for family in app.families().values(): + for key, allowed in app.ENUM_PARAMS.items(): + assert family.defaults[key] in allowed + + +# --------------------------------------------------------------------------- +# Reachability -- the defect that started this +# --------------------------------------------------------------------------- + +def test_length_view_is_reachable_from_the_composer(): + for name, family in app.families().items(): + exposed = {k for _, keys in app.profile_params(family, "Y" if name == "3x" else "Cross") + for k in keys} + assert "length_view" in exposed, name + + +def test_the_browser_is_told_the_choices(): + assert payload()["choices"]["length_view"] == ["Preview", "Full Length"] + + +# --------------------------------------------------------------------------- +# It moves the length it claims to move +# --------------------------------------------------------------------------- + +def test_preview_is_the_preview_length(): + assert length_of(length_view="Preview", preview_length_mm=100) == 100 + + +def test_full_length_is_feet_times_304_8(): + assert length_of(length_view="Full Length", member_length_ft=10) == 3048 + + +def test_full_length_follows_member_length_ft(): + """ + The positive control for the pair above: a length that ignored its input + would satisfy either test alone by returning a constant. + """ + assert length_of(length_view="Full Length", member_length_ft=20) == 6096 + + +def test_preview_ignores_member_length_ft(): + assert length_of(length_view="Preview", member_length_ft=40) == 100 + + +# --------------------------------------------------------------------------- +# A value outside the set never reaches the build +# --------------------------------------------------------------------------- + +@pytest.mark.parametrize("bad", ["Full Length ", "full length", "FULL LENGTH", + "", "Fully Lengthed", "Preview\n"]) +def test_an_unrecognised_value_falls_back_to_the_default(bad): + """ + Not a rejection -- a fallback. The family default is a valid model, and + the composer is a viewer rather than a validator. What must not happen is + the string reaching ``model_length_mm`` and being silently ignored there, + which is how a trailing space becomes a 100 mm export of a 10 ft member. + """ + p = payload(length_view=bad, member_length_ft=10) + assert p["ok"] + assert p["values"]["length_view"] == "Preview" + assert p["report"]["LENGTH_MM"] == 100 + + +def test_the_unrecognised_value_is_not_merely_unused_but_absent(): + """ + Asserts the mechanism rather than its effect. If the string were passed + through and happened to be ignored downstream, the test above would still + pass and a later change to ``model_length_mm`` would turn it into a defect. + """ + p = payload(length_view="Full Length ", member_length_ft=10) + assert "Full Length " not in p["values"].values() + + +def test_a_recognised_value_does_reach_the_build(): + """The positive control: the fallback is not simply dropping everything.""" + p = payload(length_view="Full Length", member_length_ft=10) + assert p["values"]["length_view"] == "Full Length" + assert p["report"]["LENGTH_MM"] == 3048 + + +# --------------------------------------------------------------------------- +# The length control does not disturb identity +# --------------------------------------------------------------------------- + +def test_length_view_changes_the_design_id(): + """ + It is a build parameter, so it belongs in the hash -- unlike the author. + Two members of different lengths are different designs. + """ + preview = payload(length_view="Preview")["input_id"] + full = payload(length_view="Full Length")["input_id"] + assert preview != full + + +def test_an_unrecognised_value_yields_the_default_design_id(): + """Follows from the fallback, and is the property someone would rely on.""" + assert payload(length_view="Full Length ")["input_id"] == payload()["input_id"]