Migrate roads to new course numbers

[georgiashay]

Migrates roads to use the new course numbers. Currently this code will let the roads load in and wait until subjects are loaded to migrate to the new numberings - so your road will briefly show up with the old numbers before being migrated. I figured this was better than withholding the whole road until the catalog loads, but lmk if this is a bad idea.

[miriam-rittenberg]

It seems like this transitions my current course numbers to the new ones, but if I then re-add eg 6.009 to a road it stays as 6.009 permanently. Is this the behavior we want / do we want it to still be possible to add the old course numbers at all?

[georgiashay]

Currently Fireroad keeps classes that aren't around anymore as "historical classes." I'm not sure what the right approach is.

[georgiashay]

I've thought about it and removing the old courses is probably the best approach. This will need a change on the server code, tracking in https://github.com/venkatesh-sivaraman/fireroad-server/pull/31

[psvenk]

This looks good overall, but it would be nice if:

• Searching for subjects looks at old_id in addition to subject_id, so that, e.g., a search for "6.009" turns up 6.1010
• The old subject ID is displayed in addition to the new one when opening the description for a subject.

[psvenk]

Here is a patch for the first issue:

diff --git a/src/components/ClassSearch.vue b/src/components/ClassSearch.vue
index 70ff0c3..dafb99e 100644
--- a/src/components/ClassSearch.vue
+++ b/src/components/ClassSearch.vue
@@ -81,7 +81,7 @@ var unitsGte15 = new MathFilter('>15', '>15', [15, undefined], false, ['total_un
 var termFall = new BooleanFilter('Fall', 'FA', false, ['offered_fall']);
 var termIAP = new BooleanFilter('IAP', 'IAP', false, ['offered_IAP']);
 var termSpring = new BooleanFilter('Spring', 'SP', false, ['offered_spring']);
-var textFilter = new RegexFilter('Subject ID', 'ID', '', 'nameInput', ['subject_id', 'title'], 'OR');
+var textFilter = new RegexFilter('Subject ID', 'ID', '', 'nameInput', ['subject_id', 'title', 'old_id'], 'OR');
 var instructorFilter = new ArrayFilter('Instructor', 'Prof', RegexFilter, ['', 'nameInput'], ['instructors'], 'OR');

 export default {

[georgiashay]

Thanks for the catch, and the patch! Should be fixed now

[psvenk]

Now that https://github.com/venkatesh-sivaraman/fireroad-server/pull/31 has been merged, is this ready to merge (and deploy once that is deployed on production) or is further work needed?

[georgiashay]

I have a few bugs (prior credit not being transitioned, and vuex is upset at the way we are modifying the state), so I'm going to work on fixing those before merging.

[georgiashay]

Ready for another review

[georgiashay]

Are you seeing this behavior when loading roads already on your account, or when importing a road? Currently Courseroad can only import roads with classes it knows about (the new code migrates old courses -> new courses first). Regardless, yes we can deploy this before the backend.

We have wanted to support custom subjects for a while (and we do display them if you create them in the fireroad app). If you are interested, we have an open issue (#423) for this and we're always looking for more people to help out! We have a courseroad development channel in mattermost as well if you're interested.

[psvenk]

I only tested with imported roads; it's good that this behavior (as a general thing) is only for imported roads. I did see that the migration is done before removing unknown courses, which is good.

Is there a technical reason why unknown subjects in general are deleted? (I assume that it won't be necessary once custom subjects are fully supported.)

Thanks for inviting me to Mattermost; I just joined the channel a few minutes ago. I would be happy to help with adding support for custom subjects when I have some more time on my hands.

[georgiashay]

The current reason why unknown subjects are deleted is that on import, we make sure that the road subjects have all the required fields and fill them in if they do not. If the subject is unknown, we don't know those values if they're not provided. This was I believe originally for compatibility in case of old road documents. A better idea might be to just set these fields to empty string/0/undefined/etc as appropriate.