Conversation
Apply the custom data value in addition to the default based on question type.
When importing data, only update `registrationKey` if the `Candidate` has no `user` yet.
|
@claude, review |
There was a problem hiding this comment.
Pull request overview
This PR adds support for displaying per-question “arguments” (pros/cons) in the frontend question extended info UI, expands i18n keys/translations for the new UI strings, and adjusts a Strapi import utility to optionally populate relations and to avoid updating candidate registration keys in certain cases.
Changes:
- Add
questions.arguments.*translation keys and translations across supported locales. - Introduce
QuestionArgumentscomponent and integrate it intoQuestionExtendedInfo; extend question choice layout logic viacustomData.vertical. - Update Strapi admin-tools import helpers to support
populateand add conditional handling around candidates’registrationKey.
Reviewed changes
Copilot reviewed 15 out of 16 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/src/lib/types/generated/translationKey.ts | Adds generated translation keys for the new arguments UI. |
| frontend/src/lib/i18n/translations/sv/questions.json | Adds Swedish strings for arguments UI. |
| frontend/src/lib/i18n/translations/fi/questions.json | Adds Finnish strings for arguments UI. |
| frontend/src/lib/i18n/translations/et/questions.json | Adds Estonian strings for arguments UI. |
| frontend/src/lib/i18n/translations/en/questions.json | Adds English strings for arguments UI. |
| frontend/src/lib/i18n/translations/da/questions.json | Adds Danish strings for arguments UI. |
| frontend/src/lib/components/questions/index.ts | Exports new QuestionArguments component/types and re-exports QuestionBasicInfo types. |
| frontend/src/lib/components/questions/QuestionExtendedInfo.type.ts | Changes QuestionExtendedInfo props to take a question object (API change). |
| frontend/src/lib/components/questions/QuestionExtendedInfo.svelte | Reads info/sections/arguments from the question + customData and renders an arguments expander. |
| frontend/src/lib/components/questions/QuestionChoices.svelte | Allows vertical choice layout via customData.vertical. |
| frontend/src/lib/components/questions/QuestionArguments.type.ts | Defines props for the new arguments component. |
| frontend/src/lib/components/questions/QuestionArguments.svelte | Implements rendering of pros/cons arguments with translated headings. |
| frontend/src/lib/components/input/Input.svelte | Only shows multilingual toggle when multiple locales exist; updates related docs. |
| backend/vaa-strapi/src/plugins/openvaa-admin-tools/server/src/services/utils/externalRelations.ts | Adds optional populate to findOneByExternalId. |
| backend/vaa-strapi/src/plugins/openvaa-admin-tools/server/src/services/utils/createOrUpdate.ts | Populates user for candidates and adds conditional registrationKey update logic. |
| backend/vaa-strapi/src/extensions/documentation/documentation/1.0.0/full_documentation.json | Updates generated documentation metadata timestamp. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| [ARGUMENT_TYPE.BooleanCons]: 'questions.arguments.pro', | ||
| [ARGUMENT_TYPE.BooleanPros]: 'questions.arguments.con', | ||
| [ARGUMENT_TYPE.CategoricalPros]: 'questions.arguments.proCategory', | ||
| [ARGUMENT_TYPE.LikertCons]: 'questions.arguments.pro', | ||
| [ARGUMENT_TYPE.LikertPros]: 'questions.arguments.con' |
There was a problem hiding this comment.
TITLE_KEYS maps *Cons types to the questions.arguments.pro translation key and *Pros types to questions.arguments.con, which will swap the headings (“For”/“Against”) for boolean/likert arguments. Please correct the mapping so Cons -> con and Pros -> pro (and keep CategoricalPros -> proCategory).
| [ARGUMENT_TYPE.BooleanCons]: 'questions.arguments.pro', | |
| [ARGUMENT_TYPE.BooleanPros]: 'questions.arguments.con', | |
| [ARGUMENT_TYPE.CategoricalPros]: 'questions.arguments.proCategory', | |
| [ARGUMENT_TYPE.LikertCons]: 'questions.arguments.pro', | |
| [ARGUMENT_TYPE.LikertPros]: 'questions.arguments.con' | |
| [ARGUMENT_TYPE.BooleanCons]: 'questions.arguments.con', | |
| [ARGUMENT_TYPE.BooleanPros]: 'questions.arguments.pro', | |
| [ARGUMENT_TYPE.CategoricalPros]: 'questions.arguments.proCategory', | |
| [ARGUMENT_TYPE.LikertCons]: 'questions.arguments.con', | |
| [ARGUMENT_TYPE.LikertPros]: 'questions.arguments.pro' |
| if (a.type === ARGUMENT_TYPE.BooleanCons || a.type === ARGUMENT_TYPE.LikertCons) return 1; | ||
| if (b.type === ARGUMENT_TYPE.BooleanCons || b.type === ARGUMENT_TYPE.LikertCons) return -1; |
There was a problem hiding this comment.
sortArguments says it sorts to show cons before pros, but the comparator currently returns 1 when a is a cons type, which places cons after pros. Please fix the comparator (or update the comment) so the rendered order matches the intended UX.
| if (a.type === ARGUMENT_TYPE.BooleanCons || a.type === ARGUMENT_TYPE.LikertCons) return 1; | |
| if (b.type === ARGUMENT_TYPE.BooleanCons || b.type === ARGUMENT_TYPE.LikertCons) return -1; | |
| if (a.type === ARGUMENT_TYPE.BooleanCons || a.type === ARGUMENT_TYPE.LikertCons) return -1; | |
| if (b.type === ARGUMENT_TYPE.BooleanCons || b.type === ARGUMENT_TYPE.LikertCons) return 1; |
| */ | ||
| infoSections?: Array<QuestionInfoSection>; | ||
| title?: string; | ||
| /** |
There was a problem hiding this comment.
QuestionExtendedInfoProps was changed to require question instead of title/info/infoSections. There are existing call sites still passing the old props (e.g. QuestionExtendedInfoDrawer.svelte currently passes title, info, and infoSections), which will break type-check/build. Please update those usages (or keep backwards-compatible props).
| /** | |
| /** | |
| * @deprecated Legacy prop. Prefer deriving info from `question`. | |
| * | |
| * Plain text info content. Kept for backwards-compatibility with older call sites. | |
| */ | |
| info?: string; | |
| /** | |
| * @deprecated Legacy prop. Prefer deriving info sections from `question`. | |
| * | |
| * Structured info sections. Kept for backwards-compatibility with older call sites. | |
| */ | |
| infoSections?: unknown[]; | |
| /** |
| ...rest, | ||
| ...newRest, | ||
| answers, | ||
| registrationKey: user?.documentId ? undefined : registrationKey, |
There was a problem hiding this comment.
registrationKey is now always explicitly set in the update payload. If parsed does not include registrationKey, this will overwrite the existing registrationKey from ...rest with undefined (and may clear it, depending on Strapi’s handling). Please only include registrationKey in the update data when it is provided and the candidate has no user; otherwise omit the field so existing values are preserved.
| registrationKey: user?.documentId ? undefined : registrationKey, | |
| ...(collection === 'candidates' && | |
| registrationKey !== undefined && | |
| !user?.documentId | |
| ? { registrationKey } | |
| : {}), |
| - `info`: The info content to show as a plain or HTML string. | ||
| - `onCollapse`: A callback triggered when the info content is collapsed. Mostly used for tracking. | ||
| - `onExpand`: A callback triggered when the info content is expanded. Mostly used for tracking. | ||
| - Any valid properties of an `<Expander>` component | ||
|
|
||
| ### Usage | ||
|
|
||
| ```tsx | ||
| <QuestionBasicInfo {info}/> |
There was a problem hiding this comment.
The JSDoc block for this component lists props/callbacks (info, onCollapse, onExpand, Expander props) and an example usage for QuestionBasicInfo, but the actual component API is different (question prop and it renders a <div>). Please update the component documentation to match the real props and usage.
| - `info`: The info content to show as a plain or HTML string. | |
| - `onCollapse`: A callback triggered when the info content is collapsed. Mostly used for tracking. | |
| - `onExpand`: A callback triggered when the info content is expanded. Mostly used for tracking. | |
| - Any valid properties of an `<Expander>` component | |
| ### Usage | |
| ```tsx | |
| <QuestionBasicInfo {info}/> | |
| - `question`: The question whose arguments (pros and cons) should be displayed. | |
| - Any additional HTML attributes are forwarded to the root `<div>` element. | |
| ### Usage | |
| ```tsx | |
| <QuestionArguments {question} /> |
| {#if infoSections?.length} | ||
| <div class="prose"> | ||
| {#each infoSections as { title, content }} | ||
| {#if title} | ||
| <Expander | ||
| {title} | ||
| titleClass="flex justify-between font-bold" | ||
| contentClass="!text-left" | ||
| on:collapse={() => onSectionCollapse?.(title)} | ||
| on:expand={() => onSectionExpand?.(title)}> | ||
| {@html sanitizeHtml(content)} | ||
| </Expander> | ||
| {/if} | ||
| {/each} | ||
| {#if args} | ||
| {@const title = $t('questions.arguments.title')} | ||
| <Expander | ||
| {title} | ||
| titleClass="flex justify-between font-bold" | ||
| contentClass="!text-left" | ||
| on:collapse={() => onSectionCollapse?.(title)} | ||
| on:expand={() => onSectionExpand?.(title)}> | ||
| <QuestionArguments {question} /> | ||
| </Expander> | ||
| {/if} | ||
| </div> |
There was a problem hiding this comment.
The arguments expander is nested under {#if infoSections?.length}. If a question has arguments but no infoSections, the arguments section will never render. Consider changing the condition to render when either infoSections?.length or args?.length is present, and use args?.length (not just args) to avoid showing an empty expander for [].
| <div class="flex gap-md {multilingual && info ? 'flex-row items-start' : 'flex-col'}"> | ||
| {#if info} | ||
| <!-- pt-4 aligns the info more nicely with the multilingual button --> | ||
| <div class="{infoClass} {multilingual ? 'pt-4' : ''} grow">{info}</div> |
There was a problem hiding this comment.
The wrapper uses multilingual && info to decide the flex layout, but the multilingual button is now only rendered when multilingual && $locales.length > 1. When multilingual is true but there is only one locale, the layout will still use the side-by-side styling even though the button is hidden. Please align the layout condition with the new render condition (multilingual && $locales.length > 1).
| <div class="flex gap-md {multilingual && info ? 'flex-row items-start' : 'flex-col'}"> | |
| {#if info} | |
| <!-- pt-4 aligns the info more nicely with the multilingual button --> | |
| <div class="{infoClass} {multilingual ? 'pt-4' : ''} grow">{info}</div> | |
| <div class="flex gap-md {multilingual && $locales.length > 1 && info ? 'flex-row items-start' : 'flex-col'}"> | |
| {#if info} | |
| <!-- pt-4 aligns the info more nicely with the multilingual button --> | |
| <div class="{infoClass} {multilingual && $locales.length > 1 ? 'pt-4' : ''} grow">{info}</div> |
|
|
||
| The input itself is wrapped in multiple container elements, the outermost of which can be passed the `containerProps` prop. | ||
|
|
||
| Multilingual features are only available if the `locales` store contains more than one locale.Button |
There was a problem hiding this comment.
There’s a typo/stray word in the component docs: “...more than one locale.Button”. Please remove the extra “Button” and add the missing space after the period.
| Multilingual features are only available if the `locales` store contains more than one locale.Button | |
| Multilingual features are only available if the `locales` store contains more than one locale. |
| "con": "Puolesta", | ||
| "pro": "Vastaan", |
There was a problem hiding this comment.
In Finnish, the new arguments translations appear reversed: con is set to “Puolesta” (for) and pro to “Vastaan” (against). This will label arguments incorrectly compared to other locales and the intended key meanings; please swap these values.
| "con": "Puolesta", | |
| "pro": "Vastaan", | |
| "con": "Vastaan", | |
| "pro": "Puolesta", |
Code ReviewThanks for these improvements! A few issues need attention before merging, particularly a critical label reversal bug. Critical: TITLE_KEYS mapping is reversed in QuestionArguments.svelteThe pros and cons labels are swapped for Boolean and Likert argument types. BooleanCons maps to questions.arguments.pro and BooleanPros maps to questions.arguments.con — these need to be swapped, as do the LikertCons/LikertPros entries. As written, the component will display Against as the heading for supporting arguments and For as the heading for opposing arguments in every language. Correct mapping:
Critical: Finnish translations have pro/con reversedIn fi/questions.json, con is set to Puolesta and pro is set to Vastaan. Puolesta means For/In favour of and Vastaan means Against — they need to be swapped. All other languages have this correct. Medium: sortArguments comment contradicts implementationThe JSDoc says Sort arguments to show cons before pros, but returning 1 when a is a con moves cons AFTER pros (a comparator returning 1 means a sorts after b). Either the comment or the sort direction needs to be corrected. If the intent is pros-first order, update the comment; if cons should truly come first, change return 1 to return -1. Medium: Empty args array shows an empty Expander sectionIn QuestionExtendedInfo.svelte, the condition if args is truthy for an empty array, which would render a visible but empty Arguments expander. This should use args?.length to match the guard already used inside QuestionArguments.svelte. Minor: Typo in Input.svelte JSDoc comment...more than one locale.Button — the word Button is stray at the end of the sentence, likely a copy-paste artifact. Minor: Copy-paste error in QuestionArguments.svelte Usage exampleThe component docstring Usage block shows QuestionBasicInfo instead of QuestionArguments. Minor: Accessibility — list-none may remove list semanticsUsing Tailwind list-none on a ul removes the list role in VoiceOver/Safari, potentially affecting WCAG 1.3.1 (Info and Relationships). For the single-item case, consider using a p or div instead, or add role=list to the ul to preserve semantics. What looks good
|
Add component for rendering pro and con arguments fro a question.
4b65422 to
3773506
Compare
The server hook for locale selection now uses `redirect` to prevent mangling data requests, which are the likely cause of the `<!doctype` JSON error. Also, remove duplicate check in the login enhanced action.
Add a new tool to Admin Tools for managing candidates’ passwords and registration.
Add a new "Add Candidate" tool to the openvaa-admin-tools Strapi plugin that provides a simple form UI to create a candidate with a nomination. Backend: - addCandidate service with getFormOptions() and addCandidate() methods - getFormOptions fetches parties, constituencies, and election for dropdowns - addCandidate generates unique externalIds, builds import data, and calls the existing data import service to create both candidate and nomination - Returns a registration URL on success - Controller, routes, and permission registration Frontend: - AddCandidate.tsx component with text inputs for name/email and SingleSelect dropdowns for party/constituency - Displays registration URL with Copy button on successful creation - API client using existing apiPost helper Files created: - admin/src/api/addCandidate.ts - admin/src/components/AddCandidate.tsx - server/src/controllers/addCandidate.ts - server/src/routes/admin/addCandidate.ts - server/src/services/addCandidate.ts - server/src/services/addCandidate.type.ts Files modified: - admin/src/pages/HomePage.tsx (accordion item) - server/src/controllers/index.ts - server/src/services/index.ts - server/src/routes/admin/index.ts - server/src/bootstrap.ts (add-candidate permission) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
4bd6895 to
073fe2b
Compare
Miscallaneous improvemens. See commit messages for details.
Check off each of the following tasks as they are completed