Remove as2 #618

zachary-kent · 2021-12-07T22:35:15Z

Summary

This pull request removes the AS1 and AS2 options from the onboarding college dropdown, replacing these with one "Arts and Sciences" option. Upon onboarding submission, whether the user is actually enrolled in AS1 or AS2 is determined by checking to entrance year and season the user recorded.

Test Plan

The new "Arts and Sciences" option:

Whether you're in AS1 or AS2 is inferred on onboarding submission:

NEW Confirm that an entrance year of Spring 2020 is AS before Fall 2020 (now that season is taken into account).

Future STODOs

Handle the ISST major in the same way
Add a test to confirm this works (maybe Fall 2019, Spring 2020, Fall 2020, Fall 2021) on Cypress

dti-github-bot · 2021-12-07T22:35:32Z

[diff-counting] Significant lines: 63.

github-actions · 2021-12-07T22:37:09Z

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

https://cornelldti-courseplan-dev--pr618-remove-as2-1wimklw8.web.app

_{(expires Tue, 22 Mar 2022 23:59:39 GMT)}

_{🔥 via Firebase Hosting GitHub Action 🌎}

hahnbeelee · 2021-12-22T20:40:54Z

src/components/Modals/Onboarding/OnboardingBasic.vue

@@ -188,7 +191,7 @@ export default defineComponent({
      placeholderText,
      gradYear: this.onboardingData.gradYear,
      entranceYear: this.onboardingData.entranceYear,
-      collegeAcronym: this.onboardingData.college ? this.onboardingData.college : '',
+      collegeAcronym: this.onboardingData.college ?? '',


Whoa I didn't know ?? existed. This is cool!

hahnbeelee · 2021-12-22T21:06:59Z

src/components/Modals/Onboarding/OnboardingBasic.vue

    },
    majors(): Readonly<Record<string, string>> {
      const majors: Record<string, string> = {};
      const majorJSON = reqsData.major;
+      const acr = this.collegeAcronym.replace('AS', 'AS2');


is this arbitrarily choosing AS2 to replace AS so that it passes the if statement?

I think so! The major lists for AS1 and AS2 should be the same

Confirmed this was the case in the index.ts in the data folder.

hahnbeelee

Basic functionality works, but
There's a weird bug where the chosen college + major disappears once I open the modal up again after choosing Arts & Science.

CLAassistant · 2022-01-18T02:36:25Z

All committers have signed the CLA.

willespencer

Reviewed the code and found one small change to make. After fixing that, fixing the bug @hahnbeelee pointed out, resolving merge conflicts, and testing, this should be good for someone else to review!

src/components/Modals/Onboarding/Onboarding.vue

willespencer · 2022-02-03T22:03:56Z

src/components/Modals/Onboarding/OnboardingBasic.vue

    },
    majors(): Readonly<Record<string, string>> {
      const majors: Record<string, string> = {};
      const majorJSON = reqsData.major;
+      const acr = this.collegeAcronym.replace('AS', 'AS2');


I think so! The major lists for AS1 and AS2 should be the same

jerrry1123

everything makes sense for me except the part where we're doing a special case for A&S at the end. Shouldn't we be directly changing the JSON instead?

jerrry1123 · 2022-03-09T23:29:49Z

src/utilities.ts

@@ -50,6 +50,9 @@ export const yearRange = 6;

 export function getCollegeFullName(acronym: string | undefined): string {
  // Return empty string if college is not in requirementJSON
+  if (acronym === 'AS') {


should we be doing a special case here instead of changing the JSON directly?

This question is totally fair, and it's due to an inconsistency with the frontend and "backend" (how we represent requirements data).

A&S before and after 2020 have different requirements. They are represented as AS1 and AS2 in our requirements data. That is, they are considered separate colleges (because we assume one college = one set of requirements). On the frontend, we want the user to see it as a merged college, i.e. AS. So, Zak decided to implement this as a frontend-only change by writing conversions between these two representations. An upside of this is that the backend format can stay the same. A downside of this is that A&S needs to be special-cased in few different places.

An alternative implementation would have been something along the lines of adding variants to the requirements data format. So, the backend would look something like AS:Version1 and AS:Version2, where AS refers to a single college and Version1 and Version2 refer to different sets of requirements (we now assume one college = one or more sets of requirements). An upside would be that it would be more extensible -- that is, other colleges can have multiple versions as well (which is great if requirements are changed relatively frequently). A downside would be that every other college would have to support this added complexity (for example, EN would need to turn into EN:Version1 or EN:Default, both in the requirements data and in our user database). There are probably ways to avoid this downside by making the new format backwards-compatible, but that also comes with its pros and cons (see how we implemented additionalRequirements).

I personally think a single special case like this is fine. But if there are more special cases to be added in the future, or if we somehow manage to reach the backlog, I think there should be a refactoring effort.

Thanks for explaining that Ben! If we ever have a chance to refactor this to use the alternate implementation you mentioned, that could allow us to handle more specific cases of requirements changing over time (such as the Probability and Stats requirement for ECE). It's definitely down the line, but it could be useful for our users and help to reduce some confusion.

jessfeng

Tested on preview url with entrance year before Fall 2020, Fall 2020, and after Fall 2020 and they all look good! 😄 The bug I noticed during dev meeting is when adding another major and clicking finish on the onboarding, navigating to the new added major's header doesn't display anything below it (steps and screenshots are below). It's fine after refreshing the page (progress bar + requirements show up), so I'm not sure how big of a problem this is. I tried the same steps on prod, and it seems to have expected behavior (progress bar + requirements show up after finishing edit profile w/o having to refresh).

willespencer · 2022-03-16T00:14:42Z

Tested on preview url with entrance year before Fall 2020, Fall 2020, and after Fall 2020 and they all look good! 😄 The bug I noticed during dev meeting is when adding another major and clicking finish on the onboarding, navigating to the new added major's header doesn't display anything below it (steps and screenshots are below). It's fine after refreshing the page (progress bar + requirements show up), so I'm not sure how big of a problem this is. I tried the same steps on prod, and it seems to have expected behavior (progress bar + requirements show up after finishing edit profile w/o having to refresh).

Thank you for the detailed testing plan!! However, I still am not able to reproduce this :(

@einc could you try to see if you can replicate it?
@jessfeng did you try this on the dev webapp? If not could you see if it fails there, because if so, it is probably unrelated to my changes.

Additionally, I fixed the bug @einc pointed out during the meeting that A&S was still labeled pre-post 2020 in the reqs bar! This PR should be good to go now besides this potential bug.

Changes resolved!

zachary-kent added 3 commits December 7, 2021 14:05

Implemented year inference

e476b18

Fixed issue with as needing to be clicked twice

a1fd6d1

Fixed linter errors

b06281d

zachary-kent requested a review from a team as a code owner December 7, 2021 22:35

hahnbeelee reviewed Dec 22, 2021

View reviewed changes

hahnbeelee previously requested changes Dec 22, 2021

View reviewed changes

willespencer reviewed Feb 3, 2022

View reviewed changes

willespencer added 3 commits February 3, 2022 17:14

Merge branch 'master' into remove-as2

347c63d

Fix bug where A&S disappears from college choice

c0124bb

Factor out setting AS function

ad239dc

This was referenced Feb 7, 2022

Add entrance/grad seasons to onboarding #646

Closed

Add entrance/grad seasons to onboarding #647

Merged

willespencer added 2 commits March 9, 2022 12:01

Merge branch 'master' into remove-as2

b37b494

Update AS check to be dependent on season

32637e6

willespencer requested review from benjamin-shen, jessfeng and noschiff March 9, 2022 18:20

einc requested a review from jerrry1123 March 9, 2022 22:55

jessfeng requested a review from andxu282 March 9, 2022 23:31

jerrry1123 reviewed Mar 10, 2022

View reviewed changes

jessfeng reviewed Mar 10, 2022

View reviewed changes

willespencer added 2 commits March 15, 2022 19:44

Remove pre/post 2020 text in reqs bar

af71832

Move AS check to utilities to return in all locations

b6be5c3

willespencer requested review from einc and hahnbeelee March 16, 2022 00:14

einc approved these changes Mar 29, 2022

View reviewed changes

willespencer requested review from hahnbeelee and removed request for hahnbeelee March 29, 2022 19:32

willespencer merged commit 7bf56ba into master Mar 29, 2022

willespencer deleted the remove-as2 branch March 29, 2022 19:33

willespencer mentioned this pull request Apr 13, 2022

Spring 2022 Pre-enroll Release #680

Merged

24 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove as2 #618

Remove as2 #618

zachary-kent commented Dec 7, 2021 •

edited by willespencer

Loading

dti-github-bot commented Dec 7, 2021 •

edited

Loading

github-actions bot commented Dec 7, 2021 •

edited

Loading

hahnbeelee Dec 22, 2021

hahnbeelee Dec 22, 2021

willespencer Feb 3, 2022

willespencer Feb 6, 2022

hahnbeelee left a comment

CLAassistant commented Jan 18, 2022 •

edited

Loading

willespencer left a comment •

edited

Loading

willespencer Feb 3, 2022

jerrry1123 left a comment

jerrry1123 Mar 9, 2022

benjamin-shen Mar 10, 2022 •

edited

Loading

noschiff Mar 10, 2022

jessfeng left a comment

willespencer commented Mar 16, 2022

Remove as2 #618

Remove as2 #618

Conversation

zachary-kent commented Dec 7, 2021 • edited by willespencer Loading

Summary

Test Plan

dti-github-bot commented Dec 7, 2021 • edited Loading

github-actions bot commented Dec 7, 2021 • edited Loading

hahnbeelee Dec 22, 2021

Choose a reason for hiding this comment

hahnbeelee Dec 22, 2021

Choose a reason for hiding this comment

willespencer Feb 3, 2022

Choose a reason for hiding this comment

willespencer Feb 6, 2022

Choose a reason for hiding this comment

hahnbeelee left a comment

Choose a reason for hiding this comment

CLAassistant commented Jan 18, 2022 • edited Loading

willespencer left a comment • edited Loading

Choose a reason for hiding this comment

willespencer Feb 3, 2022

Choose a reason for hiding this comment

jerrry1123 left a comment

Choose a reason for hiding this comment

jerrry1123 Mar 9, 2022

Choose a reason for hiding this comment

benjamin-shen Mar 10, 2022 • edited Loading

Choose a reason for hiding this comment

noschiff Mar 10, 2022

Choose a reason for hiding this comment

jessfeng left a comment

Choose a reason for hiding this comment

willespencer commented Mar 16, 2022

zachary-kent commented Dec 7, 2021 •

edited by willespencer

Loading

dti-github-bot commented Dec 7, 2021 •

edited

Loading

github-actions bot commented Dec 7, 2021 •

edited

Loading

CLAassistant commented Jan 18, 2022 •

edited

Loading

willespencer left a comment •

edited

Loading

benjamin-shen Mar 10, 2022 •

edited

Loading