feat: migrate 'profile/info' route to nestjs #2420

stas-laniuk · 2024-02-01T10:40:43Z

🟢 Add deploy label if you want to deploy this Pull Request to staging environment

🧑‍⚖️ Pull Request Naming Convention

Title should follow Conventional Commits
Do not put issue id in title
Do not put WIP in title. Use Draft PR functionality
Consider to add area:* label(s)

I followed naming convention rules

🤔 This is a ...

🔗 Related issue link

#2394

💡 Background and solution

added nestJs route handling for GET 'profile/info' route
removed koa-related GET 'profile/info' rote handling code (currently in progress)
fixed typos: Endorment -> Endorsement, isContactsNodesVisible -> isContactsNotesVisible(bug)
verification screenshots are below:

screenshots are presented in pairs: backed by koa in master branch/backed by nestjs in feature branch

whole bunch of screenshots: https://drive.google.com/drive/folders/1s5qb-Um8lwAyd-sGzQUfMeEBdx0ffgBb?usp=drive_link

isAdmin: true:
profileInfo page: self, mentor, no relation:

isAdmin: false:
profileInfo page: self, mentor, no relation:

dashboard page:

☑️ Self Check before Merge

⚠️ Please check all items below before review. ⚠️

Database migration is added or not needed
Documentation is updated/provided or not needed
Changes are tested locally

valerydluski · 2024-02-10T09:24:50Z

nestjs/src/courses/interviews/interviews.service.ts

+  }
+
+  /**
+   * @deprecated - should be removed once Artsiom A. makes migration of the legacy feedback format


I think it's better to attach the issue number instead of writing that Artsiom will do it.
#2281"

valerydluski · 2024-02-10T09:31:25Z

nestjs/src/courses/interviews/interviews.service.ts

+  async getStageInterviewFeedback(githubId: string): Promise<StageInterviewDetailedFeedback[]> {
+    const data = await this.stageInterviewRepository
+      .createQueryBuilder('stageInterview')
+      .select('"stageInterview"."decision" AS "decision"')
+      .addSelect('"stageInterview"."isGoodCandidate" AS "isGoodCandidate"')
+      .addSelect('"stageInterview"."score" AS "interviewScore"')
+      .addSelect('"course"."name" AS "courseName"')
+      .addSelect('"course"."fullName" AS "courseFullName"')
+      .addSelect('"stageInterviewFeedback"."json" AS "interviewResultJson"')
+      .addSelect('"stageInterviewFeedback"."updatedDate" AS "interviewFeedbackDate"')
+      .addSelect('"stageInterviewFeedback"."version" AS "feedbackVersion"')
+      .addSelect('"userMentor"."firstName" AS "interviewerFirstName"')
+      .addSelect('"userMentor"."lastName" AS "interviewerLastName"')
+      .addSelect('"userMentor"."githubId" AS "interviewerGithubId"')
+      .addSelect('"courseTask"."maxScore" AS "maxScore"')
+      .leftJoin(Student, 'student', '"student"."id" = "stageInterview"."studentId"')
+      .leftJoin(User, 'user', '"user"."id" = "student"."userId"')
+      .leftJoin(Course, 'course', '"course"."id" = "stageInterview"."courseId"')
+      .leftJoin(
+        StageInterviewFeedback,
+        'stageInterviewFeedback',
+        '"stageInterview"."id" = "stageInterviewFeedback"."stageInterviewId"',
+      )
+      .leftJoin(CourseTask, 'courseTask', '"courseTask"."id" = "stageInterview"."courseTaskId"')
+      .leftJoin(Mentor, 'mentor', '"mentor"."id" = "stageInterview"."mentorId"')
+      .leftJoin(User, 'userMentor', '"userMentor"."id" = "mentor"."userId"')
+      .where('"user"."githubId" = :githubId', { githubId })
+      .andWhere('"stageInterview"."isCompleted" = true')
+      .orderBy('"course"."updatedDate"', 'ASC')
+      .getRawMany();
+
+    return data
+      .map((feedbackData: FeedbackData) => {
+        const {
+          feedbackVersion,
+          decision,
+          interviewFeedbackDate,
+          interviewerFirstName,
+          courseFullName,
+          courseName,
+          interviewerLastName,
+          interviewerGithubId,
+          isGoodCandidate,
+          interviewScore,
+          interviewResultJson,
+          maxScore,
+        } = feedbackData;
+        const feedbackTemplate = JSON.parse(interviewResultJson) as any;
+
+        const { score, feedback } = !feedbackVersion
+          ? InterviewsService.parseLegacyFeedback(feedbackTemplate)
+          : {
+              feedback: feedbackTemplate,
+              score: interviewScore ?? 0,
+            };
+
+        return {
+          version: feedbackVersion ?? 0,
+          date: interviewFeedbackDate,
+          decision,
+          isGoodCandidate,
+          courseName,
+          courseFullName,
+          feedback,
+          score,
+          interviewer: {
+            name:
+              this.userService.getFullName({ firstName: interviewerFirstName, lastName: interviewerLastName }) ||
+              interviewerGithubId,
+            githubId: interviewerGithubId,
+          },
+          maxScore,
+        };
+      })
+      .filter(Boolean);
+  }


Maybe we should do something like this?

private async getFeedbackData(githubId: string): Promise<FeedbackData[]> { return this.stageInterviewRepository .createQueryBuilder('stageInterview') // ... addSelects ... .leftJoin(Student, 'student', '"student"."id" = "stageInterview"."studentId"') // ... other leftJoins ... .where('"user"."githubId" = :githubId', { githubId }) .andWhere('"stageInterview"."isCompleted" = true') .orderBy('"course"."updatedDate"', 'ASC') .getRawMany(); } private parseFeedbackData(feedbackData: FeedbackData): StageInterviewDetailedFeedback { // ... existing mapping logic ... } async getStageInterviewFeedback(githubId: string): Promise<StageInterviewDetailedFeedback[]> { const feedbackData = await this.getFeedbackData(githubId); return feedbackData.map(feedbackData => this.parseFeedbackData(feedbackData)).filter(Boolean); }

AlreadyBored

Thank you, I left some comments, please take a look :)

AlreadyBored · 2024-02-11T10:18:50Z

client/src/pages/profile/index.tsx

+      const profileInfoExtendedWithCv = {
+        ...profileInfo,
        ...data,
      };


I see nothing related with CV above, why do we have CV in name of this variable? 🙃

AlreadyBored · 2024-02-11T10:22:32Z

common/models/profile.ts

@@ -44,7 +44,8 @@ export interface GeneralInfo {
  aboutMyself?: string | null;
  location: Location;
  educationHistory?: any | null;
-  englishLevel?: EnglishLevel | null;
+  // TODO: String type is too abstract for englishLevel.


Then why do we have it here? Probably you've misconfigured some BE API decorators, could you please check?

AlreadyBored · 2024-02-11T16:22:33Z

nestjs/src/courses/interviews/interviews.service.ts

+      .map((feedbackData: FeedbackData) => {
+        const {
+          feedbackVersion,
+          decision,
+          interviewFeedbackDate,
+          interviewerFirstName,
+          courseFullName,
+          courseName,
+          interviewerLastName,
+          interviewerGithubId,
+          isGoodCandidate,
+          interviewScore,
+          interviewResultJson,
+          maxScore,
+        } = feedbackData;


Suggested change

.map((feedbackData: FeedbackData) => {

const {

feedbackVersion,

decision,

interviewFeedbackDate,

interviewerFirstName,

courseFullName,

courseName,

interviewerLastName,

interviewerGithubId,

isGoodCandidate,

interviewScore,

interviewResultJson,

maxScore,

} = feedbackData;

.map(({

feedbackVersion,

decision,

interviewFeedbackDate,

interviewerFirstName,

courseFullName,

courseName,

interviewerLastName,

interviewerGithubId,

isGoodCandidate,

interviewScore,

interviewResultJson,

maxScore,

}: FeedbackData) => {

AlreadyBored · 2024-02-11T16:24:07Z

nestjs/src/courses/interviews/interviews.service.ts

+        const { score, feedback } = !feedbackVersion
+          ? InterviewsService.parseLegacyFeedback(feedbackTemplate)
+          : {
+              feedback: feedbackTemplate,
+              score: interviewScore ?? 0,
+            };


Suggested change

const { score, feedback } = !feedbackVersion

? InterviewsService.parseLegacyFeedback(feedbackTemplate)

: {

feedback: feedbackTemplate,

score: interviewScore ?? 0,

};

const { score, feedback } = feedbackVersion

? {

feedback: feedbackTemplate,

score: interviewScore ?? 0,

} : InterviewsService.parseLegacyFeedback(feedbackTemplate);

AlreadyBored · 2024-02-11T16:35:23Z

nestjs/src/profile/dto/permissions.dto.ts

+class PublicVisibilitySettings {
+  @ApiProperty()
+  @IsBoolean()
+  all: boolean;


I'd propose to use more semantic name for this flag, like getAll or requestAll

AlreadyBored · 2024-02-11T17:08:41Z

nestjs/src/profile/student-info.service.ts

+  async getStats(githubId: string, permissions: Permissions): Promise<StudentStats[]> {
+    const { isCoreJsFeedbackVisible, isExpellingReasonVisible } = permissions;
+
+    const query = await this.studentRepository


Same as above

AlreadyBored · 2024-02-11T17:09:13Z

nestjs/src/profile/student-info.service.ts

+        interviewerLastName,
+        certificateId,
+        rank,
+      }: any) => {


please no any

AlreadyBored · 2024-02-11T17:10:08Z

nestjs/src/profile/student-info.service.ts

+          .sort((a: any, b: any) => a.endDate - b.endDate)
+          .map((task: any) => omit(task, 'endDate'));


please no any

AlreadyBored · 2024-02-11T17:10:45Z

nestjs/src/profile/user-info.service.ts

+  async getUserInfo(githubId: string, permissions: Permissions): Promise<UserInfo> {
+    const {
+      isAboutVisible,
+      isEducationVisible,
+      isEnglishVisible,
+      isPhoneVisible,
+      isEmailVisible,
+      isTelegramVisible,
+      isSkypeVisible,
+      isContactsNotesVisible,
+      isLinkedInVisible,
+      isWhatsAppVisible,
+    } = permissions;


Suggested change

async getUserInfo(githubId: string, permissions: Permissions): Promise<UserInfo> {

const {

isAboutVisible,

isEducationVisible,

isEnglishVisible,

isPhoneVisible,

isEmailVisible,

isTelegramVisible,

isSkypeVisible,

isContactsNotesVisible,

isLinkedInVisible,

isWhatsAppVisible,

} = permissions;

async getUserInfo(githubId: string, {

isAboutVisible,

isEducationVisible,

isEnglishVisible,

isPhoneVisible,

isEmailVisible,

isTelegramVisible,

isSkypeVisible,

isContactsNotesVisible,

isLinkedInVisible,

isWhatsAppVisible,

}: Permissions): Promise<UserInfo> {

AlreadyBored · 2024-02-11T17:11:05Z

nestjs/src/profile/user-info.service.ts

+      isWhatsAppVisible,
+    } = permissions;
+
+    const query = this.userRepository


Same as above

stas-laniuk added 4 commits February 8, 2024 21:31

feat: add profile/info route handling via nestjs

436df25

fix: adjust types for profile-info DTOs

b8e900a

test: transfer tests for permission service

8018e56

refactor: remove unused profile/info koa route handling

b048487

stas-laniuk force-pushed the issues/2394 branch from a4a9d3f to b048487 Compare February 8, 2024 19:38

stas-laniuk marked this pull request as ready for review February 8, 2024 20:21

valerydluski reviewed Feb 10, 2024

View reviewed changes

AlreadyBored requested changes Feb 11, 2024

View reviewed changes

dzmitry-varabei closed this Jul 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: migrate 'profile/info' route to nestjs #2420

feat: migrate 'profile/info' route to nestjs #2420

stas-laniuk commented Feb 1, 2024 •

edited

Loading

valerydluski Feb 10, 2024

valerydluski Feb 10, 2024

AlreadyBored left a comment

AlreadyBored Feb 11, 2024

AlreadyBored Feb 11, 2024

AlreadyBored Feb 11, 2024

AlreadyBored Feb 11, 2024

AlreadyBored Feb 11, 2024

AlreadyBored Feb 11, 2024

AlreadyBored Feb 11, 2024

AlreadyBored Feb 11, 2024

AlreadyBored Feb 11, 2024

AlreadyBored Feb 11, 2024

		.sort((a: any, b: any) => a.endDate - b.endDate)
		.map((task: any) => omit(task, 'endDate'));

feat: migrate 'profile/info' route to nestjs #2420

feat: migrate 'profile/info' route to nestjs #2420

Conversation

stas-laniuk commented Feb 1, 2024 • edited Loading

🧑‍⚖️ Pull Request Naming Convention

🤔 This is a ...

🔗 Related issue link

💡 Background and solution

☑️ Self Check before Merge

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlreadyBored 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

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

Choose a reason for hiding this comment

stas-laniuk commented Feb 1, 2024 •

edited

Loading