Add support for year-long modules to module planner #2913

jeffsieu · 2020-10-17T16:40:12Z

Context

Resolves #1912

Implementation

Before adding

After adding

Year long modules are represented as modules in semester 0.

Adding year-long modules

Year-long modules are moved to the year-long module list whenever added from Semester 1 or 2. The year-long semester is hidden unless it is not empty, or being dragged over.

Dragging modules

Semester-long modules can only be dragged between regular semesters, exemptions and plan-to-take semesters.
Year-long modules can only be dragged between year-long semesters, exemptions and plan-to-take semesters.

Upon dragging, their type (year-long or semester-long) is stored as part of their draggableId, which is then used to update PlannerContainer.state.draggedModuleType. This prompts children PlannerYear components to display their year-long semester containers as drag targets.

Misc

"+ Add modules" section removed to avoid clutter. Also avoids the case where non year-long modules are added from the year-long "add modules" section
Adds type attribute to Droppable targets to disallow dragging of modules to the lists of the wrong module type.
Some test cases are modified to due to the extra "semester".

Represents year long modules as modules in semester 0. Whenever new modules are added, they are checked for whether they are year long modules. If they are, they are moved to the year long "semester".

codecov · 2020-10-17T16:43:44Z

Codecov Report

Merging #2913 (4ce415e) into master (1cc25c8) will increase coverage by 0.16%.
The diff coverage is 43.10%.

@@            Coverage Diff             @@
##           master    #2913      +/-   ##
==========================================
+ Coverage   55.40%   55.57%   +0.16%     
==========================================
  Files         254      255       +1     
  Lines        5315     5357      +42     
  Branches     1226     1232       +6     
==========================================
+ Hits         2945     2977      +32     
- Misses       2370     2380      +10

Impacted Files	Coverage Δ
website/src/views/planner/AddModule.tsx	`0.00% <0.00%> (ø)`
...iews/planner/PlannerContainer/PlannerContainer.tsx	`0.00% <0.00%> (ø)`
website/src/views/planner/PlannerModule.tsx	`0.00% <0.00%> (ø)`
website/src/views/planner/PlannerModuleSelect.tsx	`0.00% <0.00%> (ø)`
website/src/views/planner/PlannerSemester.tsx	`0.00% <0.00%> (ø)`
website/src/views/planner/PlannerYear.tsx	`0.00% <0.00%> (ø)`
website/src/utils/planner.ts	`69.09% <71.42%> (+0.34%)`	⬆️
website/src/actions/planner.ts	`85.18% <100.00%> (+1.85%)`	⬆️
website/src/reducers/planner.ts	`90.90% <100.00%> (+2.19%)`	⬆️
website/src/selectors/planner.ts	`91.56% <100.00%> (+0.10%)`	⬆️
... and 18 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1cc25c8...4e02420. Read the comment docs.

nusmods-deploy-bot · 2020-10-17T16:44:47Z

Deployment preview for 4ce415ee ready at http://5fec0a3d2065cf8eb2996650--nusmods-deploy-preview.netlify.app

ZhangYiJiang

This looks great! I do think we might want to change the data structure from the scraper to allow us to do this more naturally. Let me know if you need help with that

website/src/utils/planner.ts

website/src/views/planner/PlannerYear.tsx

website/src/views/planner/PlannerModule.tsx

website/src/utils/modules.ts

website/src/utils/planner.ts

website/src/views/planner/PlannerContainer/PlannerContainer.tsx

…eu/nusmods into planner-year-long-support

vercel · 2020-12-30T04:59:47Z

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployments, click below or on the icon next to each commit.

nusmods-website – ./website

🔍 Inspect: https://vercel.com/nusmodifications/nusmods-website/f9tn1djmd
✅ Preview: https://nusmods-website-git-fork-jeffsieu-planner-year-long-support.nusmodifications.vercel.app

nusmods-export – ./export

🔍 Inspect: https://vercel.com/nusmodifications/nusmods-export/ckvmvsxzp
✅ Preview: https://nusmods-export-git-fork-jeffsieu-planner-year-long-support.nusmodifications.vercel.app

jeffsieu · 2020-12-30T05:14:00Z

@ZhangYiJiang How should I proceed with this PR? I really wish to see these changes be incorporated.

ZhangYiJiang

Sorry for not coming back to review this. This looks almost ready - just a few things

The reducers and actions that are now unused should be removed
Semester 0 should not be a valid global semester value, instead we can add something specifically for the planner
Drag and drop ID and module type should be strongly typed instead of just being strings

website/src/views/planner/PlannerContainer/PlannerContainer.tsx

scrapers/nus-v2/src/tasks/CollateModules.ts

website/src/utils/planner.ts

website/src/views/planner/PlannerYear.tsx

ZhangYiJiang · 2020-12-30T19:25:54Z

website/src/views/planner/PlannerModuleSelect.tsx

@@ -121,7 +121,7 @@ export function PlannerModuleSelectComponent({
                    } else if (inputValue != null) {
                      // Otherwise we use the input value - this allows the user to
                      // enter multiple
-                      onSelect(inputValue);
+                      onSelect(modules.find((module) => module.moduleCode === inputValue) ?? null);


This would break existing functionality where this field allows multiple module codes to be entered at once here

nusmods/website/src/views/planner/AddModule.tsx

Line 133 in 3e84401

Tip: You can add multiple module at once, eg. copy from your transcript

The logic for validating module code should be on the parent component, not the child.

onSelect did have incorrect typing - ModuleCode should just be string, sorry about that

Actually, how does the adding multiple module codes part work? Since onSelect(inputValue) seems to call this:

nusmods/website/src/views/planner/AddModule.tsx

Lines 60 to 69 in 486cca0

onSelectModule = (input: ModuleCode | null) => {

if (input) {

this.props.onAddModule({

type: 'module',

moduleCode: input.trim(),

});

}

this.onCancel();

};

which seems to add only 1 module

It's in this function

nusmods/website/src/views/planner/PlannerContainer/PlannerContainer.tsx

Line 93 in 4ba9c9b

onAddModule = (year: string, semester: Semester, module: AddModuleData) => {

. Again the types are wrong and the name is a bit misleading - moduleCode is matched to search for anything that looks like a module code

@ZhangYiJiang I changed the logic so that the module code extraction is done in PlannerModuleSelect, and PlannerContainer.onAddModule now only handles a single module at a time. i.e. AddModuleData's type has also been modified. This decision was made because I needed the yearLong property of every module. And PlannerModuleSelect does have a reference to modules from which I can extract ModuleCondensed instances out of.

website/src/views/planner/PlannerSemester.tsx

PlannerModuleSemester represents a semester in the module planner. PlannerSemesterModuleType represents the module type accepted by a PlannerSemester (semester or year long or both).

vercel · 2021-01-01T09:06:59Z

@jeffsieu is attempting to deploy a commit to the NUSMods Team on Vercel.

A member of the Team first needs to authorize it.

Co-authored-by: Zhang Yi Jiang <[email protected]>

…eu/nusmods into planner-year-long-support

ZhangYiJiang

Looking very close! Please revert some of the changes that are outdated, run the linter, and avoid using cast in favor of type guards, and this PR should be good to go

website/src/types/modules.ts

website/src/selectors/planner.test.ts

website/src/utils/planner.ts

website/src/selectors/planner.ts

Co-authored-by: Zhang Yi Jiang <[email protected]>

…eu/nusmods into planner-year-long-support

jeffsieu · 2021-01-17T17:43:50Z

@ZhangYiJiang Most issues should be resolved. There's currently still a circular dependency between types/planner.ts and types/reducers.ts.... I'm not sure how to resolve this without shifting the type declarations over from a file to the other.

yangshun · 2023-07-03T06:51:57Z

Guys, merge this plox

Add support for year-long modules to module planner

22ca05d

Represents year long modules as modules in semester 0. Whenever new modules are added, they are checked for whether they are year long modules. If they are, they are moved to the year long "semester".

jeffsieu marked this pull request as draft October 17, 2020 16:51

Fix module planner dragging

1029b35

jeffsieu marked this pull request as ready for review October 17, 2020 18:51

jeffsieu added 5 commits October 18, 2020 03:07

Fix dragging planner modules to bin

725a1e9

Add test cases for isYearLong and isYearLongModule

3cb6eb0

Merge branch 'master' into planner-year-long-support

958a8aa

Merge branch 'master' into planner-year-long-support

e517ba1

Merge branch 'master' into planner-year-long-support

a486808

ZhangYiJiang requested changes Oct 20, 2020

View reviewed changes

jeffsieu added 6 commits October 22, 2020 16:20

Remove extraneous code

adf1ab5

Merge branch 'planner-year-long-support' of https://github.com/jeffsi…

2692430

…eu/nusmods into planner-year-long-support

Merge branch 'master' into planner-year-long-support

527b8e7

Include ModuleCondensed.yearLong in scraper

8a13d84

Merge branch 'planner-year-long-support' of https://github.com/jeffsi…

f10cd31

…eu/nusmods into planner-year-long-support

Merge branch 'master' into planner-year-long-support

d59b7cf

jeffsieu requested a review from ZhangYiJiang October 24, 2020 08:02

jeffsieu added 4 commits October 30, 2020 15:26

Merge branch 'master' into planner-year-long-support

cec521f

Merge branch 'master' into planner-year-long-support

814f828

Merge branch 'master' into planner-year-long-support

70bc84c

Merge branch 'master' into planner-year-long-support

4ce415e

vercel bot had a problem deploying to Preview – nusmods-website December 30, 2020 04:59 Failure

vercel bot deployed to Preview – nusmods-export December 30, 2020 04:59 View deployment

ZhangYiJiang reviewed Dec 30, 2020

View reviewed changes

Add PlannerModuleSemester, ModuleType

88284f2

PlannerModuleSemester represents a semester in the module planner. PlannerSemesterModuleType represents the module type accepted by a PlannerSemester (semester or year long or both).

jeffsieu and others added 5 commits January 1, 2021 17:08

Update scrapers/nus-v2/src/tasks/CollateModules.ts

5e7574e

Co-authored-by: Zhang Yi Jiang <[email protected]>

Add workaround comment in PlannerYear

a356999

Merge branch 'planner-year-long-support' of https://github.com/jeffsi…

7215841

…eu/nusmods into planner-year-long-support

Module planner: Fix multiple module code entry

96f1567

Merge branch 'master' into planner-year-long-support

dd3c64b

vercel bot deployed to Preview – nusmods-website January 7, 2021 01:43 View deployment

vercel bot deployed to Preview – nusmods-export January 7, 2021 01:43 View deployment

Merge branch 'master' into planner-year-long-support

03edd12

vercel bot deployed to Preview – nusmods-export January 8, 2021 02:56 View deployment

vercel bot deployed to Preview – nusmods-website January 8, 2021 02:56 View deployment

jeffsieu requested a review from ZhangYiJiang January 8, 2021 02:56

ZhangYiJiang reviewed Jan 12, 2021

View reviewed changes

website/src/types/modules.ts Outdated Show resolved Hide resolved

website/src/selectors/planner.test.ts Outdated Show resolved Hide resolved

website/src/utils/planner.ts Outdated Show resolved Hide resolved

website/src/selectors/planner.ts Show resolved Hide resolved

Merge branch 'master' into planner-year-long-support

483e0e8

vercel bot deployed to Preview – nusmods-export January 17, 2021 15:45 View deployment

vercel bot deployed to Preview – nusmods-website January 17, 2021 15:45 View deployment

Change isSemester to type guard

f79e34e

Co-authored-by: Zhang Yi Jiang <[email protected]>

vercel bot deployed to Preview – nusmods-website January 17, 2021 15:47 View deployment

vercel bot deployed to Preview – nusmods-export January 17, 2021 15:47 View deployment

jeffsieu added 2 commits January 18, 2021 01:37

Fix lint issues

3996493

Merge branch 'planner-year-long-support' of https://github.com/jeffsi…

4e02420

…eu/nusmods into planner-year-long-support

vercel bot deployed to Preview – nusmods-export January 17, 2021 17:41 View deployment

vercel bot deployed to Preview – nusmods-website January 17, 2021 17:41 View deployment

seanlowjk mentioned this pull request Aug 9, 2021

Improve support for year long modules in the timetable #1911

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for year-long modules to module planner #2913

Add support for year-long modules to module planner #2913

jeffsieu commented Oct 17, 2020 •

edited

Loading

codecov bot commented Oct 17, 2020 •

edited

Loading

nusmods-deploy-bot bot commented Oct 17, 2020 •

edited

Loading

ZhangYiJiang left a comment

vercel bot commented Dec 30, 2020 •

edited

Loading

jeffsieu commented Dec 30, 2020

ZhangYiJiang left a comment

ZhangYiJiang Dec 30, 2020

jeffsieu Jan 1, 2021

ZhangYiJiang Jan 3, 2021

jeffsieu Jan 7, 2021

vercel bot commented Jan 1, 2021

ZhangYiJiang left a comment

jeffsieu commented Jan 17, 2021

yangshun commented Jul 3, 2023

	onSelectModule = (input: ModuleCode \| null) => {
	if (input) {
	this.props.onAddModule({
	type: 'module',
	moduleCode: input.trim(),
	});
	}

	this.onCancel();
	};

Add support for year-long modules to module planner #2913

Are you sure you want to change the base?

Add support for year-long modules to module planner #2913

Conversation

jeffsieu commented Oct 17, 2020 • edited Loading

Context

Implementation

Before adding

After adding

Adding year-long modules

Dragging modules

Misc

codecov bot commented Oct 17, 2020 • edited Loading

Codecov Report

nusmods-deploy-bot bot commented Oct 17, 2020 • edited Loading

ZhangYiJiang left a comment

Choose a reason for hiding this comment

vercel bot commented Dec 30, 2020 • edited Loading

nusmods-website – ./website

nusmods-export – ./export

jeffsieu commented Dec 30, 2020

ZhangYiJiang left a comment

Choose a reason for hiding this comment

ZhangYiJiang Dec 30, 2020

Choose a reason for hiding this comment

jeffsieu Jan 1, 2021

Choose a reason for hiding this comment

ZhangYiJiang Jan 3, 2021

Choose a reason for hiding this comment

jeffsieu Jan 7, 2021

Choose a reason for hiding this comment

vercel bot commented Jan 1, 2021

ZhangYiJiang left a comment

Choose a reason for hiding this comment

jeffsieu commented Jan 17, 2021

yangshun commented Jul 3, 2023

jeffsieu commented Oct 17, 2020 •

edited

Loading

codecov bot commented Oct 17, 2020 •

edited

Loading

nusmods-deploy-bot bot commented Oct 17, 2020 •

edited

Loading

vercel bot commented Dec 30, 2020 •

edited

Loading