fix: students interviews page #2575

stardustmeg · 2025-01-05T13:32:47Z

🟢 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

Describe the source of requirement, like related issue link.

💡 Background and solution

Background:
The student interviews page has been changed to improve user experience and maintainability. Previously, it was lacking some visual feedback on interviews details.
Additionally, there was an issue with technical screening registration that needed to be addressed to ensure the students could register and have a technical screening with a mentor.

Demonstration

Solution:
Add visual feedback for different student interview status:

Example Status

No planned interviews
Registration not started
Student not registered
Mentor not assigned
Mentor assigned
Passed the interview and got the results

☑️ 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 · 2025-01-06T15:05:50Z

client/src/pages/course/student/interviews.tsx

-        return data ? id : null;
-      });
+    const requests = interviews.map(async ({ id }) => {
+      const data = await courseService.getInterviewStudent(session.githubId, id.toString()).catch(() => null);


Use try-catch to handle errors and add a notification to inform the user.

try { const data = await courseService.getInterviewStudent(session.githubId, id.toString()); } catch (error) { message.error('Something went wrong, please try reloading the page later'); }

Added that. Thanks ❤️

I would suggest catching once and showing only 1 error instead of multiple bubble for each error

Thanks for pointing this out once again. I might have misunderstood something. Replaced this with one outer try...catch block ✅

aaliakseyenka · 2025-01-06T16:19:40Z

client/src/pages/course/student/interviews.tsx

+    return registeredInterviews.includes(id.toString());
+  };
+
+  const isRegistrationNotStarted = (interview: InterviewDto): boolean => {


we have helper function isInterviewRegistrationInProgress which can be used here. it uses 2 weeks buffer. I guess the logic should be the same

Thanks. I've seen this one. But I can't be sure about this, since some new functionality was added a couple of weeks ago and a registration date can be specified freely when the task is created by course admins, so I can't be sure it's always two weeks.
I might be missing something though. If that is the case, please, let me know.

aaliakseyenka · 2025-01-06T16:22:17Z

client/src/pages/course/student/interviews.tsx

@@ -161,6 +290,16 @@ function StatusLabel({ status }: { status: InterviewStatus }) {
  }
 }

+function InterviewPeriod(props: { startDate: string; endDate: string }) {


there is the same component at client/src/modules/Mentor/pages/Interviews/components/InterviewPeriod.tsx Probably we can mvoe to domain/interviews and reuse

You're absolutely right. Thanks! Done ✅

aaliakseyenka · 2025-01-06T16:24:25Z

client/src/pages/course/student/interviews.tsx

-        return data ? id : null;
-      });
+    const requests = interviews.map(async ({ id }) => {
+      const data = await courseService.getInterviewStudent(session.githubId, id.toString()).catch(() => null);


I would suggest catching once and showing only 1 error instead of multiple bubble for each error

aaliakseyenka · 2025-01-06T16:26:14Z

client/src/pages/course/student/interviews.tsx

+  const getInterviewCardDetails = (params: StudentInterviewDetails) => {
+    const { interviewPassed, isRegistered, registrationNotStarted, registrationStart } = params;
+
+    switch (true) {


consider replacing switch with multiple if statement. Since the switch cases use true, they are essentially conditionals, it would be cleaner to use just ifs

Thanks! Done ✅

valerydluski · 2025-01-09T22:01:16Z

client/src/pages/course/student/interviews.tsx

+  const renderInterviewsList = () => {
+    return interviews.map(interview => renderInterviewCard(interview));
+  };


The renderInterviewsList function seems unnecessary since it just maps over interviews and calls renderInterviewCard. Unless additional logic is planned, it might be cleaner to inline this mapping directly where it’s used.

Absolutely! Thank you 👍🏻 Done ✅

valerydluski · 2025-01-09T22:12:24Z

client/src/pages/course/student/interviews.tsx

+    const metaDescription = hasInterviewer || interviewPassed ? renderInterviewDetails(item) : renderExtra(interview);
+    const alertDescription = renderCardDescription({ interviewPassed, isRegistered, registrationNotStarted });
+    const { cardMessage } = getInterviewCardDetails({
+      interviewPassed,
+      isRegistered,
+      registrationNotStarted,
+      registrationStart,
+    });
+
+    return (
+      <Col key={id} xs={24} lg={12}>
+        <Card
+          bodyStyle={{ paddingTop: 0 }}
+          hoverable
+          title={
+            <Button type="link" href={descriptionUrl} target="_blank" style={{ padding: 0, fontWeight: 500 }}>
+              {name}
+            </Button>
+          }
+          extra={<InterviewPeriod startDate={startDate} endDate={endDate} shortDate />}
+        >
+          <Meta style={{ minHeight: 80, alignItems: 'center', textAlign: 'center' }} description={metaDescription} />
+
+          <Alert
+            message={<div style={{ minHeight: 50 }}>{cardMessage}</div>}
+            icon={<InfoCircleTwoTone />}
+            showIcon
+            type="info"
+            description={alertDescription}
+            style={{ minHeight: 275 }}
+          />
+        </Card>
+      </Col>
+    );
+  };
+
+  const getInterviewCardDetails = (params: StudentInterviewDetails) => {
+    const { interviewPassed, isRegistered, registrationNotStarted, registrationStart } = params;
+
+    if (interviewPassed) {
+      return {
+        cardMessage: 'You have your interview result. Congratulations!',
+        backgroundImage: 'url(https://cdn.rs.school/sloths/cleaned/congratulations.svg)',
+      };
+    }
+
+    if (isRegistered) {
+      return {
+        cardMessage: 'You’re all set! Prepare for your upcoming interview.',
+        backgroundImage: 'url(https://cdn.rs.school/sloths/cleaned/its-a-good-job.svg)',
+      };
+    }
+
+    if (registrationNotStarted) {
+      return {
+        cardMessage: (
+          <div>
+            Remember to come back and register after{' '}
+            <span style={{ whiteSpace: 'nowrap' }}>{formatShortDate(registrationStart ?? '')}</span>!
+          </div>
+        ),
+        backgroundImage: 'url(https://cdn.rs.school/sloths/cleaned/listening.svg)',
+      };
+    }
+
+    return {
+      cardMessage: 'Register and get ready for your exciting interview!',
+      backgroundImage: 'url(https://cdn.rs.school/sloths/cleaned/take-notes.svg)',
+    };
+  };
+
+  const renderCardDescription = (params: StudentInterviewDetails) => {
+    const { backgroundImage } = getInterviewCardDetails(params);
+    return (
+      <>
+        <div className={iconGroup.className} style={{ backgroundImage }} />
+        {iconGroup.styles}
+      </>
+    );
+  };
+
+  const renderInterviewDetails = (item: InterviewDetails) => {
+    const { interviewer, status, result } = item;
+
+    return (
+      <div style={{ padding: '8px 0' }}>
+        <Descriptions layout="vertical" size="small">
+          <Descriptions.Item label="Interviewer">
+            <GithubUserLink value={interviewer.githubId} />
+          </Descriptions.Item>
+          <Descriptions.Item label="Status">
+            <StatusLabel status={status} />
+          </Descriptions.Item>
+          <Descriptions.Item label="Result">
+            <b>{getInterviewResult(result as Decision) ?? '-'}</b>
+          </Descriptions.Item>
+        </Descriptions>
+      </div>
+    );
+  };


To improve readability and maintainability, I recommend breaking down this code into separate files under a modular folder structure. For example:

modules/ └── interview/ └── student/ ├── renderNoInterviews.tsx ├── renderInterviewCard.tsx ├── getInterviewCardDetails.ts ├── renderCardDescription.tsx ├── renderInterviewDetails.tsx

This is, of course, just a suggestion

Thanks! Added some single components ✅

stardustmeg added 8 commits December 31, 2024 01:11

refactor: redesign student interviews page

002f73b

fix: add no intervews notification

4c6bfc8

refactor: remove redundant styles

561358c

fix: technical screening registration issue

7e45589

refactor: remove redundant results logic

cdf2218

fix: not started condition

3e2d580

fix: replace svgs with cdn links

aecccd2

fix: card details choice

ac5c83c

valerydluski reviewed Jan 6, 2025

View reviewed changes

fix: add error handling from review suggestion

f073dc0

aaliakseyenka reviewed Jan 6, 2025

View reviewed changes

fix: apply review suggestions

e779058

valerydluski reviewed Jan 9, 2025

View reviewed changes

stardustmeg added 3 commits January 10, 2025 04:34

refactor: remove redundant function

03e0714

refactor: separate student interview components

e079b58

fix: data typing

9aa85b6

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: students interviews page #2575

fix: students interviews page #2575

stardustmeg commented Jan 5, 2025 •

edited

Loading

valerydluski Jan 6, 2025

stardustmeg Jan 6, 2025

aaliakseyenka Jan 6, 2025

stardustmeg Jan 6, 2025 •

edited

Loading

aaliakseyenka Jan 6, 2025

stardustmeg Jan 6, 2025 •

edited

Loading

aaliakseyenka Jan 6, 2025

stardustmeg Jan 6, 2025

aaliakseyenka Jan 6, 2025

aaliakseyenka Jan 6, 2025

stardustmeg Jan 6, 2025

valerydluski Jan 9, 2025

stardustmeg Jan 10, 2025

valerydluski Jan 9, 2025

stardustmeg Jan 12, 2025

fix: students interviews page #2575

Are you sure you want to change the base?

fix: students interviews page #2575

Conversation

stardustmeg commented Jan 5, 2025 • 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

Choose a reason for hiding this comment

stardustmeg Jan 6, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stardustmeg Jan 6, 2025 • edited Loading

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

stardustmeg commented Jan 5, 2025 •

edited

Loading

stardustmeg Jan 6, 2025 •

edited

Loading

stardustmeg Jan 6, 2025 •

edited

Loading