Misaligned module code in Prerequisite Tree

[Jiwei99]

Describe the bug

Some module code in the Pre-Requisite Tree is misaligned.

To Reproduce

Steps to reproduce the behavior:

Layout of FIN2704 is different from rest of the modules
https://nusmods.com/courses/FIN3716/financial-modelling

Module codes are not aligned properly, and the alignment is inconsistent
https://nusmods.com/courses/BSP3701A/strategic-management

Desktop (please complete the following information):

• OS: [MacOS]
• Browser [Chrome, Safari]

[zehata]

Looks like it's caused by FIN2704 being an available module, which wraps it in a button that is invisible

[zehata]

FIN2704 is an active mod that links to FIN2704/FIN2704X, so getModuleCondensed("FIN2704") is truthy but moduleActive for FIN2704 is falsey (name is not exact match). This causes the node to be a clickable button that appears invisible.

Fix is trivial but UX requires consideration, there are a few options:

1. Remove the button, keep the text. Preferred for consistency.
   
2. Keep the button, and make it visible
   
3. Keep the button, keep it invisible(??) Really strange UI
   

[Jiwei99]

@zehata What if "Course starting with" is replaced with an enumeration of all versions of the module using a "one of" branch? Think that would be more aligned with the design of the rest of the prerequisites trees (but not sure how feasible is this)

[zehata]

@Jiwei99 Something like this? I agree with you that this solution seems the cleanest, since it is more consistent with mods that don't use wildcard prerequisites.

However, I noticed that if the required grade is not D, the UI will look this: (not the prettiest)

Update: looks like doing it this way will also fix a previously unfound bug - if required grade is not D, the current UI will not show it because the prefix get overridden:

nusmods/website/src/views/modules/ModuleTree.tsx

Lines 56 to 67 in 0f33a55

	if (res.name.includes(GRADE_REQUIREMENT_SEPARATOR)) {
	const [moduleName, requiredGrade] = res.name.split(GRADE_REQUIREMENT_SEPARATOR);
	if (requiredGrade !== PASSING_GRADE) {
	res.prefix = `Minimally ${requiredGrade} for`;
	}
	res.name = moduleName;
	}
	if (res.name.includes(MODULE_NAME_WILDCARD)) {
	const [beforeWildcard, afterWildcard] = res.name.split(MODULE_NAME_WILDCARD);
	res.prefix = 'Course starting with';
	res.name = `"${beforeWildcard}" ${afterWildcard}`;
	}

[zehata]

Alternatively:

[Jiwei99]

Alternatively:

Yeah I think this looks the cleanest, but should the grade requirement be shown? I don’t remember seeing it in the current prereq tree

[zehata]

The grade requirement is only shown when it is not the default 'D' grade.

[zehata]

I think it is an edge case in which prerequisites don't use the default 'D' passing grade AND use the "Courses starting with" pattern. So I think that is not of priority and could be fixed with a separate PR.

[Jiwei99]

Alternatively:

Ah, then I would think this looks the best. The other version with the prefix in the module button looks too chunky

[zehata]

It is possible to do this, but it's possible that situations like this will happen, which might appear messy to some people? (I think it's fine though). If it's possible, I hope someone from the core team can weigh in on this UI. (The tree below is just test data)

[zehata]

Also @ core team, an alternative is to update ANTLR to branch when encountering wildcard modules instead of appending % when scraping, which is a cleaner implementation but will also be more complex computationally on the server. I think practically speaking this frontend implementation is sufficient.

[ravern]

but will also be more complex computationally on the server.

I think this is fine -- we only run the scraper once every hour.

I can see this working by modifying this parse function to build up a trie of modules (opposed to a list) and then passing that to the parseString function, which can then expand modules it comes across. Alternatively we can add a new function expandWildcards after the parseString function call.

nusmods/scrapers/nus-v2/src/services/requisite-tree/index.ts

Lines 27 to 48 in 0f33a55

	function parse(data: ModuleWithoutTree[], subLogger: Logger): PrereqTreeMap {
	const results: PrereqTreeMap = {};
	for (const module of data) {
	const { moduleCode, prerequisiteRule: value } = module;
	if (
	// Filter out empty values
	value
	) {
	const moduleLog = subLogger.child({ moduleCode });
	const parsedValue = parseString(value, moduleLog);
	if (parsedValue) {
	results[module.moduleCode] = parsedValue;
	}
	}
	}
	return results;
	}

If we decide we want to expand wildcards, I'm slightly in favour of going the backend/scraper route, but would love to hear input from the other maintainers @nusmodifications/nusmods-developers.

---

But going back to earlier discussion, do we want to expand these "courses starting with" in the first place? I'm afraid that it might generate huge and unwieldy pre-req trees in some cases.

If anyone has some time, could we perhaps get a sense of what's the largest pre-req tree this could potentially generate (in terms of number of rows/lines)? Probably by writing a quick script that runs across all the modules in moduleList.json and expands everything.

---

I like this UI actually. Just have a question on the second (turquoise) branch where it says "one of" but only has one module. Is this just bad test data for the screenshot or is this a possible scenario?