feat(site): allow disabling email verification#1540
Conversation
LinkinStars
left a comment
There was a problem hiding this comment.
The compatibility and default-safe behavior in this change look solid, and I agree with the overall direction. That said, I think there are still a few backend design / maintainability issues worth revisiting before merge:
-
OptionalBool feels too heavy for this use case. To handle the omitted / null / false cases for require_email_verification, the PR introduces a dedicated OptionalBool type in the schema layer. The issue is that this API does not read like a clearly defined patch-style endpoint; it looks more like a normal save/update API. Adding a special tri-state wrapper for a single field increases the cognitive load in both the schema and save logic.
If this is intended to be a full save API, I would prefer requiring the client to send the full field set. If this is intended to be a partial update API, then it would be better to make that explicit and handle partial-update semantics consistently rather than special-casing one field. -
Treating null as true is safe, but not very intuitive semantically.
In the current implementation, both the migration path and the runtime save path normalize null to true. I understand why this was done from a security/default-safety perspective, but from an API semantics perspective, null usually reads more like “unset” than “explicitly enabled”. If we want to keep this behavior, I think it should at least be documented more explicitly in code/comments; otherwise it is not obvious to future maintainers why null does not preserve the current value, fail validation, or behave like an omitted field. -
applyEmailVerificationSetting is a very thin abstraction. This helper currently does little more than req.SkipEmailVerification = !siteInfo.RequireEmailVerification. Extracting that into a separate function adds another jump for the reader without really encapsulating meaningful logic yet. Unless we expect more registration policy logic to live there soon, I think keeping this mapping inline at the call site would actually be clearer.
-
The naming mixes a positive configuration flag with a negative execution flag. At the config level we have RequireEmailVerification, while at the request/service level we introduce SkipEmailVerification, which means we have to invert the meaning in between. This works, but it hurts readability and makes future changes easier to get wrong. I would strongly prefer keeping the naming direction consistent across layers, for example using RequireEmailVerification end-to-end, or using a single enabled/disabled wording consistently.
Replace OptionalBool with an explicit require_email_verification value for the login settings save request while keeping legacy read defaults intact. Use positive RequireEmailVerification naming through the registration flow and inline the site setting mapping. Add validation, save-path, and registration coverage for the explicit email verification setting.
|
Thanks for the detailed review. I updated the login settings save endpoint to work as a full-save API instead of adding partial-update semantics for this one field. I removed OptionalBool and made require_email_verification required in the save request. Null or omitted values are now rejected by validation and by SaveSiteLogin. For existing stored configs, the safe legacy default is still preserved: missing/null values default to true, while an explicit false stays false. I also added a short comment in the v34 migration to make that default clear. I removed applyEmailVerificationSetting and renamed the internal registration flag from SkipEmailVerification to RequireEmailVerification, so the naming stays positive and consistent through the registration flow. The registration fallback behavior is still covered by tests. I regenerated Swagger so require_email_verification is marked as a required request field. Validation:
|
Fixes #1517
Proposed Changes
require_email_verificationlogin setting that defaults totrueto preserve the current behavior.Testing
go test ./internal/migrations -run 'TestBackfillRequireEmailVerification|TestAddRequireEmailVerification|TestSplitLegalMenuKeepsRequireEmailVerificationDefaultTrue' -count=1 -vgo test ./...make lintmake uimake build