Conversation
|
Great job! No new security vulnerabilities introduced in this pull request |
|
@claude be kind |
|
Claude finished @jrmccannon's task in 1m 2s —— View job Kind Review of Your PR
Hey Jared! Nice work on this one. The overall design is clean and well thought out. Here are my thoughts: What I really like
A couple of small observations
Overall this is a well-structured, incremental improvement. The FusionCache integration looks correct, the feature flag gives you a safe rollout path, and the tests give good confidence. Nice job! 👍 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7443 +/- ##
==========================================
+ Coverage 58.66% 58.68% +0.01%
==========================================
Files 2066 2068 +2
Lines 91089 91127 +38
Branches 8106 8111 +5
==========================================
+ Hits 53440 53478 +38
Misses 35740 35740
Partials 1909 1909 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
BuildCacheKeyForOrganizationAbility isn't a constant :P and I think there's little benefit to being in its own class - I suggest these are private properties in the cache service itself.
| public async Task<IDictionary<Guid, OrganizationAbility>> GetOrganizationAbilitiesAsync(IEnumerable<Guid> orgIds) | ||
| { | ||
| var allOrganizationAbilities = await inMemoryApplicationCacheService.GetOrganizationAbilitiesAsync(); | ||
| return orgIds | ||
| .Distinct() | ||
| .Where(allOrganizationAbilities.ContainsKey) | ||
| .ToDictionary(id => id, id => allOrganizationAbilities[id]); | ||
| } |
There was a problem hiding this comment.
Missed this one. Fwiw, I don't think there's a bulk interface for FusionCache - this is for convenience only. It should have a corresponding interface on the new cache service, which would just await Task.WhenAll individual cache fetches.
| services.AddScoped<IApplicationCacheService, FeatureRoutedCacheService>(); | ||
| services.AddExtendedCache(OrganizationAbilityCacheConstants.CacheName, globalSettings); | ||
| services.AddScoped<IOrganizationAbilityCacheService, ExtendedOrganizationAbilityCacheService>(); | ||
|
|
There was a problem hiding this comment.
It would be better not to expose this at the top level here. Can we have a services extension method - e.g. AddOrganizationAbilityCache - which registers both? Similar to how our commands/queries/handlers are registered.
There was a problem hiding this comment.
It's not a blocker, but I'm not sure of the value of these tests. The class is extremely thin, there's not much to test here by design, and these tests either use mocks or assert against the same prod code that you're testing (i.e. BuildCacheKeyForOrganizationAbility). Consider deleting them and/or relying on integration tests instead.
jrmccannon
force-pushed
the
jmccannon/ac/pm-32068-org-ability-cache
branch
from
9ffa092 to
4737558
Compare
…with specific endpoints tested. moved registration to ext method
|
2 participants
🎟️ Tracking
PM-32068
📔 Objective
This implements retrieving the org ability record via the Extended Cache (fusion cache). This will be used instead of the in memory cache in order to reduce load on the org ability database table and solve the issue with servicebus.