Bug/ab#28307 auto generated 7 digit applicant id error message #1401
Conversation
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
Fix: GetNextUnityApplicantIdAsync to find first available ID and improve performance Resolved an issue where GetNextUnityApplicantIdAsync incorrectly returned MaxId + 1, which failed to account for gaps in the existing ID sequence. The updated logic now correctly identifies the first available integer ID that is greater than or equal to 100001. Key improvements include: - Optimized data retrieval: Fetches only non-null UnityApplicantId strings from the database, significantly reducing network traffic and memory consumption by avoiding full Applicant object materialization. - Robust client-side processing: Implemented correct parsing of string IDs to integers, filtering for valid IDs (>= 100001), and sorting in memory. - Proper async/await usage: Ensured LINQ queries are applied to the awaited IQueryable for correct database translation. - SonarQube compliance: Addressed "Nullable value type may be null" warning by safely unwrapping nullable integers using GetValueOrDefault(), aligning with clean code practices.
Corrected the lower bound of the search range to 100000 from 100001.
…tion/Applicants/ApplicantAppService.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
jimmyPasta
reviewed
Jun 5, 2025
| .Select(id => id.GetValueOrDefault()) // Safely unwrap nullable int | ||
| .OrderBy(id => id) | ||
| .ToList(); | ||
|
|
Contributor
There was a problem hiding this comment.
The ticket originates from 27913 -
Users can click on the 'Generate' button next to the Applicant ID field. The system will then generate an ID with the following pattern:
A 6-digit numeric value starting with 000001.
Each tenant maintains its own sequential numbering
The number is sequential and it needs to check for the next available number
User can edit the generated number
When a user modifies the Auto generated Applicant ID and clicks 'Save', the system should validate the uniqueness of the entered ID. If a duplicate ID is found, the system should display the following error message:
Applicant ID already exists. Please enter a unique ID.
jimmyPasta
reviewed
Jun 5, 2025
| .Select(a => a.UnityApplicantId) | ||
| .ToListAsync(); | ||
|
|
||
| // Parse, filter for IDs >= 100000, and sort client-side. |
Contributor
There was a problem hiding this comment.
ok, so this parses out the next available sequence as opposed to blindly incrementing it
Contributor
Author
There was a problem hiding this comment.
Fight smarter not harder.
…d clarity Bug/ab#28307 auto generated 7 digit applicant id error message This update refines the `GetNextUnityApplicantIdAsync` method to implement a more robust strategy for determining the next available Unity Applicant ID. The logic now efficiently identifies the first unused ID starting from 100000, which contributes to more optimal utilization of the ID sequence by filling potential gaps. The approach to data retrieval and processing has been enhanced. It now focuses on fetching only essential ID strings from the database, thereby minimizing data transfer. Subsequent in-memory operations for parsing and validation have been structured into more explicit stages, contributing to a clearer and more maintainable code flow. Additionally, static analysis feedback regarding nullable value types was addressed, ensuring the code adheres to quality standards. The revised implementation builds successfully and performs as expected.
…message' of https://github.com/bcgov/Unity into bug/AB#28307-auto-generated-7-digit-Applicant-ID-error-message
Refactor: Enhance applicant ID generation for accuracy and efficiency This update improves the `GetNextUnityApplicantIdAsync` method to reliably identify the next available applicant ID. The previous approach of finding the maximum ID and incrementing it has been replaced with a robust algorithm that finds the first available ID, starting from 100000, effectively utilizing gaps in the existing sequence. The revised implementation enhances data handling by: - Transitioning from fetching all applicant records to more efficiently querying and projecting only necessary ID strings from the database. - Introducing a multi-stage in-memory process for parsing and validating Unity Applicant IDs, ensuring correct handling of non-numeric values and range filtering. - Implementing explicit nullability checks and type unwrapping to improve code safety and address static analysis guidelines. This refactoring delivers a more accurate and performant ID allocation mechanism.
…message' of https://github.com/bcgov/Unity into bug/AB#28307-auto-generated-7-digit-Applicant-ID-error-message
Remove the unused local variable 'allUnityIdStrings'. var allUnityIdStrings = await applicantQuery
…-ID-error-message
Contributor
There was a problem hiding this comment.
Pull Request Overview
Enhances ID generation in GetNextUnityApplicantIdAsync to locate the first unused 7-digit ID ≥ 100000 and refactors OrgBook lookups to use System.Text.Json.
- Optimizes applicant ID allocation by querying only
UnityApplicantIdvalues and iterating gaps to find the next available ID. - Replaces Newtonsoft JSON parsing with
System.Text.JsoninMatchApplicantOrgNamesAsync, updatingUpdateApplicantOrgNumberAsyncandUpdateApplicantNamesAsync. - Cleans up using directives and removes JToken dependencies.
Comments suppressed due to low confidence (2)
applications/Unity.GrantManager/src/Unity.GrantManager.Application/Applicants/ApplicantAppService.cs:233
- Use a higher-severity log method (e.g., LogWarning or LogError) for exception handling so that errors are more visible in monitoring tools.
Logger.LogInformation(ex, "UpdateApplicantOrgMatchAsync: Exception: {ExceptionMessage}", ex.Message);
applications/Unity.GrantManager/src/Unity.GrantManager.Application/Applicants/ApplicantAppService.cs:249
- If
namesChildrenis null or the JSON lacks anamesarray, this loop will throw; add a null check or useTryGetPropertyguard before iterating.
foreach (var name in namesChildren)
|
|
||
| var relevantUnityIds = await applicantQuery | ||
| .Where(a => a.UnityApplicantId != null) | ||
| .Select(a => new { UnityApplicantId = a.UnityApplicantId, ParsedId = (int?)null }) |
There was a problem hiding this comment.
[nitpick] The anonymous type includes UnityApplicantId, but that property isn’t used later; consider projecting only ParsedId or parsing inline to simplify the code.
Suggested change
| .Select(a => new { UnityApplicantId = a.UnityApplicantId, ParsedId = (int?)null }) | |
| .Select(a => new { ParsedId = (int?)null }) |
Comment on lines
+170
to
190
| var relevantUnityIds = await applicantQuery | ||
| .Where(a => a.UnityApplicantId != null) | ||
| .Select(a => new { UnityApplicantId = a.UnityApplicantId, ParsedId = (int?)null }) | ||
| .ToListAsync(); | ||
|
|
||
| var updatedRelevantUnityIds = relevantUnityIds | ||
| .Select(item => | ||
| { | ||
| if (int.TryParse(item.UnityApplicantId, out var parsedId) && parsedId >= 100000) | ||
| { | ||
| return new { UnityApplicantId = item.UnityApplicantId ?? string.Empty, ParsedId = (int?)parsedId }; | ||
| } | ||
| return new { UnityApplicantId = item.UnityApplicantId ?? string.Empty, ParsedId = item.ParsedId }; | ||
| }) | ||
| .ToList(); | ||
|
|
||
| var orderedIds = updatedRelevantUnityIds | ||
| .Where(a => a.ParsedId.HasValue) | ||
| .Select(a => a.ParsedId!.Value) // Use the null-forgiving operator (!) to assert that ParsedId is not null | ||
| .OrderBy(id => id) | ||
| .ToList(); |
There was a problem hiding this comment.
The code materializes two lists (relevantUnityIds and updatedRelevantUnityIds) causing multiple in-memory enumerations; consider combining parsing, filtering, and ordering in one projection to reduce overhead.
jimmyPasta
approved these changes
Jun 5, 2025
Contributor
jimmyPasta
left a comment
jimmyPasta
left a comment
There was a problem hiding this comment.
Please merge this when you have done local testing
Stephan-McColm
deleted the
bug/AB#28307-auto-generated-7-digit-Applicant-ID-error-message
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Enhance: GetNextUnityApplicantIdAsync for optimized ID allocation and performance
Improved the ID generation logic within
GetNextUnityApplicantIdAsyncto more robustly identify the next available applicant ID. The updated implementation now efficiently finds the smallest available ID at or above 100000, effectively using any potential gaps in the existing sequence of IDs. This refines the previous ID generation approach to cover a broader range of allocation scenarios.Performance enhancements include targeted database queries to retrieve only essential ID strings, thereby minimizing data transfer. Client-side processing (parsing and filtering) has been refined for efficiency, and asynchronous
IQueryableoperations are handled for improved database interaction.