Feat: add course, module, unit, exercises, tags and studentExercises models #54
Conversation
prisma/schema.prisma
Outdated
| model Cohort { | ||
| id Int @id @default(autoincrement()) | ||
| name String? | ||
| course String? @unique |
There was a problem hiding this comment.
I'm not sure this should be unique, what if we want to have more than one cohort on a course?
There was a problem hiding this comment.
I mean, as far as i understand the name field that we have should be the cohort one, like: Cohort 1, but the course should be, for example, the Software Development, right? I thought that it shouldn't be possible to have the same course name, but the cohorts are fine, having different cohorts for uniques courses.
There was a problem hiding this comment.
What if we want to run another Software Development course? We'd have to name it Software Development 2, then 3, 4, 5...
I think a Course should be an entity, not just a string. We might want to store more information about the course, such as its modules (another entity), the exercises associated with the course, etc.
src/controllers/cohort.js
Outdated
| const foundCohorts = await getAllCohorts() | ||
|
|
||
| const courseInUse = foundCohorts.some((cohort) => | ||
| cohort.course | ||
| .split(' ') | ||
| .join('') | ||
| .toLowerCase() | ||
| .includes(course.split(' ').join('').toLowerCase()) | ||
| ) |
There was a problem hiding this comment.
If we have 100 cohorts and 5 teachers decide to create a new cohort, this will load 500 cohort objects into memory and then you'll be linearly looping over all of them to perform this some check. This is probably really inefficient.
I think we should do the filtering at the database level if it needs to be done. Have a prisma query that finds cohorts by course, it's much more efficient to do it at a DB level than in JS.
There was a problem hiding this comment.
For sure, will change it indeed!
LeonardoSaraceli
dismissed stale reviews from MyrtheDullaart and FBagdeli
via
7d58261
LeonardoSaraceli
changed the title
|
It seems like this is the result of some conversations and considerations. Please reflect these onto the github ticket description, so I can understand what exactly you are aiming to achieve with this and I can double check you implemented what was intended |
So, basically we needed to develop some models in the schema to be able to create cohorts. From this, we realized that what until then was a "Course" field needed to be a model, as it had several layers. After this analysis, I created the respective models. In order for you to understand more about the reason, you could take a look onto this design: https://www.figma.com/design/j9QIHvgM0wOVw3xDe9KdWB/My-Copy%3A-Cohort-Manager-app---Designs?node-id=239-14654&t=pqKAECywojG9w0oY-0 |
I see, but the attached github issue has no description and the title is simply "Update the cohort model and endpoint: add course field". Please update the github issue with a description of what exactly you are implementing, also remember to keep it up to date if there are further discussions here that might change it If needed, don't be afraid to split the ticket into more than one! |
|
The issue is still pretty vague: https://github.com/orgs/boolean-uk/projects/13/views/1?pane=issue&itemId=72756051 Imagine that someone else was building this feature and for some reason they're not able to come into work for a week so you have to pick up their feature and finish it, how much information would you want? You'd want to know exactly what the feature was, the decisions that were made and why, details about the approach taken, what the feature is solving, which epic it's related to etc. There's no such thing as too much context for things like this |
Uh oh!
There was an error while loading. Please reload this page.