[ahmedxgouda]

Proposed change

Resolves #5286

Optimized EntityChannelNode

Checklist

• Required: I followed the contributing workflow
• Required: I verified that my code works as intended and resolves the issue as described
• Required: I ran all required checks and tests locally; all warnings addressed and failures resolved
• I used AI for code, documentation, tests, or communication related to this PR

[coderabbitai]

Summary by CodeRabbit

• Performance Improvements
  • Improved loading of entity channel names and external IDs by batching requests.
  • Preserved result ordering and handled unavailable channel information gracefully.
• Bug Fixes
  • Ensured only relevant conversation channels are used when resolving entity channel details.

Walkthrough

Adds batched dataloaders for entity-channel external IDs and names. Registers them in the OWASP dataloader collection and updates EntityChannelNode resolvers to fetch values asynchronously by entity-channel primary key.

Changes

Entity channel optimization

Layer / File(s)	Summary
Entity-channel loader implementation backend/src/apps/owasp/api/internal/dataloaders/entity_channel.py, backend/tests/unit/apps/owasp/api/internal/dataloaders/entity_channel_test.py	Adds shared and field-specific batched loaders for Slack channel external IDs and names. The loaders filter conversation content types, preserve input ordering, return nullable values, and receive factory and behavior tests.
Loader registry and resolver integration backend/src/apps/owasp/api/internal/dataloaders/__init__.py, backend/src/apps/owasp/api/internal/nodes/entity_channel.py, backend/tests/unit/apps/owasp/api/internal/dataloaders/dataloaders_test.py, backend/tests/unit/apps/owasp/api/internal/nodes/entity_channel_test.py	Registers both loaders. Updates external_id and name resolvers to use asynchronous dataloader lookups by primary key. Tests cover loader registration, resolver delegation, returned values, and None results.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

• OWASP/Nest#5240: Adds and registers related OWASP entity/project dataloaders and updates resolver access.
• OWASP/Nest#5256: Adds OWASP dataloaders and converts node resolvers to asynchronous context-based loading.
• OWASP/Nest#5299: Adds OWASP entity-channel dataloaders and integrates them with GraphQL resolvers.

Suggested reviewers: arkid15r

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	The dataloaders and asynchronous resolvers implement the optimization requested in issue #5286.
Out of Scope Changes check	✅ Passed	All reported code and test changes support the EntityChannelNode optimization objective.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Title check	✅ Passed	The title clearly summarizes the main change: optimizing EntityChannelNode with dataloaders.
Description check	✅ Passed	The description directly relates to optimizing EntityChannelNode and references the resolved issue.

✨ Finishing Touches 💡 1

🛠️ Fix failing CI checks 💡

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.84%. Comparing base (744e417) to head (22ba58f).
⚠️ Report is 1 commits behind head on feature/graphql-dataloaders.

Additional details and impacted files

@@                     Coverage Diff                      @@
##           feature/graphql-dataloaders    #5395   +/-   ##
============================================================
  Coverage                        98.84%   98.84%           
============================================================
  Files                              554      555    +1     
  Lines                            17594    17614   +20     
  Branches                          2483     2483           
============================================================
+ Hits                             17391    17411   +20     
  Misses                             101      101           
  Partials                           102      102           

Flag	Coverage Δ
backend	99.36% <100.00%> (+<0.01%)	⬆️
frontend	97.31% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
...s/owasp/api/internal/dataloaders/entity_channel.py	100.00% <100.00%> (ø)
...rc/apps/owasp/api/internal/nodes/entity_channel.py	100.00% <100.00%> (ø)

---

Continue to review full report in Codecov by Harness.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f4963b3...22ba58f. Read the comment docs.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@backend/src/apps/owasp/api/internal/dataloaders/entity_channel.py`:
- Around line 16-36: The loader function
_load_channel_field_by_entity_channel_id lacks focused coverage. Add tests
verifying results preserve input ID order, return None for missing IDs, and
return None for non-Conversation channels; also assert the expected fixed query
count when loading multiple IDs in one batch to preserve the batching
optimization.

🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 80c6c594-d2dc-4627-ba87-5515499ac8a7

📥 Commits

Reviewing files that changed from the base of the PR and between f4963b3 and 2e390a5.

📒 Files selected for processing (3)

• backend/src/apps/owasp/api/internal/dataloaders/__init__.py
• backend/src/apps/owasp/api/internal/dataloaders/entity_channel.py
• backend/src/apps/owasp/api/internal/nodes/entity_channel.py

[cubic-dev-ai]

3 issues found across 3 files

Confidence score: 3/5

• In backend/src/apps/owasp/api/internal/dataloaders/entity_channel.py, empty stored channel names now come back as GraphQL null instead of "", which can break clients that rely on the existing field contract and null/empty-string semantics—preserve empty strings and return None only for truly missing rows.
• In backend/src/apps/owasp/api/internal/nodes/entity_channel.py, resolver signatures now require info and async awaiting, so existing EntityChannelNode tests fail and CI stability is at risk until they’re updated—add async test coverage with an owasp_dataloaders context and assert loader/key usage.
• The new batch-query dataloader path in backend/src/apps/owasp/api/internal/dataloaders/entity_channel.py (filtering, subquery annotation, key reordering) has no unit tests, leaving ordering and mapping regressions hard to catch—add focused tests for content-type filtering and deterministic key-to-result alignment.

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="backend/src/apps/owasp/api/internal/nodes/entity_channel.py">

<violation number="1" location="backend/src/apps/owasp/api/internal/nodes/entity_channel.py:26">
P2: Existing EntityChannelNode resolver tests fail because these methods now require `info` and must be awaited; update them to provide an `owasp_dataloaders` context and assert the requested loader/key. This keeps CI passing and covers the new batching path.

(Based on your team's feedback about adding or updating tests for new logic.)</violation>
</file>

<file name="backend/src/apps/owasp/api/internal/dataloaders/entity_channel.py">

<violation number="1" location="backend/src/apps/owasp/api/internal/dataloaders/entity_channel.py:32">
P2: Channels with an empty stored name now resolve as GraphQL `null` rather than `""`, changing the existing field contract during this optimization. Preserve empty strings and let only missing query results remain `None`.</violation>

<violation number="2" location="backend/src/apps/owasp/api/internal/dataloaders/entity_channel.py:39">
P3: This PR adds a new dataloader module with real batch-query logic (content-type filtering, correlated Subquery annotation, and result reordering via get_result_by_keys), but it ships without any unit tests. Every sibling dataloader added under apps/owasp/api/internal/dataloaders (chapter, committee, member_snapshot, project, snapshot, board_of_directors) has a corresponding test file in backend/tests/unit/apps/owasp/api/internal/dataloaders/. Adding an entity_channel_test.py covering load_external_id_by_entity_channel_id and load_name_by_entity_channel_id (including the ordering/no-match and empty-channel cases) would keep this new behavior protected against regressions and align it with the existing pattern.</violation>
</file>

<sub>Reply with feedback, questions, or to request a fix.

Re-trigger cubic</sub>

[cubic-dev-ai]

0 issues found across 4 files (changes from recent commits).

Confidence score: 5/5

• Automated review surfaced no issues in the provided summaries.
• No files require special attention.

_{Re-trigger cubic}

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[cubic-dev-ai]

1 issue found across 1 file (changes from recent commits).

Confidence score: 4/5

• In backend/tests/unit/apps/owasp/api/internal/dataloaders/dataloaders_test.py, removing the entity-channel keys from EXPECTED_LOADER_KEYS while get_owasp_dataloaders() still includes them can let registration drift go undetected, weakening coverage and masking a future regression in dataloader wiring — align the expected keys with current get_entity_channel_loaders() behavior (or update production registration intentionally and consistently).

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="backend/tests/unit/apps/owasp/api/internal/dataloaders/dataloaders_test.py">

<violation number="1">
P3: This change removes the two entity-channel loaders from the aggregated registration test (EXPECTED_LOADER_KEYS) while get_owasp_dataloaders() still registers them via get_entity_channel_loaders() at this PR head. As a result, the test no longer verifies that EXTERNAL_ID_BY_ENTITY_CHANNEL_ID_LOADER and NAME_BY_ENTITY_CHANNEL_ID_LOADER are included in the aggregated loader mapping, so a future removal of the get_entity_channel_loaders() call from the aggregator would go uncaught. The commit message indicates the loaders' removal is intended but deferred to a follow-up registration-tests PR; until the loaders are actually removed from entity_channel.py and __init__.py, consider keeping these keys in EXPECTED_LOADER_KEYS so the test stays consistent with the module, and drop them in the same change that removes the loaders.</violation>
</file>

<sub>Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic</sub>