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

collection backend #944

Merged
merged 6 commits into from
Oct 29, 2024
Merged

collection backend #944

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
c475ccb
added collections in store. add backend methods and gtags
plumshum Oct 2, 2024
3108134
added basic functionality for save course
plumshum Oct 5, 2024
32add4c
fixed functionality for save courses to mutliple collections
plumshum Oct 5, 2024
36f9925
finished backend and fixed frontend
plumshum Oct 17, 2024
5d4df16
created editabilify for collection names
plumshum Oct 18, 2024
726dbe3
fixed backend error and logic
plumshum Oct 28, 2024
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: 17 additions & 8 deletions src/components/Course/Course.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,9 @@
<save-course-modal
:courseCode="courseCode"
@close-save-course-modal="closeSaveCourseModal"
@add-course-collection="addCourseCollection"
@save-course="saveCourse"
@add-collection="addCollection"
@edit-collection="editCollection"
v-if="isSaveCourseOpen"
/>
<edit-color
Expand Down Expand Up @@ -88,9 +89,9 @@ import {
reportCourseColorChange,
reportSubjectColorChange,
} from '@/components/BottomBar/BottomBarState';
import { isCourseConflict } from '@/store';
import { clickOutside } from '@/utilities';
import EditColor from '../Modals/EditColor.vue';
import { isCourseConflict } from '@/store';

export default defineComponent({
components: { CourseCaution, CourseMenu, EditColor, SaveCourseModal },
Expand All @@ -113,6 +114,11 @@ export default defineComponent({
'course-on-click': (course: FirestoreSemesterCourse) => typeof course === 'object',
'edit-course-credit': (credit: number, uniqueID: number) =>
typeof credit === 'number' && typeof uniqueID === 'number',
'save-course': (course: FirestoreSemesterCourse, collections: string[]) =>
typeof course === 'object' && typeof collections === 'object',
'add-collection': (name: string) => typeof name === 'string',
'edit-collection': (name: string, oldname: string) =>
typeof name === 'string' && typeof oldname === 'string',
},
data() {
return {
Expand Down Expand Up @@ -182,13 +188,16 @@ export default defineComponent({
closeEditColorModal() {
this.isEditColorOpen = false;
},
addCollection() {
this.isSaveCourseOpen = false;
// TODO: implement add collection
addCollection(name: string) {
this.$emit('add-collection', name);
},
addCourseCollection() {
this.isSaveCourseOpen = false;
// TODO: implement save course
saveCourse(collections: string[]) {
const course = { ...this.courseObj };
this.$emit('save-course', course, collections);
},
/* only to rename the collection */
Copy link
Contributor

Choose a reason for hiding this comment

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

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

editCollection(name: string, oldname: string) {
this.$emit('edit-collection', name, oldname);
},
colorCourse(color: string) {
this.$emit('color-course', color, this.courseObj.uniqueID, this.courseObj.code);
Expand Down
57 changes: 42 additions & 15 deletions src/components/Modals/SaveCourseModal.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,19 +17,24 @@
<!-- create a rectangular border-->
<div class="saveCourseModal-header-text">
<span> Collections </span>
<button class="saveCourseModal-header-text-addButton" @click="addNewCollection">
<button class="saveCourseModal-header-text-addButton" @click="addCollection">
<img src="@/assets/images/plus.svg" alt="add new collection" />
</button>
</div>
</div>

<div class="saveCourseModal-body">
<div class="saveCourseModal-body-content">
<p>{{ collection }}</p>
<!--Must find all possible collections
Checkbox Style
Need a collections variable in firestore
-->
<div v-for="collection in collections" :key="collection">
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

<input
v-if:="!isDefaultCollection"
type="checkbox"
id="collection"
:value="collection"
v-model="checkedCollections"
/>
<label for="collection">{{ collection }}</label>
Copy link
Member

Choose a reason for hiding this comment

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

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 👍 .

</div>
</div>
</div>
</teleport-modal>
Expand All @@ -38,30 +43,52 @@
<script lang="ts">
import { defineComponent } from 'vue';
import TeleportModal from '@/components/Modals/TeleportModal.vue';
import store from '@/store';

export default defineComponent({
components: { TeleportModal },
props: {
courseCode: { type: String, required: true },
collection: { type: String, default: 'No collections added yet' },
isdefaultCollection: { type: Boolean, default: true },
},
data() {
return {
checkedCollections: [] as string[], // New data property to manage checked state
Copy link
Member

Choose a reason for hiding this comment

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

Good work with the type safety here~

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

Choose a reason for hiding this comment

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

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.

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

Choose a reason for hiding this comment

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

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).

Copy link
Contributor

Choose a reason for hiding this comment

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

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

// eslint-disable-next-line no-loop-func
while (oldcollections.find(p => p === `New Collection ${newCollectionNum}`)) {
newCollectionNum += 1;
}
return `New Collection ${newCollectionNum}`;
},
},
emits: {
'close-save-course-modal': () => true,
'save-course': (name: string) => typeof name === 'string',
'open-add-collection-modal': () => true,
'save-course': (collections: string[]) => typeof collections === 'object',
'add-collection': (name: string) => typeof name === 'string',
},
methods: {
closeCurrentModal() {
this.$emit('close-save-course-modal');
},
saveCourse() {
this.$emit('save-course', this.courseCode);
this.$emit('close-save-course-modal');
this.$emit('save-course', this.checkedCollections);
this.closeCurrentModal();
},
addNewCollection() {
this.$emit('close-save-course-modal');
this.$emit('open-add-collection-modal');
addCollection() {
this.$emit('add-collection', this.placeholder_name);
},
},
});
Expand Down
35 changes: 35 additions & 0 deletions src/components/Semester/Semester.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,9 @@
@color-subject="colorSubject"
@course-on-click="courseOnClick"
@edit-course-credit="editCourseCredit"
@save-course="saveCourse"
@add-collection="addCollection"
@edit-collection="editCollection"
/>
<placeholder
v-else
Expand Down Expand Up @@ -165,6 +168,9 @@ import {
deleteCourseFromSemester,
deleteAllCoursesFromSemester,
updateRequirementChoices,
addCollection,
addCourseToCollections,
editCollection,
} from '@/global-firestore-data';
import store, { updateSubjectColorData } from '@/store';
import {
Expand Down Expand Up @@ -408,6 +414,35 @@ export default defineComponent({
closeConfirmationModal() {
this.isConfirmationOpen = false;
},

saveCourse(course: FirestoreSemesterCourse, collections: string[]) {
addCourseToCollections(store.state.currentPlan, this.year, this.season, course, collections);
this.openConfirmationModal(`Saved ${course.code} to ${collections.join(', ')}`);
},
addCollection(name: string) {
addCollection(name, []);
this.confirmationText = `${name} has been added!`;
this.isConfirmationOpen = true;
setTimeout(() => {
this.isConfirmationOpen = false;
}, 2000);
},
editCollection(oldname: string, name: string) {
const { collections } = store.state;
const toEdit = collections.find(collection => collection.name === oldname);
const updater = (collection: Collection): Collection => ({
name,
courses: collection.courses,
});
if (toEdit !== undefined) {
editCollection(oldname, updater);
}
this.confirmationText = `${oldname} has been renamed to ${name}!`;
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

this.isConfirmationOpen = true;
setTimeout(() => {
this.isConfirmationOpen = false;
}, 2000);
},
// TODO @willespencer refactor the below methods after gatekeep removed (to only 1 method)
addCourse(data: CornellCourseRosterCourse, choice: FirestoreCourseOptInOptOutChoices) {
const newCourse = cornellCourseRosterCourseToFirebaseSemesterCourseWithGlobalData(data);
Expand Down
6 changes: 6 additions & 0 deletions src/global-firestore-data/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,18 @@ export {
updateSawNewFeature,
} from './user-onboarding-data';
export {
editCollections,
editCollection,
editPlans,
editPlan,
editSemesters,
editSemester,
addCollection,
addCourseToCollections,
addSemester,
addPlan,
deleteCollection,
deleteCourseFromCollection,
deletePlan,
deleteSemester,
addCourseToSemester,
Expand Down
88 changes: 88 additions & 0 deletions src/global-firestore-data/user-semesters.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,13 @@ import {
deleteCoursesFromRequirementChoices,
} from './user-overridden-fulfillment-choices';

export const editCollections = async (
updater: (oldCollections: readonly Collection[]) => readonly Collection[]
): Promise<void> => {
const collections = updater(store.state.collections);
store.commit('setCollections', collections);
};

export const editSemesters = (
plan: Plan,
updater: (oldSemesters: readonly FirestoreSemester[]) => readonly FirestoreSemester[]
Expand Down Expand Up @@ -41,6 +48,15 @@ export const setOrderByNewest = (orderByNewest: boolean): void => {
});
};

export const editCollection = (
name: string,
updater: (oldCollection: Collection) => Collection
): void => {
editCollections(oldCollection =>
oldCollection.map(collection => (collection.name === name ? updater(collection) : collection))
);
};

export const editSemester = (
plan: Plan,
year: number,
Expand All @@ -56,6 +72,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,
courses: readonly FirestoreSemesterCourse[]
): {
name: string;
courses: readonly FirestoreSemesterCourse[];
} => ({
name,
courses,
});

const createSemester = (
year: number,
season: FirestoreSemesterSeason,
Expand Down Expand Up @@ -88,6 +115,15 @@ export const semesterEquals = (
season: FirestoreSemesterSeason
): boolean => semester.year === year && semester.season === season;

export const addCollection = async (
name: string,
courses: readonly FirestoreSemesterCourse[],
gtag?: VueGtag
): Promise<void> => {
GTagEvent(gtag, 'add-collection');
await editCollections(oldCollections => [...oldCollections, createCollection(name, courses)]);
};

export const addSemester = (
plan: Plan,
year: number,
Expand All @@ -112,6 +148,16 @@ export const addPlan = async (
);
};

/** [deleteCollection] delete an entire collection. Now all courses from
* the collection can be added to semesters.
*/
export const deleteCollection = async (name: string, gtag?: VueGtag): Promise<void> => {
GTagEvent(gtag, 'delete-collection');
if (store.state.collections.some(p => p.name === name)) {
await editCollections(oldCollections => oldCollections.filter(p => p.name !== name));
}
};

export const deleteSemester = (
plan: Plan,
year: number,
Expand All @@ -138,6 +184,48 @@ export const deletePlan = async (name: string, gtag?: VueGtag): Promise<void> =>
store.commit('setCurrentPlan', store.state.plans[0]);
};

/** Add one course to multiple collections.
* This course is removed from the requirement choices.
*/
export const addCourseToCollections = (
plan: Plan,
year: number,
season: FirestoreSemesterSeason,
newCourse: FirestoreSemesterCourse,
collectionIDs: string[], // Array of collection IDs
Copy link
Member

Choose a reason for hiding this comment

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

Rather redundant to have this comment~

gtag?: VueGtag
): void => {
GTagEvent(gtag, 'add-course-collections');
editCollections(oldCollections =>
oldCollections.map(collection => {
if (collectionIDs.includes(collection.name)) {
return { ...collection, courses: [...collection.courses, newCourse] };
}
return collection;
})
);

deleteCourseFromSemester(plan, year, season, newCourse.uniqueID);
Copy link
Member

Choose a reason for hiding this comment

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

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.

deleteCourseFromRequirementChoices(newCourse.uniqueID);
};

/** Delete a course from a certain collection. */
export const deleteCourseFromCollection = (
plan: Plan,
year: number,
season: FirestoreSemesterSeason,
name: string,
courseUniqueID: number,
gtag?: VueGtag
): void => {
// delete course from collection
GTagEvent(gtag, 'delete-course-collection');
editCollection(name, oldCollection => ({
...oldCollection,
courses: oldCollection.courses.filter(course => course.uniqueID !== courseUniqueID),
}));
};

export const addCourseToSemester = (
plan: Plan,
year: number,
Expand Down
Loading
Loading