Skip to content

Sanity Issue 4511 Report Dashboard #1585

Merged
rakeshSgr merged 9 commits intodevelopfrom
SANITY_BUG_4511_Dasboard_ISSUE
Feb 24, 2026
Merged

Sanity Issue 4511 Report Dashboard #1585
rakeshSgr merged 9 commits intodevelopfrom
SANITY_BUG_4511_Dasboard_ISSUE

Conversation

@sumanvpacewisdom
Copy link
Copy Markdown
Collaborator

@sumanvpacewisdom sumanvpacewisdom commented Feb 23, 2026

Refactor SQL queries in migration file to improve readability and maintainability by standardizing indentation and formatting. This includes adjustments to tenant code conditions and session duration calculations.

Release Notes

  • Refactored SQL in report migration for improved readability and maintainability (standardized indentation/formatting).
  • Standardized session-type handling: 'All' includes both PUBLIC and PRIVATE; accepts Public/Private casing.
  • Adjusted tenant code conditions to consistently use the tenantCode parameter.
  • Simplified date filter logic to handle NULL start/end dates and unified comparisons.
  • Replaced complex CTEs and nested subqueries with direct SELECTs against public.sessions and related tables.
  • Replaced ownership-centric joins with session-centric conditions; simplified mentor/owner relationship logic.
  • Reworked duration calculations to use COALESCE and SUM-based expressions (consistent HH24:MI:SS formatting) to ensure accurate total/public/private hours.
  • Standardized output field aliases (sessions_title, sessions_created_by, date_of_session, session_type, categories, session_attended, duration_of_sessions_attended_in_minutes).
  • Switched certain JOINs to LEFT JOINs to preserve sessions when related data is missing and improve data integrity.
  • Added verbose runtime console logging in getEntityTypeWithEntitiesBasedOnOrg for inputs, branch decisions (CACHE vs DIRECT_DB), cache/DB results, and error paths.
  • Minor behavioral change: modelName filter now wraps single model names in an array when constructing filter.model_names.

Contribution Summary

Author Additions Deletions
sumanvpacewisdom 451 535

…ntainability by standardizing indentation and formatting. This includes adjustments to tenant code conditions and session duration calculations.
@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Feb 23, 2026

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Walkthrough

Replaces ownership/CTE-heavy report SQL with direct queries against public.sessions, public.session_attendees, and related tables; standardizes session_type mapping, simplifies date/NULL and duration logic, reshapes output fields, and adds verbose runtime logging and a minor filter change in entity resolution helper.

Changes

Cohort / File(s) Summary
Report Queries SQL Refactor
src/database/migrations/20251225065401-update-reports-queries-with-tenant-code.js
Full rewrite of multiple report queries: removes multi-CTE and ownership-join patterns in favor of direct selects on public.sessions and related tables; unifies session_type mapping (All/Public/Private → PUBLIC/PRIVATE), simplifies date/NULL comparisons, consolidates duration aggregation (COALESCE/TO_CHAR SUMs), standardizes output field names and counts, and adjusts mentor/session-owner logic to use mentor_id and session status.
Entity Helper Logging & Filter tweak
src/helpers/getOrgIdAndEntityTypewithEntitiesBasedOnPolicy.js
Adds extensive runtime/debug logging (branch decisions, cache vs DB, counts, errors) and changes model_names filter to wrap a single modelName in an array ({ [Op.contains]: [modelName] }) while preserving functional flow and signatures.

Sequence Diagram(s)

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Poem

🐇 I hopped through JOINs and found a new way,
Trimmed the CTE brambles so sessions could play.
Minutes counted neat, tenant codes in sight,
Logs now chatter softly through day and night.
A carrot for queries—hoppy, bright delight. 🥕

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name Status Explanation Resolution
Title check ❓ Inconclusive The title is vague and generic, using non-descriptive terms like 'Report Dashboard' that don't convey the specific technical changes or improvements made in the PR. Use a more descriptive title that reflects the main changes, such as 'Refactor report queries for tenant code and duration calculations' or 'Simplify session-centric report queries and improve debugging logs'.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)

✅ Copyable Unit Test edits generated.

  • Create PR with unit tests
  • Commit unit tests in branch SANITY_BUG_4511_Dasboard_ISSUE
  • Post copyable unit tests in a comment

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

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (3)
src/database/migrations/20251225065401-update-reports-queries-with-tenant-code.js (3)

12-21: ⚠️ Potential issue | 🔴 Critical

Bug: Hardcoded 'All' = 'All' is always true — should use :session_type.

The CASE WHEN 'All' = 'All' on lines 13 and 18 is a tautology that always evaluates to true, so public_count and private_count will always be returned regardless of the actual :session_type parameter. This should reference :session_type to match the filtering logic on lines 36–39.

🐛 Proposed fix
                 CASE 
-                    WHEN 'All' = 'All' THEN 
+                    WHEN :session_type = 'All' THEN 
                         COUNT(*) FILTER (WHERE Session.type = 'PUBLIC') -- Count for Public sessions
                     ELSE NULL 
                 END AS public_count,
                 CASE 
-                    WHEN 'All' = 'All' THEN 
+                    WHEN :session_type = 'All' THEN 
                         COUNT(*) FILTER (WHERE Session.type = 'PRIVATE') -- Count for Private sessions
                     ELSE NULL 
                 END AS private_count
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@src/database/migrations/20251225065401-update-reports-queries-with-tenant-code.js`
around lines 12 - 21, The CASE expressions that compute public_count and
private_count use the tautology "WHEN 'All' = 'All'" and must instead check the
query parameter; update both CASE WHEN clauses to reference the :session_type
parameter (e.g., CASE WHEN :session_type = 'All' THEN ... or matching the
existing session_type checks used elsewhere) so that public_count and
private_count are only computed when the incoming :session_type indicates "All"
(or follows the same logic as the filters using :session_type and Session.type);
modify the clauses that produce public_count and private_count and keep the
COUNT(*) FILTER (WHERE Session.type = 'PUBLIC'/'PRIVATE') parts unchanged.

32-33: ⚠️ Potential issue | 🟡 Minor

Date range uses exclusive bounds (> / <) — sessions exactly at boundary dates are excluded.

All queries use start_date > :start_date and end_date < :end_date, meaning sessions starting or ending exactly at the specified dates are excluded. Typically, at least the start boundary is inclusive (>=). Verify this is the intended behavior, as it could cause unexpected omissions in reports.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@src/database/migrations/20251225065401-update-reports-queries-with-tenant-code.js`
around lines 32 - 33, The WHERE clauses currently use exclusive bounds
(Session.start_date > :start_date and Session.end_date < :end_date) which
exclude sessions exactly on the provided boundaries; update the CASE expressions
to use inclusive comparisons—change Session.start_date > :start_date to
Session.start_date >= :start_date and Session.end_date < :end_date to
Session.end_date <= :end_date (keeping the NULL guards with :start_date and
:end_date intact) so boundary-date sessions are included in reports.

