Repository navigation
Conversation
mxhash
left a comment
There was a problem hiding this comment.
Thanks for this — the feature is useful and the core is solid: the migration applies and rolls back cleanly, alembic check shows no model drift, the cap is enforced server-side (400 on violation), and foreign groups from other polls can't be hijacked via client_key. A few things need fixing before it can go in:
1. Rebase needed
#14 is merged now, so add() in polls-edit.js conflicts. Suggested resolution, keeping both type and group preselection:
add() {
const above = this.options[this.options.length - 1];
const newOption = mk(above ? { type: above.type } : null);
newOption.group_uid = above ? above.group_uid : null;
this.options.push(newOption);
},2. Lint fails
src/struudel/models/poll_option.py:101 is 101 chars → ruff check (E501) and ruff format --check fail. make format fixes it.
3. Duplicate group client_keys silently merge groups
groupSeq restarts at 1 when the form is re-rendered after a server-side error, so new-N keys can collide. Repro: new poll → add two groups → delete the first → submit so that the server rejects it (e.g. PRIVATE + ACTIVE without audience) → on the re-rendered form add another group → both groups now carry new-2. _apply_groups then creates two groups, all options end up in the second one and the first stays empty.
Suggested fix: reject duplicate client_keys in PollForm (next to _check_option_group_references) and generate unique keys client-side, e.g. crypto.randomUUID().
4. 500 on non-ASCII digits in client_key
_apply_groups uses g.client_key.isdigit() and then int(...). "²".isdigit() is True, int("²") raises ValueError → unhandled 500. Only reachable with crafted requests, but easy to close, e.g. isascii() and isdigit() or a pattern on PollOptionGroupData.client_key.
5. SINGLE_CHOICE: can't switch within a capped group
In SINGLE_CHOICE the YES button doesn't toggle. Once one option of a group with max_selections is selected, canSelect() disables the other members of that group, so switching from A to B only works via a detour through an ungrouped option. Groups add nothing in SINGLE_CHOICE anyway — I'd skip the group check (or hide the group UI) for that mode.
6. Tests
The PR adds validation logic and a migration but no tests. Please add at least: cap rejects too many YES / allows YES + MAYBE / ungrouped options uncapped / group without max_selections is unlimited, update keeps group id and changes cap, removing a group ungroups its options, unknown group reference is rejected, plus a route test for the 400 on vote POST. Cases for 3. and 4. would be good as regression tests.
Poll authors can group text/date options and cap how many of a group may be selected (e.g. "max 1 of A and B"), enforced independently of the poll's response_mode. Adds PollOptionGroup, per-option group assignment in the edit UI, per-group vote enforcement client- and server-side, and the poll_option_groups migration. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Poll authors can group text/date options and cap how many of a group may be selected (e.g. "max 1 of A and B"), enforced independently of the poll's response_mode. Adds PollOptionGroup, per-option group assignment in the edit UI, per-group vote enforcement client- and server-side, and the poll_option_groups migration.