Feature/email service#62
Merged
Merged
Conversation
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughAdds a templated email system with Razor rendering and background-job dispatch; EmailAppService/IEmailAppService, EmailJobParameters, RefListEmailType, EmailTemplateRenderer, Razor/HTML templates and CSS, SendEmailBackgroundJob; CourseApplicationAppService and StudentAppService enqueue email jobs; also adds a Next.js withdraw page and provider wiring. ChangesEmail System with Background Job Processing
Sequence Diagram(s)sequenceDiagram
participant User as Student
participant App as CourseApplicationAppService
participant JobMgr as IBackgroundJobManager
participant Job as SendEmailBackgroundJob
participant EmailSvc as EmailAppService
participant Renderer as IEmailTemplateRenderer
participant Sender as IEmailSender
User->>App: Approve/Reject/Withdraw request
App->>App: Update application state
App->>JobMgr: Enqueue SendEmailBackgroundJob(EmailJobParameters)
Note over JobMgr: Job queued for async processing
JobMgr->>Job: Execute(params)
Job->>EmailSvc: Invoke SendXEmail(...)
EmailSvc->>Renderer: RenderAsync(template, model)
Renderer-->>EmailSvc: Rendered HTML
EmailSvc->>Sender: SendAsync(email, subject, body)
Sender-->>EmailSvc: Success/Failure
EmailSvc-->>Job: Task completion
Job-->>JobMgr: Execution done
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 15
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (6)
aspnet-core/src/Moipone.PublicSite.Core/Domain/CourseApplications/RefListApplicationStatus.cs-16-17 (1)
16-17:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winImplement withdrawal functionality for the new
Withdrawnstatus.The
Withdrawnenum value is defined but has no corresponding business logic or method to handle application withdrawals. Add aWithdrawApplicationmethod to both the interface and implementation, plus any necessary email dispatch for withdrawal notifications. Note: database migrations are not required for enum value additions—EF Core will persist the value as an integer without schema changes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aspnet-core/src/Moipone.PublicSite.Core/Domain/CourseApplications/RefListApplicationStatus.cs` around lines 16 - 17, Add a WithdrawApplication flow: add a WithdrawApplication(Guid applicationId, string reason, string requestedBy) method signature to the public service interface (e.g., ICourseApplicationService) and implement it in the concrete service class (e.g., CourseApplicationService or CourseApplicationManager). In the implementation, load the application entity, set its status to RefListApplicationStatus.Withdrawn, record withdrawal metadata (reason, withdrawnAt, withdrawnBy) on the entity, persist changes via the repository/DbContext SaveChangesAsync, and dispatch a withdrawal notification using the existing email/notification abstraction (e.g., IEmailSender or NotificationService) reusing the project’s templating patterns; also add/update unit tests for WithdrawApplication to assert status change, metadata set, persistence called, and email sent. Ensure method names and the enum RefListApplicationStatus.Withdrawn are used exactly as identifiers in the code.aspnet-core/src/Moipone.PublicSite.Application/Moipone.PublicSite.Application.csproj-13-13 (1)
13-13:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
CS8618:has a spurious trailing colon that may silently break the suppression.The canonical Roslyn warning code is
CS8618, notCS8618:. The colon suffix is not a valid part of the code, so MSBuild/Roslyn may not recognise it and the warning will still fire (or break builds if warnings-as-errors is ever enabled).🛠️ Proposed fix
- <NoWarn>$(NoWarn);1591;NU1902;NU1903;CS8618:</NoWarn> + <NoWarn>$(NoWarn);1591;NU1902;NU1903;CS8618</NoWarn>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aspnet-core/src/Moipone.PublicSite.Application/Moipone.PublicSite.Application.csproj` at line 13, The NoWarn property value contains a spurious trailing colon on the warning code "CS8618:" which prevents MSBuild/Roslyn from recognizing the suppression; edit the NoWarn entry in the Moipone.PublicSite.Application.csproj (the <NoWarn> property) and remove the trailing colon so the code appears exactly as "CS8618" (comma/semicolon-separated format preserved) to correctly suppress that warning.aspnet-core/src/Moipone.PublicSite.Web.Core/Emails/Templates/moipone-reminder.html-54-55 (1)
54-55:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFooter metadata is inconsistent with the rest of the new templates.
2025and123 Somewhere Streetdiffer from the other templates in this PR (2026and the Andrew Maphetho/Thami Mnyele address). Please align these values to avoid sending inconsistent brand/legal footer content.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aspnet-core/src/Moipone.PublicSite.Web.Core/Emails/Templates/moipone-reminder.html` around lines 54 - 55, Update the footer in the moipone-reminder.html template so it matches the other new templates: change the year string "2025" to "2026" and replace the address line "123 Somewhere Street · Johannesburg · South Africa" with the same contact line used elsewhere (e.g., "Andrew Maphetho · Thami Mnyele · Johannesburg · South Africa") so the copyright and contact metadata are consistent across templates.aspnet-core/src/Moipone.PublicSite.Web.Core/Emails/Templates/moipone-email.css-15-15 (1)
15-15:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winResolve stylelint
font-family-name-quotesviolations forLexend.These lines currently violate the configured lint rule and may break CI/lint checks. Either remove quotes from
Lexendin all occurrences or adjust the lint rule if quoted names are intended project-wide.Also applies to: 94-94, 123-123, 132-132, 154-154, 178-178, 186-186, 200-200, 223-223, 236-236, 251-251, 278-278
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aspnet-core/src/Moipone.PublicSite.Web.Core/Emails/Templates/moipone-email.css` at line 15, Replace the quoted font-family declaration occurrences that trigger stylelint by removing the quotes around Lexend (i.e., change any "font-family: 'Lexend', sans-serif;" instances to use unquoted Lexend) across the file; update every occurrence listed (the font-family declarations at the lines showing 'Lexend') so they read without single quotes, or alternatively adjust the stylelint rule if you intend to keep quoted family names project-wide—target the literal font-family declarations in moipone-email.css to make the change.aspnet-core/src/Moipone.PublicSite.Web.Core/Emails/Templates/_MoiponeEmailLayout.cshtml-13-13 (1)
13-13:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGoogle Fonts will not load in most email clients.
The vast majority of email clients (Gmail, Outlook for Windows, many mobile clients) block external
<link>requests, so the Lexend/Cormorant fonts will silently fall back to system defaults. This won't break rendering, but the designed typography won't appear for most recipients. Consider inlining afont-facedeclaration pointing to hosted woff2 assets, or accept the system-font fallback and remove the<link>to keep HTML lean.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aspnet-core/src/Moipone.PublicSite.Web.Core/Emails/Templates/_MoiponeEmailLayout.cshtml` at line 13, The external Google Fonts <link> in _MoiponeEmailLayout.cshtml will be blocked by most email clients; either remove that <link> to accept system-font fallback (cleaner HTML) or replace it by inlining `@font-face` rules that reference hosted woff2 assets included on your CDN/email-safe host and update the CSS font-family fallbacks accordingly; locate the existing <link> tag in _MoiponeEmailLayout.cshtml and either delete it and ensure body/font-family lists system stacks (e.g., Lexend fallback -> system sans-serif), or add inline <style> with `@font-face` declarations pointing to woff2 URLs and adjust the existing CSS to use those font-family names.aspnet-core/src/Moipone.PublicSite.Application/Services/Emails/EmailAppService.cs-32-33 (1)
32-33:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse a format specifier for culture-stable date and duration output.
course.StartDate.ToString()andcourse.Duration.ToString()inSendCourseReminderEmail(lines 32-33) andSendAdmissionEmail(lines 115-116) rely on the server's ambient culture. If the thread culture ever changes (e.g., different hosting locale), the value rendered inside the email template will differ without any code change.🔧 Proposed fix
-StartDate = course.StartDate.ToString(), -Duration = course.Duration.ToString(), +StartDate = course.StartDate.ToString("MMMM dd, yyyy", System.Globalization.CultureInfo.InvariantCulture), +Duration = course.Duration.ToString(), // use a domain-appropriate format once the type is confirmedAdjust the format string to match the desired display style and confirm the
Durationproperty type so you can apply a deterministic format (e.g.,"hh\\:mm"forTimeSpan, or a numeric format for anint/decimal).Also applies to: 115-116
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aspnet-core/src/Moipone.PublicSite.Application/Services/Emails/EmailAppService.cs` around lines 32 - 33, SendCourseReminderEmail and SendAdmissionEmail are calling course.StartDate.ToString() and course.Duration.ToString() which are culture-dependent; change these to use explicit, culture-stable format specifiers and invariant culture: format StartDate with a deterministic date format (e.g., ISO "yyyy-MM-dd" or round-trip "o") and format Duration based on its actual type (if Duration is a TimeSpan use a fixed TimeSpan format like @"hh\:mm" or if numeric use a numeric format) via ToString(format, CultureInfo.InvariantCulture); confirm the type of Duration and pick the appropriate format string and apply it where StartDate and Duration are currently converted.
🧹 Nitpick comments (7)
aspnet-core/src/Moipone.PublicSite.Web.Host/Startup/Startup.cs (1)
179-182: 💤 Low value
ConfigureEmailSmtp()is empty and never called — remove or implement it.The method has no body and is not invoked anywhere in
ConfigureServicesorConfigure. It appears to be a placeholder that was never completed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aspnet-core/src/Moipone.PublicSite.Web.Host/Startup/Startup.cs` around lines 179 - 182, The ConfigureEmailSmtp() method is an empty unused placeholder; either remove it or implement and wire it up. If removing: delete the ConfigureEmailSmtp() method and any dead references. If implementing: add SMTP setup logic (reading mail settings from configuration, registering a mail service or SmtpClientFactory, and configuring DI) inside ConfigureEmailSmtp(), then call ConfigureEmailSmtp() from Startup.ConfigureServices (or Configure) where other email services are registered. Ensure the method uses the Startup IConfiguration and IServiceCollection so the app actually registers the email-related services.aspnet-core/src/Moipone.PublicSite.Application/Services/Emails/TemplateModels/Rejection.cs (1)
3-8: 💤 Low valueUse
requiredor initialize to avoid nullable suppression dependency.With
<Nullable>enable</Nullable>, these uninitialized non-nullablestringproperties rely entirely on the blanketCS8618suppression (which itself has a trailing-colon bug, see the.csprojcomment). Usingrequired(C# 11+) or initializing tostring.Emptymakes the intent explicit and eliminates the need for the suppression.♻️ Proposed refactor
public class Rejection { - public string FirstName { get; set; } + public required string FirstName { get; set; } - public string RejectionReason { get; set; } + public required string RejectionReason { get; set; } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aspnet-core/src/Moipone.PublicSite.Application/Services/Emails/TemplateModels/Rejection.cs` around lines 3 - 8, The Rejection model's non-nullable properties FirstName and RejectionReason are uninitialized under nullable-enabled builds; update the Rejection class to either mark these properties with the C# 11 required modifier (e.g., required string FirstName { get; set; }) or initialize them to defaults (e.g., string.Empty) to remove reliance on CS8618 suppression; make the same change to both FirstName and RejectionReason so the intent is explicit and compiler warnings are resolved.aspnet-core/src/Moipone.PublicSite.Web.Core/Moipone.PublicSite.Web.Core.csproj (1)
37-41: 💤 Low valueConsider
PreserveNewestinstead ofAlwaysfor template content items.
CopyToOutputDirectory=Alwayscopies every template on every build regardless of whether the files changed.PreserveNewestavoids redundant I/O on incremental builds.⚙️ Proposed change
<Content Include="Emails\Templates\**"> - <CopyToOutputDirectory>Always</CopyToOutputDirectory> + <CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory> </Content>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aspnet-core/src/Moipone.PublicSite.Web.Core/Moipone.PublicSite.Web.Core.csproj` around lines 37 - 41, The project file currently uses CopyToOutputDirectory set to Always for the Content Include "Emails\Templates\**", causing all templates to be copied on every build; change the CopyToOutputDirectory value from Always to PreserveNewest for the Content Include "Emails\Templates\**" so MSBuild only copies updated files during incremental builds (edit the ItemGroup containing the Content Include and update the CopyToOutputDirectory element accordingly).aspnet-core/src/Moipone.PublicSite.Web.Core/Moipone.PublicSite.Web.Core.sln (1)
1-24: ⚡ Quick winThis single-project
.slnfile looks like an accidentally committed developer artifact.It lives inside the
Web.Coreproject directory and references only that one.csproj. The main solution for the repo is at a higher level and already includes this project. This file adds no CI/build value and will cause confusion. It should be removed and added to.gitignore.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aspnet-core/src/Moipone.PublicSite.Web.Core/Moipone.PublicSite.Web.Core.sln` around lines 1 - 24, The committed single-project solution file Moipone.PublicSite.Web.Core.sln is an accidental developer artifact that duplicates the higher-level solution; delete Moipone.PublicSite.Web.Core.sln from the repo and add an entry to .gitignore to prevent future commits (e.g., ignore Moipone.PublicSite.Web.Core.sln or *.sln in that subdirectory), and ensure the project remains referenced only by the top-level solution (the csproj {B14A29A1-DC71-0414-2D9E-2ECCD10DBEF1} / Moipone.PublicSite.Web.Core.csproj) so CI/builds continue to use the main solution.aspnet-core/src/Moipone.PublicSite.Application/Services/Emails/TemplateModels/Reminder.cs (1)
3-16: 💤 Low valueNon-nullable
stringproperties will produce CS8618 warnings if NRT is enabled.
IEmailAppService.csusesstring?syntax, indicating nullable reference types are active. All properties here are non-nullablestringwithout initializers orrequiredmodifiers, which conflicts with NRT semantics. Mark themstring?(orrequired stringfor fields that are always expected), consistent with howrejectionReasonis declared inIEmailAppService.♻️ Proposed change
public class Reminder { - public string FirstName { get; set; } - public string CourseTitle { get; set; } - public string StartDate { get; set; } - public string Duration { get; set; } - public string CourseDescription { get; set; } - public string WithdrawLink { get; set; } + public string? FirstName { get; set; } + public string? CourseTitle { get; set; } + public string? StartDate { get; set; } + public string? Duration { get; set; } + public string? CourseDescription { get; set; } + public string? WithdrawLink { get; set; } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aspnet-core/src/Moipone.PublicSite.Application/Services/Emails/TemplateModels/Reminder.cs` around lines 3 - 16, The Reminder model's string properties (FirstName, CourseTitle, StartDate, Duration, CourseDescription, WithdrawLink) are non-nullable while the project uses nullable reference types; update these properties to be nullable (string?) or mark them with required string if they must always be provided, matching the nullable usage in IEmailAppService (see how rejectionReason is declared) so CS8618 warnings are resolved and intent is explicit.aspnet-core/src/Moipone.PublicSite.Application/Services/Emails/BackgroundJobs/SendEmailBackgroundJob.cs (1)
17-38: ⚡ Quick winAdd a
defaultcase to the switch to catch unhandledEmailTypevalues.Without it, a misconfigured or future
EmailTypesilently produces a no-op — the job completes with no email sent and no indication of failure.♻️ Proposed fix
case RefListEmailType.Custom: _emailAppService.SendCustomEmail(args.Student, args.CustomEmail).GetAwaiter().GetResult(); break; + + default: + Logger.Warn($"SendEmailBackgroundJob: unhandled EmailType '{args.EmailType}'. No email sent."); + break; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aspnet-core/src/Moipone.PublicSite.Application/Services/Emails/BackgroundJobs/SendEmailBackgroundJob.cs` around lines 17 - 38, The switch over args.EmailType in SendEmailBackgroundJob must handle unexpected values: add a default case after the existing RefListEmailType cases that logs the unrecognized value and fails the job (throw an InvalidOperationException or similar) so misconfigured/future RefListEmailType values don't silently no-op; reference the switch on args.EmailType in SendEmailBackgroundJob and include the actual args.EmailType in the log/exception message and keep existing calls to _emailAppService.SendWelcomeEmail/SendAdmissionEmail/SendRejectionEmail/SendCourseReminderEmail/SendCustomEmail unchanged.aspnet-core/src/Moipone.PublicSite.Application/Services/Emails/EmailAppService.cs (1)
30-30: 💤 Low value
FirstNamereceives the full name — inconsistent with every other template model.All other methods assign only
student.NametoFirstName. Here, the concatenated full name is stored in the same property. If theReminderRazor template uses@Model.FirstNamewith the expectation of a first name, email recipients will see their full name where only the first name is expected. If a full name is intentional, rename the model property (e.g.,FullName) for clarity and template accuracy.♻️ Suggested fix (if a first-name-only greeting is intended)
-FirstName = student.Name + " " + student.Surname, +FirstName = student.Name,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aspnet-core/src/Moipone.PublicSite.Application/Services/Emails/EmailAppService.cs` at line 30, In EmailAppService (the model assignment for the Reminder email) FirstName is being set to student.Name + " " + student.Surname which differs from other templates; either set FirstName = student.Name to keep consistency with other template models (update the assignment in the Reminder model population) or, if the intent is to supply the full name, rename the model property from FirstName to FullName (and update any usages in the Reminder Razor template and model class) so templates remain accurate; locate the assignment where FirstName is populated and make the chosen change (adjusting template references to `@Model.FullName` if renaming).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@aspnet-core/src/Moipone.PublicSite.Application/CourseApplications/CourseApplicationAppService.cs`:
- Around line 179-183: The EmailJobParameters passed to
_backgroundJobManager.Enqueue is missing the EmailType, so set
EmailJobParameters.EmailType to the admission enum value (e.g.,
RefListEmailType.Admission) when enqueuing; update the object passed to
_backgroundJobManager.Enqueue<SendEmailBackgroundJob, EmailJobParameters> in
CourseApplicationAppService (where Student and Course are mapped) to include
EmailType = RefListEmailType.Admission so SendEmailBackgroundJob.Execute
receives the correct case.
- Around line 208-213: In RejectApplication, the email job is missing EmailType,
RejectionReason and uses unloaded navigation props; update the method so after
loading the application via _courseApplicationRepository.GetAsync(input) you
explicitly load the related student and shortCourse (same pattern used in
ApproveApplication), set EmailJobParameters.EmailType to the rejection enum
value, set EmailJobParameters.RejectionReason = application.DecisionReason (or
the local reason param), and pass
ObjectMapper.Map<StudentDto>(application.Student) and
ObjectMapper.Map<ShortCourseDto>(application.ShortCourse) into the
_backgroundJobManager.Enqueue<SendEmailBackgroundJob, EmailJobParameters> call
so the job gets the correct email type, reason and recipient data.
In
`@aspnet-core/src/Moipone.PublicSite.Application/Services/Emails/BackgroundJobs/EmailJobParameters.cs`:
- Around line 10-13: EmailJobParameters currently serializes full StudentDto and
ShortCourseDto (including PII and nested collections) into background jobs;
replace those properties with only the IDs and the scalar fields required by the
email templates so jobs don't persist PII or collections and are resilient to
DTO shape changes. Concretely: remove StudentDto and ShortCourseDto properties
from EmailJobParameters and add StudentId (GUID/long), StudentName,
StudentSurname, StudentEmail, CourseId, CourseTitle, CourseStartDate,
CourseDuration, CourseDescription (types matching existing usage); update the
job enqueueing call sites to populate these scalars, and update the job
handler/EmailAppService to load fresh Student/Course entities from the
repository by StudentId/CourseId when any additional up-to-date state is needed,
using the scalar fields only for the template placeholders.
In
`@aspnet-core/src/Moipone.PublicSite.Application/Services/Emails/BackgroundJobs/RefListEmailType.cs`:
- Around line 3-10: Add an explicit Unknown = 0 member to the RefListEmailType
enum so default/zero-initialized values are valid (update RefListEmailType to
include Unknown = 0 before Welcome), and update any background job
dispatcher/handler that switches on RefListEmailType (e.g., the job processing
method/class that routes by EmailType) to treat Unknown as a safe default
path—either reject the job with a clear error/log or route to a fallback
handler—to make deserialization/default initialization deterministic.
In
`@aspnet-core/src/Moipone.PublicSite.Application/Services/Emails/BackgroundJobs/SendEmailBackgroundJob.cs`:
- Around line 20-36: In SendEmailBackgroundJob, replace all usages of
Task.Wait() to synchronously block while preserving original exceptions: change
the .Wait() calls after invoking _emailAppService.SendWelcomeEmail,
SendAdmissionEmail, SendRejectionEmail, SendCourseReminderEmail, and
SendCustomEmail to use .GetAwaiter().GetResult() so thrown exceptions (e.g.,
SmtpException) are propagated directly instead of being wrapped in
AggregateException.
In
`@aspnet-core/src/Moipone.PublicSite.Application/Services/Emails/EmailAppService.cs`:
- Around line 47-51: In EmailAppService's catch blocks (e.g., in
SendCourseReminderEmail and the other methods SendWelcomeEmail, SendCustomEmail,
SendAdmissionEmail, SendRejectionEmail) stop discarding the exception and
leaking ex.Message to clients: call Logger.Error with the exception overload
(Logger.Error("Failed to send ... email", ex)) so stack/inner details are
preserved in logs, and throw a UserFriendlyException with a static safe message
(or omit details) instead of passing ex.Message as the second argument; apply
this pattern to all five catch blocks listed.
In `@aspnet-core/src/Moipone.PublicSite.EntityFrameworkCore/clear_database.ps1`:
- Around line 2-6: This file currently contains committed live DB credentials in
the parameter defaults (DbHost, Port, Database, Username, Password); immediately
rotate the exposed password in Render and then remove all hardcoded defaults
from the script so it reads these values from environment variables or a secret
store at runtime (do not keep any plaintext defaults in the repo). Change the
Password parameter to use [SecureString] (e.g., SecureString Password) and
update any usage that sets $env:PGPASSWORD to convert the SecureString to
plaintext only at the moment of export for the subprocess, avoiding storing
plaintext in variables or history; also make the script require explicit
confirmation or a --dry-run flag before performing destructive operations (DROP
… CASCADE) to prevent accidental runs.
In
`@aspnet-core/src/Moipone.PublicSite.EntityFrameworkCore/Moipone.PublicSite.EntityFrameworkCore.csproj`:
- Line 9: Remove the suppressed NuGet vulnerability warnings NU1902 and NU1903
from the NoWarn property in the project files (e.g., the <NoWarn> element shown
containing "$(NoWarn);1591;NU1902;NU1903" in
Moipone.PublicSite.EntityFrameworkCore.csproj and the other listed csproj
files); edit each <NoWarn> value to omit NU1902 and NU1903 while preserving
other existing warning IDs (e.g., keep "$(NoWarn)" and "1591" if present), save
the csproj files, and run a restore/build to ensure only those two suppressions
are removed.
In
`@aspnet-core/src/Moipone.PublicSite.Web.Core/Emails/Rendering/EmailTemplateRenderer.cs`:
- Around line 7-39: The EmailTemplateRenderer class is declared at global scope
and needs the proper project namespace; wrap the EmailTemplateRenderer class
(and its interface reference IEmailTemplateRenderer) inside the
Moipone.PublicSite.Web.Core (or the consistent Moipone.PublicSite.*) namespace
to match other classes and avoid type collisions, update any using directives if
necessary, and ensure the file declares the namespace before the public class so
the compiler recognizes EmailTemplateRenderer within the correct namespace.
In
`@aspnet-core/src/Moipone.PublicSite.Web.Core/Emails/Templates/moipone-custom-email.cshtml`:
- Around line 34-35: The template currently uses `@Raw`(customBody) which outputs
unencoded HTML; replace it with encoded rendering (e.g., use `@Model.CustomBody`
or `@customBody` without Raw) unless you explicitly intend admin-authored HTML—if
you do intend rich HTML, ensure CustomBody is only set from trusted admin code
paths and run it through an HTML sanitizer before rendering (sanitize where
CustomBody is assigned or just before rendering in the view model); update
documentation to state that CustomBody accepts HTML and add tests verifying
untrusted input is encoded/sanitized (references: `@Raw`(customBody), CustomBody,
moipone-rejection.cshtml usage of `@rejectionReason`).
In
`@aspnet-core/src/Moipone.PublicSite.Web.Core/Emails/Templates/moipone-custom-email.html`:
- Around line 26-57: The HTML template is missing the closing tag for the
container with class "email-body", which breaks nesting and causes rendering
issues; add a closing </div> to close the <div class="email-body"> before the
existing <div class="email-footer"> so the "email-body" block (containing the
intro-block, section-label, custom-body-area and closing paragraph) is properly
terminated and the "email-footer" starts as a separate sibling element.
In
`@aspnet-core/src/Moipone.PublicSite.Web.Core/Moipone.PublicSite.Web.Core.csproj`:
- Line 20: Edit the NoWarn property in the project file to stop suppressing
package vulnerability warnings: remove NU1902 and NU1903 from the <NoWarn> list
while retaining 1591 if you still want XML-doc suppression; specifically update
the NoWarn entry (currently containing "1591;NU1902;NU1903") to only include the
desired codes (e.g., "1591") so that the build will surface NU1902/NU1903
vulnerability alerts for packages.
- Line 30: Replace the unsupported/vulnerable RazorLight dependency in the
project by updating the PackageReference that currently reads RazorLight
Version="2.3.1": remove or replace it with a maintained fork/version that
supports newer .NET (for example the jcamp-code fork v3.0.0) and ensure the
project builds against .NET 10; update the PackageReference element for
RazorLight accordingly and run a restore/build to verify no transitive
Microsoft.Extensions.Caching.Memory v6.0.0 remains (if it does, add a direct
updated PackageReference to a safe Microsoft.Extensions.* version to override
the transitive dependency).
In `@aspnet-core/src/Moipone.PublicSite.Web.Core/PublicSiteWebCoreModule.cs`:
- Around line 84-93: The code is persisting SMTP credentials
(EmailSettingNames.Smtp.Password) and hardcoding port/SSL via
settingManager.ChangeSettingForApplication in PublicSiteWebCoreModule.cs; stop
writing sensitive secrets into the DB by removing the call that persists the
password (ChangeSettingForApplication for EmailSettingNames.Smtp.Password) and
instead read the password directly from configuration or a secrets store inside
your mail sender implementation, or alternatively override the ABP setting
definition (from AbpMailKitModule) to set IsEncrypted = true and rely on
ISettingEncryptionService to encrypt/decrypt values; also restore the
config-driven values for EmailSettingNames.Smtp.Port and
EmailSettingNames.Smtp.EnableSsl (use
_appConfiguration["Abp.Net.Mail.Smtp.Port"] and
_appConfiguration["Abp.Net.Mail.Smtp.EnableSsl"] instead of hardcoded
"587"/"false") so environments can control them without code changes.
In
`@aspnet-core/src/Moipone.PublicSite.Web.Host/Moipone.PublicSite.Web.Host.csproj`:
- Line 41: The project currently references RazorLight (PackageReference
RazorLight Version="2.3.1") which is incompatible with .NET 10; remove that
PackageReference and replace it with a .NET‑10 compatible template engine (e.g.,
add a PackageReference for RazorEngineCore or another vetted alternative), then
update any code that depends on RazorLight types (search for RazorLightEngine,
TemplatePage<>, RazorLight.Razor.* usage) to the new engine's API (create the
equivalent template service/engine, compile/render calls, and template model
bindings) and run/build tests to ensure all template compilation and runtime
paths are migrated and any obsolete RazorLight-specific imports/usings are
removed.
---
Minor comments:
In
`@aspnet-core/src/Moipone.PublicSite.Application/Moipone.PublicSite.Application.csproj`:
- Line 13: The NoWarn property value contains a spurious trailing colon on the
warning code "CS8618:" which prevents MSBuild/Roslyn from recognizing the
suppression; edit the NoWarn entry in the Moipone.PublicSite.Application.csproj
(the <NoWarn> property) and remove the trailing colon so the code appears
exactly as "CS8618" (comma/semicolon-separated format preserved) to correctly
suppress that warning.
In
`@aspnet-core/src/Moipone.PublicSite.Application/Services/Emails/EmailAppService.cs`:
- Around line 32-33: SendCourseReminderEmail and SendAdmissionEmail are calling
course.StartDate.ToString() and course.Duration.ToString() which are
culture-dependent; change these to use explicit, culture-stable format
specifiers and invariant culture: format StartDate with a deterministic date
format (e.g., ISO "yyyy-MM-dd" or round-trip "o") and format Duration based on
its actual type (if Duration is a TimeSpan use a fixed TimeSpan format like
@"hh\:mm" or if numeric use a numeric format) via ToString(format,
CultureInfo.InvariantCulture); confirm the type of Duration and pick the
appropriate format string and apply it where StartDate and Duration are
currently converted.
In
`@aspnet-core/src/Moipone.PublicSite.Core/Domain/CourseApplications/RefListApplicationStatus.cs`:
- Around line 16-17: Add a WithdrawApplication flow: add a
WithdrawApplication(Guid applicationId, string reason, string requestedBy)
method signature to the public service interface (e.g.,
ICourseApplicationService) and implement it in the concrete service class (e.g.,
CourseApplicationService or CourseApplicationManager). In the implementation,
load the application entity, set its status to
RefListApplicationStatus.Withdrawn, record withdrawal metadata (reason,
withdrawnAt, withdrawnBy) on the entity, persist changes via the
repository/DbContext SaveChangesAsync, and dispatch a withdrawal notification
using the existing email/notification abstraction (e.g., IEmailSender or
NotificationService) reusing the project’s templating patterns; also add/update
unit tests for WithdrawApplication to assert status change, metadata set,
persistence called, and email sent. Ensure method names and the enum
RefListApplicationStatus.Withdrawn are used exactly as identifiers in the code.
In
`@aspnet-core/src/Moipone.PublicSite.Web.Core/Emails/Templates/_MoiponeEmailLayout.cshtml`:
- Line 13: The external Google Fonts <link> in _MoiponeEmailLayout.cshtml will
be blocked by most email clients; either remove that <link> to accept
system-font fallback (cleaner HTML) or replace it by inlining `@font-face` rules
that reference hosted woff2 assets included on your CDN/email-safe host and
update the CSS font-family fallbacks accordingly; locate the existing <link> tag
in _MoiponeEmailLayout.cshtml and either delete it and ensure body/font-family
lists system stacks (e.g., Lexend fallback -> system sans-serif), or add inline
<style> with `@font-face` declarations pointing to woff2 URLs and adjust the
existing CSS to use those font-family names.
In
`@aspnet-core/src/Moipone.PublicSite.Web.Core/Emails/Templates/moipone-email.css`:
- Line 15: Replace the quoted font-family declaration occurrences that trigger
stylelint by removing the quotes around Lexend (i.e., change any "font-family:
'Lexend', sans-serif;" instances to use unquoted Lexend) across the file; update
every occurrence listed (the font-family declarations at the lines showing
'Lexend') so they read without single quotes, or alternatively adjust the
stylelint rule if you intend to keep quoted family names project-wide—target the
literal font-family declarations in moipone-email.css to make the change.
In
`@aspnet-core/src/Moipone.PublicSite.Web.Core/Emails/Templates/moipone-reminder.html`:
- Around line 54-55: Update the footer in the moipone-reminder.html template so
it matches the other new templates: change the year string "2025" to "2026" and
replace the address line "123 Somewhere Street · Johannesburg · South Africa"
with the same contact line used elsewhere (e.g., "Andrew Maphetho · Thami Mnyele
· Johannesburg · South Africa") so the copyright and contact metadata are
consistent across templates.
---
Nitpick comments:
In
`@aspnet-core/src/Moipone.PublicSite.Application/Services/Emails/BackgroundJobs/SendEmailBackgroundJob.cs`:
- Around line 17-38: The switch over args.EmailType in SendEmailBackgroundJob
must handle unexpected values: add a default case after the existing
RefListEmailType cases that logs the unrecognized value and fails the job (throw
an InvalidOperationException or similar) so misconfigured/future
RefListEmailType values don't silently no-op; reference the switch on
args.EmailType in SendEmailBackgroundJob and include the actual args.EmailType
in the log/exception message and keep existing calls to
_emailAppService.SendWelcomeEmail/SendAdmissionEmail/SendRejectionEmail/SendCourseReminderEmail/SendCustomEmail
unchanged.
In
`@aspnet-core/src/Moipone.PublicSite.Application/Services/Emails/EmailAppService.cs`:
- Line 30: In EmailAppService (the model assignment for the Reminder email)
FirstName is being set to student.Name + " " + student.Surname which differs
from other templates; either set FirstName = student.Name to keep consistency
with other template models (update the assignment in the Reminder model
population) or, if the intent is to supply the full name, rename the model
property from FirstName to FullName (and update any usages in the Reminder Razor
template and model class) so templates remain accurate; locate the assignment
where FirstName is populated and make the chosen change (adjusting template
references to `@Model.FullName` if renaming).
In
`@aspnet-core/src/Moipone.PublicSite.Application/Services/Emails/TemplateModels/Rejection.cs`:
- Around line 3-8: The Rejection model's non-nullable properties FirstName and
RejectionReason are uninitialized under nullable-enabled builds; update the
Rejection class to either mark these properties with the C# 11 required modifier
(e.g., required string FirstName { get; set; }) or initialize them to defaults
(e.g., string.Empty) to remove reliance on CS8618 suppression; make the same
change to both FirstName and RejectionReason so the intent is explicit and
compiler warnings are resolved.
In
`@aspnet-core/src/Moipone.PublicSite.Application/Services/Emails/TemplateModels/Reminder.cs`:
- Around line 3-16: The Reminder model's string properties (FirstName,
CourseTitle, StartDate, Duration, CourseDescription, WithdrawLink) are
non-nullable while the project uses nullable reference types; update these
properties to be nullable (string?) or mark them with required string if they
must always be provided, matching the nullable usage in IEmailAppService (see
how rejectionReason is declared) so CS8618 warnings are resolved and intent is
explicit.
In
`@aspnet-core/src/Moipone.PublicSite.Web.Core/Moipone.PublicSite.Web.Core.csproj`:
- Around line 37-41: The project file currently uses CopyToOutputDirectory set
to Always for the Content Include "Emails\Templates\**", causing all templates
to be copied on every build; change the CopyToOutputDirectory value from Always
to PreserveNewest for the Content Include "Emails\Templates\**" so MSBuild only
copies updated files during incremental builds (edit the ItemGroup containing
the Content Include and update the CopyToOutputDirectory element accordingly).
In `@aspnet-core/src/Moipone.PublicSite.Web.Core/Moipone.PublicSite.Web.Core.sln`:
- Around line 1-24: The committed single-project solution file
Moipone.PublicSite.Web.Core.sln is an accidental developer artifact that
duplicates the higher-level solution; delete Moipone.PublicSite.Web.Core.sln
from the repo and add an entry to .gitignore to prevent future commits (e.g.,
ignore Moipone.PublicSite.Web.Core.sln or *.sln in that subdirectory), and
ensure the project remains referenced only by the top-level solution (the csproj
{B14A29A1-DC71-0414-2D9E-2ECCD10DBEF1} / Moipone.PublicSite.Web.Core.csproj) so
CI/builds continue to use the main solution.
In `@aspnet-core/src/Moipone.PublicSite.Web.Host/Startup/Startup.cs`:
- Around line 179-182: The ConfigureEmailSmtp() method is an empty unused
placeholder; either remove it or implement and wire it up. If removing: delete
the ConfigureEmailSmtp() method and any dead references. If implementing: add
SMTP setup logic (reading mail settings from configuration, registering a mail
service or SmtpClientFactory, and configuring DI) inside ConfigureEmailSmtp(),
then call ConfigureEmailSmtp() from Startup.ConfigureServices (or Configure)
where other email services are registered. Ensure the method uses the Startup
IConfiguration and IServiceCollection so the app actually registers the
email-related services.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: fa525c4a-9617-4da2-8bcc-ce15cc5fd524
⛔ Files ignored due to path filters (1)
next-ts/public/images/moipone-logo-peach.pngis excluded by!**/*.png
📒 Files selected for processing (46)
aspnet-core/src/Moipone.PublicSite.Application/CourseApplications/CourseApplicationAppService.csaspnet-core/src/Moipone.PublicSite.Application/Moipone.PublicSite.Application.csprojaspnet-core/src/Moipone.PublicSite.Application/Services/Emails/BackgroundJobs/EmailJobParameters.csaspnet-core/src/Moipone.PublicSite.Application/Services/Emails/BackgroundJobs/RefListEmailType.csaspnet-core/src/Moipone.PublicSite.Application/Services/Emails/BackgroundJobs/SendEmailBackgroundJob.csaspnet-core/src/Moipone.PublicSite.Application/Services/Emails/EmailAppService.csaspnet-core/src/Moipone.PublicSite.Application/Services/Emails/IEmailAppService.csaspnet-core/src/Moipone.PublicSite.Application/Services/Emails/Rendering/IEmailTemplateRenderer.csaspnet-core/src/Moipone.PublicSite.Application/Services/Emails/TemplateModels/Admission.csaspnet-core/src/Moipone.PublicSite.Application/Services/Emails/TemplateModels/CustomEmail.csaspnet-core/src/Moipone.PublicSite.Application/Services/Emails/TemplateModels/Rejection.csaspnet-core/src/Moipone.PublicSite.Application/Services/Emails/TemplateModels/Reminder.csaspnet-core/src/Moipone.PublicSite.Application/Services/Emails/TemplateModels/WelcomeEmail.csaspnet-core/src/Moipone.PublicSite.Application/Students/StudentAppService.csaspnet-core/src/Moipone.PublicSite.Core/Domain/CourseApplications/RefListApplicationStatus.csaspnet-core/src/Moipone.PublicSite.Core/Moipone.PublicSite.Core.csprojaspnet-core/src/Moipone.PublicSite.EntityFrameworkCore/EntityFrameworkCore/Seed/Host/DefaultSettingsCreator.csaspnet-core/src/Moipone.PublicSite.EntityFrameworkCore/Moipone.PublicSite.EntityFrameworkCore.csprojaspnet-core/src/Moipone.PublicSite.EntityFrameworkCore/clear_database.ps1aspnet-core/src/Moipone.PublicSite.Migrator/Moipone.PublicSite.Migrator.csprojaspnet-core/src/Moipone.PublicSite.Web.Core/Controllers/TokenAuthController.csaspnet-core/src/Moipone.PublicSite.Web.Core/Emails/Models/Admission.csaspnet-core/src/Moipone.PublicSite.Web.Core/Emails/Models/CustomEmail.csaspnet-core/src/Moipone.PublicSite.Web.Core/Emails/Models/Rejection.csaspnet-core/src/Moipone.PublicSite.Web.Core/Emails/Models/Reminder.csaspnet-core/src/Moipone.PublicSite.Web.Core/Emails/Models/WelcomeEmail.csaspnet-core/src/Moipone.PublicSite.Web.Core/Emails/Rendering/EmailTemplateRenderer.csaspnet-core/src/Moipone.PublicSite.Web.Core/Emails/Templates/_MoiponeEmailLayout.cshtmlaspnet-core/src/Moipone.PublicSite.Web.Core/Emails/Templates/moipone-admission.cshtmlaspnet-core/src/Moipone.PublicSite.Web.Core/Emails/Templates/moipone-admission.htmlaspnet-core/src/Moipone.PublicSite.Web.Core/Emails/Templates/moipone-custom-email.cshtmlaspnet-core/src/Moipone.PublicSite.Web.Core/Emails/Templates/moipone-custom-email.htmlaspnet-core/src/Moipone.PublicSite.Web.Core/Emails/Templates/moipone-email.cssaspnet-core/src/Moipone.PublicSite.Web.Core/Emails/Templates/moipone-rejection.cshtmlaspnet-core/src/Moipone.PublicSite.Web.Core/Emails/Templates/moipone-rejection.htmlaspnet-core/src/Moipone.PublicSite.Web.Core/Emails/Templates/moipone-reminder.cshtmlaspnet-core/src/Moipone.PublicSite.Web.Core/Emails/Templates/moipone-reminder.htmlaspnet-core/src/Moipone.PublicSite.Web.Core/Emails/Templates/moipone-welcome.htmlaspnet-core/src/Moipone.PublicSite.Web.Core/Emails/Templates/welcome.cshtmlaspnet-core/src/Moipone.PublicSite.Web.Core/Moipone.PublicSite.Web.Core.csprojaspnet-core/src/Moipone.PublicSite.Web.Core/Moipone.PublicSite.Web.Core.slnaspnet-core/src/Moipone.PublicSite.Web.Core/PublicSiteWebCoreModule.csaspnet-core/src/Moipone.PublicSite.Web.Core/Views/Emails/Templates/Welcome.cshtmlaspnet-core/src/Moipone.PublicSite.Web.Host/Moipone.PublicSite.Web.Host.csprojaspnet-core/src/Moipone.PublicSite.Web.Host/Startup/Program.csaspnet-core/src/Moipone.PublicSite.Web.Host/Startup/Startup.cs
💤 Files with no reviewable changes (2)
- aspnet-core/src/Moipone.PublicSite.Web.Host/Startup/Program.cs
- aspnet-core/src/Moipone.PublicSite.Web.Core/Views/Emails/Templates/Welcome.cshtml
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
aspnet-core/src/Moipone.PublicSite.Application/CourseApplications/CourseApplicationAppService.cs (1)
244-259:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the provided
reasonin withdrawal email payload.
reasonis accepted by the method but not forwarded to the email job in Lines 253-259, so recipients lose context for withdrawal communication.✉️ Suggested fix
_backgroundJobManager.Enqueue<SendEmailBackgroundJob, EmailJobParameters>( new EmailJobParameters { EmailType = RefListEmailType.Rejection, Student = ObjectMapper.Map<StudentDto>(student), - Course = ObjectMapper.Map<ShortCourseDto>(shortCourse) + Course = ObjectMapper.Map<ShortCourseDto>(shortCourse), + RejectionReason = application.DecisionReason });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aspnet-core/src/Moipone.PublicSite.Application/CourseApplications/CourseApplicationAppService.cs` around lines 244 - 259, The withdrawal reason passed into the method is not forwarded to the email job payload; update the SendEmailBackgroundJob enqueue to include the provided method parameter (reason) in the EmailJobParameters (e.g., set EmailJobParameters.Reason = reason) so recipients get the actual withdrawal context. If EmailJobParameters lacks a Reason field, add it and propagate that property through the enqueue call inside CourseApplicationAppService where SendEmailBackgroundJob is enqueued. Ensure you use the method parameter name reason (not only application.DecisionReason) when setting the payload.
🧹 Nitpick comments (1)
aspnet-core/src/Moipone.PublicSite.Core/Authorization/PermissionNames.cs (1)
13-15: ⚡ Quick winNaming pattern inconsistency; aligns with role names rather than page-based permissions.
The new permission constants break from the established naming convention in this file:
- Existing pattern:
Pages_Tenants = "Pages.Tenants",Pages_Users = "Pages.Users",Pages_Roles = "Pages.Roles"- New constants:
Admin = "Admin",Employee = "Employee",Instructor = "Instructor"Additionally, these permission names closely match role names defined in
StaticRoleNames.cs(Host.Admin = "Admin",Tenants.Admin = "Admin"), which could create confusion between roles and permissions in authorization logic.If these are intentionally role-based permissions (as opposed to page-based), either align the naming with the existing
Pages_*pattern for consistency, or add a descriptive#regionname to clarify the distinction. The permissions are actively used inEmailAppService.cs, so renaming would require updating those references.♻️ Suggested alignment with existing naming convention
`#region` -public const string Admin = "Admin"; -public const string Employee = "Employee"; -public const string Instructor = "Instructor"; +public const string Pages_Admin = "Pages.Admin"; +public const string Pages_Employee = "Pages.Employee"; +public const string Pages_Instructor = "Pages.Instructor";Update references in
EmailAppService.cs(lines 27, 58, 80, 112, 142).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aspnet-core/src/Moipone.PublicSite.Core/Authorization/PermissionNames.cs` around lines 13 - 15, The new constants Admin, Employee, Instructor in PermissionNames.cs break the established Pages_* permission naming and collide with role names; either rename these constants to follow the page-based pattern (e.g., Pages_Admin, Pages_Employee, Pages_Instructor) or move them into a clearly labeled region (e.g., `#region` Role-based permissions) to indicate they represent role-related permissions, and then update all usages accordingly (notably the references in EmailAppService.cs where these constants are used) to match the new names/placement so compilation and authorization logic remain consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@aspnet-core/src/Moipone.PublicSite.Application/CourseApplications/CourseApplicationAppService.cs`:
- Around line 222-223: The WithdrawApplication method in
CourseApplicationAppService is missing authorization; add an [AbpAuthorize(...)]
attribute to the WithdrawApplication method (or the containing
CourseApplicationAppService class) using the same permission used by the
equivalent decision endpoints (e.g., the permission constant used on
Accept/Reject methods) so the state-changing WithdrawApplication(Guid input,
string? reason) call is protected the same way as other decision actions.
- Around line 232-251: The withdrawal status update uses
shortCourse.Applications.Contains(application) which is unreliable and may skip
setting Status/DecisionDate/DecisionReason; in CourseApplicationAppService
replace the navigation-based check with a deterministic check (e.g., compare
application.ShortCourseId to shortCourse.Id or search shortCourse.Applications
by application.Id) and always set application.Status =
RefListApplicationStatus.Withdrawn, application.DecisionDate = DateTime.UtcNow
and application.DecisionReason = "Student withdrew application voluntarily" when
the application belongs to that short course, then call
_courseApplicationRepository.UpdateAsync(application) once (and only after
setting these fields); keep the subsequent student fetch and
_shortCourseRepository.UpdateAsync(shortCourse) behavior intact.
---
Duplicate comments:
In
`@aspnet-core/src/Moipone.PublicSite.Application/CourseApplications/CourseApplicationAppService.cs`:
- Around line 244-259: The withdrawal reason passed into the method is not
forwarded to the email job payload; update the SendEmailBackgroundJob enqueue to
include the provided method parameter (reason) in the EmailJobParameters (e.g.,
set EmailJobParameters.Reason = reason) so recipients get the actual withdrawal
context. If EmailJobParameters lacks a Reason field, add it and propagate that
property through the enqueue call inside CourseApplicationAppService where
SendEmailBackgroundJob is enqueued. Ensure you use the method parameter name
reason (not only application.DecisionReason) when setting the payload.
---
Nitpick comments:
In `@aspnet-core/src/Moipone.PublicSite.Core/Authorization/PermissionNames.cs`:
- Around line 13-15: The new constants Admin, Employee, Instructor in
PermissionNames.cs break the established Pages_* permission naming and collide
with role names; either rename these constants to follow the page-based pattern
(e.g., Pages_Admin, Pages_Employee, Pages_Instructor) or move them into a
clearly labeled region (e.g., `#region` Role-based permissions) to indicate they
represent role-related permissions, and then update all usages accordingly
(notably the references in EmailAppService.cs where these constants are used) to
match the new names/placement so compilation and authorization logic remain
consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 243b6dce-9ad6-4780-8e20-e6cfe2455968
📒 Files selected for processing (6)
aspnet-core/src/Moipone.PublicSite.Application/CourseApplications/CourseApplicationAppService.csaspnet-core/src/Moipone.PublicSite.Application/CourseApplications/ICourseApplicationAppService.csaspnet-core/src/Moipone.PublicSite.Application/Services/Emails/BackgroundJobs/RefListEmailType.csaspnet-core/src/Moipone.PublicSite.Application/Services/Emails/BackgroundJobs/SendEmailBackgroundJob.csaspnet-core/src/Moipone.PublicSite.Application/Services/Emails/EmailAppService.csaspnet-core/src/Moipone.PublicSite.Core/Authorization/PermissionNames.cs
🚧 Files skipped from review as they are similar to previous changes (3)
- aspnet-core/src/Moipone.PublicSite.Application/Services/Emails/BackgroundJobs/RefListEmailType.cs
- aspnet-core/src/Moipone.PublicSite.Application/Services/Emails/BackgroundJobs/SendEmailBackgroundJob.cs
- aspnet-core/src/Moipone.PublicSite.Application/Services/Emails/EmailAppService.cs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
next-ts/src/providers/application-provider/index.tsx (1)
149-161:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix undefined reason in query string.
If
reasonis undefined, the URL will contain the literal string"undefined"(e.g.,?input=...&reason=undefined), which the backend may interpret as the text "undefined" rather than an absent parameter.🔧 Proposed fix using conditional query construction
const rejectApplication = async (id: string, reason?: string) => { dispatch(rejectApplicationPending()); - const endpoint = `CourseApplication/RejectApplication?input=${id}&reason=${reason}`; + const endpoint = `CourseApplication/RejectApplication?input=${id}${reason ? `&reason=${encodeURIComponent(reason)}` : ''}`; await instance .put(endpoint)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@next-ts/src/providers/application-provider/index.tsx` around lines 149 - 161, The rejectApplication function builds endpoint with a reason that can be undefined, causing "&reason=undefined" to be sent; change endpoint construction in rejectApplication to only append the reason query parameter when reason is non-empty (or use URLSearchParams to build the query and omit undefined values), ensure id and reason are URL-encoded (e.g., via encodeURIComponent) before adding them, and keep dispatch/error handling as-is so the backend receives either no reason param or a properly encoded value instead of the literal "undefined".
♻️ Duplicate comments (2)
aspnet-core/src/Moipone.PublicSite.Web.Core/PublicSiteWebCoreModule.cs (1)
85-94:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftCritical security issue: SMTP password stored in plaintext and hardcoded configuration remain unaddressed.
The past review flagged that line 90 persists the SMTP password to the ABP settings table without encryption, and lines 87-88 and 91-92 hardcode port and SSL settings instead of reading from configuration. These issues have not been resolved.
Security risk: Anyone with database read access (developers, DBAs, backup files) can retrieve the SMTP credential in plaintext.
Configuration rigidity: Hardcoded port
"587"and SSL"false"prevent environment-specific settings without code changes.Required fixes:
- Either stop writing the password to the database (read directly from configuration in the mail sender), OR override the ABP setting definition to set
IsEncrypted = trueand useISettingEncryptionService- Restore configuration-driven values:
settingManager.ChangeSettingForApplication(EmailSettingNames.Smtp.Port, _appConfiguration["Abp.Net.Mail.Smtp.Port"]); settingManager.ChangeSettingForApplication(EmailSettingNames.Smtp.EnableSsl, _appConfiguration["Abp.Net.Mail.Smtp.EnableSsl"]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aspnet-core/src/Moipone.PublicSite.Web.Core/PublicSiteWebCoreModule.cs` around lines 85 - 94, The code persists the SMTP password in plaintext and hardcodes port/SSL; fix by (1) removing the call that writes the SMTP password into ABP settings (the ChangeSettingForApplication call for EmailSettingNames.Smtp.Password) so the mail sender reads the secret directly from _appConfiguration, or alternatively mark the SMTP password setting as encrypted (override the setting definition for EmailSettingNames.Smtp.Password to set IsEncrypted = true and ensure ISettingEncryptionService is wired so ChangeSettingForApplication stores an encrypted value), and (2) restore configuration-driven values by replacing the hardcoded port and SSL ChangeSettingForApplication calls (the calls that set EmailSettingNames.Smtp.Port and EmailSettingNames.Smtp.EnableSsl) to use _appConfiguration["Abp.Net.Mail.Smtp.Port"] and _appConfiguration["Abp.Net.Mail.Smtp.EnableSsl"] respectively.aspnet-core/src/Moipone.PublicSite.Application/Services/Emails/EmailAppService.cs (1)
75-79:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInconsistent exception handling:
SendWelcomeEmailstill has both issues flagged in past review.While the other four email methods correctly pass the exception to
Logger.Errorand avoid leakingex.Message,SendWelcomeEmailstill has both problems:
- Line 77: Exception object not passed to logger — stack trace and inner details are lost
- Line 78:
ex.Messageis exposed to clients viaUserFriendlyExceptiondetails, potentially leaking internal SMTP errors or server addressesThis is inconsistent with
SendCourseReminderEmail,SendCustomEmail,SendAdmissionEmail, andSendRejectionEmail, which were all corrected.🛡️ Proposed fix to align with other methods
catch (Exception ex) { - Logger.Error($"Failed to send welcome email"); - throw new UserFriendlyException("Failed to send welcome email", ex.Message); + Logger.Error("Failed to send welcome email", ex); + throw new UserFriendlyException("Failed to send welcome email"); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aspnet-core/src/Moipone.PublicSite.Application/Services/Emails/EmailAppService.cs` around lines 75 - 79, SendWelcomeEmail currently logs only a message and exposes ex.Message to clients; change it to pass the full exception to the logger and avoid leaking ex.Message by throwing a UserFriendlyException with a generic message only. Specifically, in SendWelcomeEmail replace Logger.Error($"Failed to send welcome email") with Logger.Error(ex, "Failed to send welcome email") (or the project logger overload that accepts an Exception) and throw new UserFriendlyException("Failed to send welcome email") without passing ex.Message or any internal details.
🧹 Nitpick comments (3)
next-ts/app/(student)/withdraw/page.tsx (1)
80-90: ⚡ Quick winConsider logging the error for observability.
The catch block shows a generic user-facing message but discards the error details. While the generic message is good UX, logging the actual error would help with debugging production issues.
🔍 Proposed enhancement
try { await applicationActions.withdrawApplication( applicationId, finalReason, ); setSubmitted(true); - } catch { + } catch (error) { + console.error('Withdrawal failed:', error); message.error("We're unable to withdraw your application right now."); } finally {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@next-ts/app/`(student)/withdraw/page.tsx around lines 80 - 90, Update the catch in the withdraw flow to capture the thrown error and log it for observability before showing the user-facing message; specifically, in the try/catch around applicationActions.withdrawApplication (the block that calls setSubmitted and setLoading), change the catch to accept the error (e) and call your app logger or console.error with context (e.g., "withdrawApplication failed", applicationId, e) and then keep the existing message.error("We're unable to withdraw your application right now."); ensure the error variable is typed/handled safely if needed.aspnet-core/src/Moipone.PublicSite.Application/ShortCourses/Dto/ShortCourseEmailDto.cs (1)
11-14: ⚡ Quick winRemove unused properties or initialize the Features collection.
CapacityandFeaturesare not consumed by any email template inEmailAppService(checkedSendCourseReminderEmailandSendAdmissionEmail). Including them inShortCourseEmailDtoincreases the background job payload size without benefit.Additionally, the uninitialized
Featurescollection could cause aNullReferenceExceptionif code attempts to enumerate or count it before assignment.Recommendation: Remove
CapacityandFeaturesif they won't be used in email templates, or initializeFeatures = new List<string>()if future templates will need it.♻️ Proposed fix
If removing unused properties:
public class ShortCourseEmailDto : EntityDto<Guid> { public string Title { get; set; } public string Description { get; set; } - public int Capacity { get; set; } public DateTime StartDate { get; set; } public TimeSpan Duration { get; set; } - public ICollection<string> Features { get; set; } }Or if keeping for future use, initialize the collection:
- public ICollection<string> Features { get; set; } + public ICollection<string> Features { get; set; } = new List<string>();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aspnet-core/src/Moipone.PublicSite.Application/ShortCourses/Dto/ShortCourseEmailDto.cs` around lines 11 - 14, ShortCourseEmailDto currently declares Capacity and Features which are not used by EmailAppService.SendCourseReminderEmail or SendAdmissionEmail and inflate background job payloads; either remove the unused Capacity and Features properties from ShortCourseEmailDto, or if you intend to keep Features for future templates, initialize it to an empty collection (e.g., set Features = new List<string>() in the ShortCourseEmailDto constructor or inline property initializer) to avoid potential NullReferenceException when enumerating/counting Features.aspnet-core/src/Moipone.PublicSite.Application/Services/Emails/EmailAppService.cs (1)
40-40: ⚡ Quick winHardcoded withdrawal URL should come from configuration.
The withdraw link
"https://www.moiponeacademy.org/"is hardcoded, making it impossible to adjust for different environments (development, staging, production) without code changes.Recommendation: Add a configuration key (e.g.,
"App:BaseUrl"or"App:WithdrawUrl") and injectIConfigurationRootor read from a settings provider.♻️ Example configuration-based approach
In your configuration (appsettings.json):
{ "App": { "BaseUrl": "https://www.moiponeacademy.org" } }In the service (inject
IConfigurationif not already available via base class):+ private readonly IConfiguration _configuration; - WithdrawLink = "https://www.moiponeacademy.org/" + WithdrawLink = $"{_configuration["App:BaseUrl"]}/"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aspnet-core/src/Moipone.PublicSite.Application/Services/Emails/EmailAppService.cs` at line 40, The hardcoded WithdrawLink value in EmailAppService (the WithdrawLink assignment) should be read from configuration; inject IConfiguration into the EmailAppService constructor (or obtain it from the existing base/service provider) and replace the literal "https://www.moiponeacademy.org/" with a config lookup such as _configuration["App:WithdrawUrl"] (or build from _configuration["App:BaseUrl"] + path) and add the corresponding key to appsettings (e.g. App:WithdrawUrl or App:BaseUrl) so different environments can override it.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@aspnet-core/src/Moipone.PublicSite.Application/Services/Emails/EmailAppService.cs`:
- Around line 82-83: The SendCustomEmail method is missing the
RemoteService(false) attribute like the other internal-only email methods; add
[RemoteService(false)] above the SendCustomEmail declaration (while keeping the
existing [AbpAuthorize(...)] attribute) so it is not exposed as a remote HTTP
endpoint and remains callable only internally (e.g., by SendEmailBackgroundJob),
matching the behavior of SendCourseReminderEmail, SendWelcomeEmail,
SendAdmissionEmail, and SendRejectionEmail.
In `@next-ts/src/providers/application-provider/index.tsx`:
- Around line 163-175: The withdrawApplication function builds the endpoint with
reason possibly undefined causing "undefined" in the query string; change
endpoint construction in withdrawApplication to only include the reason
parameter when reason is provided (e.g., build query with URLSearchParams or
conditional string append using encodeURIComponent(reason)) so that when reason
is undefined the URL is `CourseApplication/WithdrawApplication?input=<id>` and
when provided it becomes `...?input=<id>&reason=<encodedReason>`; preserve
existing dispatch calls (withdrawApplicationPending, withdrawApplicationSuccess,
withdrawApplicationError) and error handling.
---
Outside diff comments:
In `@next-ts/src/providers/application-provider/index.tsx`:
- Around line 149-161: The rejectApplication function builds endpoint with a
reason that can be undefined, causing "&reason=undefined" to be sent; change
endpoint construction in rejectApplication to only append the reason query
parameter when reason is non-empty (or use URLSearchParams to build the query
and omit undefined values), ensure id and reason are URL-encoded (e.g., via
encodeURIComponent) before adding them, and keep dispatch/error handling as-is
so the backend receives either no reason param or a properly encoded value
instead of the literal "undefined".
---
Duplicate comments:
In
`@aspnet-core/src/Moipone.PublicSite.Application/Services/Emails/EmailAppService.cs`:
- Around line 75-79: SendWelcomeEmail currently logs only a message and exposes
ex.Message to clients; change it to pass the full exception to the logger and
avoid leaking ex.Message by throwing a UserFriendlyException with a generic
message only. Specifically, in SendWelcomeEmail replace Logger.Error($"Failed to
send welcome email") with Logger.Error(ex, "Failed to send welcome email") (or
the project logger overload that accepts an Exception) and throw new
UserFriendlyException("Failed to send welcome email") without passing ex.Message
or any internal details.
In `@aspnet-core/src/Moipone.PublicSite.Web.Core/PublicSiteWebCoreModule.cs`:
- Around line 85-94: The code persists the SMTP password in plaintext and
hardcodes port/SSL; fix by (1) removing the call that writes the SMTP password
into ABP settings (the ChangeSettingForApplication call for
EmailSettingNames.Smtp.Password) so the mail sender reads the secret directly
from _appConfiguration, or alternatively mark the SMTP password setting as
encrypted (override the setting definition for EmailSettingNames.Smtp.Password
to set IsEncrypted = true and ensure ISettingEncryptionService is wired so
ChangeSettingForApplication stores an encrypted value), and (2) restore
configuration-driven values by replacing the hardcoded port and SSL
ChangeSettingForApplication calls (the calls that set
EmailSettingNames.Smtp.Port and EmailSettingNames.Smtp.EnableSsl) to use
_appConfiguration["Abp.Net.Mail.Smtp.Port"] and
_appConfiguration["Abp.Net.Mail.Smtp.EnableSsl"] respectively.
---
Nitpick comments:
In
`@aspnet-core/src/Moipone.PublicSite.Application/Services/Emails/EmailAppService.cs`:
- Line 40: The hardcoded WithdrawLink value in EmailAppService (the WithdrawLink
assignment) should be read from configuration; inject IConfiguration into the
EmailAppService constructor (or obtain it from the existing base/service
provider) and replace the literal "https://www.moiponeacademy.org/" with a
config lookup such as _configuration["App:WithdrawUrl"] (or build from
_configuration["App:BaseUrl"] + path) and add the corresponding key to
appsettings (e.g. App:WithdrawUrl or App:BaseUrl) so different environments can
override it.
In
`@aspnet-core/src/Moipone.PublicSite.Application/ShortCourses/Dto/ShortCourseEmailDto.cs`:
- Around line 11-14: ShortCourseEmailDto currently declares Capacity and
Features which are not used by EmailAppService.SendCourseReminderEmail or
SendAdmissionEmail and inflate background job payloads; either remove the unused
Capacity and Features properties from ShortCourseEmailDto, or if you intend to
keep Features for future templates, initialize it to an empty collection (e.g.,
set Features = new List<string>() in the ShortCourseEmailDto constructor or
inline property initializer) to avoid potential NullReferenceException when
enumerating/counting Features.
In `@next-ts/app/`(student)/withdraw/page.tsx:
- Around line 80-90: Update the catch in the withdraw flow to capture the thrown
error and log it for observability before showing the user-facing message;
specifically, in the try/catch around applicationActions.withdrawApplication
(the block that calls setSubmitted and setLoading), change the catch to accept
the error (e) and call your app logger or console.error with context (e.g.,
"withdrawApplication failed", applicationId, e) and then keep the existing
message.error("We're unable to withdraw your application right now."); ensure
the error variable is typed/handled safely if needed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 64ae2ce8-8f28-4aa5-9e6d-3b63154bced2
📒 Files selected for processing (18)
aspnet-core/src/Moipone.PublicSite.Application/CourseApplications/CourseApplicationAppService.csaspnet-core/src/Moipone.PublicSite.Application/CourseApplications/ICourseApplicationAppService.csaspnet-core/src/Moipone.PublicSite.Application/Services/Emails/BackgroundJobs/EmailJobParameters.csaspnet-core/src/Moipone.PublicSite.Application/Services/Emails/EmailAppService.csaspnet-core/src/Moipone.PublicSite.Application/Services/Emails/IEmailAppService.csaspnet-core/src/Moipone.PublicSite.Application/ShortCourses/Dto/ShortCourseEmailDto.csaspnet-core/src/Moipone.PublicSite.Application/Students/Dto/StudentEmailDto.csaspnet-core/src/Moipone.PublicSite.Application/Students/StudentAppService.csaspnet-core/src/Moipone.PublicSite.Web.Core/Emails/Rendering/EmailTemplateRenderer.csaspnet-core/src/Moipone.PublicSite.Web.Core/PublicSiteWebCoreModule.csnext-ts/app/(student)/layout.tsxnext-ts/app/(student)/withdraw/page.tsxnext-ts/app/(student)/withdraw/style.tsnext-ts/src/lib/common/constants.tsxnext-ts/src/providers/application-provider/actions.tsnext-ts/src/providers/application-provider/context.tsnext-ts/src/providers/application-provider/index.tsxnext-ts/src/providers/application-provider/reducer.ts
✅ Files skipped from review due to trivial changes (3)
- next-ts/src/lib/common/constants.tsx
- aspnet-core/src/Moipone.PublicSite.Application/Students/Dto/StudentEmailDto.cs
- next-ts/app/(student)/withdraw/style.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- aspnet-core/src/Moipone.PublicSite.Application/Services/Emails/IEmailAppService.cs
- aspnet-core/src/Moipone.PublicSite.Application/Students/StudentAppService.cs
- aspnet-core/src/Moipone.PublicSite.Web.Core/Emails/Rendering/EmailTemplateRenderer.cs
- aspnet-core/src/Moipone.PublicSite.Application/CourseApplications/CourseApplicationAppService.cs
Owner
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
mblebelo
approved these changes
May 11, 2026
Collaborator
mblebelo
left a comment
mblebelo
left a comment
There was a problem hiding this comment.
All checks passed and conversations resolved ✅
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Improvements