Skip to content

Improve beta request emails#1953

Merged
myieye merged 2 commits into
developfrom
improve-beta-request-emails
Aug 22, 2025
Merged

Improve beta request emails#1953
myieye merged 2 commits into
developfrom
improve-beta-request-emails

Conversation

@myieye

@myieye myieye commented Aug 22, 2025

Copy link
Copy Markdown
Collaborator

Adding memberSearch=${encodedEmail} will be quite convenient. I basically always do that manually atm.
I think there's only been one request from a user with no email address, so that's not as critical, but it's still an improvement.

@coderabbitai

coderabbitai Bot commented Aug 22, 2025

Copy link
Copy Markdown
📝 Walkthrough

Walkthrough

Backend adjusts JoinFwLiteBetaEmail recipient logic to fall back to username when email is missing. Frontend updates approval URL construction by pre-encoding the email and adding memberSearch alongside userSearch, removing the extra slash before query and reusing the encoded value.

Changes

Cohort / File(s) Summary of changes
Backend: email recipient fallback
backend/LexBoxApi/Services/EmailService.cs
JoinFwLiteBetaEmail now receives user.Email ?? user.Username ?? "" instead of user.Email ?? "", adding a username fallback when email is absent.
Frontend: approval URL query params
frontend/src/lib/email/JoinFwLiteBetaRequest.svelte
Introduced encodedEmail = encodeURIComponent(email); approveUrl now uses /admin?userSearch=${encodedEmail}&memberSearch=${encodedEmail} (removed /admin/?, added memberSearch, reused pre-encoded value).

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

  • Display beta requester's email #1780 — Also modifies JoinFwLiteBetaRequest.svelte to change how the requester’s email is handled in the template, closely related to URL/email handling.

Suggested labels

📦 Lexbox

Suggested reviewers

  • rmunn

Poem

A hop, a bop, I tweak the link just right—
Two searches nibble on one encoded byte.
If email’s lost, a username we’ll try,
Carrot-colored URLs, oh my!
(\_/)\n> ( •_•)✨\n> />🥕 Shipping tonight!

Tip

🔌 Remote MCP (Model Context Protocol) integration is now available!

Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats.

✨ Finishing Touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch improve-beta-request-emails

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

CodeRabbit Commands (Invoked using PR/Issue comments)

Type @coderabbitai help to get the list of available commands.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Status, Documentation and Community

  • Visit our Status Page to check the current availability of CodeRabbit.
  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@github-actions github-actions Bot added the 📦 Lexbox issues related to any server side code, fw-headless included label Aug 22, 2025
@myieye myieye requested a review from Copilot August 22, 2025 07:20
@github-actions

Copy link
Copy Markdown
Contributor

UI unit Tests

 1 files  ±  0   3 suites   - 37   0s ⏱️ -23s
10 tests  -  72  10 ✅  -  72  0 💤 ±0  0 ❌ ±0 
10 runs   - 106  10 ✅  - 106  0 💤 ±0  0 ❌ ±0 

Results for commit 83871e5. ± Comparison against base commit c6b5cc8.

This pull request removes 82 and adds 10 tests. Note that renamed tests count towards both.
src/lib/components/stomp/stomp-guard.svelte.test.ts ‑ StompGuard > accepts parent values when not dirty
src/lib/components/stomp/stomp-guard.svelte.test.ts ‑ StompGuard > does NOT guard against stomping deep changes, because StompGuard can't detect them
src/lib/components/stomp/stomp-guard.svelte.test.ts ‑ StompGuard > ignores parent values when dirty
src/lib/components/stomp/stomp-guard.svelte.test.ts ‑ StompGuard > initializes with the parent value
src/lib/components/stomp/stomp-guard.svelte.test.ts ‑ StompGuard > keeps subscribers up to date when it becomes dirty
src/lib/components/stomp/stomp-guard.svelte.test.ts ‑ StompGuard > pushs changes to parent
src/lib/components/stomp/stomp-guard.svelte.test.ts ‑ StompGuard > reverts new parent values when ignored
src/lib/components/stomp/stomp-guard.svelte.test.ts ‑ StompGuard > starts accepting parent changes again after a flush
src/lib/components/ui/format/format-duration.test.ts ‑ formatDuration > formats durations
src/lib/components/ui/format/format-duration.test.ts ‑ formatDuration > formats durations with smallest unit
…
src/index.test.ts ‑ password hashing > can hash a pw using sha1
src/lib/i18n/i18n.test.ts ‑ buildRegionalLocaleRegex > should find all regional locales for available locales
src/lib/i18n/i18n.test.ts ‑ pickBestLocale > should return en by default
src/lib/i18n/i18n.test.ts ‑ pickBestLocale > should return en if no user locale is provided and acceptLanguageHeader does not have any supported locales
src/lib/i18n/i18n.test.ts ‑ pickBestLocale > should return regional locale from acceptLanguageHeader if it has a higher quality rating than the regionless locale
src/lib/i18n/i18n.test.ts ‑ pickBestLocale > should return supported locale from acceptLanguageHeader with highest quality rating if no user locale is provided
src/lib/i18n/i18n.test.ts ‑ pickBestLocale > should return user locale if acceptLanguageHeader does not provide a regional/more specific locale
src/lib/i18n/i18n.test.ts ‑ pickBestLocale > should return user locale if acceptLanguageHeader does not provide a regional/more specific locale with a higher quality rating
src/lib/i18n/i18n.test.ts ‑ pickBestLocale > should return user locale if acceptLanguageHeader is not provided
src/lib/user.test.ts ‑ jwtToUser > should convert a jwt token to a LexAuthUser

