Update explore-page.component.ts#9407
Update explore-page.component.ts#9407akshitha-1210 wants to merge 17 commits intoSunbird-Ed:release-8.1.0from
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 Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. ✨ Finishing touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Pull Request Overview
This PR adds metadata enrichment logic to the enrolled courses section by fetching additional content data and merging framework-specific filter categories into enrolled course items. The implementation extends the existing RxJS pipeline with nested switchMap and map operators.
- Enriches enrolled course content with framework metadata from fetched page content
- Updates enrolled section with enriched content and proper section naming
- Adds logic to find and set the correct enrolled section based on tab selection
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/app/client/src/app/modules/explore-page/components/explore-page/explore-page.component.ts
Outdated
Show resolved
Hide resolved
| }), | ||
| switchMap(this.fetchEnrolledCoursesSection.bind(this)) | ||
| switchMap(this.fetchEnrolledCoursesSection.bind(this)), | ||
| switchMap((enrolledSection:any) => |
There was a problem hiding this comment.
Inconsistent spacing: there's a missing space after (enrolledSection:any). It should be (enrolledSection: any) to match TypeScript conventions.
| switchMap((enrolledSection:any) => | |
| switchMap((enrolledSection: any) => |
| } | ||
|
|
||
| }) | ||
|
|
There was a problem hiding this comment.
Unnecessary blank line after the closing parenthesis should be removed for better code consistency.
|
|
||
| this.fetchContents().pipe( | ||
| map((pageContentData: any[]) => { | ||
|
|
There was a problem hiding this comment.
Unnecessary blank line before the map closing parenthesis should be removed for better code consistency.
| } | ||
|
|
||
| return content; | ||
| }); |
There was a problem hiding this comment.
Inconsistent indentation: The closing brace should be indented to align with the opening of the map function at the same level.
| switchMap(this.fetchEnrolledCoursesSection.bind(this)), | ||
| switchMap((enrolledSection:any) => | ||
|
|
||
| this.fetchContents().pipe( |
There was a problem hiding this comment.
The nested fetchContents() call will execute for every enrolled section emission, potentially causing unnecessary API calls and performance issues. Consider caching the results or restructuring the logic to avoid redundant fetches, especially if fetchContents() is an expensive operation that queries the backend.
|
|
||
|
|
||
| }), | ||
| ) |
There was a problem hiding this comment.
The new enrichment logic in the pipeline lacks error handling. If fetchContents() fails or if there's an error accessing metadata, the entire pipeline could break. Consider adding a catchError operator to handle potential failures gracefully and prevent the observable stream from terminating unexpectedly.
| ) | |
| catchError((err) => { | |
| // Optionally log the error or show a notification | |
| // this.toasterService.error('Failed to fetch contents'); | |
| // Return a fallback value (empty section) | |
| return of({ | |
| ...enrolledSection, | |
| contents: [], | |
| count: 0, | |
| name: this.getSectionName(get(this.activatedRoute, 'snapshot.queryParams.selectedTab')) | |
| }); | |
| }) |
| switchMap((enrolledSection:any) => | ||
|
|
||
| this.fetchContents().pipe( | ||
| map((pageContentData: any[]) => { | ||
|
|
||
| const allContents = _.flatMap(pageContentData, section => section.contents || []); | ||
| const metadataMap = _.keyBy(allContents, 'identifier'); | ||
|
|
||
| const enrichedContents = (this.enrolledSection.contents || []).map(content => { |
There was a problem hiding this comment.
The code assumes enrolledSection is always defined and has expected properties. If fetchEnrolledCoursesSection returns null or undefined in some cases, this will cause runtime errors. Consider adding a null check at the beginning of this switchMap: if (!enrolledSection) return of(enrolledSection);
| ), | ||
|
|
||
| tap((finalSection) => { | ||
| if (!finalSection) return; |
There was a problem hiding this comment.
Missing semicolon after the return statement. While JavaScript allows automatic semicolon insertion, it's inconsistent with the coding style in the rest of the file.
|
|
||
|
|
There was a problem hiding this comment.
Unnecessary blank lines should be removed for better code readability and consistency with the rest of the codebase.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 8 comments.
Comments suppressed due to low confidence (1)
src/app/client/src/app/modules/explore-page/components/explore-page/explore-page.component.ts:1460
- Incorrect brace placement and nesting. The closing braces at lines 375-378 don't properly close the nested for loops and if statements. Line 375 closes the inner if statement, line 376 should close the inner for loop, line 377 should close the outer if statement, and line 378 should close the outer for loop. The current indentation and structure suggests line 377 has incorrect placement.
}
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return this.coursesService.enrolledCourseData$ | ||
| .pipe( | ||
| tap(({ enrolledCourses, err }) => { | ||
| map(({ enrolledCourses, err }) => { |
There was a problem hiding this comment.
Changing from tap to map breaks the RxJS stream contract. The map operator expects a return value that will be emitted downstream, but this function performs side effects (setting this.enrolledSection, this.completedCourseSection) and doesn't return a meaningful value until line 384. The nested subscription at lines 360-379 is also problematic within a map operator as it creates a detached async operation. This should remain as tap since the primary purpose is side effects, not transformation.
| this.searchService.contentSearch({ filters: { identifier: _.map(this.enrolledCourses, 'content.identifier')}, fields: _.get(this.getCurrentPageData(), 'search.fields')}) | ||
| .subscribe((response) => { |
There was a problem hiding this comment.
This nested subscription within the RxJS operator creates an unmanaged subscription that will never be unsubscribed. This can lead to memory leaks. The subscription should be either: 1) added to a subscription collection and unsubscribed in ngOnDestroy, or 2) refactored to use RxJS operators like switchMap/mergeMap with takeUntil(this.unsubscribe$) to properly manage the subscription lifecycle.
|
|
||
| if (metadata && filterCategories) { | ||
| for (const category of filterCategories) { | ||
| if (category.type == 'framework') { |
There was a problem hiding this comment.
Use strict equality operator === instead of ==. The codebase consistently uses strict equality for string comparisons (as seen throughout the file), and this is a TypeScript/JavaScript best practice to avoid type coercion issues.
| if (category.type == 'framework') { | |
| if (category.type === 'framework') { |
| this.searchService.contentSearch({ filters: { identifier: _.map(this.enrolledCourses, 'content.identifier')}, fields: _.get(this.getCurrentPageData(), 'search.fields')}) | ||
| .subscribe((response) => { | ||
| const allContents = get(response, 'result.content'); | ||
| const metadataMap = _.keyBy(allContents, 'identifier'); | ||
| const filterCategories = this.cslFrameworkService.getGlobalFilterCategoriesObject(); | ||
| for (const content of this.enrolledSection.contents) { | ||
| const courseId = _.get(content, 'metaData.courseId'); | ||
| const metadata = metadataMap[courseId]; | ||
|
|
||
| if (metadata && filterCategories) { | ||
| for (const category of filterCategories) { | ||
| if (category.type == 'framework') { | ||
| content[category.code] = | ||
| _.get(metadata, category.code) ?? | ||
| _.get(metadata, category.alternativeCode); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| }); | ||
|
|
There was a problem hiding this comment.
The content search is called with potentially empty identifiers array. If this.enrolledCourses is empty, this will make an API call with an empty identifier filter, which may result in an error or unintended behavior. Add a guard condition to check if this.enrolledCourses has content before making the API call.
| this.searchService.contentSearch({ filters: { identifier: _.map(this.enrolledCourses, 'content.identifier')}, fields: _.get(this.getCurrentPageData(), 'search.fields')}) | |
| .subscribe((response) => { | |
| const allContents = get(response, 'result.content'); | |
| const metadataMap = _.keyBy(allContents, 'identifier'); | |
| const filterCategories = this.cslFrameworkService.getGlobalFilterCategoriesObject(); | |
| for (const content of this.enrolledSection.contents) { | |
| const courseId = _.get(content, 'metaData.courseId'); | |
| const metadata = metadataMap[courseId]; | |
| if (metadata && filterCategories) { | |
| for (const category of filterCategories) { | |
| if (category.type == 'framework') { | |
| content[category.code] = | |
| _.get(metadata, category.code) ?? | |
| _.get(metadata, category.alternativeCode); | |
| } | |
| } | |
| } | |
| } | |
| }); | |
| if (this.enrolledCourses && this.enrolledCourses.length > 0) { | |
| this.searchService.contentSearch({ filters: { identifier: _.map(this.enrolledCourses, 'content.identifier')}, fields: _.get(this.getCurrentPageData(), 'search.fields')}) | |
| .subscribe((response) => { | |
| const allContents = get(response, 'result.content'); | |
| const metadataMap = _.keyBy(allContents, 'identifier'); | |
| const filterCategories = this.cslFrameworkService.getGlobalFilterCategoriesObject(); | |
| for (const content of this.enrolledSection.contents) { | |
| const courseId = _.get(content, 'metaData.courseId'); | |
| const metadata = metadataMap[courseId]; | |
| if (metadata && filterCategories) { | |
| for (const category of filterCategories) { | |
| if (category.type == 'framework') { | |
| content[category.code] = | |
| _.get(metadata, category.code) ?? | |
| _.get(metadata, category.alternativeCode); | |
| } | |
| } | |
| } | |
| } | |
| }); | |
| } |
| this.enrolledSection = enrolledSection; | ||
| this.completedCourseSection = completedCourseSection; |
There was a problem hiding this comment.
These assignments occur inside the nested subscription (lines 360-379), meaning they execute asynchronously after the map operator completes. This creates a race condition where enrolledSection and completedCourseSection are set twice: once immediately at these lines, and potentially again later when the search completes. This duplicates the assignments from before the subscription and can lead to inconsistent state.
| import { cloneDeep, get, find, map as _map, pick, omit, groupBy, sortBy, replace, uniqBy, forEach, has, uniq, flatten, each, isNumber, toString, partition, toLower, includes } from 'lodash-es'; | ||
| import { IInteractEventEdata, IImpressionEventInput, TelemetryService } from '@sunbird/telemetry'; | ||
| import { map, tap, switchMap, skipWhile, takeUntil, catchError, startWith } from 'rxjs/operators'; | ||
| import { map, tap, switchMap, skipWhile, takeUntil, catchError, startWith} from 'rxjs/operators'; |
There was a problem hiding this comment.
Missing space before closing brace in import statement. Should be startWith } instead of startWith}.
| import { map, tap, switchMap, skipWhile, takeUntil, catchError, startWith} from 'rxjs/operators'; | |
| import { map, tap, switchMap, skipWhile, takeUntil, catchError, startWith } from 'rxjs/operators'; |
| // this.userPreference = this.setUserPreferences(); | ||
| this.showLoader = false; | ||
| const userProfileSubjects = _.get(this.userService, `userProfile.framework.${this.frameworkCategoriesList[3]}`) || []; | ||
| const userProfileSubjects = _.get(this.userService,`userProfile.framework.${this.frameworkCategoriesList[3]}`) || []; |
There was a problem hiding this comment.
Missing space after comma in function arguments. Should be _.get(this.userService, userProfile...instead of_.get(this.userService,userProfile....
| const userProfileSubjects = _.get(this.userService,`userProfile.framework.${this.frameworkCategoriesList[3]}`) || []; | |
| const userProfileSubjects = _.get(this.userService, `userProfile.framework.${this.frameworkCategoriesList[3]}`) || []; |
| @@ -306,7 +310,6 @@ export class ExplorePageComponent implements OnInit, OnDestroy, AfterViewInit { | |||
| let filteredCourses = _.filter(enrolledCourses || [], enrolledContentPredicate); | |||
| filteredCourses = _.orderBy(filteredCourses, [sortingField], [sortingOrder]); | |||
| this.enrolledCourses = filteredCourses | |||
There was a problem hiding this comment.
Avoid automated semicolon insertion (95% of all statements in the enclosing function have an explicit semicolon).
| this.enrolledCourses = filteredCourses | |
| this.enrolledCourses = filteredCourses; |
1 participant
SunbirdEd - Portal
Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.
Please choose applicable option
Example
Type of change
Please choose appropriate options.
Checklist: