Implement 404 status code to retrieveCourse

[a1-su]

Proposed Changes

This PR adds a 404 status code return to retrieveCourse function in Controllers/Courses.hs when the course is not found (in addition to the corresponding 200 status code return when it is).

It additionally modifies the tests to include the added status codes, and updates the front-end functions that use the route to the change.

Type of Change

(Write an X or a brief description next to the type or types that best describe your changes.)

Type	Applies?
🚨 Breaking change (fix or feature that would cause existing functionality to change)	X
✨ New feature (non-breaking change that adds functionality)
🐛 Bug fix (non-breaking change that fixes an issue)
🎨 User interface change (change to user interface; provide screenshots)
♻️ Refactoring (internal change to codebase, without changing functionality)
🚦 Test update (change that only adds or modifies tests)
📦 Dependency update (change that updates a dependency)
🔧 Internal (change that only affects developers or continuous integration)	X

Checklist

(Complete each of the following items for your pull request. Indicate that you have completed an item by changing the [ ] into a [x] in the raw text, or by clicking on the checkbox in the rendered description on GitHub.)

Before opening your pull request:

• I have performed a self-review of my changes.
  • Check that all changed files included in this pull request are intentional changes.
  • Check that all changes are relevant to the purpose of this pull request, as described above.
• I have added tests for my changes, if applicable.
  • This is required for all bug fixes and new features.
• I have updated the project documentation, if applicable.
  • This is required for new features.
• If this is my first contribution, I have added myself to the list of contributors.
• I have updated the project Changelog (this is required for all changes).

After opening your pull request:

• I have verified that the CircleCI checks have passed.
• I have requested a review from a project maintainer.

Questions and Comments

(Include any questions or comments you have regarding your changes.)

1. When initially trying to commit, I ran into the issue of an unused variable error, for TIMEOUT_NAMES_ENUM, from eslint in the Graph.js file on the front-end. When I checked the original commit I wasn't sure as to how the change had originally passed eslint.

   courseography/js/components/graph/Graph.js

   Lines 25 to 27 in 7d974d2

   	const TIMEOUT_NAMES_ENUM = {
   	INFOBOX: 0
   	}

   From checking the previous commits on master, the only changes between the commit that added this change and now were from dependabot, which to my knowledge didn't affect the eslint configuration. I unfortunately wasn't able to separate the unused variable removal and 404 status code changes into separate commits since committing my main changes without removing the unused variable meant I ran into the same eslint error, so if this change should have been made in a separate pull request I would be happy to fix it.

2. In addition to modifying the front-end function, getCourse in components/common/utils.js, I modified the functions that called this utility since from what I gathered they did not handle the not found case correctly, although since this was not in the original description let me know if I should revert it.

   courseography/js/components/common/react_modal.js.jsx

   Lines 103 to 112 in 5fe4f9b

   	getCourse(this.state.courseId).then(course => {
   	if (!course) {
   	this.setState({
   	course: {},
   	sessions: {},
   	courseTitle: "Course Not Found",
   	})
   	console.error(`Course with code ${this.state.courseId} not found`)
   	return
   	}

   courseography/js/components/grid/course_panel.js.jsx

   Lines 153 to 165 in 5fe4f9b

   	getCourse(props.courseCode).then(data => {
   	const course = {
   	courseCode: "",
   	F: [],
   	S: [],
   	Y: [],
   	}
   	if (!data) {
   	setCourseInfo(course)
   	console.error(`Course with code ${props.courseCode} not found`)
   	return
   	}

   Originally, if the course was not found, it would also return null, which ostensibly causes an error should the old functions try to access a key in the response body. It instead now returns a default value and logs the error on the console; I'm not completely sure if this standard for this project, since from what I saw similar functions on the front-end either did not check error conditions or have a need to in the first place.

3. I've added the status code itself to the tests for the retrieveCourse function, although no other test cases in the application follow a similar pattern, either because they do not return a response with a status code, or because the only status code that they return is 200 so it would be redundant to test it. I was uncertain if this was part of the requested changes, so feel free to let me know should it need to be reverted.

4. To my understanding, this change is a breaking change since it changes the API contract for the retrieveCourse function used by the /course endpoint in the application, but if this is mislabelled I am happy to fix it.

[coveralls]

Pull Request Test Coverage Report for Build 21703d1f-38f6-41e1-a1a0-89ca8dc43624

Details

• 7 of 16 (43.75%) changed or added relevant lines in 4 files are covered.
• 1 unchanged line in 1 file lost coverage.
• Overall coverage decreased (-0.09%) to 55.25%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
js/components/common/utils.js	3	5	60.0%
js/components/common/react_modal.js.jsx	1	4	25.0%
js/components/grid/course_panel.js.jsx	0	4	0.0%

Files with Coverage Reduction	New Missed Lines	%
js/components/grid/course_panel.js.jsx	1	17.82%

Totals
Change from base Build da33a828-ced2-4b81-8cb6-0099288bb4f2:	-0.09%
Covered Lines:	2236
Relevant Lines:	3975

---

💛 - Coveralls

[david-yz-liu]

Hi @a1-su, great work and thanks for the detailed questions.

1. Please make a separate PR for the eslint change. But I think it would be worthwhile running eslint on the entire codebase and fixing any other. I'm not sure how this slipped by the pre-commit hooks, and I'll look into that further.
2. You're absolutely correct that the existing front-end code didn't check for an error beforehand. I support adding new error-handling logic, but to do so I'd rather have getCourse raise an error (i.e., no special case for 404), and then in the calling code have a .catch that resets the state rather than embedding the logic within the .then success callback. Please also add front-end tests for this new logic.
3. Modifying the retrieveCourse tests to check status code is great
4. Because we don't publish the Courseography back-end routes publicly as an official API, we wouldn't consider this a breaking change. I think you can put this under "enhancements" and frame this as improving the error handling behaviour for this route.