Sn/es#33
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThis pull request introduces Elasticsearch support across multiple search and lookup components via a feature flag (isEnableES) initialized from environment configuration. Components branch their search logic to use Elasticsearch endpoints (identityQuickSearchES, advanceSearchIdentityES) when enabled, otherwise fall back to traditional search paths. Response handling and result restructuring logic is added to map Elasticsearch results into component-compatible formats. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant SearchComp as Search Component
participant RegistrarSvc as Registrar Service
participant ES as Elasticsearch<br/>(when enabled)
participant OldAPI as Traditional API<br/>(when disabled)
User->>SearchComp: Enter search term & trigger search
SearchComp->>SearchComp: Validate term (alphanumeric)
alt isEnableES === true
SearchComp->>RegistrarSvc: identityQuickSearchES(term)
RegistrarSvc->>ES: POST to elasticSearchUrl
ES-->>RegistrarSvc: Return ES results
RegistrarSvc-->>SearchComp: Response with data[0]...
SearchComp->>SearchComp: handleESSearchResponse()
SearchComp->>SearchComp: searchRestructES() - map results<br/>(benName, genderName, district, etc.)
SearchComp->>SearchComp: Display restructured results in table
else isEnableES === false
SearchComp->>RegistrarSvc: identityQuickSearch(term)
RegistrarSvc->>OldAPI: Traditional search call
OldAPI-->>RegistrarSvc: Return traditional results
RegistrarSvc-->>SearchComp: Response
SearchComp->>SearchComp: Use existing result handling
SearchComp->>SearchComp: Display results in table
end
SearchComp-->>User: Show search results or error alert
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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 review |
✅ Actions performedReview triggered.
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/registrar/family-tagging/family-tagging-details/family-tagging-details.component.ts (1)
418-434: Potential null pointer: missing null check ondatabefore accessinglength.In the non-ES path,
beneficiaryDetails.data.lengthis accessed without first verifying thatbeneficiaryDetails.dataexists. The ES path correctly checksbeneficiaryDetails.data &&before accessing length.🔎 Suggested fix
this.registrarService.identityQuickSearch(benReqObj).subscribe( (beneficiaryDetails: any) => { - if (beneficiaryDetails && beneficiaryDetails.data.length === 1) { + if (beneficiaryDetails && beneficiaryDetails.data && beneficiaryDetails.data.length === 1) { this.benFamilyId =src/registrar/beneficiary-details/beneficiary-details.component.ts (1)
140-223: Race condition: Image fetch executes before beneficiary data is populated.The
getBeneficiaryImagecall (lines 213-221) is executed immediately after subscribing to the search, but before the search response arrives. This meansthis.beneficiary.benImage = data.benImageat line 219 could fail becausethis.beneficiarymay still be undefined.Move the image fetch inside the success callbacks of both ES and non-ES branches.
🔎 Suggested fix
getBenFamilyDetails() { this.route.params.subscribe((param) => { if (this.isEnableES) { // Use Elasticsearch search const searchTerm = param['beneficiaryId']; this.registrarService .identityQuickSearchES({ search: searchTerm }) .subscribe( (res: any) => { if (res && res.data && res.data.length === 1) { this.beneficiary = res.data[0]; this.benFamilyId = res.data[0].familyID || res.data[0].familyId; this.beneficiaryName = this.beneficiary.firstName + (this.beneficiary.lastName !== undefined && this.beneficiary.lastName !== null && this.beneficiary.lastName !== '' ? ' ' + this.beneficiary.lastName : ''); this.regDate = moment .utc(this.beneficiary.createdDate) .format('DD-MM-YYYY hh:mm A'); + // Fetch image after beneficiary is set + this.fetchBeneficiaryImage(param['beneficiaryRegID']); } else if (res && res.data && res.data.length > 1) { // Multiple results handling... } else { // Not found handling... } }, // error handling... ); } else { // Traditional search // ... similar change needed here + // Move image fetch inside success callback } - - // Fetch beneficiary image (common for both ES and non-ES) - const benFlowID: any = param['beneficiaryRegID']; - this.beneficiaryDetailsService - .getBeneficiaryImage(benFlowID) - .subscribe((data: any) => { - if (data && data.benImage) { - this.beneficiary.benImage = data.benImage; - } - }); }); } + + private fetchBeneficiaryImage(beneficiaryRegID: string) { + this.beneficiaryDetailsService + .getBeneficiaryImage(beneficiaryRegID) + .subscribe((data: any) => { + if (data && data.benImage && this.beneficiary) { + this.beneficiary.benImage = data.benImage; + } + }); + }
🧹 Nitpick comments (5)
src/registrar/services/registrar.service.ts (1)
124-127: Minor formatting: missing blank line after method.There's a missing blank line between
identityQuickSearchESandclearBeneficiaryEditDetails, which is inconsistent with the rest of the file's formatting.🔎 Suggested fix
identityQuickSearchES(searchTerm: any) { return this.http.post(environment.elasticSearchUrl, searchTerm); } + clearBeneficiaryEditDetails() {src/registrar/registration/personal-information/personal-information.component.ts (1)
354-359: Remove debug console.log statements before merging.Multiple
console.logstatements are present for debugging ES and parent search results. Consider removing these before production deployment.🔎 Locations to clean up
- console.log('ES Parent search result:', JSON.stringify(response, null, 4)); this.personalInfoFormGroup.patchValue({ parentRegID: response.data[0].benPhoneMaps[0].parentBenRegID, parentRelation: 11, }); - console.log('Parent Reg ID (ES):', this.personalInfoFormGroup.value.parentRegID);And similar occurrences at lines 365, 371, 398, 403, 409, 415.
Also applies to: 365-366, 398-403, 409-415
src/registrar/search/search.component.ts (2)
130-156: Good implementation of debounced search with RxJS.The use of
debounceTime,distinctUntilChanged, andswitchMapis the correct pattern for typeahead search functionality.However, the subscribe error callback pattern is deprecated in RxJS 7+. Consider using the observer object pattern.
🔎 Modern RxJS subscribe pattern
.subscribe( - (response: any) => { - this.handleESSearchResponse(response); - }, - (error: any) => { - this.confirmationService.alert(error, 'error'); - } + { + next: (response: any) => { + this.handleESSearchResponse(response); + }, + error: (error: any) => { + this.confirmationService.alert(error, 'error'); + } + } );
159-166: Consider adding loading state for better UX.The debounced search triggers after 3+ characters are typed, but there's no loading indicator while waiting for the debounce or API response.
src/registrar/search/search.component.html (1)
14-14: Redundant ternary operator - both branches are identical.The placeholder has identical values for both
isEnableEStrue and false cases. Simplify to just use the value directly.🔎 Suggested fix
- placeholder="{{ isEnableES ? currentLanguageSet?.ro?.benIDOrPhNo : currentLanguageSet?.ro?.benIDOrPhNo }}" + placeholder="{{ currentLanguageSet?.ro?.benIDOrPhNo }}"
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/registrar/beneficiary-details/beneficiary-details.component.tssrc/registrar/family-tagging/family-tagging-details/family-tagging-details.component.tssrc/registrar/registration/personal-information/personal-information.component.tssrc/registrar/search/search.component.htmlsrc/registrar/search/search.component.tssrc/registrar/services/registrar.service.ts
🧰 Additional context used
🧠 Learnings (8)
📚 Learning: 2025-07-09T12:40:58.733Z
Learnt from: abhijeetw035
Repo: PSMRI/Common-UI PR: 20
File: src/registrar/registration/personal-information/personal-information.component.ts:29-29
Timestamp: 2025-07-09T12:40:58.733Z
Learning: In the AMRIT project, Common-UI is a git submodule (as defined in .gitmodules) that provides shared components and services. Import paths like `import { AmritTrackingService } from 'Common-UI/src/tracking';` are correct and resolve properly in the runtime environment where the submodule is initialized, even though they may not be visible in development environments where submodules aren't initialized.
Applied to files:
src/registrar/registration/personal-information/personal-information.component.ts
📚 Learning: 2025-07-09T12:40:58.733Z
Learnt from: abhijeetw035
Repo: PSMRI/Common-UI PR: 20
File: src/registrar/registration/personal-information/personal-information.component.ts:29-29
Timestamp: 2025-07-09T12:40:58.733Z
Learning: The Common-UI repository is designed to be used as a git submodule in other AMRIT applications. Import paths like `import { AmritTrackingService } from 'Common-UI/src/tracking';` are correct because they resolve properly in consuming applications where Common-UI is included as a submodule, even though the Common-UI repository itself doesn't contain a .gitmodules file.
Applied to files:
src/registrar/registration/personal-information/personal-information.component.ts
📚 Learning: 2024-12-10T08:42:10.330Z
Learnt from: helenKaryamsetty
Repo: PSMRI/Common-UI PR: 4
File: src/registrar/abha-components/abha-generation-success-component/abha-generation-success-component.component.ts:83-85
Timestamp: 2024-12-10T08:42:10.330Z
Learning: In this project, calling `assignSelectedLanguage()` inside `ngDoCheck()` is the standard approach used across components to handle language updates consistently.
Applied to files:
src/registrar/family-tagging/family-tagging-details/family-tagging-details.component.tssrc/registrar/beneficiary-details/beneficiary-details.component.tssrc/registrar/search/search.component.ts
📚 Learning: 2024-12-12T10:46:10.098Z
Learnt from: helenKaryamsetty
Repo: PSMRI/Common-UI PR: 5
File: src/registrar/abha-components/abha-enter-mobile-otp-component/abha-enter-mobile-otp-component.component.ts:44-48
Timestamp: 2024-12-12T10:46:10.098Z
Learning: In this Angular project, it is common practice to directly instantiate components like `SetLanguageComponent` for language management in methods such as `assignSelectedLanguage()`, rather than using a service. This is done to maintain consistency across the codebase.
Applied to files:
src/registrar/family-tagging/family-tagging-details/family-tagging-details.component.tssrc/registrar/beneficiary-details/beneficiary-details.component.tssrc/registrar/search/search.component.ts
📚 Learning: 2024-12-10T08:55:14.016Z
Learnt from: helenKaryamsetty
Repo: PSMRI/Common-UI PR: 4
File: src/registrar/abha-components/generate-abha-component/generate-abha-component.component.ts:79-84
Timestamp: 2024-12-10T08:55:14.016Z
Learning: In the Angular component `GenerateAbhaComponentComponent` (file path `src/registrar/abha-components/generate-abha-component/generate-abha-component.component.ts`), the `res.data.message` displayed using `confirmationService.alert()` contains only safe success messages and does not require additional sanitization before displaying to users.
Applied to files:
src/registrar/family-tagging/family-tagging-details/family-tagging-details.component.ts
📚 Learning: 2024-12-10T08:41:11.391Z
Learnt from: helenKaryamsetty
Repo: PSMRI/Common-UI PR: 4
File: src/registrar/abha-components/abha-generation-success-component/abha-generation-success-component.component.ts:70-78
Timestamp: 2024-12-10T08:41:11.391Z
Learning: In the `AbhaGenerationSuccessComponentComponent` class (`src/registrar/abha-components/abha-generation-success-component/abha-generation-success-component.component.ts`), the `GivePageToMobileEnterOtp()` method intentionally closes the newly opened dialog immediately after opening it with `dialogRef.close();` because, after routing to another component, the previous component should be closed.
Applied to files:
src/registrar/search/search.component.ts
📚 Learning: 2024-12-10T08:58:21.154Z
Learnt from: helenKaryamsetty
Repo: PSMRI/Common-UI PR: 4
File: src/registrar/abha-components/generate-abha-component/generate-abha-component.component.ts:37-41
Timestamp: 2024-12-10T08:58:21.154Z
Learning: In this project, the `assignSelectedLanguage()` method in components directly creates an instance of `SetLanguageComponent` to maintain a consistent language implementation across the project.
Applied to files:
src/registrar/search/search.component.ts
📚 Learning: 2024-12-10T08:59:11.297Z
Learnt from: helenKaryamsetty
Repo: PSMRI/Common-UI PR: 4
File: src/registrar/abha-components/abha-generation-success-component/abha-generation-success-component.component.ts:86-90
Timestamp: 2024-12-10T08:59:11.297Z
Learning: In this project, directly instantiating `SetLanguageComponent` is the standard method for language implementation to maintain consistency across the codebase.
Applied to files:
src/registrar/search/search.component.ts
🧬 Code graph analysis (1)
src/registrar/search/search.component.ts (1)
src/registrar/search-dialog/search-dialog.component.ts (1)
AfterViewChecked(269-271)
🔇 Additional comments (10)
src/registrar/services/registrar.service.ts (1)
143-147: LGTM - ES advanced search method follows existing patterns.The new
advanceSearchIdentityESmethod is consistent with the existingadvanceSearchIdentitymethod pattern.src/registrar/family-tagging/family-tagging-details/family-tagging-details.component.ts (2)
116-117: LGTM - Feature flag initialization follows project patterns.The ES feature flag is correctly initialized with a fallback to
false.
392-415: Field naming difference:familyID(ES) vsfamilyId(non-ES).The ES path accesses
familyID(uppercase D) while the non-ES path usesfamilyId(lowercase d). Ensure the ES API consistently returnsfamilyIDto avoid mapping issues.src/registrar/registration/personal-information/personal-information.component.ts (1)
343-383: ES path looks correct with proper null checks and error handling.The ES branch properly validates the response structure before accessing nested properties.
src/registrar/beneficiary-details/beneficiary-details.component.ts (1)
149-161: Good defensive coding: fallback for field name inconsistency.The use of
res.data[0].familyID || res.data[0].familyIdhandles potential field naming differences between ES and non-ES responses.src/registrar/search/search.component.ts (3)
222-246: ES restructuring handles nested properties well with fallbacks.The
searchRestructESmethod properly handles variations in the ES response structure with fallback chains forgenderName,districtName, andvillageName.
391-405: Good defensive null check added for phoneMaps.The added check
if (!phoneMaps || !phoneMaps.length)at line 392 properly handles cases where ES results may not have phone mappings.
123-127: Proper cleanup in ngOnDestroy.The subscription cleanup follows Angular best practices for preventing memory leaks.
src/registrar/search/search.component.html (2)
17-18: LGTM - Event handlers updated for ES-aware search.The input and button now correctly route through the new unified
onSearchButtonClickandonSearchInputChangehandlers that branch based onisEnableES.Also applies to: 27-27
159-162: Good - Table header adapts to ES mode.When ES is enabled, the column header shows "Beneficiary Reg ID" to match the ES response structure. This provides correct labeling for users.
2 participants
📋 Description
JIRA ID:
Amm-2038
✅ Type of Change
ℹ️ Additional Information
Elasticsearch API integration foe Advance search.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.