small refactors on account manager - #11784
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests.
Additional details and impacted files@@ Coverage Diff @@
## main #11784 +/- ##
=============================================
- Coverage 19.78% 3.53% -16.26%
=============================================
Files 6371 487 -5884
Lines 575903 41867 -534036
Branches 70496 7913 -62583
=============================================
- Hits 113951 1479 -112472
+ Misses 449525 40173 -409352
+ Partials 12427 215 -12212
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull Request Overview
This PR performs small refactoring improvements to the AccountManagerImpl class to enhance code readability and maintainability. The changes include simplifying conditional logic, extracting reusable methods, and updating logging patterns.
- Simplified multiple conditional statements to use direct return expressions
- Extracted domain ID calculation logic into a reusable private method
- Updated logging to use parameterized format and modern charset handling
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
9dc2f0d to
bee7667
Compare
bee7667 to
9c9bb32
Compare
9c9bb32 to
10fa999
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Block when is not in the list of allowed IPs | ||
| if (!NetUtils.isIpInCidrList(loginIpAddress, accessAllowedCidrs.split(","))) { | ||
| logger.warn("Request by account '" + account.toString() + "' was denied since " + loginIpAddress.toString().replace("/", "") + " does not match " + accessAllowedCidrs); | ||
| logger.warn("Request by account '{}' was denied since {} does not match {}", account , loginIpAddress.toString().replace("/", ""), accessAllowedCidrs); |
There was a problem hiding this comment.
There's an extra space before the comma after the first placeholder. The code has "account ," but it should be "account," for consistent formatting.
| logger.warn("Request by account '{}' was denied since {} does not match {}", account , loginIpAddress.toString().replace("/", ""), accessAllowedCidrs); | |
| logger.warn("Request by account '{}' was denied since {} does not match {}", account, loginIpAddress.toString().replace("/", ""), accessAllowedCidrs); |
10fa999 to
a9df597
Compare
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 18881 |
|
[SF] Trillian test result (tid-16768)
|
|
@DaanHoogland could you merge main into this branch please? The PR targets main (24.0) but the branch is still on |
ef1e1d8 to
55eae45
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The domain-grouping map lookup in checkAccess still uses entity.getDomainId() instead of the resolved domainId, which can drop earlier entities and break the new test expectations.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
| Account account = ApiDBUtils.findAccountById(entity.getAccountId()); | ||
| domainId = account != null ? account.getDomainId() : -1; | ||
| } | ||
| long domainId = getDomainIdFor(entity); |
Description
This PR does some small refactors encountered during trouble shooting. ...
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
How did you try to break this feature and the system with this change?