Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
152 changes: 152 additions & 0 deletions code-reviews/general-1004.md
Original file line number Diff line number Diff line change
@@ -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 <token>` 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 |
Empty file.
119 changes: 119 additions & 0 deletions code-reviews/program-hardcodings.md
Original file line number Diff line number Diff line change
@@ -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.
6 changes: 6 additions & 0 deletions docs/overview/about-pratham.md
Original file line number Diff line number Diff line change
@@ -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.
3 changes: 3 additions & 0 deletions docs/overview/environment.md
Original file line number Diff line number Diff line change
@@ -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
24 changes: 24 additions & 0 deletions docs/overview/programs-scp.md
Original file line number Diff line number Diff line change
@@ -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
15 changes: 15 additions & 0 deletions docs/tech/about-pratham-platform.md
Original file line number Diff line number Diff line change
@@ -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
11 changes: 6 additions & 5 deletions src/screens/Dashboard/Contents.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
9 changes: 8 additions & 1 deletion src/screens/Dashboard/Courses/CourseContentList.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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();
}
};

Expand Down
7 changes: 6 additions & 1 deletion src/screens/Dashboard/Courses/Courses.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
Loading