Skip to content

fix(store): preserve user profile on upsert and set default role - #18

Closed
GormYa wants to merge 4 commits into
tabloy:mainfrom
GormYa:main
Closed

fix(store): preserve user profile on upsert and set default role#18
GormYa wants to merge 4 commits into
tabloy:mainfrom
GormYa:main

Conversation

@GormYa

@GormYa GormYa commented Aug 27, 2026

Copy link
Copy Markdown

Ensure UpsertUser does not overwrite existing name and avatar_url with empty values on conflict (such as during OTP login or Stripe sync). Also default missing user role to 'user' on insert.

What does this PR do?

  • Updates UpsertUser in internal/store/store.go to conditionally update name and avatar_url on conflict (ON CONFLICT (email) DO UPDATE).
  • Prevents existing profile fields (name, avatar_url) from being wiped out by empty strings ("") when a user signs in via OTP or is upserted via Stripe webhook.
  • Ensures new users without an explicit role default to model.RoleUser during insertion.

Why?

  • Fixes Display Name is cleared from database on login #17
  • Previously, invoking UpsertUser with only an email unconditionally replaced name and avatar_url with empty values, destroying previously saved profile data (e.g. populated via OAuth or profile updates).

How to test

  1. Create/insert a user with name and avatar_url populated (e.g., through OAuth login or direct store insert).
  2. Trigger an upsert flow passing only the email (e.g., OTP login or UpsertUser(ctx, &model.User{Email: email})).
  3. Query the user record to verify that the existing name and avatar_url remain intact.
  4. Insert a new user without a role and verify that the role column defaults to user.
  5. Run tests: go test ./... and go vet ./....

Checklist

  • go vet ./... passes
  • go test ./... passes
  • Frontend builds (cd web && bun run build)
  • Tested manually in browser

Ensure UpsertUser does not overwrite existing name and avatar_url with empty values on conflict (such as during OTP login or Stripe sync). Also default missing user role to 'user' on insert.
Add full Turkish (tr) localization across all frontend pages and wire up i18n support in the releases management interface.

- Create web/src/i18n/locales/tr.ts with 623 translation keys.
- Update web/src/i18n/index.tsx to register the Turkish locale and support browser language detection for Turkish.
- Add Turkish language selection to admin settings and persist it in backend configuration.
- Extract all hardcoded strings in the releases page into i18n keys across English, Turkish, and Chinese translation files.
- Update README.md to list Turkish under built-in i18n languages.
@GormYa

GormYa commented Aug 27, 2026

Copy link
Copy Markdown
Author

Add Turkish (tr) localization support and i18n for releases page

What does this PR do?

  • Adds complete Turkish (tr) localization support across all frontend pages with 623 translated keys (web/src/i18n/locales/tr.ts).
  • Updates web/src/i18n/index.tsx to register the tr locale and support automatic Turkish browser language detection.
  • Adds Turkish (Türkçe) to the language selection menu in Admin Settings and includes language in FORM_KEYS to persist the site default language in backend settings.
  • Extracts all previously hardcoded strings in web/src/pages/admin/releases.tsx into i18n keys across English (en.ts), Turkish (tr.ts), and Chinese (zh.ts).
  • Updates README.md to list Turkish under the built-in i18n languages.

Why?

  • Enables Turkish language support for both the admin panel and the customer portal.
  • Ensures the Releases page adheres to the internationalization standard used across the rest of the application rather than rendering hardcoded English strings.

How to test

  1. Open the application and navigate to Admin > Settings.
  2. Select Türkçe from the Language (Dil) dropdown menu and click Save (Kaydet).
  3. Verify that all dashboard pages (Dashboard, Products, Plans, Releases, Licenses, API Keys, Webhooks, Analytics, Audit Log, Settings, Customer Portal, Login) render in Turkish.
  4. Go to Releases (/admin/releases) and test dialogs (Create Release, Add Artifact, Yank, Signing Keys) and status badges to verify all UI elements display translated strings.
  5. Switch between English, Chinese, and Turkish to verify smooth transitions and translation parity.
  6. Run automated checks:
    • cd web && npm run typecheck
    • cd web && npm run build
    • go test ./...

Checklist

  • Key and placeholder parameter parity verified across en.ts, tr.ts, and zh.ts (623 keys each)
  • TypeScript typecheck passes (npm run typecheck)
  • Frontend builds cleanly (npm run build)
  • Backend tests pass (go test ./...)
  • Tested manually in browser

@alanisme alanisme left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this is a solid PR. The upsert fix is right, and the Turkish translation is complete — key parity is exact and all placeholders line up. Nice catch on wiring language into the settings form too, that was dead until now.

Two things before merge:

  1. The TIMEZONES list lost 7 entries (Europe/Paris, Asia/Karachi, Asia/Dhaka, America/Caracas, Pacific/Midway, Atlantic/Azores, Atlantic/South_Georgia). Anyone already saved on one of those will see an empty select. Can you add Istanbul without dropping the others?

  2. bunx biome check --write — tr.ts, zh.ts and releases.tsx fail lint. bun run build doesn't run it.

@GormYa

GormYa commented Aug 28, 2026

