Implemented MVP CRUDs & Secured Internal Endpoints#20
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughAdds a PowerShell scaffolder for ABP app services; implements a CourseApplication feature (DTO, interface, CRUD app service with approve/reject and query-by-course); adjusts Employees app layer and interface (authorization attributes, removed custom methods, DTO type change); restores ShortCourse applications collection; bumps multiple package versions; expands logging and host config. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
aspnet-core/src/Moipone.PublicSite.Web.Host/appsettings.json (1)
9-9: Move localhost CORS origins to environment-specific configuration.Line 9 hard-codes development localhost origins in the main
appsettings.json. These should be moved toappsettings.Development.json(which does not currently exist) or configured via environment variables. The main config should contain only production-appropriate origins, with environment-specific overrides for development and staging scenarios.aspnet-core/src/Moipone.PublicSite.Application/ShortCourses/ShortCourseAppService.cs (1)
419-420: The navigation propertyEnrolledStudentsis not loaded, causingCountto always return 0.Since lazy loading is disabled by default in EF Core and no explicit
.Include()is used inGetAsync(id), theEnrolledStudentscollection will be empty. This makescourse.EnrolledStudents.Countalways return 0, resulting in incorrect capacity calculation. Add.Include(s => s.EnrolledStudents)to the query at line 410, or modifyGetAsyncin the repository to include related entities.
🤖 Fix all issues with AI agents
In
`@aspnet-core/src/Moipone.PublicSite.Application/CourseApplications/CourseApplicationAppService.cs`:
- Around line 14-24: The CourseApplicationAppService currently exposes CRUD
operations without authorization; add authorization attributes to protect
sensitive actions by either applying [AbpAuthorize] at the class level on
CourseApplicationAppService (if all methods require protection) or by decorating
specific methods such as Create, Update, Delete, Get, and GetAll (inherited
AsyncCrudAppService overrides or newly overridden methods) with [AbpAuthorize]
(or fine-grained permission names) so only authorized callers can perform those
operations; update any overridden methods in CourseApplicationAppService to
include the attribute and ensure required permission constants or names match
your permission definitions.
- Around line 125-145: RejectApplication currently allows state changes from any
status; add the same pending-state validation used in ApproveApplication so only
applications with application.Status == RefListApplicationStatus.Pending can be
rejected. In RejectApplication, check application.Status and if it is not
RefListApplicationStatus.Pending throw a UserFriendlyException (e.g., "Only
pending applications can be rejected.") before setting Status, DecisionDate,
DecisionReason and calling _courseApplicationRepository.UpdateAsync; keep the
existing fields (DecisionDate, DecisionReason) update behavior otherwise.
- Around line 103-123: ApproveApplication currently only blocks re-approving
approved records but allows invalid transitions (e.g., approving a previously
Declined application); update CourseApplicationAppService.ApproveApplication to
validate current application.Status and throw a UserFriendlyException if Status
is Declined or any non-pending state (allow only Pending→Approved), and likewise
update CourseApplicationAppService.RejectApplication to prevent rejecting an
already Approved application (allow only Pending→Declined). Also enable nullable
reference checks for the file by adding a "#nullable enable" directive at the
top so the string? parameter on ApproveApplication is correctly recognized.
In
`@aspnet-core/src/Moipone.PublicSite.Application/CourseApplications/Dto/CourseApplicationDto.cs`:
- Around line 1-16: CourseApplicationDto uses nullable reference types (e.g.,
string? DecisionReason) but the project lacks a consistent nullable-context
setting; align this by either enabling nullable reference types project-wide in
the .csproj (so DTOs like CourseApplicationDto and StudentDto are consistent) or
suppress CS8632 warnings globally (avoid adding file-level `#nullable`
directives); update the project-level configuration so CourseApplicationDto, its
properties (StudentId, ShortCourseId, Status, DecisionReason, DecisionDate) and
StudentDto follow the same nullable-context policy.
In `@aspnet-core/src/Moipone.PublicSite.Application/Employees/Dto/EmployeeDto.cs`:
- Around line 8-11: EmployeeDto is empty so
CreateAsync/GetAsync/GetAllAsync/UpdateAsync only handle Id; add nullable
properties matching the domain Employee to the EmployeeDto (same nullable
pattern as StudentDto) so mapping works and partial updates don't clear
fields—specifically add nullable properties: FirstName, LastName, Age, Gender,
PersonalEmail, EmployeeEmail, IdNumber, DateOfBirth, PhoneNumber, Address,
HireDate, EndDate, Position, Department, EmploymentStatus, Salary,
EmployeeNumber, EmergencyContactName, EmergencyContactPhone,
EmergencyContactRelationship, CertifiedId, ProofOfResidence, CurriculumVitae,
CertifiedHighestQualification, PoliceClearance to the EmployeeDto class
annotated with [AutoMap(typeof(Employee))] so
CreateAsync/GetAsync/GetAllAsync/UpdateAsync will receive and return the full
set of employee fields.
In
`@aspnet-core/src/Moipone.PublicSite.Application/Employees/EmployeeAppService.cs`:
- Around line 42-57: GetAllAsync lacks a null check for its
PagedAndSortedResultRequestDto input, so references to input.SkipCount and
input.MaxResultCount can NRE; add a guard at the start of GetAllAsync (in
EmployeeAppService.GetAllAsync) that throws ArgumentNullException(nameof(input))
or uses input = input ?? new PagedAndSortedResultRequestDto() as appropriate to
match project conventions, then proceed to use input.SkipCount and
input.MaxResultCount when building the query and returning the
PagedResultDto<EmployeeDto>.
- Around line 27-28: Add a specific employee permission and apply it to the
EmployeeAppService CRUD methods: define a Pages_Employees constant in
PermissionNames (e.g., PermissionNames.Pages_Employees), register that
permission in PublicSiteAuthorizationProvider so it appears in the permission
tree, and replace the generic [AbpAuthorize] on EmployeeAppService methods
(e.g., CreateAsync, UpdateAsync, DeleteAsync, GetAsync/ListAsync) with
[AbpAuthorize(PermissionNames.Pages_Employees)] so only users granted
Pages_Employees can perform employee operations.
In
`@aspnet-core/src/Moipone.PublicSite.EntityFrameworkCore/Moipone.PublicSite.EntityFrameworkCore.csproj`:
- Around line 16-27: The project currently references the deprecated
System.Data.SqlClient package in the csproj; replace the PackageReference
Include="System.Data.SqlClient" Version="4.9.0" with a PackageReference to
Microsoft.Data.SqlClient (appropriate version for your target framework) so the
project uses the actively maintained driver; update any using statements or code
that reference the System.Data.SqlClient namespace to Microsoft.Data.SqlClient
and ensure the new package version is added to the csproj (same element name
PackageReference) and restore packages.
In
`@aspnet-core/src/Moipone.PublicSite.Web.Core/Moipone.PublicSite.Web.Core.csproj`:
- Around line 26-32: The ABP package versions referenced (Abp.AspNetCore,
Abp.ZeroCore, Abp.AspNetCore.SignalR) use "10.3.0", which is not a public
release; update those <PackageReference> entries to the intended, valid ABP
version (e.g., "10.0.0" or your specific pre-release like "10.1.0-rc.2") so
NuGet restore succeeds, then run dotnet restore/build to verify; ensure all
three symbols (Abp.AspNetCore, Abp.ZeroCore, Abp.AspNetCore.SignalR) use the
same, supported version.
In `@aspnet-core/src/Moipone.PublicSite.Web.Host/App_Data/Logs/Logs.txt`:
- Around line 9758-9985: The committed runtime log App_Data/Logs/Logs.txt
contains sensitive runtime info and must be removed from the repository; delete
the file from history/working tree (e.g., git rm --cached App_Data/Logs/Logs.txt
and commit), add the directory App_Data/Logs/ to .gitignore (or add the logging
sink path) to prevent future commits, and update your logging configuration
(where Serilog/NLog/ASP.NET Core logging is configured) to write logs to an
external storage or rotated files with retention/PII scrubbing instead of
committing Logs.txt.
In `@aspnet-core/src/Moipone.PublicSite.Web.Host/appsettings.json`:
- Line 3: The appsettings.json contains a hard-coded connection string under the
"Default" key (exposes live username/password); remove the literal credentials
from the "Default" entry and change the configuration to read the connection
string from a secure source (environment variable or secret store) at runtime
(e.g., via Configuration/ConnectionStrings binding or
Environment.GetEnvironmentVariable for the ConnectionStrings:Default key),
update startup/deployment to provide the secret, and immediately
rotate/invalidate the exposed DB credentials so the leaked secret is revoked.
🧹 Nitpick comments (6)
aspnet-core/Generate-AppService.ps1 (2)
33-33: Naive pluralization may produce incorrect folder/namespace names.Appending "s" won't handle irregular plurals or words ending in 'y', 's', 'x', 'ch', 'sh' (e.g.,
Category→Categorysinstead ofCategories,Status→Statussinstead ofStatuses).Consider using a simple pluralization helper or accepting an optional
-PluralNameparameter for edge cases.♻️ Suggested improvement
param( [Parameter(Mandatory = $true)] [string]$EntityName, [Parameter(Mandatory = $false)] - [string]$Namespace = "Moipone.PublicSite" + [string]$Namespace = "Moipone.PublicSite", + + [Parameter(Mandatory = $false)] + [string]$PluralName = $null )Then update the naming section:
# ------------------ Naming ------------------ -$PluralEntityName = "$EntityName`s" +$PluralEntityName = if ($PluralName) { $PluralName } else { "$EntityName`s" } $EntityLower = $EntityName.Substring(0,1).ToLower() + $EntityName.Substring(1)
87-150: Consider whether all CRUD overrides are necessary.The generated service extends
AsyncCrudAppService<>which already provides default implementations for all CRUD operations. The overrides here add null/ID validation but duplicate most of the base class logic.Options to consider:
- Remove overrides and rely on base class (simpler, less boilerplate)
- Only override methods that need custom behavior
- Add authorization attributes to the generated class (e.g.,
[AbpAuthorize])♻️ Minimal service relying on base class
If the goal is just scaffolding, a leaner approach:
namespace $Namespace.$PluralEntityName { [AbpAuthorize] // Consider adding authorization public class ${EntityName}AppService : AsyncCrudAppService<$EntityName, ${EntityName}Dto, Guid, PagedAndSortedResultRequestDto, ${EntityName}Dto, ${EntityName}Dto>, I${EntityName}AppService { public ${EntityName}AppService(IRepository<$EntityName, Guid> repository) : base(repository) { } // Override only when custom logic is needed } }aspnet-core/src/Moipone.PublicSite.Migrator/Moipone.PublicSite.Migrator.csproj (1)
19-19: Consider using Microsoft.Data.SqlClient instead of legacy System.Data.SqlClient.System.Data.SqlClient is generally considered legacy for modern .NET; Microsoft.Data.SqlClient is actively maintained and recommended. Switch unless you have a hard requirement for System.Data.SqlClient.
aspnet-core/src/Moipone.PublicSite.Application/ShortCourses/Dto/ShortCourseDto.cs (1)
26-27: Initialize collection properties to avoid null references.Both
EnrolledStudentsandApplicationsare non-nullable collections without initializers. Consumers may encounterNullReferenceExceptionwhen iterating over these properties if not explicitly set.♻️ Proposed fix
- public ICollection<StudentDto> EnrolledStudents { get; set; } - public ICollection<CourseApplicationDto> Applications { get; set; } + public ICollection<StudentDto> EnrolledStudents { get; set; } = new List<StudentDto>(); + public ICollection<CourseApplicationDto> Applications { get; set; } = new List<CourseApplicationDto>();aspnet-core/src/Moipone.PublicSite.Application/ShortCourses/ShortCourseAppService.cs (1)
330-370:ReopenApplicationsAsyncduplicatesOpenApplicationsAsynclogic.Both methods set
IsActive = truewith identical implementation. Consider consolidating or havingReopenApplicationsAsyncdelegate toOpenApplicationsAsync.♻️ Proposed simplification
[AbpAuthorize] public async Task<ShortCourseDto> ReopenApplicationsAsync(Guid id) { - try - { - if (id == Guid.Empty) - { - throw new UserFriendlyException( - "Invalid short course ID.", - Abp.Logging.LogSeverity.Warn - ); - } - - var shortCourse = await _shortCourseRepository.GetAsync(id); - - if (shortCourse == null) - { - throw new UserFriendlyException( - $"Short course with ID {id} not found.", - Abp.Logging.LogSeverity.Warn - ); - } - - shortCourse.IsActive = true; - var updatedShortCourse = await _shortCourseRepository.UpdateAsync(shortCourse); - - return ObjectMapper.Map<ShortCourseDto>(updatedShortCourse); - } - catch (UserFriendlyException) - { - throw; - } - catch (Exception ex) - { - Logger.Error($"Error reopening applications for short course with ID {id}", ex); - throw new UserFriendlyException( - $"Could not reopen applications. Error: {ex.Message}", - Abp.Logging.LogSeverity.Error - ); - } + // Reopen is semantically equivalent to Open + return await OpenApplicationsAsync(id); }aspnet-core/src/Moipone.PublicSite.Core/Moipone.PublicSite.Core.csproj (1)
21-21: Consider usingMicrosoft.Data.SqlClientinstead ofSystem.Data.SqlClient.
System.Data.SqlClientis in deprecation and maintenance mode. Microsoft recommendsMicrosoft.Data.SqlClientfor all modern .NET projects (including .NET 10), as it receives active development, newer SQL Server features, and continuous security updates.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@aspnet-core/src/Moipone.PublicSite.Web.Host/Startup/Startup.cs`:
- Line 13: The file imports the wrong OpenAPI namespace; update the using to
reference Microsoft.OpenApi.Models so types used in ConfigureSwagger
(OpenApiInfo, OpenApiContact, OpenApiLicense, OpenApiSecurityScheme,
ParameterLocation, SecuritySchemeType) resolve correctly; locate the
ConfigureSwagger method and replace the incorrect Microsoft.OpenApi import with
Microsoft.OpenApi.Models to fix the compilation errors.
blebelo
requested a deployment
to
apis/implementation - Moipone PS API PR #20
Render
In progress
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@aspnet-core/src/Moipone.PublicSite.Application/CourseApplications/CourseApplicationAppService.cs`:
- Around line 68-80: UpdateAsync currently maps all fields from
CourseApplicationDto into the entity allowing uncontrolled changes to workflow
fields; modify CourseApplicationAppService.UpdateAsync so that
workflow-controlled fields (Status, DecisionDate, DecisionReason) are not
overwritten by the incoming DTO — either remove those properties from the
ObjectMapper.Map call (map only allowed fields or use a manual assign for safe
fields) or explicitly set entity.Status/DecisionDate/DecisionReason back to
their original values before calling _courseApplicationRepository.UpdateAsync;
alternatively, protect the method with an authorization attribute (e.g., add
[AbpAuthorize] to UpdateAsync) if the intent is to restrict updates to
authorized users.
🧹 Nitpick comments (3)
aspnet-core/src/Moipone.PublicSite.Application/CourseApplications/CourseApplicationAppService.cs (3)
19-25: Redundant repository field.The
_courseApplicationRepositoryfield duplicates theRepositoryproperty inherited fromAsyncCrudAppService. You can useRepositorydirectly throughout the service.♻️ Proposed simplification
public class CourseApplicationAppService : AsyncCrudAppService<CourseApplication, CourseApplicationDto, Guid, PagedAndSortedResultRequestDto, CourseApplicationDto, CourseApplicationDto>, ICourseApplicationAppService { - private readonly IRepository<CourseApplication, Guid> _courseApplicationRepository; - public CourseApplicationAppService(IRepository<CourseApplication, Guid> courseApplicationRepository) : base(courseApplicationRepository) { - _courseApplicationRepository = courseApplicationRepository; }Then replace
_courseApplicationRepositorywithRepositoryin the methods.
27-38: Consider validating required fields.The method validates that
inputis not null, but doesn't validate thatStudentIdandShortCourseIdare non-empty GUIDs. Invalid GUIDs could result in orphaned or invalid application records.Based on learnings, keeping this method public (no auth) is intentional.
🛡️ Proposed validation
public override async Task<CourseApplicationDto> CreateAsync(CourseApplicationDto input) { if (input == null) { throw new UserFriendlyException("CourseApplication data cannot be null."); } + if (input.StudentId == Guid.Empty) + { + throw new UserFriendlyException("Student ID is required."); + } + + if (input.ShortCourseId == Guid.Empty) + { + throw new UserFriendlyException("Course ID is required."); + } + var entity = ObjectMapper.Map<CourseApplication>(input); var result = await _courseApplicationRepository.InsertAsync(entity); return ObjectMapper.Map<CourseApplicationDto>(result); }
40-55: Sorting parameter is ignored.The
PagedAndSortedResultRequestDtoinput includes aSortingproperty, but the implementation always sorts byId. Consider honoring the sorting parameter or usingPagedResultRequestDtoinstead if sorting isn't needed.♻️ Option 1: Honor sorting with dynamic ordering
You can use ABP's
IQueryableextensions or a library likeSystem.Linq.Dynamic.Coreto apply dynamic sorting:public override async Task<PagedResultDto<CourseApplicationDto>> GetAllAsync(PagedAndSortedResultRequestDto input) { var query = Repository.GetAll(); var totalCount = await AsyncQueryableExecuter.CountAsync(query); + if (!string.IsNullOrWhiteSpace(input.Sorting)) + { + query = query.OrderBy(input.Sorting); + } + else + { + query = query.OrderBy(x => x.Id); + } + var items = await AsyncQueryableExecuter.ToListAsync( - query.OrderBy(x => x.Id) - .Skip(input.SkipCount) + query.Skip(input.SkipCount) .Take(input.MaxResultCount) );
blebelo
temporarily deployed
to
apis/implementation - Moipone PS API PR #20
Render
Destroyed
|
@coderabbitai resolve |
✅ Actions performedComments resolved. |
mblebelo
left a comment
mblebelo
left a comment
There was a problem hiding this comment.
All checks passed and conversations resolved ✅
3 participants
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.