Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove as2 #618

Merged
merged 10 commits into from
Mar 29, 2022
Merged

Remove as2 #618

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
e476b18
Implemented year inference
zachary-kent Dec 7, 2021
a1fd6d1
Fixed issue with as needing to be clicked twice
zachary-kent Dec 7, 2021
b06281d
Fixed linter errors
zachary-kent Dec 7, 2021
347c63d
Merge branch 'master' into remove-as2
willespencer Feb 3, 2022
c0124bb
Fix bug where A&S disappears from college choice
willespencer Feb 6, 2022
ad239dc
Factor out setting AS function
willespencer Feb 6, 2022
b37b494
Merge branch 'master' into remove-as2
willespencer Mar 9, 2022
32637e6
Update AS check to be dependent on season
willespencer Mar 9, 2022
af71832
Remove pre/post 2020 text in reqs bar
willespencer Mar 15, 2022
b6be5c3
Move AS check to utilities to return in all locations
willespencer Mar 15, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 22 additions & 3 deletions src/components/Modals/Onboarding/Onboarding.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ import OnboardingBasic from '@/components/Modals/Onboarding/OnboardingBasic.vue'
import OnboardingTransfer from '@/components/Modals/Onboarding/OnboardingTransfer.vue';
import OnboardingReview from '@/components/Modals/Onboarding/OnboardingReview.vue';
import { setAppOnboardingData, populateSemesters } from '@/global-firestore-data';
import { getMajorFullName, getMinorFullName, getGradFullName } from '@/utilities';
import { getMajorFullName, getMinorFullName, getGradFullName, SeasonOrdinal } from '@/utilities';
import timeline1Text from '@/assets/images/timeline1text.svg';
import timeline2Text from '@/assets/images/timeline2text.svg';
import timeline3Text from '@/assets/images/timeline3text.svg';
Expand Down Expand Up @@ -213,10 +213,11 @@ export default defineComponent({
},
methods: {
submitOnboarding() {
const revised = this.setASCollegeReqs();
this.clearTransferCreditIfGraduate();
setAppOnboardingData(this.name, this.onboarding);
setAppOnboardingData(this.name, revised);
// indicates first time user onboarding
if (!this.isEditingProfile) populateSemesters(this.onboarding);
if (!this.isEditingProfile) populateSemesters(revised);
this.$emit('onboard');
},
goBack() {
Expand Down Expand Up @@ -297,6 +298,24 @@ export default defineComponent({
this.cancel();
}
},
// determine which AS reqs to set depending on user's starting semester
// AS1 for students entering before Fall 2020, AS2 for students after
setASCollegeReqs() {
const revised = { ...this.onboarding };
if (revised.college === 'AS') {
const year = Number.parseInt(revised.entranceYear, 10);
const season: FirestoreSemesterSeason =
revised.entranceSem === '' ? 'Fall' : revised.entranceSem; // default to fall if not provided
if (year < 2020) {
revised.college = 'AS1';
} else if (year === 2020 && SeasonOrdinal[season] < SeasonOrdinal.Fall) {
revised.college = 'AS1';
} else {
revised.college = 'AS2';
}
}
return revised;
},
},
});
</script>
Expand Down
33 changes: 25 additions & 8 deletions src/components/Modals/Onboarding/OnboardingBasic.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,7 @@
<script lang="ts">
import { PropType, defineComponent } from 'vue';
import reqsData from '@/requirements/typed-requirement-json';
import { clickOutside, getCurrentYear, yearRange } from '@/utilities';
import { clickOutside, getCollegeFullName, getCurrentYear, yearRange } from '@/utilities';
import OnboardingBasicMultiDropdown from './OnboardingBasicMultiDropdown.vue';
import OnboardingBasicSingleDropdown from './OnboardingBasicSingleDropdown.vue';

Expand All @@ -193,7 +193,10 @@ export default defineComponent({
components: { OnboardingBasicMultiDropdown, OnboardingBasicSingleDropdown },
props: {
userName: { type: Object as PropType<FirestoreUserName>, required: true },
onboardingData: { type: Object as PropType<AppOnboardingData>, required: true },
onboardingData: {
type: Object as PropType<AppOnboardingData>,
required: true,
},
isEditingProfile: { type: Boolean, required: true },
},
emits: {
Expand Down Expand Up @@ -227,6 +230,12 @@ export default defineComponent({
if (majorAcronyms.length === 0) majorAcronyms.push('');
if (minorAcronyms.length === 0) minorAcronyms.push('');

// convert AS1/AS2 acronym in firebase to AS for the frontend
let collegeAcronym = this.onboardingData.college ?? '';
if (collegeAcronym === 'AS1' || collegeAcronym === 'AS2') {
collegeAcronym = 'AS';
}

// if sem has not been previously filled out, choice is automatically is "Fall"/"Spring" for new users, old users have no data
let { gradSem } = this.onboardingData;
let { entranceSem } = this.onboardingData;
Expand All @@ -244,7 +253,7 @@ export default defineComponent({
gradSem,
entranceYear: this.onboardingData.entranceYear,
entranceSem,
collegeAcronym: this.onboardingData.college ? this.onboardingData.college : '',
collegeAcronym,
majorAcronyms,
minorAcronyms,
gradAcronym: this.onboardingData.grad ? this.onboardingData.grad : '',
Expand All @@ -255,16 +264,20 @@ export default defineComponent({
},
computed: {
colleges(): Readonly<Record<string, string>> {
return Object.fromEntries(
Object.entries(reqsData.college).map(([key, { name }]) => [key, name])
);
const base = Object.entries(reqsData.college)
.filter(college => !college[0].startsWith('AS'))
.map(([key, { name }]) => [key, name]);
base.push(['AS', getCollegeFullName('AS')]);
base.sort((c1, c2) => c1[1].localeCompare(c2[1]));
return Object.fromEntries(base);
},
majors(): Readonly<Record<string, string>> {
const majors: Record<string, string> = {};
const majorJSON = reqsData.major;
const acr = this.collegeAcronym !== 'AS' ? this.collegeAcronym : 'AS2';
Object.keys(majorJSON).forEach(key => {
// only show majors for schools the user is in
if (majorJSON[key].schools.includes(this.collegeAcronym)) {
if (majorJSON[key].schools.includes(acr)) {
majors[key] = majorJSON[key].name;
}
});
Expand Down Expand Up @@ -330,7 +343,11 @@ export default defineComponent({
this.majorAcronyms.filter(it => it !== ''),
this.minorAcronyms.filter(it => it !== ''),
this.gradAcronym,
{ firstName: this.firstName, middleName: this.middleName, lastName: this.lastName }
{
firstName: this.firstName,
middleName: this.middleName,
lastName: this.lastName,
}
);
},
// Clear a major if a new college is selected and the major is not in it
Expand Down
3 changes: 3 additions & 0 deletions src/utilities.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

@benjamin-shen benjamin-shen Mar 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return 'Arts and Sciences';
}
const college = acronym ? requirementJSON.college[acronym] : null;
return college ? college.name : '';
}
Expand Down