Add support for MPE attributes #3241
Conversation
|
This pull request is being automatically deployed with Vercel (learn more). nusmods-website – ./website🔍 Inspect: https://vercel.com/nusmodifications/nusmods-website/9L2Yw3vfeZZ59W95MBMqxoGYPAKn nusmods-export – ./export🔍 Inspect: https://vercel.com/nusmodifications/nusmods-export/Gh9BShhSDTGBFatQiWRLn6K6wZ8g |
Codecov Report
@@ Coverage Diff @@
## master #3241 +/- ##
==========================================
+ Coverage 55.35% 55.37% +0.01%
==========================================
Files 255 255
Lines 5320 5320
Branches 1228 1228
==========================================
+ Hits 2945 2946 +1
+ Misses 2375 2374 -1
Continue to review full report at Codecov.
|
|
Deployment preview for |
| @@ -54,6 +54,7 @@ const attributeMap: { [attribute: string]: keyof NUSModuleAttributes } = { | |||
| LABB: 'lab', | |||
| ISM: 'ism', | |||
| HFYP: 'fyp', | |||
| // MPE handled separately as it maps to multiple attributes | |||
There was a problem hiding this comment.
We could add a MPE map to make this a bit more declarative, what do you think?
const mpeValueMap: { [attribute: string]: (keyof NUSModuleAttributes)[] } = {
S1: ['mpes1'],
S2: ['mpes2'],
'S1&S2': ['mpes1', 'mpes2'],
};
// ...
if (entry.CourseAttribute === 'MPE') {
const mappedAttributes = mpeValueMap[entry.CourseAttributeValue];
if (!mappedAttributes) {
logger.warn(
{ value: entry.CourseAttributeValue, key: entry.CourseAttribute },
'Non-standard course attribute value',
);
} else {
for (const attr of mappedAttributes) {
nusAttributes[attr] = true;
}
}
continue;
}
|
@taneliang NUS have updated their modules with the new attributes. We can now test this PR with the following values: Available for Sem 1
Available for Sem 2
Available for Sem 1 & 2
|
|
Tested locally, seems to work.
|
|
Merging without approval due to deadlines |
Context
#3035
Following this email sent from NUSIT to @chuabingquan:
This PR adds support for the MPE course attribute to /scraper and /website.
cc @alissayarmantho
Implementation
Though the MPE attribute has multiple possible values, I've flattened them into multiple separate boolean attributes in our API. Definitely not an ideal solution, but I think our Elasticsearch index expects the attributes to be a list of strings.
Other Information
Not tested because it seems like no module in the NUS API has the MPE course attribute yet, so the new code isn't executed. @chuabingquan @alissayarmantho do yall know when NUS plans to add these attributes?