feat: add shared attribute, add markshared mutation, add templateCour… #914

paul-jaworski · 2023-12-02T18:01:17Z

No description provided.

…ses mutation

Jonasdoubleyou

Looks pretty good already!
You also need to adapt authorizations.ts and mark the shared attribute as public, it's not something we need to keep private.

graphql/course/fields.ts

graphql/course/mutations.ts

Jonasdoubleyou · 2023-12-02T18:15:24Z

And this could use an integration test where one instructor publishes a course, and then another instructor is able to find it (c.f.

backend/integration-tests/07_course.ts

Line 240 in fef1203

void test('Search further instructors', async () => {

)

Jonasdoubleyou · 2023-12-02T18:17:45Z

And we should also relax the check in

backend/graphql/subcourse/mutations.ts

Line 82 in fef1203

await hasAccess(context, 'Course', course);

- The access check on the course should be checked if the course is shared.

paul-jaworski · 2023-12-02T18:52:53Z

And we should also relax the check in

backend/graphql/subcourse/mutations.ts

Line 82 in fef1203

await hasAccess(context, 'Course', course);

The access check on the course should be checked if the course is shared.

If I make the hasAccess method call conditional, the authorization method must not be @AuthorizedDeferred then right?

Jonasdoubleyou · 2023-12-02T19:00:05Z

Ah, true, we need two checks then. @Authorized(Role.INSTRUCTOR) and then manually check inside subcourseAdd whether the course is shared or the user is an instructor

paul-jaworski · 2024-01-13T18:51:05Z

Hey Jonas, this got a bit confusing to me after a long break. May you please sum up which methods and auth checks should be changed?

Thank you very much :)

Jonasdoubleyou · 2024-01-14T12:29:47Z

subcourseCreate should be available to all Role.INSTRUCTOR, then it should be checked whether the course is shared or owned by the instructor
An integration test is missing

paul-jaworski · 2024-01-14T18:14:31Z

I see that there is a method call "await hasAccess(context, 'Course', course);". Should the check whether the course is shared or owned by the instructor be included there?

…nto feat-974

Jonasdoubleyou

looks good, some minor touchups to be made then this is ready :)

Jonasdoubleyou · 2024-01-17T09:33:40Z

graphql/subcourse/mutations.ts


-        const student = await getSessionStudent(context, studentId);
-        await prisma.subcourse_instructors_student.create({ data: { subcourseId: result.id, studentId: student.id } });
+        if (isCourseSharedOrOwned) {


unnecessary if

Thanks, you are right since I already throw an error in line 92 when the if condition isnt met :)

Jonasdoubleyou · 2024-01-17T09:34:28Z

graphql/subcourse/mutations.ts

            },
        });
+        const isCourseSharedOrOwned = !!courseInstructorAssociation || course.shared;
+        if (!isCourseSharedOrOwned) {
+            logger.error(`Subcourse(${courseId}) is not shared or Student(${studentId}) is not an instructor of this subcourse`);


Subcourse -> Course

Jonasdoubleyou · 2024-01-17T09:36:08Z

graphql/subcourse/mutations.ts

@@ -71,7 +71,7 @@ class PublicLectureInput {
 @Resolver((of) => GraphQLModel.Subcourse)
 export class MutateSubcourseResolver {
    @Mutation((returns) => GraphQLModel.Subcourse)
-    @AuthorizedDeferred(Role.ADMIN, Role.OWNER)
+    @AuthorizedDeferred(Role.INSTRUCTOR)


Ah, and this must be @Authorized instead of @AuthorizedDeferred

Jonasdoubleyou

sorry, overlooked this earlier

Jonasdoubleyou · 2024-01-18T20:11:16Z

graphql/subcourse/mutations.ts

+        const courseInstructorAssociation = await prisma.course_instructors_student.findFirst({
+            where: {
+                courseId: courseId,
+                studentId: studentId,


Ah, this for sure is wrong, we cannot trust our users to pass in their id. Please do

const student = await getSessionStudent(context, studentId); ... studentId: student.id

Jonasdoubleyou

apart from fixing the integration tests this should be good now

Jonasdoubleyou · 2024-01-19T19:34:15Z

integration-tests/07_course.ts

@@ -21,6 +22,7 @@ const courseOne = test('Create Course One', async () => {
            description: "Why should I test if my users can do that for me in production?"
            category: club
            allowContact: true
+            shared: true


I think you need a separate mutation markShared afterwards

Can you please rephrase this comment? Do you mean that we need a separate test for shared and nonshared courses? For the current integration test I manually set the shared value to false. See line 414

No the courseCreate Mutation does not take a shared attribute, thus the test is failing?

Jonasdoubleyou · 2024-01-19T19:34:32Z

integration-tests/07_course.ts

@@ -126,7 +129,7 @@ export const subcourseOne = test('Create Subcourse', async () => {
                allowChatContactProspects: true
                allowChatContactParticipants: true
                groupChatType: ${ChatType.NORMAL}
-            }) { id }
+            } studentId: ${student.userID}) { id }


and this can be reverted as we now use the session student

paul-jaworski · 2024-01-27T18:05:03Z

@Jonasdoubleyou or @realmayus Can you please have another look here?

Jonasdoubleyou · 2024-01-28T12:15:56Z

Um, the tests are still failing, have you seen my comment regarding shared: true?

integration-tests/07_course.ts

graphql/course/fields.ts

graphql/subcourse/mutations.ts

integration-tests/07_course.ts

Jonasdoubleyou

nice!

feat: add shared attribute, add markshared mutation, add templateCour…

fb13997

…ses mutation

realmayus temporarily deployed to backend-feat-974-c5gmwsq0gykbg December 2, 2023 18:01 Inactive

paul-jaworski requested a review from Jonasdoubleyou December 2, 2023 18:01

Jonasdoubleyou reviewed Dec 2, 2023

View reviewed changes

graphql/course/fields.ts Outdated Show resolved Hide resolved

graphql/course/mutations.ts Outdated Show resolved Hide resolved

graphql/course/mutations.ts Show resolved Hide resolved

realmayus had a problem deploying to backend-feat-974-0jydgkpvtuy96 December 2, 2023 18:26 Failure

fix: fix authorization

1837993

realmayus temporarily deployed to backend-feat-974-0jydgkpvtuy96 December 2, 2023 18:29 Inactive

feat: add logging to courseMarkShared mutation

4bbd331

add shared to public fiields

7ed6d73

realmayus temporarily deployed to backend-feat-974-snrfhtlg33tmn December 2, 2023 19:05 Inactive

realmayus temporarily deployed to backend-feat-974-snrfhtlg33tmn December 2, 2023 19:21 Inactive

paul-jaworski and others added 2 commits December 2, 2023 20:21

integrate courseSearch method call in templateCourses mutation

17f52f1

Merge branch 'master' into feat-974

ace89df

paul-jaworski self-assigned this Jan 13, 2024

paul-jaworski added 2 commits January 17, 2024 10:00

feat: implement pr comments

b72f00d

Merge branch 'feat-974' of https://github.com/corona-school/backend i…

9ba7e7d

…nto feat-974

Jonasdoubleyou reviewed Jan 17, 2024

View reviewed changes

paul-jaworski added 2 commits January 18, 2024 18:14

fix: pr comments

f0d7984

feat: add integration tests for templateCourses

0a234d6

Jonasdoubleyou reviewed Jan 18, 2024

View reviewed changes

fix: pr comment

0182728

Jonasdoubleyou reviewed Jan 19, 2024

View reviewed changes

paul-jaworski and others added 3 commits January 22, 2024 19:59

fix: pr comments

45415b2

Merge branch 'master' into feat-974

a1ea25c

Merge branch 'master' into feat-974

8674a32

paul-jaworski added 2 commits February 1, 2024 19:48

Merge branch 'master' into feat-974

a656cbd

fix integration test

81db359

Jonasdoubleyou reviewed Feb 1, 2024

View reviewed changes

integration-tests/07_course.ts Outdated Show resolved Hide resolved

paul-jaworski added 4 commits February 4, 2024 20:53

add ) back

33f7da8

pass studentid to createsubcourse

381fbbf

add ,

a3db03c

Merge branch 'master' into feat-974

92eb0a8

Jonasdoubleyou reviewed Feb 10, 2024

View reviewed changes

graphql/course/fields.ts Outdated Show resolved Hide resolved

paul-jaworski added 3 commits February 11, 2024 20:08

fix integration tests

ebebc1d

Merge branch 'master' into feat-974

6c663d3

fix linting

927e7fc

Jonasdoubleyou reviewed Feb 11, 2024

View reviewed changes

paul-jaworski and others added 8 commits February 12, 2024 20:25

fix pr comments

e51f428

remove whitespace

9904d1a

fix pr comments

db66075

test query templateCourses eith non empty search string

449fe94

remove secondtest

5bd4a7e

Merge branch 'master' into feat-974

9e9d56d

Merge branch 'master' into feat-974

db9455e

rewrite prisma graphql query

ddd0684

paul-jaworski requested review from Jonasdoubleyou and realmayus March 30, 2024 06:21

Jonasdoubleyou approved these changes Mar 30, 2024

View reviewed changes

paul-jaworski merged commit b654673 into master Apr 1, 2024
2 checks passed

paul-jaworski deleted the feat-974 branch April 1, 2024 17:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add shared attribute, add markshared mutation, add templateCour… #914

feat: add shared attribute, add markshared mutation, add templateCour… #914

paul-jaworski commented Dec 2, 2023

Jonasdoubleyou left a comment

Jonasdoubleyou commented Dec 2, 2023

Jonasdoubleyou commented Dec 2, 2023

paul-jaworski commented Dec 2, 2023

Jonasdoubleyou commented Dec 2, 2023

paul-jaworski commented Jan 13, 2024

Jonasdoubleyou commented Jan 14, 2024

paul-jaworski commented Jan 14, 2024

Jonasdoubleyou left a comment

Jonasdoubleyou Jan 17, 2024

paul-jaworski Jan 18, 2024

Jonasdoubleyou Jan 17, 2024

Jonasdoubleyou Jan 17, 2024

Jonasdoubleyou left a comment

Jonasdoubleyou Jan 18, 2024

Jonasdoubleyou left a comment

Jonasdoubleyou Jan 19, 2024

paul-jaworski Jan 22, 2024

Jonasdoubleyou Jan 22, 2024

Jonasdoubleyou Jan 19, 2024

paul-jaworski commented Jan 27, 2024

Jonasdoubleyou commented Jan 28, 2024

Jonasdoubleyou left a comment

feat: add shared attribute, add markshared mutation, add templateCour… #914

feat: add shared attribute, add markshared mutation, add templateCour… #914

Conversation

paul-jaworski commented Dec 2, 2023

Jonasdoubleyou left a comment

Choose a reason for hiding this comment

Jonasdoubleyou commented Dec 2, 2023

Jonasdoubleyou commented Dec 2, 2023

paul-jaworski commented Dec 2, 2023

Jonasdoubleyou commented Dec 2, 2023

paul-jaworski commented Jan 13, 2024

Jonasdoubleyou commented Jan 14, 2024

paul-jaworski commented Jan 14, 2024

Jonasdoubleyou 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

Jonasdoubleyou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jonasdoubleyou 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

paul-jaworski commented Jan 27, 2024

Jonasdoubleyou commented Jan 28, 2024

Jonasdoubleyou left a comment

Choose a reason for hiding this comment