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.

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

EW-1060 common cartridge export via cc-microservice #5351

Open
wants to merge 74 commits into
base: main
Choose a base branch
from
Open

Conversation

Fshmit
Copy link
Contributor

@Fshmit Fshmit commented Nov 22, 2024

Description

Courses can now be exported in Common Cartridge format using the CC microservice. This export process has been transferred to the Common Cartridge microservice, which collects all course data via http from the schulcloud-server.

Links to Tickets or other pull requests

Ticket

Changes

Datasecurity

Deployment

New Repos, NPM pakages or vendor scripts

Approval for review

  • DEV: If api was changed - generate-client:server was executed in vue frontend and changes were tested and put in a PR with the same branch name.
  • QA: In addition to review, the code has been manually tested (if manual testing is possible)
  • All points were discussed with the ticket creator, support-team or product owner. The code upholds all quality guidelines from the PR-template.

Notice: Please remove the WIP label if the PR is ready to review, otherwise nobody will review it.

@Fshmit Fshmit self-assigned this Nov 22, 2024
Copy link
Contributor

@psachmann psachmann left a comment

Choose a reason for hiding this comment

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

One Typo, rest locks fine.

public readonly tasks!: string[];

@IsArray()
@ApiProperty({
Copy link
Contributor

Choose a reason for hiding this comment

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

discuss if @ApiProperty or @ApiPropertyOptional

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done ✅

Comment on lines 15 to 18

timeStamps: TimestampResponseDto;

constructor(
id: string,
title: string,
height: number,
elements: CardResponseElementsInnerDto[],
visibilitySettings: VisibilitySettingsResponseDto,
timestamps: TimestampResponseDto
) {
this.id = id;
this.title = title;
this.height = height;
this.elements = elements;
this.visibilitySettings = visibilitySettings;
this.timeStamps = timestamps;
constructor(props: Readonly<CardResponseDto>) {
Copy link
Contributor

Choose a reason for hiding this comment

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

accessibility modifier missing((is this new esLint rules)).
same for all dto files

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done ✅

Comment on lines 10 to 11
required: true,
nullable: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

default from ApiProperty can be removed
otherwise ApiPropertyOptional

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done ✅

}

public mapTaskToResource(task: BoardTaskDto, version: CommonCartridgeVersion): CommonCartridgeResourceProps {
const intendedUse = (() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing return type on function

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const intendedUse = (() => {
const intendedUse = (() => {
``` const intendedUse = ((): CommonCartridgeIntendedUseType => {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done ✅

task: LessonLinkedTaskDto,
version: CommonCartridgeVersion
): CommonCartridgeResourceProps {
const intendedUse = (() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done ✅

Comment on lines +116 to +117
if (key === 'title') title = value;
if (key === 'url') url = value;
Copy link
Contributor

Choose a reason for hiding this comment

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

missing test case ( coverage )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it is a part of the bug in EW-1090. it will be solved also there.

allowedAttributes: {},
}).slice(0, 20);

return title.length > 20 ? `${title}...` : title;
Copy link
Contributor

Choose a reason for hiding this comment

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

missing test case ( coverage )

this.addComponentToOrganization(content, lessonsOrganization);
});

lesson.linkedTasks?.forEach((task) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

test case when empty

identifier: createIdentifier(columnId),
});

if (column.cards?.length) {
Copy link
Contributor

Choose a reason for hiding this comment

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

test case when no cards

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done ✅

Comment on lines 16 to 23
import { BoardTaskDto } from '../common-cartridge-client/room-client/dto/board-task.dto';
import { createIdentifier } from '../export/utils';
import { BoardElementDto } from '../common-cartridge-client/room-client/dto/board-element.dto';
import { BoardElementDtoType } from '../common-cartridge-client/room-client/enums/board-element.enum';
import { CardResponseDto } from '../common-cartridge-client/card-client/dto/card-response.dto';
import { CardResponseElementsInnerDto } from '../common-cartridge-client/card-client/types/card-response-elements-inner.type';
import { RichTextElementResponseDto } from '../common-cartridge-client/card-client/dto/rich-text-element-response.dto';
import { LinkElementResponseDto } from '../common-cartridge-client/card-client/dto/link-element-response.dto';
Copy link
Contributor

Choose a reason for hiding this comment

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

index file for Dtos no deep import

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done ✅

Comment on lines +25 to 30
const isRichTextElement = (reference: unknown): reference is RichTextElementResponseDto =>
reference instanceof RichTextElementResponseDto;

const isLinkElement = (reference: unknown): reference is LinkElementResponseDto =>
reference instanceof LinkElementResponseDto;

Copy link
Contributor

Choose a reason for hiding this comment

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

move into the class

Copy link
Contributor Author

@Fshmit Fshmit Dec 13, 2024

Choose a reason for hiding this comment

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

it is just used in this service. not necessary to move it to a separate class.

Copy link
Contributor

Choose a reason for hiding this comment

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

i mean inside the service class

Comment on lines 1 to 245
hidden: faker.datatype.boolean(),
};
});

export const lessonContentFactory = Factory.define<LessonContentDto>(({ sequence }) => {
return {
id: sequence.toString(),
type: faker.lorem.word(),
content: { text: 'text' },
title: faker.lorem.sentence(),
component: 'text',
hidden: faker.datatype.boolean(),
};
});

export const lessonFactory = Factory.define<LessonDto>(({ sequence }) => {
return {
lessonId: sequence.toString(),
name: faker.lorem.word(),
courseId: undefined,
courseGroupId: faker.string.uuid(),
hidden: faker.datatype.boolean(),
position: faker.number.int(),
contents: [lessonContentFactory.build(), lernstoreContentFactory.build()],
materials: [],
linkedTasks: [lessonLinkedTaskFactory.build(), lessonLinkedTaskFactory.build()],
};
});

export const boardLessonFactory = Factory.define<BoardLessonDto>(() => {
return {
id: faker.string.uuid(),
name: faker.lorem.word(),
courseName: undefined,
hidden: faker.datatype.boolean(),
numberOfPublishedTasks: faker.number.int(),
numberOfDraftTasks: faker.number.int(),
numberOfPlannedTasks: faker.number.int(),
createdAt: faker.date.recent().toISOString(),
updatedAt: faker.date.recent().toISOString(),
};
});

export const boardTaskFactory = Factory.define<BoardTaskDto>(({ sequence }) => {
return {
id: sequence.toString(),
name: faker.lorem.word(),
createdAt: faker.date.recent().toISOString(),
updatedAt: faker.date.recent().toISOString(),
availableDate: faker.date.recent().toISOString(),
courseName: undefined,
description: faker.lorem.word(),
displayColor: faker.lorem.word(),
dueDate: faker.date.recent().toISOString(),
status: new BoardTaskStatusDto({
submitted: faker.number.int(),
maxSubmissions: faker.number.int(),
graded: faker.number.int(),
isDraft: faker.datatype.boolean(),
isSubstitutionTeacher: faker.datatype.boolean(),
isFinished: faker.datatype.boolean(),
}),
};
});

export const boardCloumnBoardFactory = Factory.define<BoardColumnBoardDto>(() => {
return {
id: faker.string.uuid(),
title: faker.lorem.word(),
published: faker.datatype.boolean(),
createdAt: faker.date.recent().toISOString(),
updatedAt: faker.date.recent().toISOString(),
columnBoardId: faker.string.uuid(),
layout: BoardLayout.COLUMNS,
};
});

export const roomFactory = Factory.define<RoomBoardDto>(({ sequence }) => {
return {
roomId: sequence.toString(),
title: faker.lorem.word(),
displayColor: faker.lorem.word(),
elements: [
{
type: BoardElementDtoType.TASK,
content: boardTaskFactory.build(),
},
{
type: BoardElementDtoType.LESSON,
content: boardLessonFactory.build(),
},
{
type: BoardElementDtoType.COLUMN_BOARD,
content: boardCloumnBoardFactory.build(),
},
],
isArchived: faker.datatype.boolean(),
isSynchronized: faker.datatype.boolean(),
};
});
Copy link
Contributor

Choose a reason for hiding this comment

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

each class separate file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done ✅

@@ -1,4 +1,4 @@
import { ApiProperty } from '@nestjs/swagger';
import { ApiExtraModels, ApiProperty, getSchemaPath } from '@nestjs/swagger';
Copy link
Contributor

@MajedAlaitwniCap MajedAlaitwniCap Dec 11, 2024

Choose a reason for hiding this comment

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

each class separate file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it will be solved in EW-1090

Copy link

sonarcloud bot commented Dec 13, 2024

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants