Bug/accmgmt rolesyncservice replaced paralleljob howie #1831
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
…actor duplicated code - Renamed DoAllTheThings to ProcessEntityAndAssignmentUpdates for clarity - Fixed inefficient duplicate detection logic for parent role tracking (BEDR/AAFY) - Added null/empty validation in MapToAssignment with specific exception types - Extracted GetMatchingAssignments helper to eliminate duplicated query logic - Added validation and logging for missing entities after database queries - Fixed typo: "proccessing" → "processing"
howieandersen
changed the base branch from
main
to
bug/accmgmt-rolesyncservice-replaced-paralleljob
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Improve validation, duplicate detection, and refactor duplicated code
Description
Duplicate Logic Between Methods
• Extracted the common assignment matching logic into a new static helper method GetMatchingAssignments
• Both RemoveAssignments and ProcessEntityAndAssignmentUpdates now use this shared method
• Added .Distinct() to optimize the query for FromId values
Missing Null Check After Database Query
• Added validation in ProcessEntityAndAssignmentUpdates to check if all expected entities were retrieved
• Logs a warning with the count and IDs of missing entities (limited to first 10 for readability)
• The code continues processing available entities but alerts about missing ones
Inefficient Duplicate Detection Logic
• Changed the logic to add both BEDR and AAFY roles to the seen set first
• Then checks if either failed (meaning it was already present)
• This ensures both roles are properly tracked and prevents the issue where the first call fails and flushes before checking the second
Missing Validation in MapToAssignment
• Added null check for the model parameter
• Validates that FromParty and ToParty are not empty GUIDs
• Validates that RoleIdentifier is not null or whitespace
• Changed exception types to be more specific (ArgumentNullException, ArgumentException, InvalidOperationException)
• Improved error messages to include contextual information
Additional improvements:
• Renamed DoAllTheThings to ProcessEntityAndAssignmentUpdates (more descriptive)
• Fixed typo: "proccessing" → "processing"
• Made the helper method GetMatchingAssignments static since it doesn't use instance state