feat(filesharing): optional name disjunction via condiscon; fix canEncrypt reactivity#240
Conversation
…ence Replace the previous (PR #239) mandatory pbdf.gemeente.personalData.fullname disclosure with a Yivi disjunction-of-conjunctions: the signer must disclose a name, but they may satisfy that from any one of four credentials -- gemeente fullname, OR firstName+lastName from pbdf.pbdf.{passport,idcard,drivinglicence}. This addresses dobby's review of #239: requiring only the gemeente credential silently locked out everyone without a Dutch municipality attestation. The disjunction is mandatory -- disclosure refuses to complete unless at least one option is satisfied. Optional mobilenumber and dateofbirth remain unchanged. The disjunction lives in a new signAttributes.ts module exporting a typed AttrConItem[] consumed by SendButton.svelte. Splitting it out keeps the component file focused and the attribute list reviewable in isolation. Locale copy updates flagged by dobby on en.json/nl.json: - emailSenderSubHeading describes the four-credential rule. - yiviTip no longer frames the name as optional. The $derived.by + (!canEncrypt) call-site fixes from #239 are preserved. Depends on encryption4all/postguard#198 (PKG), postguard-js#78 (sign.yivi attrs), and cryptify#170 (render firstName+lastName). Supersedes #239 in this repo. npm run check: 0 errors, 0 warnings.
|
Dobby is out of tokens — usage limit hit. Resets 6/1/2026, 5:04:52 PM. I'll pick this up automatically then! |
|
Dobby has received the request! Routing to the right specialist now... |
Remove the mandatory name disjunction from SIGN_ATTRIBUTES so senders only need to prove their email address (the pre-#239 UX). The condiscon infrastructure in postguard/pg-js remains available for future use. Update locale strings to no longer mention the name requirement.
|
/dobby review |
|
Dobby has received the request! Routing to the right specialist now... |
![dobby-coder[bot]](https://avatars.githubusercontent.com/in/3365457?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code review
Final code reverts the disjunction feature and only keeps the Svelte 5 reactivity fix; the PR title, body, in-code comment, test plan, and listed companion PRs still describe the abandoned feature. Also a silent regression: the previously-optional pbdf.gemeente.personalData.fullname is gone, so senders can no longer disclose their gemeente fullname — confirm this is intended and call it out in the body.
Rule compliance
conventional-commit-pr-titles: prefixfeat(filesharing):is wrong — the delivered change is a refactor + reactivity fix, not a new feature. Retitle to e.g.refactor(filesharing): extract sign attributes; fix canEncrypt reactivity.pr-close-issue-keywords:Closes #239targets a PR, not an issue — closing keywords only auto-close issues. Drop the keyword or point it at the original tracking issue.tests-required-on-fixes: no Playwright regression test for the$derived→$derived.byfix. Add one (or note explicitly why a manual smoke is the only practical verification).
| ) | ||
|
|
||
| // Build sign method — email always included, other attributes optional | ||
| // Build sign method — email and a name attribute are always |
There was a problem hiding this comment.
[Code review] Comment is wrong: claims a name attribute is required and points to a "disjunction" in signAttributes.ts, but that file has no name attribute and no disjunction — only optional mobilenumber + dateofbirth. [Rule: no-justification-paragraphs-for-simple-changes] Also over-explained for a small refactor — trim to one line that matches the actual contents (email-only required; phone/dob optional).
| * attribute, so it's not listed here. Everything here is optional — the | ||
| * sender only needs to prove their email address. | ||
| */ | ||
| export const SIGN_ATTRIBUTES = [ |
There was a problem hiding this comment.
[Code review] Behavior change vs main: the previously-available optional pbdf.gemeente.personalData.fullname is dropped. Senders who would have disclosed their gemeente fullname can no longer do so. Confirm this is intentional and surface it in the PR body so it isn't a silent regression.
Bump @e4a/pg-js to 1.11.0 which adds AttrDiscon support. Use it to request the sender's name optionally from any of four credentials (gemeente fullname, passport, ID card, or driving licence). The leading empty alternative keeps the group skippable for senders without any of these credentials.
|
/dobby review |
|
Dobby has received the request! Routing to the right specialist now... |
There was a problem hiding this comment.
Code review
Stage 1 (re-review): code matches in-code comments and AttrConItem/AttrDiscon types from pg-js 1.11.0; npm run check passes 0/0; prior findings resolved.
Rule compliance
One promised-vs-delivered mismatch worth resolving before merge: the PR body says "the signer must disclose a name" and the test plan asks you to "confirm disclosure refuses" when no name credential is present — but the code (and in-code comment) makes the name group optional via the leading [] alternative. Either drop the [] to actually require a name, or update the PR body + test plan to reflect that name is optional. No other rule violations.
| */ | ||
| export const SIGN_ATTRIBUTES: AttrConItem[] = [ | ||
| [ | ||
| [], |
There was a problem hiding this comment.
[Rule: promised-vs-delivered] This leading [] makes the whole name group optional, but the PR body promises name is mandatory ("the signer must disclose a name", test plan: "confirm disclosure refuses" without any of the four credentials). Pick one: remove [] (and update the doc comment above) to match the PR body, or update the PR body + test plan to match the optional-name behavior the code actually implements.
rubenhensen
changed the title
|
/dobby review |
|
Dobby has received the request! Routing to the right specialist now... |
There was a problem hiding this comment.
Code review
Round 3 re-review: PR body, test plan, in-code comment, and code now consistently describe name disclosure as optional via the leading [] discon alternative. npm run check clean; pg-js 1.11.0 exports AttrConItem/AttrDiscon and the structure type-checks. All prior round findings resolved.
Rule compliance
No issues found.
1 participant
Summary
pbdf.pbdf.{passport,idcard,drivinglicence}, or skip name disclosure entirely.[]alternative in the disjunction follows Yivi convention for optional discons — senders without any of the four credentials can still send.src/lib/components/filesharing/signAttributes.tsexports the typedAttrConItem[];SendButton.svelteconsumes it.en.json/nl.json: updateemailSenderSubHeadingto reflect that name is optional and lists the accepted credential types.$derived.by(canEncrypt)andif (!canEncrypt) returnreactivity fixes from feat(filesharing): require signer fullname; fix canEncrypt reactivity #239.Why
Dobby's review of #239 flagged that requiring
pbdf.gemeente.personalData.fullnamesilently locks out anyone without a Dutch municipality credential. This PR makes name disclosure optional but accepts any of four ID credentials when the sender does choose to share their name, minimising friction for senders.Companion PRs
The disjunction shape requires changes in the wire format and rendering layer. All are backwards compatible:
encryption4all/postguard#198— PKG: accept Yivi condiscon inIrmaAuthRequest.encryption4all/postguard-js#78—pg.sign.yivi({ attributes })accepts discons (shipped as 1.11.0).encryption4all/cryptify#170—sender_displayrendersfirstName + lastNamefrom the three ID credentials; falls back to "PostGuard" when no name is disclosed.Supersedes #239
Test plan
npm run check— 0 errors, 0 warnings.pbdf.pbdf.passportcredential; confirm disclosure completes and the recipient mail showsfirstName lastName. Repeat withidcardanddrivinglicence.