diff --git a/code-reviews/general-1004.md b/code-reviews/general-1004.md new file mode 100644 index 00000000..26848925 --- /dev/null +++ b/code-reviews/general-1004.md @@ -0,0 +1,152 @@ +Now I have a thorough understanding of the codebase. Here is the full code review: + +--- + +## Code Review — Pratham Learner App + +**Context:** A React Native Android app for Second Chance Program learners — young women in underserved communities, often on low-resource devices with unreliable mobile data. + +--- + +### 1. Critical Security Issues + +**a) Hardcoded credentials in production code** +`src/utils/API/ApiCalls.js:27–28` +```js +username: 'test1', +password: '12345', +``` +The `getAccessToken` function (which appears to be a vestigial test function) contains real-looking credentials. Even if this function is no longer called, it should not exist in the codebase — it will show up in any security audit and in the compiled APK. + +**b) Sensitive data logged on every storage write** +`src/utils/JsHelper/Helper.js:24` +```js +console.log('########## setDataInStorage', name, data); +``` +Every call to `setDataInStorage` logs both the key and value. This means tokens, user IDs, and any other stored data appear in device logs. On Android, device logs can be read by other apps with the `READ_LOGS` permission on older API levels. This needs to be removed entirely. + +**c) cURL commands with auth headers logged** +`src/utils/API/AuthService.js:77–78`, `ApiCalls.js:586–591` +Multiple functions construct full cURL commands (including `Authorization: Bearer ` headers) and log them. Some are commented out, but several `console.log('cURL Command:', curlCommand)` calls are still active (e.g., `userExist`, `sendOtp`, `refreshToken`). This leaks bearer tokens to device logs in production. + +--- + +### 2. No Network Timeout Configured + +`src/utils/API/RestClient.ts` and all `axios.request(config)` calls have zero timeout configuration. On a 2G/3G connection with packet loss, requests will hang indefinitely. For the target user base (poor network areas, low-resource devices), this will cause the app to appear frozen. Every axios config should include at minimum: +```js +timeout: 15000, // 15 seconds +``` + +--- + +### 3. Offline-First Architecture is Inconsistent + +The app has an offline cache layer via SQLite (`SqliteHelper.js`) and the `storeApiResponse`/`getApiResponse` pattern. However, it is applied inconsistently: +- `courseDetails`, `contentTrackingStatus`, `CourseInProgress` support offline fallback. +- `readContent`, `hierarchyContent`, `listQuestion` do not — they just return `null` on failure. + +For learners in areas with intermittent connectivity, content that doesn't load offline will feel broken. The offline strategy needs to be applied uniformly, especially for content-reading APIs. + +--- + +### 4. Unreachable Code + +`src/utils/JsHelper/Helper.js:16–17` +```js +return null; +console.error('Error retrieving credentials:', e); // dead — never executes +``` +The `return` statement before the `console.error` makes the error logging dead code. This is the foundational storage getter — errors here are silently swallowed. + +--- + +### 5. Empty / Silent Catch Blocks + +Several catch blocks silently discard errors: +- `ApiCalls.js:51` — `getAccessToken` network failure: `.catch((error) => {})` +- `ApiCalls.js:503` — inside `contentTracking` course status update +- `AuthService.js:1215, 1242, 1288, 1366, 1410` — five instances in a row +- `Assessment/TestDetailView.js:60` + +These make failures invisible and will make debugging production issues extremely difficult. + +--- + +### 6. App.js — Render-Phase Side Effects + +`src/App.js:496–511` +```js +// Called directly in the component body, not inside useEffect: +PushNotification.createChannel({ ... }); +PushNotification.configure({ ... }); +``` +These calls execute on every render of `App`. They should be inside a `useEffect(() => { ... }, [])`. Additionally, `PushNotification.configure` is called twice — once in the component body and once inside a `useEffect` (line 513–523). + +--- + +### 7. The Entire LoginScreen is Commented Out + +`src/screens/LoginScreen/LoginScreen.js` — the entire file (200+ lines) is commented out. A dead file that exports nothing. If it's not needed, it should be deleted. Commented-out code in version control creates confusion about intent. + +--- + +### 8. Giant Files / Poor Separation of Concerns + +| File | Lines | +|---|---| +| `AuthService.js` | 2,364 | +| `RegistrationForm.js` | 1,566 | +| `StandAlonePlayer.js` | 1,815 | +| `ATMAssessment.js` | 1,416 | + +`AuthService.js` mixes login, token refresh, profile management, content fetching, course enrollment, certificate issuance, attendance, telemetry, and sync logic. This makes it hard to maintain and test. It should be split by domain (auth, content, tracking, certificates). + +--- + +### 9. Duplicate `getHeaders` Function + +`getHeaders` is defined independently in both `ApiCalls.js:13` and `AuthService.js:17` with identical logic. If the token key name or tenantId logic changes, it will diverge silently. This should be a shared utility. + +--- + +### 10. Deep Link Handling Forces App Restart + +`src/App.js:247` +```js +if (isRunning) { + RNRestart.restart(); +} +``` +When a deep link is received while the app is running, it restarts the entire app. This is a jarring UX — the user loses their current position. The proper approach is to navigate to the target screen via the navigation ref. + +--- + +### 11. 1,371 `console.log` Calls in Production + +There is no log-stripping configured. Every debug statement ships to production. On low-resource devices, excessive logging contributes to memory pressure and slightly slower JS execution. More critically, it exposes internal app logic and data shapes. At minimum, wrap logs with `__DEV__` guards or use a logging library that strips in production builds. + +--- + +### 12. Performance: `setTimeout` Hacks for Async Coordination + +`ATMAssessment.js` has 5 `setTimeout` calls used to sequence async operations. `UnitList.js`, `Contents.js`, `Courses.js` each use `setTimeout` for debouncing. While debouncing is fine, using `setTimeout` to coordinate state updates (e.g., `setTimeout(() => { /* do something after state update */ }, 300)`) is fragile — it assumes a specific rendering time that can fail on slower devices. + +--- + +### Summary by Priority + +| Priority | Issue | +|---|---| +| **P0** | Remove `console.log` from `setDataInStorage` (token leak) | +| **P0** | Delete hardcoded `test1`/`12345` credentials | +| **P0** | Remove active cURL logging with auth tokens | +| **P1** | Add axios request timeouts for poor-network environments | +| **P1** | Fix unreachable `console.error` in `getDataFromStorage` | +| **P1** | Fix `PushNotification` calls in render body | +| **P2** | Unify offline cache coverage across all content APIs | +| **P2** | Delete `LoginScreen.js` or restore it; delete `EndUrls_old.js` | +| **P2** | Fix silent catch blocks to at minimum log errors | +| **P3** | Extract `getHeaders` into a shared utility | +| **P3** | Break up `AuthService.js` by domain | +| **P3** | Replace `RNRestart` on deep link with navigation | \ No newline at end of file diff --git a/code-reviews/program-hardcodings-fixproposals.md b/code-reviews/program-hardcodings-fixproposals.md new file mode 100644 index 00000000..e69de29b diff --git a/code-reviews/program-hardcodings.md b/code-reviews/program-hardcodings.md new file mode 100644 index 00000000..fc12d384 --- /dev/null +++ b/code-reviews/program-hardcodings.md @@ -0,0 +1,119 @@ +Here's a comprehensive report of all program-specific hardcodings found in the codebase: + +--- + +## Program-Specific Hardcodings Found + +### Category 1 — Hardcoded String Literals in Conditionals + +The most widespread issue. Program names and identifiers appear as raw strings in logic, instead of being resolved from server data. + +**`'scp'` as a userType string** — appears in 6+ files: +- `LoginScreen.js:627, 637` +- `ProgramsScreen.js:188, 200` +- `PlpWebViewScreen.js:218+` +- `LanguageScreen.js:229, 261` +- `Assessment/TestResultModal.js:26–28` +- `Profile/NoCertificateBox.js:31` + +**`'youthnet'` as a userType string** — same pattern: +- `LoginScreen.js`, `ProgramsScreen.js`, `PlpWebViewScreen.js`, `Profile.js:353`, `TabScreen.js:93` + +**`'Camp to Club'` as a raw string**: +- `Profile.js:353` — `userType == 'Camp to Club'` + +--- + +### Category 2 — Hardcoded `'Second Chance Program'` Name in Filters + +Three separate files filter tenants by matching the program name as a literal string instead of using a constant: + +```js +// LoginScreen.js:616, ProgramsScreen.js:177, PlpWebViewScreen.js:209 +const scp = tenantDetails + ?.filter((item) => item.name === 'Second Chance Program') + ?.map((item) => item.tenantId); +``` + +`TENANT_DATA` constants exist in `app-constants.js` but are not consistently used. + +--- + +### Category 3 — Program-Specific Navigation Routes + +The app navigates to different tab screens based on program identity: + +```js +navigation.navigate('SCPUserTabScreen'); // SCP-specific +navigation.reset({ routes: [{ name: 'YouthNetTabScreen' }] }); // YouthNet-specific +``` + +Found in: `LoginScreen.js`, `ProgramsScreen.js`, `PlpWebViewScreen.js`, `LanguageScreen.js`, `NoCertificateBox.js`, `ProgramSwitch.js`, `DeepLink.js` + +--- + +### Category 4 — Hardcoded Channel IDs + +SCP-specific content filtering logic using hardcoded channel/framework strings: + +```js +// Courses.js:292, ContinueLearning.js:83 +if (channelId == 'scp-channel') { + mergedFilter.targetBoardIds = ["scp-framework_board_cocurricular"]; +} + +// FilterList.js:692 +if (filteredDefaultFormData.gradeLevel && channelId === 'scp-channel') { + delete filteredDefaultFormData.gradeLevel; +} +``` + +--- + +### Category 5 — Hardcoded Display Name Logic + +```js +// ProgramSwitch.js:620 +{currentUserType === "scp" ? "Second Chance Program" + : currentUserType === "youthnet" ? "Vocational Traning" // ← also a typo + : currentUserType} +``` + +--- + +### Category 6 — Complex Duplicated Routing Logic + +The same program-routing block is copy-pasted across 4+ files (`LoginScreen`, `ProgramsScreen`, `PlpWebViewScreen`, `ProgramSwitch`, `DeepLink`): + +```js +if (scpTenantIds?.includes(tenantId)) { + await setDataInStorage('userType', 'scp'); + navigation.reset({ routes: [{ name: 'SCPUserTabScreen' }] }); +} else if (youthnetTenantIds?.includes(tenantId)) { + await setDataInStorage('userType', 'youthnet'); + ... +} else if (campToClubTenantIds?.includes(tenantId)) { + ... +} +``` + +Every new program requires touching all these files. + +--- + +### Summary Table + +| # | Type | Severity | Files Affected | +|---|------|----------|----------------| +| 1 | `'scp'`/`'youthnet'`/`'Camp to Club'` string literals in conditionals | High | 8+ files | +| 2 | `'Second Chance Program'` literal in filter (not using constant) | Medium | 3 files | +| 3 | Program-specific navigation routes hardcoded | High | 7 files | +| 4 | Hardcoded `scp-channel` and `scp-framework_board_cocurricular` | High | 3 files | +| 5 | Display name ternary with hardcoded strings (+ typo) | Medium | `ProgramSwitch.js` | +| 6 | Routing logic duplicated across 5 files | High | 5 files | + +--- + +### Root Cause + +The app stores a `userType` string (`'scp'`, `'youthnet'`, etc.) in AsyncStorage after login, then branches on this string everywhere. This string is essentially a program-specific enum that has leaked into almost every navigation-critical file. The fix would be to drive behavior from config served by the API (e.g., a tenant capability map) rather than from a hardcoded program identity string. \ No newline at end of file diff --git a/docs/overview/about-pratham.md b/docs/overview/about-pratham.md new file mode 100644 index 00000000..e4800e72 --- /dev/null +++ b/docs/overview/about-pratham.md @@ -0,0 +1,6 @@ +# Overview +Pratham Education Foundation (Pratham) is one of the largest NGOs in India, with a track record of innovative, rigorously evaluated programs that have inspired similar initiatives across three continents. Pratham’s journey began in 1995 in Mumbai with the mission: Every Child in School and Learning Well. From the slums of India’s biggest city to today’s presence in more than 26 states and union territories, our journey has been one of learning and scale—from evidence to practice, from small innovations to systemic reforms. Pratham’s work extends from programs for early and elementary years to initiatives for girls and women, and youth skilling. + +We reach 7 million children and youth in a typical year through a combination of efforts: working directly with children and youth in communities, as well as through collaborations with state and district-level governments. + +From Early Childhood Education to Elementary Education, Pratham identifies education-related gaps and opportunities in each segment and develops context-based solutions. We collaborate with children, schools, families, and communities through direct programs and government partnerships. \ No newline at end of file diff --git a/docs/overview/environment.md b/docs/overview/environment.md new file mode 100644 index 00000000..97b959f9 --- /dev/null +++ b/docs/overview/environment.md @@ -0,0 +1,3 @@ +The users are often in areas of bad network so mobile data access is unsure. + +Also, a lot of users may be using low resource devices \ No newline at end of file diff --git a/docs/overview/programs-scp.md b/docs/overview/programs-scp.md new file mode 100644 index 00000000..8c4873de --- /dev/null +++ b/docs/overview/programs-scp.md @@ -0,0 +1,24 @@ +# Overview +Over the last decade or so, girls have dropped out of school disproportionately due to factors like early marriage, chores at home and cost of education [National Family Health Survey-5 (2019-21) data]. + +Pratham’s experience indicates that once a learner has dropped out of school, it is near impossible for them to find their way back. While India has open schooling opportunities, there is lack of awareness, accessibility, adequate motivation and support for a student to use open schooling as a way to continue their education. + +Fewer people progressing for higher studies, low quality secondary education, an ever-increasing demand of a skilled labour force reinforces the need for an effective intervention in the secondary education space. A secondary school certificate is a prerequisite for entry into higher education, skill apprenticeship and for promotion in jobs. Pratham believes that by making education accessible and closer to home, it is possible to overcome logistical and social challenges and begin to bridge the gender gap in learning and education. + +Understanding the importance of secondary education, especially Grade 10, Pratham has deployed a focused intervention, the Second Chance program, to enable young girls and women to complete their secondary schooling and forge strong pathways for the future. + +Pratham-Jameel Second Chance program focuses on providing school dropouts, especially young girls and women, another chance at education. Second Chance aims to support those who could not complete their secondary education. The program focuses on completion of Grade 10. The academic certificate that they receive opens the door for further opportunities for lifelong learning and growth. The Second Chance program uses innovative teaching methods, to provide accessible learning opportunities very close to where the students live. + +The program prepares and supports students to take the Grade 10 – Secondary Board/Open School examinations. The program runs in two phases. The first phase is called the Foundation Course (FC) which lasts for 3-4 months and focuses on basic foundational concepts of subjects like English, Mathematics, Language (state-specific), Science and Home Science. Assessment in the form of a pre-test at the onset of FC and post-test at the end of FC are conducted to track the learning levels of students and their growth during the course. During this phase, life skills-focused activities are also conducted with the learners, concentrating on enhancement of communication, decision-making and critical thinking skills. + +After the Foundation Course, students move to the Main Course which focuses on helping learners prepare for the Board-mandated curriculum. The progress during the Main Course is tracked through internal assessments. Interactive methods such as activity-based learning, group work and an emphasis on self-learning are the highlights of the course. The Main Course formally ends with the students appearing for the Board Examination. + +Additionally, the program delivers non-academic modules focused on providing learners exposure to available opportunities after Grade 10. + +# Key Outcomes Tracked + +# User Personas + +## Facilitator + +## Learner \ No newline at end of file diff --git a/docs/tech/about-pratham-platform.md b/docs/tech/about-pratham-platform.md new file mode 100644 index 00000000..d4d422ab --- /dev/null +++ b/docs/tech/about-pratham-platform.md @@ -0,0 +1,15 @@ +# Overview +The pratham platform is a multi-tenant software that enabled multiple Pratham programs' digital lifecycle. + +Each tenant in the platform is a program run by Pratham. A few of the programs are +- Second Chance Program (SCP) +- Camp to Club (C2C) +- Pragyanpath + +The platform has several capabilities that are available in all the programs. Key capabilites include +- User management with user types and role based ACL +- Custom user profiles with user fields +- Attendance Marking +- Content & courses creation & consumption +- Assessment creation & consumption +- Issuance of certificates upon completion of courses \ No newline at end of file diff --git a/src/screens/Dashboard/Contents.js b/src/screens/Dashboard/Contents.js index e3e3ed48..53ae41cf 100644 --- a/src/screens/Dashboard/Contents.js +++ b/src/screens/Dashboard/Contents.js @@ -107,11 +107,12 @@ const Contents = () => { course_track_data?.data.find((course) => course.userId === userId) ?.course || []; } - // setTrackData(courseTrackData); - console.log( - '########## courseTrackData', - JSON.stringify(courseTrackData) - ); + // Ensure every content has an entry so card components trigger SQLite check + for (const courseId of contentIdList) { + if (!courseTrackData.some((c) => c.courseId === courseId)) { + courseTrackData.push({ courseId, completed_list: [], in_progress_list: [] }); + } + } setTrackData(courseTrackData); } catch (e) { console.log('Error:', e); diff --git a/src/screens/Dashboard/Courses/CourseContentList.js b/src/screens/Dashboard/Courses/CourseContentList.js index f0d5481d..49da13fb 100644 --- a/src/screens/Dashboard/Courses/CourseContentList.js +++ b/src/screens/Dashboard/Courses/CourseContentList.js @@ -167,8 +167,11 @@ const CourseContentList = ({ route }) => { course_track_data?.data.find((course) => course.userId === userId) ?.course || []; } + // Ensure the current course has an entry so card components trigger SQLite check + if (!courseTrackData.some((c) => c.courseId === course_id)) { + courseTrackData.push({ courseId: course_id, completed_list: [], in_progress_list: [] }); + } - console.log('#### debug progress courseTrackData', courseTrackData); setTrackData(courseTrackData); setLoading(false); // Ensure to stop loading when data fetch completes @@ -277,11 +280,15 @@ const CourseContentList = ({ route }) => { const handleEnroll = async () => { if (!isConnected) { setNetworkstatus(false); + return; } const data = await courseEnroll({ course_id }); if (data?.params?.status === 'successful') { setIsModal(true); setEnrollStatus(true); + // Refresh and persist the enrolled status to SQLite so it + // survives a force-kill and is read correctly when offline. + await fetchEnrollStatus(); } }; diff --git a/src/screens/Dashboard/Courses/Courses.js b/src/screens/Dashboard/Courses/Courses.js index e5ef460f..797bed9f 100644 --- a/src/screens/Dashboard/Courses/Courses.js +++ b/src/screens/Dashboard/Courses/Courses.js @@ -136,7 +136,12 @@ const Courses = ({ route, CopilotStopped, customProp = null }) => { course_track_data?.data.find((course) => course.userId === userId) ?.course || []; } - // setTrackData(courseTrackData); + // Ensure every course has an entry so card components trigger SQLite check + for (const courseId of courseList) { + if (!courseTrackData.some((c) => c.courseId === courseId)) { + courseTrackData.push({ courseId, completed_list: [], in_progress_list: [] }); + } + } setTrackData(courseTrackData); } catch (e) { console.log('Error:', e); diff --git a/src/screens/Dashboard/Courses/UnitList.js b/src/screens/Dashboard/Courses/UnitList.js index f325e8b5..2b45d0a7 100644 --- a/src/screens/Dashboard/Courses/UnitList.js +++ b/src/screens/Dashboard/Courses/UnitList.js @@ -78,12 +78,7 @@ const UnitList = ({ route }) => { // console.log('############ in focus unit list'); setLoading(true); - //bug fix for not realtime tracking - //fetchDataTrack(); - setTimeout(() => { - // Code to run after 1 second - fetchDataTrack(); - }, 1000); // 1000 milliseconds = 1 second + fetchDataTrack(); // Clean up the event listener on component unmount return () => { diff --git a/src/screens/PlayerScreen/StandAlonePlayer/StandAlonePlayer.js b/src/screens/PlayerScreen/StandAlonePlayer/StandAlonePlayer.js index 522fbcbe..6b2eccf7 100644 --- a/src/screens/PlayerScreen/StandAlonePlayer/StandAlonePlayer.js +++ b/src/screens/PlayerScreen/StandAlonePlayer/StandAlonePlayer.js @@ -55,6 +55,7 @@ import { storeAsessmentOffline, storeTelemetryOffline, storeTrackingOffline, + deleteTrackingOfflineByRecord, } from '../../../utils/API/AuthService'; import TestResultModal from '../../Assessment/TestResultModal'; import MimeAlertModal from '../../../components/MimeAletModal/MimeAlertModal'; @@ -437,7 +438,7 @@ const StandAlonePlayer = ({ route }) => { ]; await storeData('contentEidEND', contentEidEND, 'json'); //check if exit button pressed - fetchExitData(); + await fetchExitData(); navigation.goBack(); } //check telemetry @@ -496,7 +497,7 @@ const StandAlonePlayer = ({ route }) => { // console.log('####################'); //check if exit button pressed if (data_obj?.eid == 'HEARTBEAT' && data_obj?.edata?.type == 'EXIT') { - fetchExitData(); + await fetchExitData(); navigation.goBack(); } } @@ -1300,11 +1301,17 @@ const StandAlonePlayer = ({ route }) => { //event when player closed useFocusEffect( useCallback(() => { + let isExiting = false; const onBackPress = () => { - // Handle back button press + if (isExiting) return true; // already saving — ignore repeated presses + isExiting = true; console.log('useFocusEffect Back button pressed or screen closed'); - fetchExitData(); - return false; // Return true if you want to block the back action, false to allow it + // fetchExitData now completes after the fast SQLite write, so the wait + // is imperceptible. Navigate back only after it resolves. + fetchExitData().finally(() => { + navigation.goBack(); + }); + return true; // block default back; we handle navigation above }; BackHandler.addEventListener('hardwareBackPress', onBackPress); @@ -1450,38 +1457,47 @@ const StandAlonePlayer = ({ route }) => { if (detailsObject && detailsObject.length > 0) { console.log('reached here', JSON.stringify(detailsObject)); - try { - let create_tracking = await contentTracking( - userId, - courseId, - contentId, - contentType, - contentMime, - lastAccessOn, - detailsObject, - unitId - ); - console.log('create_tracking', create_tracking); - if (create_tracking && create_tracking?.response?.responseCode == 201) { - console.log('saved data'); - } else { - console.log('no internet available'); - //store result in offline mode - await storeTrackingOffline( - userId, - courseId, - contentId, - contentType, - contentMime, - lastAccessOn, - detailsObject, - unitId - ); - } - logEvent(); - } catch (e) { - console.log(e); - } + // Always write to SQLite first so progress is visible the moment the + // user returns to UnitList, regardless of network speed. + await storeTrackingOffline( + userId, + courseId, + contentId, + contentType, + contentMime, + lastAccessOn, + detailsObject, + unitId + ); + + // Fire API in background. If it succeeds, delete the SQLite record so + // SyncCard doesn't re-submit it and create a duplicate server entry. + // If it fails, the SQLite record stays and SyncCard will sync it later. + contentTracking( + userId, + courseId, + contentId, + contentType, + contentMime, + lastAccessOn, + detailsObject, + unitId + ) + .then((create_tracking) => { + if (create_tracking?.response?.responseCode == 201) { + deleteTrackingOfflineByRecord( + userId, + courseId, + contentId, + lastAccessOn + ); + } + }) + .catch((e) => { + console.log('contentTracking error, will sync via SyncCard', e); + }); + + logEvent(); } await removeData('contentEidSTART'); @@ -1561,9 +1577,9 @@ const StandAlonePlayer = ({ route }) => { { + onPress={async () => { Orientation.lockToPortrait(); - fetchExitData(); + await fetchExitData(); navigation.goBack(); }} activeOpacity={0.8} diff --git a/src/utils/API/AuthService.js b/src/utils/API/AuthService.js index e818db18..d53f8325 100644 --- a/src/utils/API/AuthService.js +++ b/src/utils/API/AuthService.js @@ -1391,6 +1391,23 @@ export const deleteTrackingOffline = async (id) => { console.log(e); } }; +export const deleteTrackingOfflineByRecord = async ( + user_id, + course_id, + content_id, + lastAccessOn +) => { + try { + await deleteData({ + tableName: 'Tracking_Offline_2', + where: { user_id, course_id, content_id, lastAccessOn }, + }) + .then(() => console.log('deleted synced tracking record from SQLite')) + .catch((err) => console.error('deleteTrackingOfflineByRecord err', err)); + } catch (e) { + console.log(e); + } +}; export const getSyncTrackingOfflineCourse = async (user_id, course_id) => { try { //get result