5-706: 🛠️ Refactor suggestion | 🟠 Major

Overall: the queries exhibit significant structural inconsistencies that undermine the stated goal of standardization.

Summarizing cross-cutting inconsistencies across the 10 queries in this migration:

Concern Pattern A (some queries) Pattern B (other queries)
Session type casing 'Public', 'Private' 'PUBLIC', 'PRIVATE'
NULL date handling CASE WHEN :start_date IS NOT NULL ... or :start_date IS NULL OR ... Direct > :start_date (breaks on NULL)
Duration calc 3-component INTERVAL arithmetic Single INTERVAL '1 second' * FLOOR(...)
NULL duration fallback COALESCE(..., '00:00:00') No COALESCE (returns NULL)
userId filtering CASE WHEN :userId IS NOT NULL ... or :userId IS NULL OR ... Direct = :userId

Standardizing these patterns would significantly improve maintainability and prevent subtle bugs where some reports break for the same input that works fine for others.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@src/database/migrations/20251225065401-update-reports-queries-with-tenant-code.js`
around lines 5 - 706, The migration's SQL queries are inconsistent and must be
standardized: in each query updated via queryInterface.bulkUpdate (identify by
report_code values like total_number_of_sessions_attended,
total_hours_of_learning, split_of_sessions_enrolled_and_attended_by_user,
mentee_session_details, total_number_of_sessions_conducted,
total_hours_of_mentoring_conducted, split_of_sessions_conducted,
mentoring_session_details, total_number_of_hours_of_mentoring_conducted,
split_of_sessions_created_and_conducted, session_manger_session_details) replace
mixed session type literals with uppercase 'PUBLIC'/'PRIVATE', normalize userId
filtering to use "(:userId IS NULL OR <column> = :userId)" pattern, normalize
date filters to "(:start_date IS NULL OR <column> > :start_date)" / "(:end_date
IS NULL OR <column> < :end_date)" to be NULL-safe, standardize duration
calculation to the single-second INTERVAL pattern (INTERVAL '1 second' *
FLOOR(...)) and wrap duration outputs with COALESCE(..., '00:00:00'), and keep
existing DYNAMIC_* and tenant_code joins intact; apply these changes across all
query strings in this migration so every report_code uses the same patterns.
🧹 Nitpick comments (4)
src/database/migrations/20251225065401-update-reports-queries-with-tenant-code.js (4)

250-336: Overly verbose duration calculation — can be simplified to match the pattern used elsewhere in this file.

Each duration aggregate (total, public, private) repeats the same CASE WHEN ... EXTRACT(EPOCH FROM ...) ELSE 0 END expression three times (for hours, minutes, seconds components). Lines 494–508 in this same file demonstrate a far simpler equivalent pattern:

TO_CHAR(
    INTERVAL '1 second' * FLOOR(SUM(EXTRACT(EPOCH FROM (completed_at - started_at)))),
    'HH24:MI:SS'
) AS total_hours

Since this PR aims to improve readability, consider aligning this query with the simpler pattern already used in total_hours_of_sessions_created_by_session_manager and total_number_of_hours_of_mentoring_conducted.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@src/database/migrations/20251225065401-update-reports-queries-with-tenant-code.js`
around lines 250 - 336, The duration aggregates for total_hours, public_hours,
and private_hours are overly verbose and repeat the same CASE/EXTRACT logic;
replace each block with the simplified pattern used elsewhere: compute
FLOOR(SUM(EXTRACT(EPOCH FROM (Session.completed_at - Session.started_at))))
(wrapped in the same CASE filter for public/private where needed), multiply by
INTERVAL '1 second', and format with TO_CHAR(..., 'HH24:MI:SS') to produce
total_hours, public_hours, and private_hours respectively (keep the CASE WHEN
Session.type = 'PUBLIC' / 'PRIVATE' for the filtered sums and CASE WHEN
Session.type IN ('PUBLIC','PRIVATE') for total as needed).

54-103: Duration calculation in total_hours_of_learning uses a different approach than total_hours_of_sessions_created_by_session_manager.

This query (lines 55–74) uses the three-component INTERVAL '1 hour' * FLOOR(... / 3600) + INTERVAL '1 minute' * ... + INTERVAL '1 second' * ... pattern, while lines 494–508 use the simpler INTERVAL '1 second' * FLOOR(SUM(...)). Both produce the same result when formatted with 'HH24:MI:SS', but the simpler form is easier to read and maintain. Also note this query lacks COALESCE around the TO_CHAR, so it will return NULL instead of '00:00:00' when no rows match — unlike the total_hours_of_mentoring_conducted query which wraps with COALESCE.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@src/database/migrations/20251225065401-update-reports-queries-with-tenant-code.js`
around lines 54 - 103, The duration formatting in the query that produces the
aliases total_hours, public_hours and private_hours (inside the
session_durations subquery) should be simplified to use INTERVAL '1 second' *
FLOOR(SUM(...)) for each SUM(...) expression (matching the approach used in
total_hours_of_sessions_created_by_session_manager), and each TO_CHAR result
should be wrapped with COALESCE(..., '00:00:00') so empty results return
'00:00:00' instead of NULL; update the three-component INTERVAL expressions to
the simpler INTERVAL '1 second' * FLOOR(SUM(...)) form for total_hours,
public_hours and private_hours and wrap their TO_CHAR calls with COALESCE as
described.

709-709: Empty down migration — no rollback path for these query changes.

This migration modifies 10 report queries with significant logic changes (date handling, session_type filtering, duration calculations, join refactoring). If a rollback is needed, the previous queries are unrecoverable from the migration alone. Consider populating the down method with the original query values, or at minimum documenting why rollback is intentionally omitted.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@src/database/migrations/20251225065401-update-reports-queries-with-tenant-code.js`
at line 709, The migration's async down(queryInterface, Sequelize) is empty,
leaving no rollback for the 10 modified report queries; populate down with the
inverse UPDATE statements that restore the original SQL/text for each report
query you changed in up (i.e., run UPDATE reports.report_queries SET query =
'<original SQL>' WHERE id = <id> or key = '<report_key>' for each changed
report), or if rollback is intentionally omitted, replace the empty down with a
clear comment and throw an Error('Rollback intentionally unsupported') or add a
migration-level comment explaining why and linking to a backup of the original
queries; ensure you reference the same identifiers used in up (report ids/keys
and the report_queries table) so the revert is deterministic.

642-646: Redundant WHERE clause — session_type check is always true for valid inputs.

This condition only checks that :session_type is one of 'All', 'Public', or 'Private' — it doesn't filter any table column. For any valid parameter value it's always true, and the actual type filtering is already handled inside each FILTER clause above. This is dead logic that adds noise.

Proposed removal
                 FROM public.sessions s
                 WHERE
                     s.tenant_code = :tenantCode
                     AND (:start_date IS NULL OR s.start_date > :start_date)
                     AND (:end_date IS NULL OR s.end_date < :end_date)
-                    AND (
-                        :session_type = 'All'
-                        OR :session_type = 'Public'
-                        OR :session_type = 'Private'
-                    )
                     AND s.deleted_at IS NULL
                     DYNAMIC_AND_CLAUSE;
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@src/database/migrations/20251225065401-update-reports-queries-with-tenant-code.js`
around lines 642 - 646, Remove the redundant WHERE condition block that only
checks the parameter :session_type against literal values (the AND (
:session_type = 'All' OR :session_type = 'Public' OR :session_type = 'Private' )
clause) because it doesn't filter any column and is always true for valid
inputs; update the migration SQL in
20251225065401-update-reports-queries-with-tenant-code.js by deleting that
clause and leaving the actual per-field FILTER clauses intact so existing
session-type filtering logic remains unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In
`@src/database/migrations/20251225065401-update-reports-queries-with-tenant-code.js`:
- Around line 225-228: Several queries in this migration use direct comparisons
like "Session.start_date > :start_date" which return no rows when :start_date or
:end_date is NULL; update each affected query (including the block shown and the
queries named total_hours_of_mentoring_conducted, mentoring_session_details,
total_hours_of_sessions_created_by_session_manager,
total_number_of_hours_of_mentoring_conducted, session_manger_session_details) to
use NULL guards consistent with the rest of the file, e.g. replace direct
comparisons with conditional checks of the form "(:start_date IS NULL OR
Session.start_date > :start_date)" and "(:end_date IS NULL OR Session.end_date <
:end_date)" (use the exact alias/column names used in each query like Session or
s) so queries return results when date params are NULL.
- Line 443: The condition "AND (:userId IS NOT NULL)" in the query is incorrect
because it only checks the parameter presence and causes empty results when
:userId is NULL; replace that guard with an optional-filter pattern used
elsewhere: change "AND (:userId IS NOT NULL)" to "AND (:userId IS NULL OR
<column> = :userId)" where <column> is the actual user column used by the query
(e.g., user_id or created_by), or remove the guard entirely if :userId is
mandatory and validate it at the application layer; update the clause
referencing the :userId parameter in the migration
(20251225065401-update-reports-queries-with-tenant-code.js) accordingly.
- Around line 200-204: Fix the :session_type comparisons in the listed report
queries so they match the application's title-cased input; locate the SQL blocks
for mentee_session_details, total_number_of_sessions_conducted,
total_hours_of_mentoring_conducted, mentoring_session_details,
total_hours_of_sessions_created_by_session_manager, and
total_number_of_hours_of_mentoring_conducted and replace comparisons against
'PUBLIC'/'PRIVATE' with 'Public'/'Private' (keep 'All' as-is) so the conditional
branches (:session_type = 'All' / 'Public' / 'Private') work correctly.

---

Outside diff comments:
In
`@src/database/migrations/20251225065401-update-reports-queries-with-tenant-code.js`:
- Around line 12-21: The CASE expressions that compute public_count and
private_count use the tautology "WHEN 'All' = 'All'" and must instead check the
query parameter; update both CASE WHEN clauses to reference the :session_type
parameter (e.g., CASE WHEN :session_type = 'All' THEN ... or matching the
existing session_type checks used elsewhere) so that public_count and
private_count are only computed when the incoming :session_type indicates "All"
(or follows the same logic as the filters using :session_type and Session.type);
modify the clauses that produce public_count and private_count and keep the
COUNT(*) FILTER (WHERE Session.type = 'PUBLIC'/'PRIVATE') parts unchanged.
- Around line 32-33: The WHERE clauses currently use exclusive bounds
(Session.start_date > :start_date and Session.end_date < :end_date) which
exclude sessions exactly on the provided boundaries; update the CASE expressions
to use inclusive comparisons—change Session.start_date > :start_date to
Session.start_date >= :start_date and Session.end_date < :end_date to
Session.end_date <= :end_date (keeping the NULL guards with :start_date and
:end_date intact) so boundary-date sessions are included in reports.
- Around line 5-706: The migration's SQL queries are inconsistent and must be
standardized: in each query updated via queryInterface.bulkUpdate (identify by
report_code values like total_number_of_sessions_attended,
total_hours_of_learning, split_of_sessions_enrolled_and_attended_by_user,
mentee_session_details, total_number_of_sessions_conducted,
total_hours_of_mentoring_conducted, split_of_sessions_conducted,
mentoring_session_details, total_number_of_hours_of_mentoring_conducted,
split_of_sessions_created_and_conducted, session_manger_session_details) replace
mixed session type literals with uppercase 'PUBLIC'/'PRIVATE', normalize userId
filtering to use "(:userId IS NULL OR <column> = :userId)" pattern, normalize
date filters to "(:start_date IS NULL OR <column> > :start_date)" / "(:end_date
IS NULL OR <column> < :end_date)" to be NULL-safe, standardize duration
calculation to the single-second INTERVAL pattern (INTERVAL '1 second' *
FLOOR(...)) and wrap duration outputs with COALESCE(..., '00:00:00'), and keep
existing DYNAMIC_* and tenant_code joins intact; apply these changes across all
query strings in this migration so every report_code uses the same patterns.

---

Nitpick comments:
In
`@src/database/migrations/20251225065401-update-reports-queries-with-tenant-code.js`:
- Around line 250-336: The duration aggregates for total_hours, public_hours,
and private_hours are overly verbose and repeat the same CASE/EXTRACT logic;
replace each block with the simplified pattern used elsewhere: compute
FLOOR(SUM(EXTRACT(EPOCH FROM (Session.completed_at - Session.started_at))))
(wrapped in the same CASE filter for public/private where needed), multiply by
INTERVAL '1 second', and format with TO_CHAR(..., 'HH24:MI:SS') to produce
total_hours, public_hours, and private_hours respectively (keep the CASE WHEN
Session.type = 'PUBLIC' / 'PRIVATE' for the filtered sums and CASE WHEN
Session.type IN ('PUBLIC','PRIVATE') for total as needed).
- Around line 54-103: The duration formatting in the query that produces the
aliases total_hours, public_hours and private_hours (inside the
session_durations subquery) should be simplified to use INTERVAL '1 second' *
FLOOR(SUM(...)) for each SUM(...) expression (matching the approach used in
total_hours_of_sessions_created_by_session_manager), and each TO_CHAR result
should be wrapped with COALESCE(..., '00:00:00') so empty results return
'00:00:00' instead of NULL; update the three-component INTERVAL expressions to
the simpler INTERVAL '1 second' * FLOOR(SUM(...)) form for total_hours,
public_hours and private_hours and wrap their TO_CHAR calls with COALESCE as
described.
- Line 709: The migration's async down(queryInterface, Sequelize) is empty,
leaving no rollback for the 10 modified report queries; populate down with the
inverse UPDATE statements that restore the original SQL/text for each report
query you changed in up (i.e., run UPDATE reports.report_queries SET query =
'<original SQL>' WHERE id = <id> or key = '<report_key>' for each changed
report), or if rollback is intentionally omitted, replace the empty down with a
clear comment and throw an Error('Rollback intentionally unsupported') or add a
migration-level comment explaining why and linking to a backup of the original
queries; ensure you reference the same identifiers used in up (report ids/keys
and the report_queries table) so the revert is deterministic.
- Around line 642-646: Remove the redundant WHERE condition block that only
checks the parameter :session_type against literal values (the AND (
:session_type = 'All' OR :session_type = 'Public' OR :session_type = 'Private' )
clause) because it doesn't filter any column and is always true for valid
inputs; update the migration SQL in
20251225065401-update-reports-queries-with-tenant-code.js by deleting that
clause and leaving the actual per-field FILTER clauses intact so existing
session-type filtering logic remains unchanged.

ℹ️ Review info

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4c0609b and 753e556.

📒 Files selected for processing (1)
  • src/database/migrations/20251225065401-update-reports-queries-with-tenant-code.js

@sumanvpacewisdom sumanvpacewisdom changed the title Refactor SQL queries in migration file to improve readability and maintainability by standardizing indentation and formatting Sanity Issue 4511 Report Dashboard Feb 23, 2026
…to ensure consistent casing and improve query clarity. Adjust conditions for start and end dates to handle null values appropriately.
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/database/migrations/20251225065401-update-reports-queries-with-tenant-code.js (1)

13-20: ⚠️ Potential issue | 🔴 Critical

'All' = 'All' is a hardcoded literal — public_count/private_count always non-NULL, ELSE NULL is dead code.

WHEN 'All' = 'All' compares two string constants and is unconditionally TRUE, so the ELSE NULL branch can never execute. The intended check was WHEN :session_type = 'All'. As written, public_count and private_count are always computed (returning 0 when the WHERE clause restricts to the opposite type) instead of NULL, losing the "not applicable" semantic callers likely rely on.

🐛 Proposed fix
-                CASE 
-                    WHEN 'All' = 'All' THEN 
-                        COUNT(*) FILTER (WHERE Session.type = 'PUBLIC') -- Count for Public sessions
-                    ELSE NULL 
-                END AS public_count,
-                CASE 
-                    WHEN 'All' = 'All' THEN 
-                        COUNT(*) FILTER (WHERE Session.type = 'PRIVATE') -- Count for Private sessions
-                    ELSE NULL 
-                END AS private_count
+                CASE 
+                    WHEN :session_type = 'All' THEN 
+                        COUNT(*) FILTER (WHERE Session.type = 'PUBLIC')
+                    ELSE NULL 
+                END AS public_count,
+                CASE 
+                    WHEN :session_type = 'All' THEN 
+                        COUNT(*) FILTER (WHERE Session.type = 'PRIVATE')
+                    ELSE NULL 
+                END AS private_count
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@src/database/migrations/20251225065401-update-reports-queries-with-tenant-code.js`
around lines 13 - 20, The CASE conditions for computing public_count and
private_count use the literal expression 'All' = 'All', which is always true and
makes the ELSE NULL branch dead; change those WHEN clauses to check the
session_type parameter (e.g., WHEN :session_type = 'All') so the counts are only
computed when session_type is 'All' and otherwise return NULL, updating the two
CASE expressions that reference Session.type and produce public_count and
private_count accordingly.
🧹 Nitpick comments (1)
src/database/migrations/20251225065401-update-reports-queries-with-tenant-code.js (1)

441-442: Reversed NULL-guard ordering — inconsistent with every other query in this file.

All other queries use (:param IS NULL OR column op :param). Standardise for readability:

♻️ Proposed fix
-                    AND (session.start_date > :start_date OR :start_date IS NULL)
-                    AND (session.end_date < :end_date OR :end_date IS NULL)
+                    AND (:start_date IS NULL OR session.start_date > :start_date)
+                    AND (:end_date IS NULL OR session.end_date < :end_date)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@src/database/migrations/20251225065401-update-reports-queries-with-tenant-code.js`
around lines 441 - 442, The null-guard ordering in the WHERE clause is reversed
compared to the rest of the file; change the two predicates "AND
(session.start_date > :start_date OR :start_date IS NULL)" and "AND
(session.end_date < :end_date OR :end_date IS NULL)" to the standardized form
"AND (:start_date IS NULL OR session.start_date > :start_date)" and "AND
(:end_date IS NULL OR session.end_date < :end_date)" in the
update-reports-queries migration so they match the other queries in the file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In
`@src/database/migrations/20251225065401-update-reports-queries-with-tenant-code.js`:
- Around line 694-696: The query currently uses an inner JOIN between
session_count (aliased sc) and public.user_extensions (aliased ue), which will
drop mentors that have no user_extensions row; change the JOIN to a LEFT JOIN so
mentors without an extension are retained (keep the tenant_code and mentor_id
predicate in the ON clause: "sc.mentor_id = ue.user_id AND sc.tenant_code =
ue.tenant_code") and ensure any filters in DYNAMIC_WHERE_CLAUSE that rely on ue
fields handle NULLs appropriately (e.g., by treating NULL ue columns as not
filtering out the row) to match the behavior of other queries like
mentoring_session_details and mentee_session_details.
- Around line 493-507: The SELECT expressions for
total_hours_of_sessions_created_by_session_manager and
total_number_of_hours_of_mentoring_conducted currently return NULL when SUM is
NULL; wrap each TO_CHAR(...) expression with COALESCE(..., '00:00:00') (i.e.,
COALESCE(TO_CHAR(INTERVAL '1 second' * FLOOR(SUM(...)), 'HH24:MI:SS'),
'00:00:00')) so empty-result cases produce '00:00:00'; apply the same COALESCE
pattern to the analogous expressions in the other query block that computes
total_number_of_hours_of_mentoring_conducted.
- Line 185: The SELECT list includes s.deleted_at but the query's WHERE already
has "AND s.deleted_at IS NULL", so s.deleted_at will always be NULL; remove
s.deleted_at from the SELECT clause in the migration SQL (the query block that
references s.deleted_at and the WHERE "AND s.deleted_at IS NULL") to avoid
returning a redundant column and clean up the result set.

---

Outside diff comments:
In
`@src/database/migrations/20251225065401-update-reports-queries-with-tenant-code.js`:
- Around line 13-20: The CASE conditions for computing public_count and
private_count use the literal expression 'All' = 'All', which is always true and
makes the ELSE NULL branch dead; change those WHEN clauses to check the
session_type parameter (e.g., WHEN :session_type = 'All') so the counts are only
computed when session_type is 'All' and otherwise return NULL, updating the two
CASE expressions that reference Session.type and produce public_count and
private_count accordingly.

---

Nitpick comments:
In
`@src/database/migrations/20251225065401-update-reports-queries-with-tenant-code.js`:
- Around line 441-442: The null-guard ordering in the WHERE clause is reversed
compared to the rest of the file; change the two predicates "AND
(session.start_date > :start_date OR :start_date IS NULL)" and "AND
(session.end_date < :end_date OR :end_date IS NULL)" to the standardized form
"AND (:start_date IS NULL OR session.start_date > :start_date)" and "AND
(:end_date IS NULL OR session.end_date < :end_date)" in the
update-reports-queries migration so they match the other queries in the file.

ℹ️ Review info

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 753e556 and 462fba5.

📒 Files selected for processing (1)
  • src/database/migrations/20251225065401-update-reports-queries-with-tenant-code.js

…etter handling of null values in time calculations. Update JOIN condition for user extensions to a LEFT JOIN to ensure all sessions are included, improving data integrity.
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
src/database/migrations/20251225065401-update-reports-queries-with-tenant-code.js (1)

54-103: Consider NULL-safe duration outputs for empty result sets.
TO_CHAR(...) returns NULL when SUM(...) is NULL; other duration reports in this file now return '00:00:00'. To keep outputs consistent, wrap these with COALESCE(..., '00:00:00').

♻️ Suggested adjustment
-                TO_CHAR(
-                    INTERVAL '1 hour' * FLOOR(SUM(duration) / 3600) +
-                    INTERVAL '1 minute' * FLOOR((SUM(duration) / 60)::BIGINT % 60) +
-                    INTERVAL '1 second' * FLOOR(SUM(duration)::BIGINT % 60),
-                    'HH24:MI:SS'
-                ) AS total_hours,  -- Total duration of all sessions
+                COALESCE(
+                    TO_CHAR(
+                        INTERVAL '1 hour' * FLOOR(SUM(duration) / 3600) +
+                        INTERVAL '1 minute' * FLOOR((SUM(duration) / 60)::BIGINT % 60) +
+                        INTERVAL '1 second' * FLOOR(SUM(duration)::BIGINT % 60),
+                        'HH24:MI:SS'
+                    ),
+                    '00:00:00'
+                ) AS total_hours,  -- Total duration of all sessions
                 
-                TO_CHAR(
-                    INTERVAL '1 hour' * FLOOR(SUM(CASE WHEN type = 'PUBLIC' THEN duration ELSE 0 END) / 3600) +
-                    INTERVAL '1 minute' * FLOOR((SUM(CASE WHEN type = 'PUBLIC' THEN duration ELSE 0 END) / 60)::BIGINT % 60) +
-                    INTERVAL '1 second' * FLOOR(SUM(CASE WHEN type = 'PUBLIC' THEN duration ELSE 0 END)::BIGINT % 60),
-                    'HH24:MI:SS'
-                ) AS public_hours,  -- Total duration of public sessions
+                COALESCE(
+                    TO_CHAR(
+                        INTERVAL '1 hour' * FLOOR(SUM(CASE WHEN type = 'PUBLIC' THEN duration ELSE 0 END) / 3600) +
+                        INTERVAL '1 minute' * FLOOR((SUM(CASE WHEN type = 'PUBLIC' THEN duration ELSE 0 END) / 60)::BIGINT % 60) +
+                        INTERVAL '1 second' * FLOOR(SUM(CASE WHEN type = 'PUBLIC' THEN duration ELSE 0 END)::BIGINT % 60),
+                        'HH24:MI:SS'
+                    ),
+                    '00:00:00'
+                ) AS public_hours,  -- Total duration of public sessions
                 
-                TO_CHAR(
-                    INTERVAL '1 hour' * FLOOR(SUM(CASE WHEN type = 'PRIVATE' THEN duration ELSE 0 END) / 3600) +
-                    INTERVAL '1 minute' * FLOOR((SUM(CASE WHEN type = 'PRIVATE' THEN duration ELSE 0 END) / 60)::BIGINT % 60) +
-                    INTERVAL '1 second' * FLOOR(SUM(CASE WHEN type = 'PRIVATE' THEN duration ELSE 0 END)::BIGINT % 60),
-                    'HH24:MI:SS'
-                ) AS private_hours  -- Total duration of private sessions
+                COALESCE(
+                    TO_CHAR(
+                        INTERVAL '1 hour' * FLOOR(SUM(CASE WHEN type = 'PRIVATE' THEN duration ELSE 0 END) / 3600) +
+                        INTERVAL '1 minute' * FLOOR((SUM(CASE WHEN type = 'PRIVATE' THEN duration ELSE 0 END) / 60)::BIGINT % 60) +
+                        INTERVAL '1 second' * FLOOR(SUM(CASE WHEN type = 'PRIVATE' THEN duration ELSE 0 END)::BIGINT % 60),
+                        'HH24:MI:SS'
+                    ),
+                    '00:00:00'
+                ) AS private_hours  -- Total duration of private sessions
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@src/database/migrations/20251225065401-update-reports-queries-with-tenant-code.js`
around lines 54 - 103, The SELECT's three duration fields (total_hours,
public_hours, private_hours) can be NULL when SUM(...) is NULL; update the query
by wrapping each TO_CHAR(...) expression with COALESCE(..., '00:00:00') so
total_hours, public_hours and private_hours always return '00:00:00' for empty
result sets; locate the TO_CHAR expressions in the query string (the aliases
total_hours, public_hours, private_hours inside the session_durations subquery)
and replace them accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In
`@src/database/migrations/20251225065401-update-reports-queries-with-tenant-code.js`:
- Around line 54-103: The SELECT's three duration fields (total_hours,
public_hours, private_hours) can be NULL when SUM(...) is NULL; update the query
by wrapping each TO_CHAR(...) expression with COALESCE(..., '00:00:00') so
total_hours, public_hours and private_hours always return '00:00:00' for empty
result sets; locate the TO_CHAR expressions in the query string (the aliases
total_hours, public_hours, private_hours inside the session_durations subquery)
and replace them accordingly.

ℹ️ Review info

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 462fba5 and 1034ac7.

📒 Files selected for processing (1)
  • src/database/migrations/20251225065401-update-reports-queries-with-tenant-code.js

…alue handling in time calculations, ensuring accurate total duration metrics for sessions. This enhances the robustness of the reporting queries.
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (1)
src/database/migrations/20251225065401-update-reports-queries-with-tenant-code.js (1)

560-566: Same WHEN :session_type = 'All' THEN TRUE inconsistency as in total_hours_of_sessions_created_by_session_manager.

Apply the same fix: replace THEN TRUE with THEN Session.type IN ('PUBLIC', 'PRIVATE') for consistency with the rest of the file.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@src/database/migrations/20251225065401-update-reports-queries-with-tenant-code.js`
around lines 560 - 566, In the CASE expression filtering by :session_type (the
block referencing Session.type), replace the branch "WHEN :session_type = 'All'
THEN TRUE" with "WHEN :session_type = 'All' THEN Session.type IN
('PUBLIC','PRIVATE')" so the logic matches other queries (e.g.,
total_hours_of_sessions_created_by_session_manager) and constrains Session.type
consistently instead of returning unconditional TRUE.
🧹 Nitpick comments (3)
src/database/migrations/20251225065401-update-reports-queries-with-tenant-code.js (3)

517-521: WHEN :session_type = 'All' THEN TRUE includes sessions of any type, unlike other queries.

Here the 'All' branch returns TRUE unconditionally, so sessions with type values other than 'PUBLIC' or 'PRIVATE' would be included in total_hours. Other queries in this file (e.g., total_number_of_sessions_conducted, total_hours_of_mentoring_conducted) use WHEN :session_type = 'All' THEN Session.type IN ('PUBLIC', 'PRIVATE') to explicitly restrict the scope. This inconsistency can lead to incorrect aggregation if any session has an unexpected type value.

♻️ Proposed fix for consistency
                     CASE 
-                        WHEN :session_type = 'All' THEN TRUE
+                        WHEN :session_type = 'All' THEN Session.type IN ('PUBLIC', 'PRIVATE')
                         WHEN :session_type = 'Public' THEN Session.type = 'PUBLIC'
                         WHEN :session_type = 'Private' THEN Session.type = 'PRIVATE'
                         ELSE TRUE
                     END
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@src/database/migrations/20251225065401-update-reports-queries-with-tenant-code.js`
around lines 517 - 521, The CASE branch for :session_type in the total_hours
query is overly broad (WHEN :session_type = 'All' THEN TRUE) and may include
sessions with unexpected Session.type values; change it to mirror other queries
by using WHEN :session_type = 'All' THEN Session.type IN ('PUBLIC', 'PRIVATE')
so the CASE only accepts those two types, updating the CASE expression that
references Session.type in the total_hours_of_mentoring_conducted/total_hours
query to ensure consistent filtering.

708-708: Empty down migration makes this change irreversible via db:migrate:undo.

All 12 report queries are rewritten in up, but down is a no-op. If a defect is discovered post-deployment, the only recovery path is a forward migration re-applying the old SQL. Consider storing the previous query strings and restoring them in down for operational safety.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@src/database/migrations/20251225065401-update-reports-queries-with-tenant-code.js`
at line 708, The migration's async down(queryInterface, Sequelize) is empty,
making the rewrite of the 12 report queries irreversible; capture the previous
SQL for each modified report in the migration and implement down to restore them
(e.g., use the same identifiers used in up such as the report keys or IDs and
call queryInterface.bulkUpdate / raw SQL to set the original query strings
back). Locate the modified-report logic in up (the block that rewrites the 12
report queries) and mirror it in down by re-applying the saved original query
values so db:migrate:undo returns the DB to its prior state.

12-21: Dead CASE condition — 'All' = 'All' is always TRUE; ELSE NULL is unreachable.

The literals 'All' = 'All' evaluate to TRUE at parse time, so public_count and private_count are always computed as counts (never NULL). The ELSE NULL branch can never be reached. The pattern in the file is inconsistent: both CASE expressions suggest the intent was to return NULL for non-All modes (implying WHEN :session_type = 'All'), but the hardcoded string makes them behave identically to always computing the counts — which is what total_number_of_sessions_conducted does without a CASE.

Either simplify to always compute the splits (consistent with the rest of the file):

♻️ Option A — Always compute (remove dead CASE)
-                CASE 
-                    WHEN 'All' = 'All' THEN 
-                        COUNT(*) FILTER (WHERE Session.type = 'PUBLIC') -- Count for Public sessions
-                    ELSE NULL 
-                END AS public_count,
-                CASE 
-                    WHEN 'All' = 'All' THEN 
-                        COUNT(*) FILTER (WHERE Session.type = 'PRIVATE') -- Count for Private sessions
-                    ELSE NULL 
-                END AS private_count
+                COUNT(*) FILTER (WHERE Session.type = 'PUBLIC') AS public_count,
+                COUNT(*) FILTER (WHERE Session.type = 'PRIVATE') AS private_count

Or fix to use the parameter (if NULL for non-All mode is the intended semantic):

♻️ Option B — Fix to use :session_type
-                CASE 
-                    WHEN 'All' = 'All' THEN 
-                        COUNT(*) FILTER (WHERE Session.type = 'PUBLIC') -- Count for Public sessions
-                    ELSE NULL 
-                END AS public_count,
-                CASE 
-                    WHEN 'All' = 'All' THEN 
-                        COUNT(*) FILTER (WHERE Session.type = 'PRIVATE') -- Count for Private sessions
-                    ELSE NULL 
-                END AS private_count
+                CASE 
+                    WHEN :session_type = 'All' THEN 
+                        COUNT(*) FILTER (WHERE Session.type = 'PUBLIC')
+                    ELSE NULL 
+                END AS public_count,
+                CASE 
+                    WHEN :session_type = 'All' THEN 
+                        COUNT(*) FILTER (WHERE Session.type = 'PRIVATE')
+                    ELSE NULL 
+                END AS private_count
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@src/database/migrations/20251225065401-update-reports-queries-with-tenant-code.js`
around lines 12 - 21, The CASE expressions for public_count and private_count
use the always-true literal ('All' = 'All') making ELSE NULL unreachable; fix by
either (A) removing the CASE and always computing the counts (replace the entire
CASE expression with COUNT(*) FILTER (WHERE Session.type = 'PUBLIC') and
COUNT(*) FILTER (WHERE Session.type = 'PRIVATE') respectively) or (B) restore
the intended conditional by replacing the hardcoded literal with the bind/param
(use WHEN :session_type = 'All' THEN ...) so the counts return NULL when
session_type is not 'All'; update the expressions that reference
Session.type/public_count/private_count accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In
`@src/database/migrations/20251225065401-update-reports-queries-with-tenant-code.js`:
- Around line 641-645: The outer WHERE fragment "(:session_type = 'All' OR
:session_type = 'Public' OR :session_type = 'Private')" can evaluate to NULL and
drop all rows for NULL/unexpected :session_type; remove or relax this guard.
Replace that expression with a safe predicate such as "(:session_type IS NULL OR
:session_type IN ('All','Public','Private'))" or simply remove the entire clause
so the COUNT/FILTER logic (which already handles session routing) controls
results; update the occurrence of the :session_type OR expression accordingly.

---

Duplicate comments:
In
`@src/database/migrations/20251225065401-update-reports-queries-with-tenant-code.js`:
- Around line 560-566: In the CASE expression filtering by :session_type (the
block referencing Session.type), replace the branch "WHEN :session_type = 'All'
THEN TRUE" with "WHEN :session_type = 'All' THEN Session.type IN
('PUBLIC','PRIVATE')" so the logic matches other queries (e.g.,
total_hours_of_sessions_created_by_session_manager) and constrains Session.type
consistently instead of returning unconditional TRUE.

---

Nitpick comments:
In
`@src/database/migrations/20251225065401-update-reports-queries-with-tenant-code.js`:
- Around line 517-521: The CASE branch for :session_type in the total_hours
query is overly broad (WHEN :session_type = 'All' THEN TRUE) and may include
sessions with unexpected Session.type values; change it to mirror other queries
by using WHEN :session_type = 'All' THEN Session.type IN ('PUBLIC', 'PRIVATE')
so the CASE only accepts those two types, updating the CASE expression that
references Session.type in the total_hours_of_mentoring_conducted/total_hours
query to ensure consistent filtering.
- Line 708: The migration's async down(queryInterface, Sequelize) is empty,
making the rewrite of the 12 report queries irreversible; capture the previous
SQL for each modified report in the migration and implement down to restore them
(e.g., use the same identifiers used in up such as the report keys or IDs and
call queryInterface.bulkUpdate / raw SQL to set the original query strings
back). Locate the modified-report logic in up (the block that rewrites the 12
report queries) and mirror it in down by re-applying the saved original query
values so db:migrate:undo returns the DB to its prior state.
- Around line 12-21: The CASE expressions for public_count and private_count use
the always-true literal ('All' = 'All') making ELSE NULL unreachable; fix by
either (A) removing the CASE and always computing the counts (replace the entire
CASE expression with COUNT(*) FILTER (WHERE Session.type = 'PUBLIC') and
COUNT(*) FILTER (WHERE Session.type = 'PRIVATE') respectively) or (B) restore
the intended conditional by replacing the hardcoded literal with the bind/param
(use WHEN :session_type = 'All' THEN ...) so the counts return NULL when
session_type is not 'All'; update the expressions that reference
Session.type/public_count/private_count accordingly.

ℹ️ Review info

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1034ac7 and bb58854.

📒 Files selected for processing (1)
  • src/database/migrations/20251225065401-update-reports-queries-with-tenant-code.js

…ved debugging

Enhance the getEntityTypeWithEntitiesBasedOnOrg function by adding console logs to track input parameters, filter conditions, cache results, and database query outcomes. This will facilitate easier troubleshooting and performance monitoring during execution.
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (2)
src/helpers/getOrgIdAndEntityTypewithEntitiesBasedOnPolicy.js (2)

333-347: Redundant inner try-catch causes double logging on DIRECT_DB query failure

When findUserEntityTypesAndEntities throws in the else branch, the error is:

  1. Caught at line 344 → console.error at line 345 → re-thrown
  2. Caught again at the outer catch (line 359) → console.error at line 360

The inner try-catch provides no recovery — it only adds noise. The cache-fallback path (line 316) doesn't have an equivalent inner try-catch, making the error handling inconsistent for the same underlying operation.

♻️ Proposed fix: remove the redundant inner try-catch
 		} else {
-			console.log(
-				'[getEntityTypeWithEntitiesBasedOnOrg] calling findUserEntityTypesAndEntities with filter:',
-				JSON.stringify(filter, (key, val) => (typeof val === 'symbol' ? val.toString() : val))
-			)
-			try {
-				entityTypesWithEntities = await entityTypeQueries.findUserEntityTypesAndEntities(
-					filter,
-					finalTenantCodes
-				)
-				console.log(
-					'[getEntityTypeWithEntitiesBasedOnOrg] direct DB result count:',
-					Array.isArray(entityTypesWithEntities)
-						? entityTypesWithEntities.length
-						: typeof entityTypesWithEntities
-				)
-			} catch (dbError) {
-				console.error('[getEntityTypeWithEntitiesBasedOnOrg] direct DB error:', dbError.message)
-				throw dbError
-			}
+			entityTypesWithEntities = await entityTypeQueries.findUserEntityTypesAndEntities(
+				filter,
+				finalTenantCodes
+			)
 		}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/helpers/getOrgIdAndEntityTypewithEntitiesBasedOnPolicy.js` around lines
333 - 347, Remove the redundant inner try-catch around the direct DB call to
entityTypeQueries.findUserEntityTypesAndEntities in
getEntityTypeWithEntitiesBasedOnOrg (the block starting with
entityTypesWithEntities = await
entityTypeQueries.findUserEntityTypesAndEntities(filter, finalTenantCodes));
instead let the call bubble to the existing outer catch so logging and error
handling are consistent with the cache path — keep the existing console.log that
prints the direct DB result count but delete the inner catch that does
console.error(dbError.message) and re-throws, relying on the outer catch to log
and re-throw the error uniformly.

279-354: Replace console.log/error with a structured logger; remove duplicate filter serialization

Several issues with the logging as-is:

  1. Unstructured output: All logging uses raw console.log/error. The project should use a structured/leveled logger (e.g., logger.debug(...)) so these can be silenced in production without code changes.
  2. Duplicate filter serialization: In the DIRECT_DB branch, filter is serialized at line 279–283 and again at line 329–332 in the same execution path — double the cost for no extra information.
  3. error.stack in production logs (line 360): Exposing raw stack traces in logs can leak internal path/module structure.
♻️ Suggested fix: deduplicate and use a leveled logger
-		console.log(
-			'[getEntityTypeWithEntitiesBasedOnOrg] filter:',
-			JSON.stringify(filter, (key, val) => (typeof val === 'symbol' ? val.toString() : val))
-		)
-		console.log('[getEntityTypeWithEntitiesBasedOnOrg] finalTenantCodes:', finalTenantCodes)
-		console.log(
-			'[getEntityTypeWithEntitiesBasedOnOrg] branch:',
-			modelName && !entity_types ? 'CACHE' : 'DIRECT_DB'
-		)
+		logger.debug('[getEntityTypeWithEntitiesBasedOnOrg] branch:', modelName && !entity_types ? 'CACHE' : 'DIRECT_DB')
 		} else {
-			console.log(
-				'[getEntityTypeWithEntitiesBasedOnOrg] calling findUserEntityTypesAndEntities with filter:',
-				JSON.stringify(filter, (key, val) => (typeof val === 'symbol' ? val.toString() : val))
-			)
 			try {
 		} catch (error) {
-			console.error('[getEntityTypeWithEntitiesBasedOnOrg] outer catch error:', error.message, error.stack)
+			logger.error('[getEntityTypeWithEntitiesBasedOnOrg] error:', error.message)
 			return {
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/helpers/getOrgIdAndEntityTypewithEntitiesBasedOnPolicy.js` around lines
279 - 354, Replace raw console.log/error calls in
getEntityTypeWithEntitiesBasedOnOrg with the project's structured logger (use
logger.debug for trace/info and logger.error for errors) and stop logging raw
error.stack; log cacheError.message/dbError.message and any sanitized context
instead. Compute the serializedFilter once (serialize filter with the symbol
replacer into a local variable) and reuse it for both the initial debug and the
DIRECT_DB debug to avoid duplicate serialization. Ensure calls to
entityTypeCache.getEntityTypesAndEntitiesForModel and
entityTypeQueries.findUserEntityTypesAndEntities keep the same parameters but
wrap them with logger.debug(...) for success counts (use
Array.isArray(entityTypesWithEntities) ? length : typeof) and logger.error(...)
for failures with limited error info. Finally remove duplicate console
statements that output the same serialized filter or result counts and replace
them with the single, leveled logger calls referenced above.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/helpers/getOrgIdAndEntityTypewithEntitiesBasedOnPolicy.js`:
- Around line 241-248: Move the plaintext console.log in the
getEntityTypeWithEntitiesBasedOnOrg block into the surrounding try block and
replace it with a structured/debug-level logger call that omits or masks
sensitive multi-tenant identifiers (do not log raw organization_codes or
tenantCodes); instead log non-sensitive context such as modelName, entity_types,
and flags like defaultOrgCode/defaultTenantCode or a redacted tenant count, and
ensure the logging call uses the module's logger (debug/trace level) rather than
console to keep it behind proper logging controls.

---

Nitpick comments:
In `@src/helpers/getOrgIdAndEntityTypewithEntitiesBasedOnPolicy.js`:
- Around line 333-347: Remove the redundant inner try-catch around the direct DB
call to entityTypeQueries.findUserEntityTypesAndEntities in
getEntityTypeWithEntitiesBasedOnOrg (the block starting with
entityTypesWithEntities = await
entityTypeQueries.findUserEntityTypesAndEntities(filter, finalTenantCodes));
instead let the call bubble to the existing outer catch so logging and error
handling are consistent with the cache path — keep the existing console.log that
prints the direct DB result count but delete the inner catch that does
console.error(dbError.message) and re-throws, relying on the outer catch to log
and re-throw the error uniformly.
- Around line 279-354: Replace raw console.log/error calls in
getEntityTypeWithEntitiesBasedOnOrg with the project's structured logger (use
logger.debug for trace/info and logger.error for errors) and stop logging raw
error.stack; log cacheError.message/dbError.message and any sanitized context
instead. Compute the serializedFilter once (serialize filter with the symbol
replacer into a local variable) and reuse it for both the initial debug and the
DIRECT_DB debug to avoid duplicate serialization. Ensure calls to
entityTypeCache.getEntityTypesAndEntitiesForModel and
entityTypeQueries.findUserEntityTypesAndEntities keep the same parameters but
wrap them with logger.debug(...) for success counts (use
Array.isArray(entityTypesWithEntities) ? length : typeof) and logger.error(...)
for failures with limited error info. Finally remove duplicate console
statements that output the same serialized filter or result counts and replace
them with the single, leveled logger calls referenced above.

ℹ️ Review info

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between bb58854 and 0d6f2c7.

📒 Files selected for processing (1)
  • src/helpers/getOrgIdAndEntityTypewithEntitiesBasedOnPolicy.js

…Org to streamline code and improve performance. This cleanup enhances readability and reduces clutter in the logging output during execution.
}
if (modelName) {
filter.model_names = { [Op.contains]: modelName }
filter.model_names = { [Op.contains]: [modelName] }
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sumanvpacewisdom why this change ?

…sBasedOnPolicy to correctly handle single modelName input. This change improves the accuracy of the filtering logic.
…titiesBasedOnPolicy to handle both single and array inputs for modelName. This change enhances the filtering logic's flexibility and accuracy.
…titiesBasedOnPolicy to streamline handling of modelName inputs. This change consolidates the logic for both single and array cases, enhancing code clarity and maintainability.
if (modelName) {
filter.model_names = { [Op.contains]: modelName }
filter.model_names = { [Op.contains]: Array.isArray(modelName) ? modelName : [modelName] }
}
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rakeshSgr This is added because , the modelName are sent from different places , from some functions it passes directly array , for some its only string , so we have added this , this is how we have fixed everywhere for entityType issues , nevil maken it a note later after all this , we will make it consistent

@rakeshSgr rakeshSgr merged commit 36e6044 into develop Feb 24, 2026
1 of 2 checks passed
@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Feb 24, 2026

Note

Unit test generation is a public access feature. Expect some limitations and changes as we gather feedback and continue to improve it.


Generating unit tests... This may take up to 20 minutes.

2 similar comments
@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Feb 24, 2026

Note

Unit test generation is a public access feature. Expect some limitations and changes as we gather feedback and continue to improve it.


Generating unit tests... This may take up to 20 minutes.

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Feb 24, 2026

Note

Unit test generation is a public access feature. Expect some limitations and changes as we gather feedback and continue to improve it.


Generating unit tests... This may take up to 20 minutes.

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Feb 24, 2026

✅ Created PR with unit tests: #1588

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Feb 24, 2026

Request timed out after 900000ms (requestId=3a89a48a-d160-4683-b0dd-a89e6f27a703)

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Feb 24, 2026

Request timed out after 900000ms (requestId=bb032afa-134c-4909-8be1-c6682555ad77)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants