fix: sonar duplication #397
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Playwright test results
Details
Skipped testsNo persona › tests/auth.test.ts › authenticate through Clerk UI |
| return userEmail; | ||
| }; | ||
|
|
||
| const categoryMap: Record<string, string> = { |
There was a problem hiding this comment.
Worth exporting this for tests?
There was a problem hiding this comment.
Why is exporting usefull for tests ?
There was a problem hiding this comment.
We can import the list of categories in the test file. I'm generally exporting types wherever they are defined so that when we have tests we can share the types in the test files
| @@ -22,7 +16,7 @@ export async function exportLessonPlan({ | |||
| ctx, | |||
| }: { | |||
| input: { | |||
| data: LessonSlidesInputData; | |||
| data: LessonSlidesInputData; // should this be lesson plan | |||
There was a problem hiding this comment.
Do we need input from Tom / Matt here?
There was a problem hiding this comment.
Have changed correct type
| prisma: ctx.prisma, | ||
| snapshotId, | ||
| exportType: "LESSON_SLIDES_SLIDES", | ||
| const exportType = "LESSON_SLIDES_SLIDES"; |
There was a problem hiding this comment.
This does looks like a typo in the original code
There was a problem hiding this comment.
enum LessonExportType {
STARTER_QUIZ_DOC
EXIT_QUIZ_DOC
LESSON_PLAN_DOC
LESSON_SLIDES_SLIDES
WORKSHEET_SLIDES
ADDITIONAL_MATERIALS_DOCS
@@schema("public")
}
Seems like it is the correct name
There was a problem hiding this comment.
I'd expect it to be LESSON_PLAN_SLIDES rather than LESSON_SLIDES_SLIDES - reads like a typo. Not suggesting this is a new code issue, but presumably this is hard to change if it's a database enum
There was a problem hiding this comment.
Yeah for sure, was surprised when i saw it
| }), | ||
| }), | ||
| }), | ||
| programme: Programme, |
There was a problem hiding this comment.
For Zod Schemas I normally have ProgrammeSchema for the zod schema and then Programme for the type. Should we adopt that here or is this schema already named Programme?
There was a problem hiding this comment.
The schema Programme already existed
|
There was a problem hiding this comment.
Following the review, @JBR90 was unable to replicate the problems I had seen. I had another check and also didn't see the original problem. Joe has now brought this branch up-to-date with main and I've rechecked, no issues seen. I'm assuming that the original issue seen with downloads was a temporary issue or nothing to do with this work and has since been resolved. Looks good now. 👍
|
🎉 This PR is included in version 1.19.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Description
Sonar is failing on this PR but after merge this brings the overall duplication on main down to 2%, some of the exports files were 100% duplication.
How to test
Exports and downloads / downloads all should all work as they did before
Checklist