Standardize semester type/season naming convention by benjamin-shen · Pull Request #546 · cornell-dti/course-plan

benjamin-shen · 2021-10-19T03:15:05Z

Summary

We used "semester type" before we were using TypeScript and it worked for a while, but now "type" can be very ambiguous -- meanwhile, "season" has no ambiguity and we've actually been using both terms for a while now. This pull request officially standardizes the naming convention by changing most instances of "semester type" to "semester season".

changed FirestoreSemesterType to FirestoreSemesterSeason in user-data.d.ts
changed as many instances of "semester type" to "semester season" that I could find and safely remove
standardized argument order (year then semester) in global-firestore-data/semesters.ts
simplified some functions in global-firestore-data/semesters.ts
cleaned up some util functions, clear semester functionality, and toggle semester order functionality

TODO

fully remove all instances of type after devs pull this PR. it needs to remain in some places for now; otherwise, it breaks staging

Test Plan

Verify that all semester-related functionality still works as intended.

Notes

If there are merge conflicts: accept your current changes, change "semester type" to "semester season" where appropriate, verify that the web app builds, and make sure to npm run type-check.

…nto ben/standardize-season-year-terminology

dti-github-bot · 2021-10-19T03:15:22Z

[diff-counting] Significant lines: 519.

github-actions · 2021-10-19T03:17:56Z

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

https://cornelldti-courseplan-dev--pr546-ben-standardize-seas-r6ofacz8.web.app

_{(expires Sun, 31 Oct 2021 01:41:54 GMT)}

_{🔥 via Firebase Hosting GitHub Action 🌎}

willespencer

Identified a bug - existing semesters will still have the "type" key in Firestore documents, but any new semester will have "season," leading to issues where seasons don't show up on CP. Need either a migration script or need to check for both of these keys in the code.

benjamin-shen · 2021-10-19T21:15:25Z

-              v-for="season in seasons"
-              :key="season[1]"
+              v-for="s in seasons"
+              :key="s[1]"
              class="selectSemester-dropdown-content-item"
-              @click="selectSeason(season[1])"
+              @click="selectSeason(s[1])"
              data-cyId="newSemester-seasonItem"
            >
              <img
-                :src="season[0]"
+                :src="s[0]"
                class="selectSemester-dropdown-content-season"
-                :alt="`${season[1]} icon`"
+                :alt="`${s[1]} icon`"
              />
-              {{ season[1] }}
+              {{ s[1] }}


had to rename since season is now a prop name

benjamin-shen · 2021-10-19T21:16:06Z

-    getCurrentSeason(): FirestoreSemesterType {
-      let currentSeason: FirestoreSemesterType;
-      const currentMonth = new Date().getMonth();
-      if (currentMonth === 0) {
-        currentSeason = 'Winter';
-      } else if (currentMonth <= 4) {
-        currentSeason = 'Spring';
-      } else if (currentMonth <= 7) {
-        currentSeason = 'Summer';
-      } else {
-        currentSeason = 'Fall';
-      }
-      return currentSeason;
-    },


duplicate function in @/utilities

benjamin-shen · 2021-10-19T21:19:07Z

-export function getCurrentType(): 'WI' | 'SP' | 'SU' | 'FA' {
-  const currentMonth = new Date().getMonth();
-  if (currentMonth === 0) return 'WI';
-  if (currentMonth <= 4) return 'SP';
-  if (currentMonth <= 7) return 'SU';
-  return 'FA';
-}
-


hahnbeelee · 2021-10-20T05:58:49Z

Just wanted to say - overall looks good to me. I want to check all the season semester functionality in depth but it's getting a little late so ill do it tmrw!

hahnbeelee

@vaishnavi17 's semester population doesn't seem to be working here? (I deleted my firestore data and went through onboarding again)
All other semester functionality seems to be good.

…nto ben/standardize-season-year-terminology

benjamin-shen · 2021-10-23T00:48:41Z

@hahnbeelee thanks for the catch. The bug existed because I hadn't merged Vaish's PR from master, it should be fixed now.

merge Vaish's changes
merge Toby's changes
merge Zach's changes

Then I will push to master!! 😄

…nto ben/standardize-season-year-terminology

benjamin-shen · 2021-10-24T01:15:06Z

-  props: {
-    clearSemType: { type: String, required: true },
-    clearSemYear: { type: Number, required: true },
-  },
  emits: {
    'close-clear-sem': () => true,
-    'clear-semester': (type: string, year: number) =>
-      typeof type === 'string' && typeof year === 'number',
+    'clear-semester': () => true,


These are redundant props because the <clear-semester> component is a child of the <semester> prop.

I think it was modeled after delete-semester, which does need a season & year passed in to delete the semester (because it is emitted to the grandparent <semester-view>). Instead, clear-semester is more similar to add-course and delete-course, despite what the naming suggests.

benjamin-shen · 2021-10-24T01:19:35Z

-// compare function for FirestoreSemester to determine which comes first by year and type/season
-export const compareFirestoreSemesters = (a: FirestoreSemester, b: FirestoreSemester): number => {
-  if (a.type === b.type && a.year === b.year) {
-    return 0;
-  }
-  if (a.year > b.year) {
-    return -1;
-  }
-  if (a.year < b.year) {
-    return 1;
-  }
-  if (SeasonOrdinal[a.type] < SeasonOrdinal[b.type]) {
-    return 1;
-  }
-  return -1;
-};


We can get rid of this; sortedSemesters() has the same purpose. The only difference is that we don't modify the array directly anymore (think sorted() vs .sort(), which is good to avoid side effects & when working with readonly arrays.

benjamin-shen · 2021-10-24T01:24:02Z

-export const toggleOrderByNewest = (): boolean => {
-  const toggled = !store.state.orderByNewest;
-  store.commit('setOrderByNewest', toggled);
+export const setOrderByNewest = (orderByNewest: boolean): void => {


Changed toggle (which set the negated boolean value) to set (which sets a specific boolean value) to simplify the code in the frontend component and unify the global-firestore-data functions (i.e. setBlah(newValue) or editBlah(updater)). It should have the same functionality.

Lmk if you have a different opinion and I can change this back!

benjamin-shen · 2021-10-24T01:27:12Z

  return value;
 }

-export default function getCurrentSeason(): FirestoreSemesterType {


No reason for this to be a default export

hahnbeelee

yerssirrrrrrrrrrrrrrrrrrrrrrrrr. so clean 🙏

willespencer

This is so great, thanks Ben!! Specifically tested to make sure that nothing breaks when users have the "type" prop in Firestore, and that duplicate semester prevention, re-ordering semesters, and adding courses to a semester from the req bar add modal all work as expected.

My only remaining question is if we want to write a data migration script if the code already works and handles both semester and type for existing users?

If we do choose to write it, maybe we should have a release with this change that handles both types, then run the migration script on prod so users only have season, and finally push another release that does not use type anymore, so there is never a period of time where user data does not match the prod code.

benjamin-shen added 3 commits October 18, 2021 22:34

rename semester type to season

5d30369

Merge branch 'master' of https://github.com/cornell-dti/course-plan i…

815a116

…nto ben/standardize-season-year-terminology

standardize argument order (year -> semester)

ec36ee0

willespencer reviewed Oct 19, 2021

View reviewed changes

unbreak staging by adding back type in certain places

71f7279

benjamin-shen commented Oct 19, 2021

View reviewed changes

benjamin-shen marked this pull request as ready for review October 19, 2021 21:20

benjamin-shen requested a review from a team as a code owner October 19, 2021 21:20

hahnbeelee suggested changes Oct 22, 2021

View reviewed changes

benjamin-shen added 3 commits October 22, 2021 12:16

Merge branch 'master' of https://github.com/cornell-dti/course-plan i…

450de47

…nto ben/standardize-season-year-terminology

resolve issues after merge

c015377

fix prepopulate semesters tests, add semester equals tests

6c8f689

benjamin-shen added 4 commits October 22, 2021 20:49

Merge branch 'master' of https://github.com/cornell-dti/course-plan i…

3622380

…nto ben/standardize-season-year-terminology

resolve issues after merge, simplify clear semester emit

1af8eca

Merge branch 'master' of https://github.com/cornell-dti/course-plan i…

7f3fb1b

…nto ben/standardize-season-year-terminology

resolve issues after merge, tweak toggleOrderByNewest

ef35d95

benjamin-shen commented Oct 24, 2021

View reviewed changes

Minor tweaks

348ee96

benjamin-shen requested a review from hahnbeelee October 24, 2021 01:39

hahnbeelee approved these changes Oct 25, 2021

View reviewed changes

willespencer approved these changes Oct 26, 2021

View reviewed changes

benjamin-shen merged commit 2001c50 into master Oct 26, 2021

benjamin-shen deleted the ben/standardize-season-year-terminology branch October 26, 2021 17:52

benjamin-shen mentioned this pull request Nov 16, 2021

Add semester season property from semester type #593

Merged

Uh oh!

Conversation

benjamin-shen commented Oct 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test Plan

Notes

Uh oh!

dti-github-bot commented Oct 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Oct 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

willespencer left a comment

Choose a reason for hiding this comment

Uh oh!

benjamin-shen Oct 19, 2021

Choose a reason for hiding this comment

Uh oh!

benjamin-shen Oct 19, 2021

Choose a reason for hiding this comment

Uh oh!

benjamin-shen Oct 19, 2021

Choose a reason for hiding this comment

Uh oh!

hahnbeelee commented Oct 20, 2021

Uh oh!

hahnbeelee left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benjamin-shen commented Oct 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benjamin-shen Oct 24, 2021

Choose a reason for hiding this comment

Uh oh!

benjamin-shen Oct 24, 2021

Choose a reason for hiding this comment

Uh oh!

benjamin-shen Oct 24, 2021

Choose a reason for hiding this comment

Uh oh!

benjamin-shen Oct 24, 2021

Choose a reason for hiding this comment

Uh oh!

hahnbeelee left a comment

Choose a reason for hiding this comment

Uh oh!

willespencer left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

benjamin-shen commented Oct 19, 2021 •

edited

Loading

dti-github-bot commented Oct 19, 2021 •

edited

Loading

github-actions Bot commented Oct 19, 2021 •

edited

Loading

hahnbeelee left a comment •

edited

Loading

benjamin-shen commented Oct 23, 2021 •

edited

Loading