-
Notifications
You must be signed in to change notification settings - Fork 345
refactor(nus-api): migrate to new vendor #4253
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4253 +/- ##
==========================================
+ Coverage 54.52% 56.60% +2.07%
==========================================
Files 274 297 +23
Lines 6076 6924 +848
Branches 1455 1671 +216
==========================================
+ Hits 3313 3919 +606
- Misses 2763 3005 +242 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
5ae9972 to
4da03bd
Compare
0bb413d to
1adf54c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR refactors the NUS API scraper to migrate from the old POST-based API endpoints to new RESTful GET-based endpoints with updated authentication using multiple API keys. The changes maintain backward compatibility in the output format while adapting to new internal data structures.
Key changes include:
- Refactored API service layer from POST to GET requests with query parameters and separate API keys for different services
- Updated internal TypeScript types to match new API response field names and structures
- Added pagination support for large datasets and HTML sanitization utilities
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
utils/test-utils.ts |
Extended omitted keys in module equality tests to exclude aliases and attributes fields |
utils/data.ts |
Added utility functions for HTML tag stripping and string cleaning |
utils/api.ts |
Added term-to-API parameter mapping and module info sanitization functions |
types/modules.ts |
Added FacultyCode type definition |
types/api.ts |
Restructured ModuleInfo type with new field names and added/removed fields to match new API |
tasks/GetSemesterTimetable.ts |
Updated comment to reflect streaming approach instead of per-department requests |
tasks/GetSemesterModules.ts |
Changed from department-based to faculty-based module fetching with updated logging |
tasks/GetSemesterData.ts |
Updated field mappings to transform new API format, modified attribute structure handling |
tasks/GetFacultyDepartment.ts |
Added hardcoded entry for "Non-Faculty-based Departments" faculty |
tasks/DataPipeline.ts |
Updated comment from "department" to "faculty" |
tasks/DataPipeline.test.ts |
Updated mock data structure to match new API response format |
services/nus-api.ts |
Major refactor: POST→GET requests, separate API keys per service, pagination implementation |
services/nus-api.test.ts |
Updated tests to use GET instead of POST and test new API methods |
services/io/elastic.ts |
Added early return guard for empty bulk operations |
services/__mocks__/nus-api.ts |
Removed deprecated getModuleInfo and getDepartmentModules methods |
config.ts |
Added new API key fields, changed academic year to 2024/2025 |
__mocks__/config.ts |
Added mock values for new API key fields |
env.example.json |
Added new required API key fields to example configuration |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| getFacultyModules = async (term: string, facultyCode: FacultyCode): Promise<ModuleInfo[]> => | ||
| this.callModulesEndpoint(term, { acadGroupCode: facultyCode.slice(0, 3) }); |
Copilot
AI
Dec 29, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The faculty code slicing .slice(0, 3) appears to assume all faculty codes are at least 3 characters long. If a faculty code shorter than 3 characters is passed, slice will return the full string, but this could lead to unexpected behavior. Consider adding validation to ensure faculty codes are the expected length, or document this assumption.
| getFacultyModules = async (term: string, facultyCode: FacultyCode): Promise<ModuleInfo[]> => | |
| this.callModulesEndpoint(term, { acadGroupCode: facultyCode.slice(0, 3) }); | |
| getFacultyModules = async (term: string, facultyCode: FacultyCode): Promise<ModuleInfo[]> => { | |
| const acadGroupCode = facultyCode.length >= 3 ? facultyCode.slice(0, 3) : facultyCode; | |
| return this.callModulesEndpoint(term, { acadGroupCode }); | |
| }; |
Migrate to New NUS API
Motivation
NUS updated their API endpoints with new authentication (API keys), RESTful GET endpoints, updated response formats, and pagination support. This is an internal refactor - the JSON output structure remains unchanged. The scraper now adapts to the new API while maintaining backward compatibility with existing consumers.
Strategy
Key Areas
API Service (
nus-api.ts)ttApiKey,courseApiKey,acadApiKey,acadAppKey)callApiinto base function andcallV1ApiwrapperType Definitions (
api.ts)ModuleInfotype with new field names (CourseTitle→Title,Subject→SubjectArea)OrganisationCode,UnitsMin/Max)PrintCatalogfieldModuletype unchanged (except internal field mappings)Tasks
GetSemesterModules: Department-based → faculty-based fetchingGetSemesterData: Updated field mappings to transform new API format to existing output structureGetFacultyDepartment: Updated terminologyUtilities (
utils/api.ts)mapTermToApiParamsfor term code conversionsanitizeModuleInfofor HTML cleaningConfiguration
env.example.jsonwith new required keys