Copilot AI 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.

Pull Request Overview

This PR improves the beta request email functionality by adding a memberSearch parameter to admin URLs and providing a fallback for users without email addresses.

  • Adds memberSearch parameter to admin URLs for more convenient user lookup
  • Provides username fallback when email is not available for beta request emails

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
frontend/src/lib/email/JoinFwLiteBetaRequest.svelte Adds memberSearch parameter to admin URL and extracts encoded email to a variable
backend/LexBoxApi/Services/EmailService.cs Adds username fallback when user email is null in beta request emails

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment thread frontend/src/lib/email/JoinFwLiteBetaRequest.svelte
Comment thread backend/LexBoxApi/Services/EmailService.cs
@github-actions

Copy link
Copy Markdown
Contributor

C# Unit Tests

130 tests  ±0   130 ✅ ±0   19s ⏱️ -1s
 20 suites ±0     0 💤 ±0 
  1 files   ±0     0 ❌ ±0 

Results for commit 83871e5. ± Comparison against base commit c6b5cc8.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (3)
backend/LexBoxApi/Services/EmailService.cs (2)

228-228: Fallback to username is reasonable, but tighten semantics and whitespace handling

The change aligns with the PR goal of making approvals searchable even when a user lacks an email. Two nits:

  • Trim the chosen identifier to avoid accidental leading/trailing whitespace.
  • Use a local variable to clarify intent (“identifier” rather than “email”), since this value may be a username.

Apply:

-        await RenderEmail(email, new JoinFwLiteBetaEmail(user.Name, user.Email ?? user.Username ?? ""), user.LocalizationCode);
+        var identifier = string.IsNullOrWhiteSpace(user.Email) ? (user.Username ?? "") : user.Email;
+        await RenderEmail(email, new JoinFwLiteBetaEmail(user.Name, identifier.Trim()), user.LocalizationCode);

Also confirm the email template’s body copy doesn’t literally label this field as an email when it might be a username. If it does, consider updating the wording to “email/username” in translations.


227-227: Keep the support address hardcoded; remove the TODO to prevent drift

Per prior decision (learned from PR #1650), this address should remain hardcoded. To avoid future churn, replace the TODO with an explanatory comment.

Using retrieved learnings: The support address was intentionally hardcoded; keeping it that way is desired.

-        var email = StartUserEmail("Lexbox Support", "lexbox_support@groups.sil.org"); // TODO: Get from environment
+        var email = StartUserEmail("Lexbox Support", "lexbox_support@groups.sil.org"); // Intentionally hardcoded per team decision (see PR #1650)
frontend/src/lib/email/JoinFwLiteBetaRequest.svelte (1)

8-9: Use URLSearchParams to build the admin-link
Switching to URLSearchParams makes the intent clearer and offloads encoding to the API.

Locations:

  • frontend/src/lib/email/JoinFwLiteBetaRequest.svelte (around lines 8–9)

Suggested change:

-  const encodedEmail = encodeURIComponent(email);
-  let approveUrl = new URL(`/admin?userSearch=${encodedEmail}&memberSearch=${encodedEmail}`, baseUrl);
+  // Build query params declaratively and let URLSearchParams handle encoding
+  const params = new URLSearchParams({
+    userSearch: email,
+    memberSearch: email
+  });
+  const approveUrl = new URL(`/admin?${params}`, baseUrl);

Note: In this component email is always an actual email address, so it’s safe to always include memberSearch. In the admin UI:

  • userSearch drives a “contains” filter over name, email, and username.
  • memberSearch applies an exact-match filter on project membership by email or username.
    This refactor preserves the existing behavior and improves readability.
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between c6b5cc8 and 83871e5.

📒 Files selected for processing (2)
  • backend/LexBoxApi/Services/EmailService.cs (1 hunks)
  • frontend/src/lib/email/JoinFwLiteBetaRequest.svelte (1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-05-13T09:48:47.276Z
Learnt from: rmunn
PR: sillsdev/languageforge-lexbox#1650
File: backend/LexBoxApi/Services/EmailService.cs:0-0
Timestamp: 2025-05-13T09:48:47.276Z
Learning: The email address "lexbox_supportgroups.sil.org" in the SendJoinFWLiteBetaEmail method should remain hardcoded as per decision in meetings with Tim, despite the TODO comment.

Applied to files:

  • backend/LexBoxApi/Services/EmailService.cs
📚 Learning: 2025-05-13T09:48:02.112Z
Learnt from: rmunn
PR: sillsdev/languageforge-lexbox#1650
File: backend/LexBoxApi/Services/Email/IEmailService.cs:0-0
Timestamp: 2025-05-13T09:48:02.112Z
Learning: XML documentation for the `SendJoinFWLiteBetaEmail` method in `IEmailService` interface is not needed according to the project owner.

Applied to files:

  • backend/LexBoxApi/Services/EmailService.cs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
  • GitHub Check: Build UI / publish-ui
  • GitHub Check: Build API / publish-api
  • GitHub Check: check-and-lint
  • GitHub Check: Analyze (csharp)

@myieye myieye merged commit 6e42500 into develop Aug 22, 2025
18 checks passed
@myieye myieye deleted the improve-beta-request-emails branch August 22, 2025 09:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

📦 Lexbox issues related to any server side code, fw-headless included

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants