[RFC][data-migration] `selectableRequirementChoices` -> `optOut` #553

SamChou19815 · 2021-10-22T02:17:48Z

Summary

This PR implements the data migration script that will migrate the selectableRequirementChoices into the new format where all the user choices are unified under the concept of override: opt-in and opt-out.

We originally think everything can be categorized into opt-out and opt-in, but the running result shown above indicates that we might need 3, which I propose below:

type OverrideChoiceForCourse = {
  /* unchanged */
  optOut: Requirement[];
  /**
   * same as opt-in as we discussed in previous meetings. 
   * It's for attaching completely unknown courses to a requirement 
   * (e.g. opt-in CS 2112 for history requirement)
   */
  arbitraryOptIn: Record<Requirement, slot[]>
  /* See discussion below */
  acknowledgedCheckerWarningOptIn: Requirement[];
}

acknowledgedCheckerWarningOptIn is for requirements that have a checker warning. e.g. tech elective in CS. In the old flow, they are considered more like self-checks, and never picked as default choice. For example, when you add CS 4120, the following thing will happen in the old system:

CS elective chosen as default choice
Allow you to choose tech elective, advisor approved elective, etc (those with broad, imprecise checker) as alternative choice, but never selected by default
opt-in not implemented yet, so that's irrelevant

For the new system and the 3 category thing I proposed, the default choice would be:

choice = { optOut: [], arbitraryOptIn: {}, acknowledgedCheckerWarningOptIn: null };

which corresponds to CS 4120 being connected to CS elective only and not to any broad requirements. It fits the general philosophy of the new flow: during add time, we don't make complicated computation to decide eligibility, and the default choice should be safe, non-confusing.

It's not only satisfying philosophically, but it's also necessary. It's important to distinguish between arbitrary opt-in and acknowledgedCheckerWarningOptIn, since the former is a complete escape hatch and the system knows nothing about where it should be without user intervention, the latter we do exactly know which slot it should go to (we are just unsure whether it can be safely assigned to a requirement)

Test Plan

TODO

Notes

Once the data migration script is completed, it should not be run and deployed to production immediately. We need a second part of this migration project, which makes the frontend saves data to both selectableRequirementChoices and optOut while still making selectableRequirementChoices the source of truth, so that we can safely deploy this to prod and the prod will start consistency generating data of the new format. Then in a future deploy to prod when we are ready, we can switch to use the new format to be the source of truth.

dti-github-bot · 2021-10-22T02:18:07Z

[diff-counting] Significant lines: 231.

github-actions · 2021-10-22T02:20:08Z

Visit the preview URL for this PR (updated for commit 762cdca):

https://cornelldti-courseplan-dev--pr553-opt-out-data-migrati-e4k1c0sp.web.app

_{(expires Wed, 10 Nov 2021 15:59:49 GMT)}

_{🔥 via Firebase Hosting GitHub Action 🌎}

SamChou19815 · 2021-10-22T19:16:58Z

New dry-run results. I think it's correct now.

npm run ts-node -- src/requirements/admin/opt-out-data-migration.ts [email protected]

> [email protected] ts-node
> ts-node -T -P tsconfig.node.json "src/requirements/admin/opt-out-data-migration.ts" "[email protected]"

[email protected]
MATH 1910 (42):
- selected requirement: College-EN-Mathematics
- optOut = []
- acknowledgedCheckerWarningOptIn = []
MATH 1920 (43):
- selected requirement: College-EN-Mathematics
- optOut = []
- acknowledgedCheckerWarningOptIn = []
MATH 2930 (44):
- selected requirement: College-EN-Mathematics
- optOut = []
- acknowledgedCheckerWarningOptIn = []
MATH 2940 (99):
- selected requirement: None
- optOut = [College-EN-Mathematics]
- acknowledgedCheckerWarningOptIn = []
MATH 4710 (100):
- selected requirement: None
- optOut = []
- acknowledgedCheckerWarningOptIn = []
COGST 1101 (89):
- selected requirement: College-EN-Liberal Studies: 6 courses
- optOut = []
- acknowledgedCheckerWarningOptIn = []
SOC 1101 (90):
- selected requirement: College-EN-Liberal Studies: 6 courses
- optOut = []
- acknowledgedCheckerWarningOptIn = []
LING 1101 (91):
- selected requirement: College-EN-Liberal Studies: 6 courses
- optOut = []
- acknowledgedCheckerWarningOptIn = []
ECON 2040 (92):
- selected requirement: College-EN-Liberal Studies: 6 courses
- optOut = []
- acknowledgedCheckerWarningOptIn = []
ASIAN 2212 (93):
- selected requirement: College-EN-Liberal Studies: 6 courses
- optOut = []
- acknowledgedCheckerWarningOptIn = []
CS 2112 (98):
- selected requirement: None
- optOut = []
- acknowledgedCheckerWarningOptIn = []
CS 2800 (37):
- selected requirement: Major-CS-Computer Science Core
- optOut = []
- acknowledgedCheckerWarningOptIn = []
CS 3110 (38):
- selected requirement: Major-CS-Computer Science Core
- optOut = []
- acknowledgedCheckerWarningOptIn = []
CS 3410 (39):
- selected requirement: Major-CS-Computer Science Core
- optOut = []
- acknowledgedCheckerWarningOptIn = []
CS 4820 (40):
- selected requirement: Major-CS-Computer Science Core
- optOut = []
- acknowledgedCheckerWarningOptIn = []
CS 4410 (41):
- selected requirement: Major-CS-Computer Science Core
- optOut = []
- acknowledgedCheckerWarningOptIn = []
CS 4110 (54):
- selected requirement: Major-CS-CS Electives
- optOut = []
- acknowledgedCheckerWarningOptIn = []
CS 4120 (66):
- selected requirement: Major-CS-Technical Electives
- optOut = [Major-CS-CS Electives]
- acknowledgedCheckerWarningOptIn = [Major-CS-Technical Electives]
CS 4121 (58):
- selected requirement: Major-CS-CS Practicum or Project
- optOut = [Major-CS-CS Electives]
- acknowledgedCheckerWarningOptIn = []
CS 4780 (67):
- selected requirement: Major-CS-Technical Electives
- optOut = [Major-CS-CS Electives]
- acknowledgedCheckerWarningOptIn = [Major-CS-Technical Electives]
CS 5114 (68):
- selected requirement: Major-CS-CS Electives
- optOut = []
- acknowledgedCheckerWarningOptIn = []
PE 1330 (52):
- selected requirement: None
- optOut = []
- acknowledgedCheckerWarningOptIn = []
PE 1657 (53):
- selected requirement: None
- optOut = []
- acknowledgedCheckerWarningOptIn = []
INFO 4998 (50):
- selected requirement: None
- optOut = []
- acknowledgedCheckerWarningOptIn = []
INFO 4998 (51):
- selected requirement: None
- optOut = []
- acknowledgedCheckerWarningOptIn = []
AP Computer Science A (AP Computer Science A):
- selected requirement: None
- optOut = []
- acknowledgedCheckerWarningOptIn = []
AP Physics C-Electricity & Magnetism (AP Physics C-Electricity & Magnetism):
- selected requirement: None
- optOut = []
- acknowledgedCheckerWarningOptIn = []
AP Physics C-Mechanics (AP Physics C-Mechanics):
- selected requirement: None
- optOut = []
- acknowledgedCheckerWarningOptIn = []
IB Chemistry (IB Chemistry):
- selected requirement: None
- optOut = []
- acknowledgedCheckerWarningOptIn = []

SamChou19815 · 2021-10-22T19:17:29Z

src/requirements/requirement-graph-builder-from-user-data.ts

+  /**
+   * The table below summerizes the type of all possible requirement-course edges before
+   * double-counting elimination, and where they will go in the new format.
+   *
+   * -------------------------------------------------------------------------------------------------
+   * | Requirement Type | edge exists after double-counting elim | where is it in the new format     |
+   * | ---------------- | -------------------------------------- | --------------------------------- |
+   * | selected         | True                                   | implicit (connected by default)   |
+   * | (no warning)     |                                        |                                   |
+   * |                  |                                        |                                   |
+   * | selected         | True                                   | `acknowledgedCheckerWarningOptIn` |
+   * | (has warning)    |                                        |                                   |
+   * |                  |                                        |                                   |
+   * | allow double     | True                                   | implicit (connected by default)   |
+   * | counting         |                                        |                                   |
+   * |                  |                                        |                                   |
+   * | not connected    | False                                  | `optOut`                          |
+   * | (no warning)     |                                        |                                   |
+   * |                  |                                        |                                   |
+   * | not connected    | False                                  | implicit (unconnected by default) |
+   * | (has warning)    |                                        |                                   |
+   */


I think it should cover everything now

Can you explain where the current requirement types go in this graph? Specifically, the non-double countable courses and requirements with checker warnings?

You don't have to push this but I'm trying to understand how this graph would look:

/** * The table below summerizes the type of all possible requirement-course edges before * double-counting elimination, and where they will go in the new format. * * ------------------------------------------------------------------------------------------------- * | Requirement Type | edge exists after double-counting elim | where is it in the new format | Related Requirements | * | ---------------- | -------------------------------------- | --------------------------------- | -------------------- | * | selected | True | implicit (connected by default) | | * | (no warning) | | | | * | | | | | * | selected | True | `acknowledgedCheckerWarningOptIn` | | * | (has warning) | | | | * | | | | | * | allow double | True | implicit (connected by default) | | * | counting | | | | * | | | | | * | not connected | False | `optOut` | non-double count | * | (no warning) | | | | * | | | | | * | not connected | False | implicit (unconnected by default) | | * | (has warning) | | | | */

SamChou19815 · 2021-10-22T19:18:42Z

src/requirements/requirement-graph-builder-from-user-data.ts

+    const acknowledgedCheckerWarningOptIn = courseIsAPIB(course)
+      ? []
+      : connectedRequirementsWithDoubleCountingAccounted.filter(
+          it => userRequirementsMap[it].checkerWarning != null
+        );


This is the only behavior change. I decide to do this because the old behavior that AP courses can be used to auto-fulfill requirements with checker warnings is just very wrong.

hahnbeelee · 2021-10-29T18:14:33Z

Can you explain this case to me? Why is the elective in optOut?

CS 4780 (67):
- selected requirement: Major-CS-Technical Electives
- optOut = [Major-CS-CS Electives]
- acknowledgedCheckerWarningOptIn = [Major-CS-Technical Electives]

Actually jk let me try to answer my own question.

CS 4780 can be used to fulfill the CS Elective or the the CS Technical Elective.
CS Elective and CS Tech Elec are both NOT double countable. So, the user must choose between the 2.
The user chose Tech Elec which means we need to opt out of the natural CS Elective edge.

hahnbeelee · 2021-10-29T18:15:01Z

And when you say that optOut is the same as it is in the system are you referring to Ben's unused optOut code?

hahnbeelee · 2021-10-29T18:25:43Z

Not sure if you need to fix this here but this type is deprecated. (in requirement-graph-builder-from-user-data)

hahnbeelee · 2021-10-29T18:39:10Z

How do the decisions made here affect our choice of choosing optIn or out? And if they don't affect them then why don't they?

Attempt at answering my own question:
This does affect the choice. Currently this is an optOut infrastructure. If it was optIn, the selected course will be optIn and we wouldn't care about the one we didn't select?

SamChou19815 · 2021-10-29T19:23:06Z

@hahnbeelee

And when you say that optOut is the same as it is in the system are you referring to Ben's unused optOut code?

No, I'm referring to the discussion when we first established that all user choices are modeled as opt-in/opt-out.

Not sure if you need to fix this here but this type is deprecated. (in requirement-graph-builder-from-user-data)

I intentionally mark is as deprecated. The replacement type is FirestoreCourseOptInOptOutChoices. Full removal of that deprecated type is pending on the migration.

hahnbeelee · 2021-10-29T19:37:39Z

I feel we can rename arbitraryOptIn to optIn because the existence of acknowledgedCheckerWarningOptIn kinda implies that the other optIn is arbitrary.

SamChou19815 · 2021-10-29T20:06:03Z

I feel we can rename arbitraryOptIn to optIn because the existence of acknowledgedCheckerWarningOptIn kinda implies that the other optIn is arbitrary.

This is still pending whether we go with default opt-in or opt-out. If we default by opt-out, there will be another kind of opt-in lol.

zachary-kent

Honestly, I feel like I would need to look extensively through all of your previous PR's/talk with you more in person to really understand what's going on here. However, I was always very interested in the work you did on the requirement graph and wanted to take a look at what you've been working on! For now I just left some nitty suggestions/questions.

zachary-kent · 2021-11-01T01:12:53Z

src/requirements/admin/opt-out-data-migration.ts

+
+async function main() {
+  let userEmail = process.argv[2];
+  if (userEmail != null) {


Is there reason you use != instead of !== here?

!= null will check for both null and undefined, so it's usually used and why it's allowed by linters.

src/requirements/admin/opt-out-data-migration.ts

src/requirements/requirement-graph-builder-from-user-data.ts

zachary-kent · 2021-11-01T01:23:01Z

src/requirements/requirement-graph-builder-from-user-data.ts

+     * ```
+     */
+    const complementary = new Set(connectedRequirementsWithoutDoubleCountingAccounted);
+    connectedRequirementsWithDoubleCountingAccounted.forEach(r => complementary.delete(r));


Could you change the callback for forEach to be complementary.delete instead of r => complementary.delete(r)?

That will actually crash the program due to JS this receiver unbinding. A simple example:

hahnbeelee · 2021-11-03T05:17:59Z

How do the updates from #522 affect this PR?

SamChou19815 · 2021-11-03T12:50:54Z

How do the updates from #522 affect this PR?

No change for now. The two graph approach will only be implemented after we have the design. Currently it's ok with one graph because we know existing data does not have any constraint violations.

hahnbeelee

Alright I believe the logic checks out. After you address Zak's comments let's merge.

SamChou19815 · 2021-11-03T16:04:21Z

Planning to merge once the release PR is merged.

SamChou19815 requested a review from a team as a code owner October 22, 2021 02:17

SamChou19815 marked this pull request as draft October 22, 2021 02:18

This comment has been minimized.

Sign in to view

SamChou19815 force-pushed the opt-out-data-migration-script branch 3 times, most recently from cc3a137 to 51f3305 Compare October 22, 2021 19:14

SamChou19815 force-pushed the opt-out-data-migration-script branch from 51f3305 to dfbfd35 Compare October 22, 2021 19:17

SamChou19815 commented Oct 22, 2021

View reviewed changes

SamChou19815 marked this pull request as ready for review October 25, 2021 19:47

Initial dry-run implementation

86798b9

SamChou19815 force-pushed the opt-out-data-migration-script branch from dfbfd35 to 86798b9 Compare October 30, 2021 21:21

zachary-kent reviewed Nov 1, 2021

View reviewed changes

hahnbeelee approved these changes Nov 3, 2021

View reviewed changes

SamChou19815 added 2 commits November 3, 2021 11:54

Use const

5fd132f

Shorten some variables

762cdca

SamChou19815 merged commit 7f4d7ef into master Nov 4, 2021

SamChou19815 deleted the opt-out-data-migration-script branch November 4, 2021 00:56

SamChou19815 mentioned this pull request Nov 14, 2021

Requirement builder with the new data format #588

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RFC][data-migration] `selectableRequirementChoices` -> `optOut` #553

[RFC][data-migration] `selectableRequirementChoices` -> `optOut` #553

SamChou19815 commented Oct 22, 2021 •

edited

Loading

dti-github-bot commented Oct 22, 2021 •

edited

Loading

github-actions bot commented Oct 22, 2021 •

edited

Loading

This comment has been minimized.

SamChou19815 commented Oct 22, 2021

SamChou19815 Oct 22, 2021

hahnbeelee Oct 29, 2021

hahnbeelee Oct 29, 2021

SamChou19815 Oct 22, 2021

hahnbeelee commented Oct 29, 2021 •

edited

Loading

hahnbeelee commented Oct 29, 2021 •

edited

Loading

hahnbeelee commented Oct 29, 2021 •

edited

Loading

hahnbeelee commented Oct 29, 2021 •

edited

Loading

SamChou19815 commented Oct 29, 2021

hahnbeelee commented Oct 29, 2021

SamChou19815 commented Oct 29, 2021

zachary-kent left a comment

zachary-kent Nov 1, 2021

SamChou19815 Nov 3, 2021

zachary-kent Nov 1, 2021

SamChou19815 Nov 3, 2021

hahnbeelee commented Nov 3, 2021 •

edited

Loading

SamChou19815 commented Nov 3, 2021

hahnbeelee left a comment

SamChou19815 commented Nov 3, 2021

[RFC][data-migration] selectableRequirementChoices -> optOut #553

[RFC][data-migration] selectableRequirementChoices -> optOut #553

Conversation

SamChou19815 commented Oct 22, 2021 • edited Loading

Summary

Test Plan

Notes

dti-github-bot commented Oct 22, 2021 • edited Loading

github-actions bot commented Oct 22, 2021 • edited Loading

This comment has been minimized.

SamChou19815 commented Oct 22, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hahnbeelee commented Oct 29, 2021 • edited Loading

hahnbeelee commented Oct 29, 2021 • edited Loading

hahnbeelee commented Oct 29, 2021 • edited Loading

hahnbeelee commented Oct 29, 2021 • edited Loading

SamChou19815 commented Oct 29, 2021

hahnbeelee commented Oct 29, 2021

SamChou19815 commented Oct 29, 2021

zachary-kent left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hahnbeelee commented Nov 3, 2021 • edited Loading

SamChou19815 commented Nov 3, 2021

hahnbeelee left a comment

Choose a reason for hiding this comment

SamChou19815 commented Nov 3, 2021

[RFC][data-migration] `selectableRequirementChoices` -> `optOut` #553

[RFC][data-migration] `selectableRequirementChoices` -> `optOut` #553

SamChou19815 commented Oct 22, 2021 •

edited

Loading

dti-github-bot commented Oct 22, 2021 •

edited

Loading

github-actions bot commented Oct 22, 2021 •

edited

Loading

hahnbeelee commented Oct 29, 2021 •

edited

Loading

hahnbeelee commented Oct 29, 2021 •

edited

Loading

hahnbeelee commented Oct 29, 2021 •

edited

Loading

hahnbeelee commented Oct 29, 2021 •

edited

Loading

hahnbeelee commented Nov 3, 2021 •

edited

Loading