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

Jump to bottom

BC-4973-rewrite-data-retrival-in-course #4422

Merged
merged 13 commits into from
Sep 25, 2023
Merged

BC-4973-rewrite-data-retrival-in-course #4422

merged 13 commits into from
Sep 25, 2023

Conversation

WojciechGrancow
Copy link
Contributor

@WojciechGrancow WojciechGrancow commented Sep 20, 2023 •
edited
Loading

Description

Links to Tickets or other pull requests

https://ticketsystem.dbildungscloud.de/browse/BC-4973

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.

@WojciechGrancow WojciechGrancow added the WIP This feature branch is in progress, do not merge into master. label Sep 20, 2023
@WojciechGrancow WojciechGrancow removed the WIP This feature branch is in progress, do not merge into master. label Sep 21, 2023
bn-pass
bn-pass suggested changes Sep 21, 2023
View reviewed changes
Copy link
Contributor

@bn-pass bn-pass left a comment

Choose a reason for hiding this comment

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

Please make the unit tests more verbose.

apps/server/src/modules/learnroom/service/course.service.spec.ts Show resolved Hide resolved
apps/server/src/modules/learnroom/service/coursegroup.service.spec.ts Show resolved Hide resolved
apps/server/src/modules/lesson/service/lesson.service.spec.ts Show resolved Hide resolved
@virgilchiriac
Copy link
Contributor

How do I know if this is needed? Is not currently used, nor is clear to me how it is planned to be used in the future.
We also don't want to migrate unused code from feathers.

virgilchiriac
virgilchiriac requested changes Sep 22, 2023
View reviewed changes
Copy link
Contributor

@virgilchiriac virgilchiriac left a comment

Choose a reason for hiding this comment

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

PR needs some more context, some TODO, or more implementation
as it currently is seems to me not to be finished

@WojciechGrancow
Copy link
Contributor Author

WojciechGrancow commented Sep 22, 2023 •
edited
Loading

How do I know if this is needed? Is not currently used, nor is clear to me how it is planned to be used in the future. We also don't want to migrate unused code from feathers.

@virgilchiriac we know it, but currently we rewrite only delete and retrival data for user which is deleted form fethersJs into nestJS. In next sprints we will have tickets with logic (ex. UCs) for delete users

@WojciechGrancow
Copy link
Contributor Author

PR needs some more context, some TODO, or more implementation as it currently is seems to me not to be finished

in this PR I rewrite only retrieval methods (if not exist). In next sprints we will have tickets with logic (ex. UCs) for delete users

@CeEv CeEv self-requested a review September 25, 2023 10:54
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

warning The version of Java (11.0.20) you have used to run this analysis is deprecated and we will stop accepting it soon. Please update to at least Java 17.
Read more here

@WojciechGrancow WojciechGrancow merged commit ee43c77 into main Sep 25, 2023
42 of 43 checks passed
@WojciechGrancow WojciechGrancow deleted the BC-4973-rewrite-data-retrival-in-course branch September 25, 2023 12:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bn-pass bn-pass bn-pass approved these changes

@virgilchiriac virgilchiriac virgilchiriac approved these changes

@CeEv CeEv CeEv approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@WojciechGrancow @virgilchiriac @CeEv @bn-pass