Feat: offer course template #951
Conversation
common/achievement/index.ts
Outdated
| await createAchievement(awardedAchievement.template, userAchievement.userId, event.context); | ||
| const newAchievement = await createAchievement(awardedAchievement.template, userAchievement.userId, event.context); | ||
| if (newAchievement) { | ||
| await checkUserAchievement(newAchievement, event); |
There was a problem hiding this comment.
This has great potential for an endless loop 😅
How did you notice that we need it?
There was a problem hiding this comment.
When we create a new achievement we want to directly evaluate if it was already achieved, so we could generate the subsequent achievement.
This could only loop if we had endless acheivements in a group, which is not the case. the createAchievement function only works, if there are subsequent achievements in our currently checked achievement group.
There was a problem hiding this comment.
But can't the context change between these achievements? Every metric has it's own set of actions, which has it's own context.
This means that event A coming from action A will have a different context then event B coming from action B.
There was a problem hiding this comment.
You have a point here. We included a check for a sequential achievement being the last in the group to determine, whether to directly set its achievedAt value to the current Date or not.
common/achievement/util.ts
Outdated
| return null; | ||
| } | ||
| const transformedJson: AchievementContextType = { | ||
| user: user, | ||
| match: json['match'] ? json['match'] : undefined, | ||
| subcourse: json['subcourse'] ? json['subcourse'] : undefined, | ||
| }; | ||
| keys.forEach((key) => { | ||
| if (key !== 'match' && key !== 'subcourse') { |
There was a problem hiding this comment.
Can't we just copy all the keys and remove line 103 + 104? They are basically doing the same thing.
There was a problem hiding this comment.
this would then require us to change the AchievementContextType. I set match and subcourse to optional values.
common/courses/states.ts
Outdated
| @@ -76,6 +77,15 @@ export async function allowCourse(course: Course, screeningComment: string | nul | |||
| await publishSubcourse(subcourse); | |||
| } | |||
| } | |||
|
|
|||
| const subcourse = await prisma.subcourse.findFirst({ where: { courseId: course.id } }); | |||
There was a problem hiding this comment.
Can't we use the subcourses from line 74?
There was a problem hiding this comment.
I destructured the first element from the subcourses array.
graphql/course/mutations.ts
Outdated
| const subcourse = await prisma.subcourse.findFirst({ where: { courseId: courseId } }); | ||
| const instructors = await prisma.subcourse_instructors_student.findMany({ where: { subcourseId: subcourse.id }, select: { student: true } }); | ||
| const { context: contextData } = await prisma.user_achievement.findFirst({ | ||
| where: { group: 'student_offer_course', context: { path: ['relation'], equals: `subcourse/${subcourse.id}` } }, |
There was a problem hiding this comment.
Do we need the group here? Maybe it's enough to filter by relation?
There was a problem hiding this comment.
This group selector is necessary. If we were not filtering for the specific group, every achievement that had subcourse/{{id}} as a relation would be added a courseName, even if it had no use for it.
There was a problem hiding this comment.
An other issue could be that the wrong contextData was fetched and data will be saved to the other achievement of an other group, missing essential information, previously existent.
There was a problem hiding this comment.
This group selector is necessary. If we were not filtering for the specific group, every achievement that had subcourse/{{id}} as a relation would be added a courseName, even if it had no use for it.
The downside of referencing specific groups is that it might break completely over time if someone decides to rename the group. I guess having extra unnecessary data in the context doesn't hurt.
An other issue could be that the wrong contextData was fetched and data will be saved to the other achievement of an other group, missing essential information, previously existent.
This could be solved by iterating over the achievements instead of updateMany
There was a problem hiding this comment.
Generally, we should try to not hardcode information that will be set dynamically in the database. The groups are used for grouping and not identification right now.
There was a problem hiding this comment.
Alright I fixed this part and removed the group from the filters.
graphql/course/mutations.ts
Outdated
| where: { group: 'student_offer_course', context: { path: ['relation'], equals: `subcourse/${subcourse.id}` } }, | ||
| }); | ||
| contextData['courseName'] = result.name; | ||
| await prisma.user_achievement.updateMany({ |
There was a problem hiding this comment.
Are we sure that all the user achievements will have the same context? Maybe we should iterate of the list and update each individually.
There was a problem hiding this comment.
yes we are sure, because we selected the achievements of the same group. Achievements of the same group always share the same context.
There was a problem hiding this comment.
yes we are sure, because we selected the achievements of the same group. Achievements of the same group always share the same context.
🤔 is this actually true? I thought that the achievement will get the context of the event, which might be different. I think I'm missing something. Can you please elaborate more on where/how this is happening?
There was a problem hiding this comment.
I did update this part, because I think you have a point here and I then was not satisfied the way I initially built it.
I'd appreciate you having another look at it.
| match?: ContextMatch[]; | ||
| subcourse?: ContextSubcourse[]; |
There was a problem hiding this comment.
Why are these optional now? 🤔
What was done: