Deployement of ECD child SP #71
Conversation
…igrationsql_chngs
📝 WalkthroughWalkthroughA new stored procedure Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks✅ Passed checks (3 passed)
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. Comment |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI Agents
In @src/main/resources/db/migration/dbiemr/V35__SP_ECD_Child.sql:
- Around line 9-12: The SELECT INTO for v_NextAttemptPeriod can return NULL or
multiple rows; change it to guarantee a single non-NULL value by selecting an
aggregate with a default, e.g. use SELECT COALESCE(MAX(NextAttemptPeriod),0)
INTO v_NextAttemptPeriod FROM m_mctscallconfiguration WHERE current_date()
BETWEEN effectivefrom AND EffectiveUpto AND OutboundCallType LIKE '%ecd%' AND
deleted = FALSE; this ensures no runtime error from multiple rows and that
v_NextAttemptPeriod is 0 when no config exists so the subsequent CASE logic
works.
- Around line 261-262: The migration ends the stored procedure with "END ;;" but
never restores the SQL delimiter; after the procedure terminator (the line
containing END ;;) add a line with DELIMITER ; to reset the default delimiter so
subsequent statements in this migration or session are parsed correctly.
🧹 Nitpick comments (3)
src/main/resources/db/migration/dbiemr/V35__SP_ECD_Child.sql (3)
14-131: Remove commented-out code block.This ~120-line commented block adds maintenance burden. Since migration files are version-controlled, consider removing this dead code. If needed for reference, it can be retrieved from version history.
238-240: HardcodedCalledServiceID=1714is a magic number.This value should be documented or parameterized. If it represents a specific service type (e.g., ECD Child service), consider adding a comment explaining its meaning, or retrieving it from a configuration table for maintainability.
168-169: Complex OutboundCallType transformation logic.The nested CASE with string length checks and
RIGHT()function is hard to follow. Consider adding a comment explaining the expected input formats and their transformations:
- 'introductory' → 'introductory'
- 4-character strings → 'ECD' + last character (e.g., 'ecd1' → 'ECD1')
- Other strings → 'ECD' + last 2 characters
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/resources/db/migration/dbiemr/V35__SP_ECD_Child.sql
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (java)
🔇 Additional comments (3)
src/main/resources/db/migration/dbiemr/V35__SP_ECD_Child.sql (3)
6-6: Verify DEFINER user permissions in production.The DEFINER
piramaldev@%allows execution from any host. Confirm this user exists in production and has appropriate privileges, or consider usingCURRENT_USERfor more portable deployments.
249-257: Verify date filtering logic - contains "need to crosscheck" comment.Lines 253-254 contain commented notes indicating uncertainty (
-- need to crosscheck). Given this is production code for Assam, please confirm the date filtering logic is correct:
- When
v_NextAttemptPeriod=0ORNoOfTrials=0: only date range check applies- Otherwise: requires
v_NextAttemptPerioddays to have passed sincelastmoddateAND date range check
167-199: Non-aggregated columns in GROUP BY may cause unpredictable results.The
GROUP BY A.childID(line 259) selects many non-aggregated columns (lines 171-199). This relies on MySQL's non-standard behavior whenONLY_FULL_GROUP_BYis disabled. If the SQL mode changes to strict, this query will fail. Additionally, the values returned for non-aggregated columns are indeterminate when multiple rows exist perchildID.Consider either:
- Adding all non-aggregated columns to the GROUP BY clause
- Using appropriate aggregate functions (MAX, MIN, etc.) for each column
- Using a window function with
ROW_NUMBER()to select one representative row per childAlso applies to: 258-259
⛔ Skipped due to learnings
Learnt from: vanitha1822 Repo: PSMRI/AMRIT-DB PR: 37 File: src/main/resources/db/migration/dbiemr/V19__Performance_yearlyreport.sql:16-21 Timestamp: 2025-10-07T05:18:17.680Z Learning: In the PSMRI/AMRIT-DB repository, ONLY_FULL_GROUP_BY compliance checks are not required for stored procedures and queries. The MySQL environment is configured to work without strict ONLY_FULL_GROUP_BY mode, so suggestions to wrap non-aggregated columns with ANY_VALUE() or add them to GROUP BY clauses should be skipped.
| declare v_NextAttemptPeriod int(11); | ||
| select distinct NextAttemptPeriod into v_NextAttemptPeriod from m_mctscallconfiguration where current_date() between effectivefrom and EffectiveUpto | ||
| and OutboundCallType like '%ecd%' | ||
| and deleted is false; |
There was a problem hiding this comment.
Handle NULL and multi-row scenarios for v_NextAttemptPeriod.
Two issues:
- If no matching configuration exists,
v_NextAttemptPeriodwill be NULL, causing the CASE logic (lines 249-257) to fail silently—bothv_NextAttemptPeriod=0andv_NextAttemptPeriod>0will be false, potentially returning no records. - If multiple rows match,
SELECT INTOwithDISTINCTmay still return multiple rows, causing a runtime error.
🔎 Proposed fix
declare v_NextAttemptPeriod int(11);
-select distinct NextAttemptPeriod into v_NextAttemptPeriod from m_mctscallconfiguration where current_date() between effectivefrom and EffectiveUpto
+select NextAttemptPeriod into v_NextAttemptPeriod from m_mctscallconfiguration where current_date() between effectivefrom and EffectiveUpto
and OutboundCallType like '%ecd%'
-and deleted is false;
+and deleted is false
+LIMIT 1;
+
+-- Default to 0 if no configuration found
+IF v_NextAttemptPeriod IS NULL THEN
+ SET v_NextAttemptPeriod = 0;
+END IF;🤖 Prompt for AI Agents
In @src/main/resources/db/migration/dbiemr/V35__SP_ECD_Child.sql around lines 9
- 12, The SELECT INTO for v_NextAttemptPeriod can return NULL or multiple rows;
change it to guarantee a single non-NULL value by selecting an aggregate with a
default, e.g. use SELECT COALESCE(MAX(NextAttemptPeriod),0) INTO
v_NextAttemptPeriod FROM m_mctscallconfiguration WHERE current_date() BETWEEN
effectivefrom AND EffectiveUpto AND OutboundCallType LIKE '%ecd%' AND deleted =
FALSE; this ensures no runtime error from multiple rows and that
v_NextAttemptPeriod is 0 when no config exists so the subsequent CASE logic
works.
|
|
||
| END ;; |
There was a problem hiding this comment.
Missing DELIMITER reset after procedure definition.
Add DELIMITER ; after END ;; to restore the default delimiter. This prevents issues if additional statements are executed in the same session or if this migration file is extended.
🔎 Proposed fix
END ;;
+DELIMITER ;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| END ;; | |
| END ;; | |
| DELIMITER ; |
🤖 Prompt for AI Agents
In @src/main/resources/db/migration/dbiemr/V35__SP_ECD_Child.sql around lines
261 - 262, The migration ends the stored procedure with "END ;;" but never
restores the SQL delimiter; after the procedure terminator (the line containing
END ;;) add a line with DELIMITER ; to reset the default delimiter so subsequent
statements in this migration or session are parsed correctly.
📋 Description
JIRA ID: AMM-2067
Changing restoring old original SP PR_FetchECDChildOutboundWorklist of Assam production because of slowness in ECD child ->PR_FetchECDChildOutboundWorklist
Summary by CodeRabbit
Chores
✏️ Tip: You can customize this high-level summary in your review settings.