HMS-10740: add org_id to template initiated messages#2216
Merged
Conversation
Reviewer's GuideAdds org_id propagation into template-initiated message flows so that Kafka evaluation events include org context, updating controllers, helper functions, and tests accordingly. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
getSubscribedSystem, the error paths now return an empty orgID (""), which conflicts with the new test expectations and may be inconsistent with callers that expect the originalOrgIDfrom context even on failure—consider returningorgIDinstead of "" in the error cases.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `getSubscribedSystem`, the error paths now return an empty orgID (""), which conflicts with the new test expectations and may be inconsistent with callers that expect the original `OrgID` from context even on failure—consider returning `orgID` instead of "" in the error cases.
## Individual Comments
### Comment 1
<location path="manager/kafka/kafka.go" line_range="32-35" />
<code_context>
}
-func InventoryIDs2InventoryAIDs(accountID int, inventoryIDs []string) []mqueue.EvalData {
+func InventoryIDs2InventoryAIDs(accountID int, orgID string, inventoryIDs []string) []mqueue.EvalData {
inventoryAIDs := make([]mqueue.EvalData, 0, len(inventoryIDs))
for _, v := range inventoryIDs {
- inventoryAIDs = append(inventoryAIDs, mqueue.EvalData{InventoryID: v, RhAccountID: accountID})
+ inventoryAIDs = append(inventoryAIDs, mqueue.EvalData{InventoryID: v, RhAccountID: accountID, OrgID: &orgID})
}
return inventoryAIDs
</code_context>
<issue_to_address>
**issue (bug_risk):** Consider how OrgID is represented when the context value is missing or empty
Previously `EvalData` omitted `OrgID`; now it’s always non-nil and may point to an empty string. If callers distinguish `nil` from `""` (e.g., for routing/filtering), this changes behavior. Consider only setting `OrgID` when `orgID != ""` and leaving it `nil` otherwise, or explicitly confirming that "always non-nil, possibly empty" is the desired contract.
</issue_to_address>
### Comment 2
<location path="manager/controllers/template_subscribed_systems_update_test.go" line_range="27-31" />
<code_context>
c.Set(utils.KeySystem, subscriptionUUID)
- account, systemID, err := getSubscribedSystem(c, database.DB)
+ c.Set(utils.KeyOrgID, orgID)
+ account, org, systemID, err := getSubscribedSystem(c, database.DB)
assert.Nil(t, err)
assert.Equal(t, templateAccount, account)
+ assert.Equal(t, orgID, org)
assert.Equal(t, templateSystemUUID, systemID)
}
</code_context>
<issue_to_address>
**issue (testing):** TestUnknownSubscribedSystemID is asserting a non-empty orgID even though getSubscribedSystem currently returns an empty orgID on error
Currently, `getSubscribedSystem` returns `0, "", "", err` when the system is not found, so `orgID` is not preserved on error. That makes this test’s expectation of a non-empty `org` on error inconsistent with the implementation and potentially misleading.
Please either update the test to expect an empty `org` on error, or update `getSubscribedSystem` to propagate `orgID` in the error path, so the test matches the intended contract for error cases.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #2216 +/- ##
==========================================
+ Coverage 59.21% 59.23% +0.01%
==========================================
Files 137 137
Lines 8798 8801 +3
==========================================
+ Hits 5210 5213 +3
Misses 3040 3040
Partials 548 548
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
TenSt
approved these changes
May 27, 2026
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Secure Coding Practices Checklist GitHub Link
Secure Coding Checklist
Summary by Sourcery
Include organization ID in template-driven system evaluation messages sent via Kafka.
New Features:
Enhancements:
Tests: