Add support for EECS renumbering #31
Conversation
To support the fall 2022 EECS subject renumbering, expand the course attributes to store old and new IDs for subjects affected by the renumbering.
Use the EECS renumbering data to populate the "Old Subject Id" and "New Subject Id" columns.
There was a problem hiding this comment.
Hi Pratyush, thanks for taking care of this! Overall it looks great, I just left two minor comments/questions about the field naming.
Have you spoken to @georgiashay about this yet? It would be good to deploy this branch on fireroad-dev for a bit and use it to test CourseRoad before merging and deploying on prod. Let me know if you need any help doing that!
|
Hi, Venkatesh, thank you for reviewing this so quickly. I have not yet spoken to @georgiashay about this; should I reach out to her separately? Sorry, I'm not very familiar with how FireRoad deployment works. Once this is on fireroad-dev (or I get the local setup figured out and I can |
For renumbered subjects in Course 6, store the new ID in subject_id directly instead of having a separate new_id column.
I forgot a step in 9056421, causing old_id not to be reflected in the JSON output.
|
@georgiashay Is this ready to be deployed on fireroad-dev? If not, I would be happy to make changes to bring it to that state. |
|
Sorry about the wait. I've been busy this week but I should have time to properly review this soon. |
|
It looks like the course catalog hasn't transitioned to the new numbers yet so it is hard to test whether everything still works (including the scraper) until that happens |
|
(The new listings should be up hopefully by the end of the month) |
|
Yes, I noticed that the catalog was updated today (?) with the new categories but not the new numbers. I guess we can just wait until the new listings are up and test it then. |
Now that the fall 2022 catalog has been released with a regular format for displaying old subject IDs, extract the old subject ID from the catalog instead of cross-referencing the EECS website.
psvenk
force-pushed
the
eecs-renumbering
branch
from
f625eec to
dda63d7
Compare
|
Now that the fall 2022 catalog has been released, it turns out that the old subject IDs are displayed there in a regular format, so I was able to greatly simplify the code by just parsing the catalog to extract the old ID rather than cross-referencing the EECS website. Should this be ready for testing now, or is further work needed? |
|
Catalog looks good to me. We should consider whether the backend (i.e. fireroad server) should translate roads from old to new ids, or whether the frontend (i.e. fireroad apps, courseroad) should be responsible for that. |
|
IMO, the best option is we don't touch the roads in the backend, and keep the road format such that both old and new road formats will be readable, but only the new road format will be written. If we have the ability to retrieve course data for display by either the old or new subject IDs, this should be straightforward. Does that sound good? Also, @georgiashay let me know when fireroad-dev is running this branch, or if you have any issues with the database migration etc. I will try to update the apps for compatibility as soon as I can. |
|
I've deployed this branch to fireroad-dev and updated the catalog. I will start working on getting courseroad compatible with the changes. I suspect the roads will display fine as-is (the old courses are still there as "historical courses"), but that they wouldn't show as satisfying requirements that use the new ids. Not sure if we should make the old subjects satisfy the requirements, or just force roads to be migrated to the new numbering when a user opens them. |
|
I think we might want to get rid of the historical courses that are just renamed courses. @psvenk, can you look into changing this? I think it would probably involve a change to the build_consensus function. |
|
I can work on getting rid of the historical courses in the next few days. On a somewhat related note, do you know how new courses can be made to satisfy old requirements? Or should this just be done by updating requirements (in the requirements editor, I assume) to use new IDs? |
|
We'll update the requirements to use the new IDs. Miriam (who works on Courseroad with me) has written a script to do this, and we'll deploy the new requirements once everything else is ready. |
When a course has been renumbered, remove the version under the old number so that there is only one number for the course stored in the database.
|
I can confirm that the historical courses are removed in 7ddd0c9. (I managed to get the database and server working.) |
|
Thanks for the update! This is thinking ahead, but old id's actually can be reused after 5 years. So we don't want to remove all subjects that are old id's, because some of them can come back. We only want to remove classes that are old id's in a future semester, not a past semester. |
|
(For reference, course 6 is planning on reusing 3-digit course numbers after that 5 year period) |
Only remove classes whose IDs are marked as old IDs in a future semester, not in a past semester, given that old IDs can be reused after 5 years.
|
Good point; this should now be fixed in c40933c. (I did some informal testing and there don't seem to be any regressions.) I just made sure that the old ID is from a future semester as you suggested. This does leave open some weird edge cases over 10-year periods if courses are renumbered in specific ways, but I think it's unlikely that this will be realized in practice (and anyway, being overzealous in deleting historical subjects from 10 years ago is not the end of the world). Do you think this (and sipb/courseroad2#476) is ready to merge? |
|
I am running into a weird issue where this throws errors on the dev server (but not locally). Still investigating this, I'll let you know when I figure out what's going on. |
|
I figured it out - previous semesters do not have the old id attribute, so the build_consensus will throw an error in these cases. Here is one way to fix this: Can you add this fix in? |
When checking if something is an old ID in build_consensus, handle the case that the oldID column does not exist because some of the future semesters (relative to the semester being viewed) are before fall 2022.
|
Thanks for all the great work! This is deployed on fireroad-dev, and I'll merge into master. I'll let @venkatesh-sivaraman update the mobile apps before we deploy on prod. |
|
Thank you for merging and for all of your feedback! I would like to help transition the requirements to the new numbers. Should this be done in the Requirements Editor on prod or dev? Should I wait until it's on prod before submitting a request? |
|
Miriam has already transitioned the requirements on prod, but we are waiting to deploy these changes until all the clients (web/mobile apps) are updated. I've just released these transitioned requirements on dev so that the clients have access to them for testing. |
|
I noticed that the transition only applies to the old majors — "6-1 Major", "6-2 Major", and "6-3 Major" — and not the new majors — "6-2 Major (New)", "6-3 Major (New)", and "6-4 Major". Do these still need to be transitioned to the new numbers? |
|
Those still need to be transitioned, I think I might have added these after Miriam ran her script. |
|
I am ready to deploy whenever you are, do you want to wait until the app is approved and then deploy right away? |
|
Great, you can go ahead and deploy then! The apps just got approved so I will go ahead and release them now. |
Fixes #30. I can confirm that these changes generate the "Subject Old Id" and "Subject New Id" columns properly in the output of the catalog parser, but I could not test the API endpoint because I had trouble getting the database set up properly. (The deltas were not being generated correctly; I could look into this further if necessary.)