collection backend #944

plumshum · 2024-10-02T22:11:22Z

Summary

This pull request is the first step towards adding collection into Firebase for implementing the Save Courses Feature.
Implement functionality to save selected courses into collection(s) in Firebase.

updated documentation: https://www.notion.so/Saved-Courses-8154144312034ccc914a6a37ef5ff0a6

Breaking Changes with Backend

Every user has a Plan 1 that's automatically added if the user has no plan, but the code doesn't seem to do this with the default Collection All, so that was breaking my entire Courseplan account. I was hoping a migration can fix it I also added a quick script for that.
add collection to firebase
when add a course to a collection, backend should be able to store that course into the collection
course is removed from semester view and removed from requirements
collection can be named by user before creation
front-end matches with figma design (fixed padding, highlighting and auto-boldening of the box)

Here's the latest demo:

Screen.Recording.2024-10-27.231911.mp4

Here's a Screenshot of the collections with a scrollbar and editability:

Here is a demo of adding one course into one newly created collection. (this is with the old front-end look, but the backend is the same)

Here is a demo of adding one course into multiple new created collections (this is with the old front-end look, but the backend is the same)

Remaining TODOs:

debugging saved courses (hopefully migration script can fix it!!)

Depends on #{number of PR}
#921

Test Plan

Notes

Concluded to store savedCourses in FireStoreSemesterData.

Blockers

A newly discovered dependency that hasn’t been addressed

Breaking Changes

store used to have collections now it's renamed to savedCourses for clarity.
Database schema change (anything that changes Firestore collection structure)
Other change that could cause problems (Detailed in notes)

dti-github-bot · 2024-10-02T22:11:38Z

[diff-counting] Significant lines: 615.

github-actions · 2024-10-02T22:13:02Z

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

https://cornelldti-courseplan-dev--pr944-hannah-collection-b-y8xwrfff.web.app

_{(expires Wed, 27 Nov 2024 03:25:37 GMT)}

_{🔥 via Firebase Hosting GitHub Action 🌎}

_{Sign: 6d4e0437c4559ed895272bbd63991394f1e0e933}

- can save a course when press 'done' - course is saved to an empty collection - add functions for add & edit collection

- can save one course to multiple collections w/ checkbox - saved course will be removed from semester view and from fullfilled requirements - pop up notification

Destaq · 2024-10-06T16:24:16Z

src/components/Modals/SaveCourseModal.vue

-            Checkbox Style
-            Need a collections variable in firestore
-        -->
+        <div v-for="collection in collections" :key="collection">


We would guarantee that collection names are unique then — not tracking internally with IDs and whatnot? (Is a reasonable assumption to make.)

I was thinking the same...I think this is a reasonable thing to assume for now.

Destaq · 2024-10-06T16:27:14Z

src/components/Modals/SaveCourseModal.vue

+            :value="collection"
+            v-model="checkedCollections"
+          />
+          <label for="collection">{{ collection }}</label>


Heads up, I noticed when testing that this label will always only toggle the very first collection when clicked. So e.g. the label for the fourth collection, when clicked, would toggle the checkmark for first collection.

I suspect that this is due to the non-uniqueness for id and for — could perhaps combine this with the indices or use "collections" literal value?

Additionally, would it be possible to have a little margin between the checkbox and the label? I appreciate that this is a draft PR though 👍 .

Destaq · 2024-10-06T16:27:47Z

src/components/Modals/SaveCourseModal.vue

+  },
+  data() {
+    return {
+      checkedCollections: [] as string[], // New data property to manage checked state


Good work with the type safety here~

Destaq · 2024-10-06T16:33:06Z

src/user-data.d.ts

@@ -36,6 +36,7 @@ type FirestoreSemester = {
 };

 type FirestoreSemestersData = {
+  // readonly collections: readonly Collection[]; Note: Not sure where to store collections


Hmm yes, this would be tricky. Happy to discuss with you and Nidhi sometime at dev sesh or work sesh!

Destaq · 2024-10-06T16:37:17Z

src/store.ts

@@ -326,6 +331,7 @@ export const initializeFirestoreListeners = (onLoad: () => void): (() => void) =
      const plan = getFirstPlan(data);
      store.commit('setPlans', data.plans);
      store.commit('setCurrentPlan', plan);
+      // store.commit('setCollections', data.collections); Note: toggle this on and off to save collections progress after refresh


Thank you for haring this here and in the main PR information section. Tiny quality of life improvement could be to add another // before the Note: so you don't have to add that when toggling, but really just nitpicking.

Destaq · 2024-10-06T16:40:09Z

src/gtag.ts

  | 'delete-course' // User deletes a course
+  | 'delete-course-collection' // User deletes a course from a collection


Nice stuff, thank you for commenting all of these so semantically!

Destaq · 2024-10-06T16:42:22Z

src/components/Modals/SaveCourseModal.vue

+    },
+    collections() {
+      const collections = store.state.collections.map(collection => collection.name);
+      return collections.length === 0 ? ['No collections added yet'] : collections;


Clean way of doing things, nice approach! Took a minute to understand so perhaps a one-line piece of documentation could be useful, but definitely the most straightforward approach — doesn't really on any extra attributes or anything.

Destaq · 2024-10-06T16:46:22Z

src/components/Modals/SaveCourseModal.vue

+    },
+    placeholder_name() {
+      const oldcollections = store.state.collections.map(collection => collection.name);
+      let newCollectionNum = 1;


Could we store this in the state somehow? Iterating through oldcollections like this is probably discouraged by eslint for a reason. I know it goes counter to my advice from earlier, but in this case I feel like it would be reasonable to store the latest collection number.

Though that said, if this is just a placeholder route and a user would be forced to enter a name or something, then can leave as be (again, I appreciate that this is still a draft-level PR).

I agree that storing this in state somehow would be better... also (for a very weird niche case that would be kind of annoying) if a renamed collection was named Collection 4 even if the collection was the second collection added, then the newCollectionNum wouldn't be updated correctly

Destaq · 2024-10-06T16:49:25Z

src/components/Semester/Semester.vue

+      if (toEdit !== undefined) {
+        editCollection(oldname, updater);
+      }
+      this.confirmationText = `${oldname} has been renamed to ${name}!`;


Maybe we could force capitalization of the first letter here for UI purposes.

For 'oldname'? Maybe we can ask design, but I'd think that we would have that be the exact same as the original name

Destaq · 2024-10-06T16:53:36Z

src/global-firestore-data/user-semesters.ts

+  year: number,
+  season: FirestoreSemesterSeason,
+  newCourse: FirestoreSemesterCourse,
+  collectionIDs: string[], // Array of collection IDs


Rather redundant to have this comment~

Destaq · 2024-10-06T16:54:43Z

src/global-firestore-data/user-semesters.ts

+    })
+  );
+
+  deleteCourseFromSemester(plan, year, season, newCourse.uniqueID);


I'd be curious about why we are necessarily removing something from a collection when we add from the dashboard main area, feels a bit counterintuitive. First time I tried it was annoyed that I lost the course and had to go remember what it was and find it.

Not an implementation question, more system design, would be interested to hear why this approach was taken.

Destaq

Nice work Hannah, great work on the draft PR. Added in some general comments and suggestions. I know that this is still a draft PR, so they are not definitive but hopefully helpful a bit at least — if you have any questions let me know.

My main issue / concern would be with clicking the labels and having those being incorrectly synced to the corresponding checkboxes.

- finished scrollable checkbox frontend - fixed the bug for selecting a singular checkbox - believed its stored in firestore when added to collection - renamed collection to savedCourses for firesetore clarity

- saveCourseModal allow user to name their new collections - moved scss into a separate file

nidhi-mylavarapu

Thank you for all of your hard work on this Hannah! looking forward to seeing how this aligns with the frontend :)

nidhi-mylavarapu · 2024-10-26T23:47:00Z

src/components/Course/Course.vue

+      const course = { ...this.courseObj };
+      this.$emit('save-course', course, collections);
+    },
+    /* only to rename the collection */


Thank you for adding these comments + details! your code is super clear - the add collection vs edit collection vs save course is very clearly divided. i know you're working on the merge right now but that should hopefully be pretty straightforward with your you divided all of the logic

nidhi-mylavarapu · 2024-10-26T23:54:31Z

src/components/Modals/SaveCourseModal.vue

+  },
+  data() {
+    return {
+      checkedCollections: [] as string[],


Super nit-picky, could add a comment here on what checkedCollections means just for clarification

nidhi-mylavarapu · 2024-10-27T00:03:00Z

src/components/Modals/SaveCourseModal.vue

+    },
+    placeholder_name() {
+      const oldcollections = store.state.collections.map(collection => collection.name);
+      let newCollectionNum = 1;


I agree that storing this in state somehow would be better... also (for a very weird niche case that would be kind of annoying) if a renamed collection was named Collection 4 even if the collection was the second collection added, then the newCollectionNum wouldn't be updated correctly

nidhi-mylavarapu · 2024-10-27T00:04:47Z

src/components/Semester/Semester.vue

+      if (toEdit !== undefined) {
+        editCollection(oldname, updater);
+      }
+      this.confirmationText = `${oldname} has been renamed to ${name}!`;


For 'oldname'? Maybe we can ask design, but I'd think that we would have that be the exact same as the original name

nidhi-mylavarapu · 2024-10-27T00:05:39Z

src/store.ts

@@ -326,17 +332,21 @@ export const initializeFirestoreListeners = (onLoad: () => void): (() => void) =
      const plan = getFirstPlan(data);
      store.commit('setPlans', data.plans);
      store.commit('setCurrentPlan', plan);
+      store.commit('setSavedCourses', data.savedCourses); // Note: toggle this on and off to save collections progress after refresh


Thank you for the clarifying comment here!!

nidhi-mylavarapu · 2024-10-27T00:06:33Z

src/store.ts

      const { orderByNewest } = data;
      store.commit('setSemesters', plan.semesters);
      updateDoc(doc(fb.semestersCollection, simplifiedUser.email), {
        plans: data.plans,
+        // savedCourses: data.savedCourses,


I saw the toggling comment from above... is there a more intuitive way (rather than toggling within the code) to handle the refresh progress? works for now, but something to consider.

- added updateDoc in backend functions - made edit Collections modal more functional with saveCourse

added collections in store. add backend methods and gtags

c475ccb

plumshum self-assigned this Oct 2, 2024

plumshum added 2 commits October 5, 2024 11:59

added basic functionality for save course

3108134

- can save a course when press 'done' - course is saved to an empty collection - add functions for add & edit collection

fixed functionality for save courses to mutliple collections

32add4c

- can save one course to multiple collections w/ checkbox - saved course will be removed from semester view and from fullfilled requirements - pop up notification

Destaq marked this pull request as ready for review October 6, 2024 16:15

Destaq requested a review from a team as a code owner October 6, 2024 16:15

Destaq marked this pull request as draft October 6, 2024 16:15

Destaq reviewed Oct 6, 2024

View reviewed changes

plumshum added 2 commits October 17, 2024 00:33

finished backend and fixed frontend

36f9925

- finished scrollable checkbox frontend - fixed the bug for selecting a singular checkbox - believed its stored in firestore when added to collection - renamed collection to savedCourses for firesetore clarity

created editabilify for collection names

5d4df16

- saveCourseModal allow user to name their new collections - moved scss into a separate file

plumshum marked this pull request as ready for review October 23, 2024 21:49

nidhi-mylavarapu reviewed Oct 27, 2024

View reviewed changes

plumshum marked this pull request as draft October 27, 2024 19:17

fixed backend error and logic

726dbe3

- added updateDoc in backend functions - made edit Collections modal more functional with saveCourse

plumshum marked this pull request as ready for review October 28, 2024 03:24

plumshum marked this pull request as draft October 28, 2024 03:32

plumshum merged commit f376e2c into saved-courses Oct 29, 2024
10 of 14 checks passed

plumshum deleted the hannah--collection-backend branch October 29, 2024 21:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

collection backend #944

collection backend #944

plumshum commented Oct 2, 2024 •

edited

Loading

dti-github-bot commented Oct 2, 2024 •

edited

Loading

github-actions bot commented Oct 2, 2024 •

edited

Loading

Destaq Oct 6, 2024

nidhi-mylavarapu Oct 23, 2024

Destaq Oct 6, 2024

Destaq Oct 6, 2024

Destaq Oct 6, 2024

Destaq Oct 6, 2024

Destaq Oct 6, 2024

Destaq Oct 6, 2024

Destaq Oct 6, 2024

nidhi-mylavarapu Oct 27, 2024

Destaq Oct 6, 2024

nidhi-mylavarapu Oct 27, 2024

Destaq Oct 6, 2024

Destaq Oct 6, 2024

Destaq left a comment

nidhi-mylavarapu left a comment

nidhi-mylavarapu Oct 26, 2024

nidhi-mylavarapu Oct 26, 2024

nidhi-mylavarapu Oct 27, 2024

nidhi-mylavarapu Oct 27, 2024

nidhi-mylavarapu Oct 27, 2024

nidhi-mylavarapu Oct 27, 2024

		\| 'delete-course' // User deletes a course
		\| 'delete-course-collection' // User deletes a course from a collection

collection backend #944

collection backend #944

Conversation

plumshum commented Oct 2, 2024 • edited Loading

Summary

Breaking Changes with Backend

Test Plan

Notes

Blockers

Breaking Changes

dti-github-bot commented Oct 2, 2024 • edited Loading

github-actions bot commented Oct 2, 2024 • edited Loading

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

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

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

Destaq left a comment

Choose a reason for hiding this comment

nidhi-mylavarapu 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

plumshum commented Oct 2, 2024 •

edited

Loading

dti-github-bot commented Oct 2, 2024 •

edited

Loading

github-actions bot commented Oct 2, 2024 •

edited

Loading