[Saved Courses] add collection into firesbase #921

plumshum · 2024-04-24T01:33:09Z

Summary

This pull request is the first step towards adding collection into Firebase for implementing the Save Courses Feature.

add collection to firebase
Add firebase function to modify (add, delete, edit) from collections (functionalities that aren't tested yet)
Stub functions into frontend components MOVED TO A NEW PR
- [ ] only for save course modal [Saved Courses] add save modal #912

Remaining TODOs

Depends on #{number of PR}
need to make sure it works with #912

Test Plan

Should make some JUnit tests

Notes

Blockers

A newly discovered dependency that hasn’t been addressed

Breaking Changes

store now has collections , need to update alot of code with semesters
Database schema change (anything that changes Firestore collection structure)
Other change that could cause problems (Detailed in notes)

dti-github-bot · 2024-04-24T01:33:22Z

[diff-counting] Significant lines: 147.

github-actions · 2024-04-24T01:36:11Z

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

https://cornelldti-courseplan-dev--pr921-hannah-add-firestore-hmirtull.web.app

_{(expires Fri, 31 May 2024 22:02:33 GMT)}

_{🔥 via Firebase Hosting GitHub Action 🌎}

_{Sign: 6d4e0437c4559ed895272bbd63991394f1e0e933}

Destaq

Great work Hannah! And I appreciate that this is still a quasi-draft PR (make sure that you open things appropriately in the future when not fully ready for review), so makes sense that there are some of the console.logs and TODOs. Just please ensure that these are all removed when all the other commits are finished, for the reasons I outlined.

In general, looks fantastic and you did a really good job with TypeScript too. Good to have a chance to start working with Vuex, that's very critical to the web app. In terms of future work, would just encourage you to do some minor refactoring (maybe not now, but at least before the merge request proper) and test that the Firebase writing works. Whenever you need any help finishing stuff up feel free to message or find me.

Also, if it would be possible to update the description for this PR that would be great, I think you may have left some parts of the initial boilerplate intact.

Destaq · 2024-05-02T00:51:04Z

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

+export const editCollections = async (
+  updater: (oldCollections: readonly Collection[]) => readonly Collection[]
+): Promise<void> => {
+  console.log('edit Collections');


If you don't need this for further debugging, would be great to remove — don't want to unnecessarily clutter the console.

Destaq · 2024-05-02T00:52:48Z

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

+  console.log('edit Collections');
+  const collections = updater(store.state.collections);
+  store.commit('setCollections', collections);
+  await updateDoc(doc(semestersCollection, store.state.currentFirebaseUser.email), {


If / when you are testing this on Firebase, you might get a warning about insufficient permissions. Nidhi and I just went through this last week. So a heads up if that does pop up: you need to update the 'rules' for that collection in the Firebase. Happy to help if you're experiencing trouble with that and not 100% sure what to do.

Should be ok for now -- the semestersCollection already allows for read and write

Destaq · 2024-05-02T00:53:38Z

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

@@ -56,6 +77,17 @@ export const editPlan = (name: string, updater: (oldPlan: Plan) => Plan): void =
  editPlans(oldPlan => oldPlan.map(plan => (plan.name === name ? updater(plan) : plan)));
 };

+const createCollection = (
+  name: string,


Would it be possible to abstract this into a local type? Improves a bit on maintainability.

I think it's ok for codebase consistency -- we initialize most of our objects (semester, plan, etc) like that

Destaq · 2024-05-02T00:53:58Z

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

+  courses: readonly FirestoreSemesterCourse[]
+): {
+  name: string;
+  courses: readonly FirestoreSemesterCourse[];


Great use of some advanced TS features, I don't think I've ever used readonly or even properly understand it 😁

Destaq · 2024-05-02T00:54:25Z

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

@@ -88,6 +120,15 @@ export const semesterEquals = (
  season: FirestoreSemesterSeason
 ): boolean => semester.year === year && semester.season === season;

+export const addCollection = async (
+  name: string,


See above — could abstract this (the first two parameters) and then connect to the optional parameter using the ampersand operator.

Destaq · 2024-05-02T00:55:38Z

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

+  gtag?: VueGtag
+): void => {
+  // TODO: 1) add course to collection 2) delete course from semester
+  // 3) cannot add course to collection if it is already in the collection


I imagine there will be a bit of a lull in development of other features at the beginning of next semester. See that you have a lot of TODOs; if you need any help am always available.

Destaq · 2024-05-02T00:57:06Z

src/user-data.d.ts

+
+type Collection = {
+  readonly name: string;
+  // TODO: add potential more fields


I mentioned earlier about using a local collection type — is this it? Maybe could use this?

elizabeth-tang

Awesome, it's really coming along Hannah!! I know some of the stuff Simon mentioned are present in the existing codebase so it's ok to leave to later to address. Thanks for working on this!

elizabeth-tang · 2024-05-05T17:19:59Z

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

+  console.log('edit Collections');
+  const collections = updater(store.state.collections);
+  store.commit('setCollections', collections);
+  await updateDoc(doc(semestersCollection, store.state.currentFirebaseUser.email), {


Should be ok for now -- the semestersCollection already allows for read and write

elizabeth-tang · 2024-05-05T17:25:59Z

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

+  await updateDoc(doc(semestersCollection, store.state.currentFirebaseUser.email), {
+    collections,
+  });
+  store.commit('setOrderByNewest', store.state.orderByNewest);


This shouldn't be necessary here since editing collections should not change the ordering of the semesters

elizabeth-tang · 2024-05-05T17:31:31Z

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

@@ -56,6 +77,17 @@ export const editPlan = (name: string, updater: (oldPlan: Plan) => Plan): void =
  editPlans(oldPlan => oldPlan.map(plan => (plan.name === name ? updater(plan) : plan)));
 };

+const createCollection = (
+  name: string,


I think it's ok for codebase consistency -- we initialize most of our objects (semester, plan, etc) like that

add collection into firestore

68bcd32

plumshum self-assigned this Apr 24, 2024

plumshum added 5 commits April 23, 2024 21:37

added function names

b0ea257

added code body for functions

3f485f8

add gtags

44964ce

added save courses functions to firebase

8dec983

added some fields

8d02367

plumshum changed the title ~~add collection into firesbase~~ [Saved Courses] add collection into firesbase Apr 24, 2024

plumshum added 2 commits April 30, 2024 22:32

added basic collection functinality into store

c28fb3a

edited user semesters w/ todo comments

6b5a856

plumshum marked this pull request as ready for review May 1, 2024 20:53

plumshum requested a review from a team as a code owner May 1, 2024 20:53

added functionalites for specific course in collections for firestore

1d44852

Destaq reviewed May 2, 2024

View reviewed changes

elizabeth-tang requested changes May 5, 2024

View reviewed changes

plumshum marked this pull request as draft September 19, 2024 20:55

plumshum mentioned this pull request Oct 2, 2024

collection backend #944

Merged

6 tasks

plumshum closed this Oct 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Saved Courses] add collection into firesbase #921

[Saved Courses] add collection into firesbase #921

plumshum commented Apr 24, 2024 •

edited

Loading

dti-github-bot commented Apr 24, 2024 •

edited

Loading

github-actions bot commented Apr 24, 2024 •

edited

Loading

Destaq left a comment

Destaq May 2, 2024

Destaq May 2, 2024

elizabeth-tang May 5, 2024

Destaq May 2, 2024

elizabeth-tang May 5, 2024

Destaq May 2, 2024

Destaq May 2, 2024

Destaq May 2, 2024

Destaq May 2, 2024

elizabeth-tang left a comment

elizabeth-tang May 5, 2024

elizabeth-tang May 5, 2024

elizabeth-tang May 5, 2024

[Saved Courses] add collection into firesbase #921

[Saved Courses] add collection into firesbase #921

Conversation

plumshum commented Apr 24, 2024 • edited Loading

Summary

Remaining TODOs

Test Plan

Notes

Blockers

Breaking Changes

dti-github-bot commented Apr 24, 2024 • edited Loading

github-actions bot commented Apr 24, 2024 • edited Loading

Destaq 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elizabeth-tang 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

plumshum commented Apr 24, 2024 •

edited

Loading

dti-github-bot commented Apr 24, 2024 •

edited

Loading

github-actions bot commented Apr 24, 2024 •

edited

Loading