Switching over to MaIS APIs and removing registry harvester XML option #483
Conversation
hudajkhan
changed the title
|
Relates to #410 |
lib/course_work_courses.rb
Outdated
| @@ -35,33 +35,11 @@ def cross_listings | |||
| end | |||
|
|
|||
| def self.instance | |||
| @instance ||= CourseWorkCourses.new | |||
| # @instance ||= CourseWorkCourses.new | |||
lib/course_work_courses.rb
Outdated
| else | ||
| @all_courses ||= process_all_courses_xml(self.raw_xml).to_a.reverse.uniq(&:key).reverse.to_a | ||
| end | ||
| @all_courses ||= process_all_courses(self.json_files).to_a.reverse.uniq(&:key).reverse.to_a |
There was a problem hiding this comment.
Why do we need a double reverse here? Why does it need to be cast to .to_a a second time?
There was a problem hiding this comment.
This is a copy of the original handling. I am not sure why it was included beyond the original comment above the method "We have tests that are highly sensitive to the order of the courses; this unfortunate logic preserves the bottom-most course from the xml. it's unclear whether this is incidental or a feature." I can look into this further.
There was a problem hiding this comment.
If it doesn't have a test, then it's not a feature 😄
There was a problem hiding this comment.
There is a specific test that fails when I try to change the code here to simply use .uniq(&:key) instead: https://github.com/sul-dlss/course_reserves/blob/master/spec/lib/course_work_courses_spec.rb#L165 . The reason is that, in this test, the original list of courses has two sections that are identical with respect to their CIDs and their instructor sunet id list. They only differ with respect to their section number. The test expects that, when it looks for a course with that CID, there will only be one section returned with the lower section number (i.e. 1). Based on the fixture file, the first reverse, I think, is intended to put the sections in ascending order, with uniq using the Course class's key method (which is based on CID and instructor SUNET IDs), returning the very first section with the same course id and instructors (and removing the rest), and then the next reverse puts the list back in the original order.
When I tried a sort by section id followed by using uniq based on the key method, it changes the original order of the courses, which make other tests fail.
I could write a separate method which does what this line of code is trying to do (preserve the first section among courses that have the same CID and same set of instructors, while removing the rest), while preserving the original order of the courses.
There was a problem hiding this comment.
I am not sure why the section number matters to these tests in this way, because the app only displays the course information without section information included, but I do see section id is referenced in other code, and I don't want to change the part about returning the first section among courses that are identical in the other ways without understanding the consequences.
There was a problem hiding this comment.
I've added another method which does have the tests pass. If there's a better/more concise way to do that, please feel free to suggest.
lib/course_work_courses.rb
Outdated
| @@ -35,7 +35,7 @@ def cross_listings | |||
| end | |||
|
|
|||
| def self.instance | |||
| # @instance ||= CourseWorkCourses.new | |||
| # This used to be memoized, i.e. ||= but we want the JSON loaded each time without restarting | |||
There was a problem hiding this comment.
Can we get rid of the instance method then? The name of this method sort of indicates this is the "Singleton Pattern". But now that this isn't memoized, it's not a Singleton.
lib/course_work_courses.rb
Outdated
| else | ||
| @all_courses ||= process_all_courses_xml(self.raw_xml).to_a.reverse.uniq(&:key).reverse.to_a | ||
| end | ||
| # Process |
There was a problem hiding this comment.
I'm not sure what this means. Can you elaborate?
There was a problem hiding this comment.
Probably part of an earlier comment that I've since removed. I'll remove the line.
This pull request has these main objectives:
a) Removal of the registry harvester XML based code (i.e. the previous way of getting course term and course information for the app). To that end, lib/course_work_courses.rb has been changed to exclusively use the API JSON-based initialization, loading of courses, and returning the course list through the all_courses method (which is triggered by the AJAX request that helps populate the list of courses in the app). Since the Setting variable that allowed switching between the registry harvester method and the new method is no longer needed, the code that handled that has also been removed. The tests have been updated to remove mention of the Setting and a fixture files related to the registry harvester method has also been removed.
b) Use of "fetch_courses" as the scheduled rake task in schedule.rb. Additionally, the time the rake task is called has been changed to an hour earlier (3:30 AM instead of 4:30 AM) to give the rake task more time to complete since the individual course API requests take a few hours to complete.
c) Changing the CourseWorkCourses initialization method to remove the memoization to enable loading the JSON files into the app each time this class is initialized. This change enables the updated files in the lib directory to be read in instead of requiring an app restart (which is what the previous code did). We will also create a new issue to suggest the use of a database to hold this information, as opposed to static files that need to be overwritten. but that can be handled separately.