Copy link
Copy Markdown
Author
  • Updates EmailService in internal/service/email.go to dynamically resolve active SMTP configuration (smtp_host, smtp_port, smtp_username, smtp_password, smtp_from) from the database (settings table) at runtime.
  • Enables SMTP configuration updates made in the Web Admin Dashboard to take effect immediately without requiring a server restart or .env modification.
  • Adds native Port 465 (Implicit TLS / SMTPS) support via tls.DialWithDialer, while preserving standard STARTTLS (Explicit TLS) upgrades on port 587 and port 25.
  • Adds unit tests in internal/service/email_auth_test.go covering Port 465 implicit TLS handshakes and dynamic configuration fallback behavior.

Why?

  • EmailService previously relied exclusively on static environment variables loaded at startup; configuring or modifying SMTP settings from the Web Admin Dashboard was ignored by the email sender and evaluated IsConfigured() to false, causing "SMTP is not configured on this server" errors when sending test emails or OTPs.
  • Connecting to Port 465 over standard TCP caused connection timeouts (connectex) with providers that require immediate TLS handshakes (such as Yandex Mail, cPanel, and direct SMTPS relays).

How to test

  1. Open the application and navigate to Admin > Settings > Email / SMTP.
  2. Fill in the SMTP credentials (e.g., Host: smtp.yandex.com, Port: 465 or 587, Username, App Password, and From email) and click Save.
  3. Click Send Test Email (Test E-postası Gönder) and verify that the test email is sent successfully without needing to restart the backend service.
  4. Test with both Port 465 (Implicit TLS) and Port 587 (STARTTLS) to verify both encryption modes work.
  5. Remove SMTP_* variables from .env and verify that the database-persisted settings continue to work.
  6. Run automated checks:
    • go test -v -run "TestSendOnce|TestEmailService" ./internal/service/...
    • go test ./...

Checklist

  • Dynamic SMTP configuration resolution implemented with database settings taking precedence over environment variables
  • Port 465 (Implicit TLS) and Port 587/25 (STARTTLS) dual compatibility verified
  • Unit tests added for Port 465 implicit TLS delivery and dynamic config resolution (email_auth_test.go)
  • All backend unit and integration tests pass (go test ./...)
  • Tested manually with SMTP endpoints

@GormYa

GormYa commented Aug 28, 2026

Copy link
Copy Markdown
Author

Thanks, this is a solid PR. The upsert fix is right, and the Turkish translation is complete — key parity is exact and all placeholders line up. Nice catch on wiring language into the settings form too, that was dead until now.

Two things before merge:

  1. The TIMEZONES list lost 7 entries (Europe/Paris, Asia/Karachi, Asia/Dhaka, America/Caracas, Pacific/Midway, Atlantic/Azores, Atlantic/South_Georgia). Anyone already saved on one of those will see an empty select. Can you add Istanbul without dropping the others?
  2. bunx biome check --write — tr.ts, zh.ts and releases.tsx fail lint. bun run build doesn't run it.

I'm on it; I'll push the corrected version shortly.

- Restore dropped timezone entries and add new regions including Europe/Istanbul
- Fix code formatting and linting issues in tr.ts, zh.ts, and releases.tsx
@GormYa

GormYa commented Aug 28, 2026

Copy link
Copy Markdown
Author

Restored all dropped timezone entries in TIMEZONES and added Europe/Istanbul & others.
Formatted and verified tr.ts, zh.ts, releases.tsx, and settings.tsx with Biome lint/format rules (biome check passes cleanly).

@alanisme alanisme left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Timezones and lint look good now, thanks.

Could you move the SMTP commit into its own PR? It's unrelated to #17 and people are waiting on that fix, and the config merge has something I'd rather not rush.

smtp_password is deliberately write-only — admins can save it but never read it back. getConfig overrides the host whenever the DB has one, but only overrides the password when the DB value is non-empty. So an admin who points smtp_host at another server and leaves the password field blank gets the env SMTP_PASSWORD sent to that server. I checked it against a local SMTP server and it authenticates with the env password. The username branch already goes for "DB host means DB owns auth" — the password just needs the same rule.

Smaller things for that PR: getConfig queries the settings table on every IsConfigured() and every Send(), so one queue tick can be 20 extra reads, and it uses context.Background() so there's no timeout. TestEmailService_DynamicConfig passes a nil store, so the DB path isn't actually covered. The 465 support itself looks right.

Two things on this PR:

The language Select now drives both the admin's own UI language and the site-wide default from one control — switching my dashboard to Turkish also changes what new visitors get. And it reads value={locale}, so two admins see different values for the same setting. Could you bind the saved field to form.language and leave setLocale as the personal switcher?

Some of the Turkish uses the "Türkçe (English)" form, 22 keys. Reasonable for jargon, but a few land where there's no room: status.revoked renders inside a Badge, licenses.seats is a tab label, analytics.churned is a table header and a chart legend. zh only does this for releases.unyank. Could you drop the gloss on those?

@GormYa
GormYa marked this pull request as draft August 29, 2026 10:59
@GormYa GormYa closed this Aug 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Display Name is cleared from database on login

2 participants