feat(con/profile page): mentees with active mentor can't view profile of other mentor #984
Conversation
… who's not their mentor"
WalkthroughThe pull request introduces modifications to the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
apps/redi-connect/src/pages/app/find-a-mentor/FindAMentor.tsx (1)
204-211: LGTM: Improved mentorship status check.The new implementation robustly checks for any accepted mentorship match, aligning with the PR objective. It's more reliable than the previous version which assumed the first match was the active one.
Consider a minor optimization:
- profile?.mentorshipMatches?.some(match => - match.status === MentorshipMatchStatus.Accepted - ) + profile?.mentorshipMatches?.some(match => match.status === MentorshipMatchStatus.Accepted) - const matchId = profile?.mentorshipMatches?.find(match => - match.status === MentorshipMatchStatus.Accepted - ).id + const matchId = profile?.mentorshipMatches?.find(match => match.status === MentorshipMatchStatus.Accepted)?.idThis change reduces line count and improves readability without affecting functionality.
apps/redi-connect/src/pages/app/profile/Profile.tsx (1)
329-350: Ensure consistent usage ofactiveMentorshipMatchIn the
determineViewModefunction, you have definedactiveMentorshipMatchto find the active mentorship of the current user. Make sure that this variable is consistently used and that its scope and purpose are clear throughout the function. Additionally, consider checking if redundant conditions can be simplified.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- apps/redi-connect/src/pages/app/find-a-mentor/FindAMentor.tsx (2 hunks)
- apps/redi-connect/src/pages/app/profile/Profile.tsx (4 hunks)
🔇 Additional comments (4)
apps/redi-connect/src/pages/app/find-a-mentor/FindAMentor.tsx (2)
5-5: LGTM: Import statement addition is appropriate.The addition of
MentorshipMatchStatusto the import statement is consistent with the changes in the component logic and necessary for the new implementation of checking mentorship match status.
Line range hint
1-391: Summary: Core logic implemented, but additional verification needed.The changes in this file successfully implement the core logic for restricting mentees with active mentorships from viewing other mentor profiles. The implementation is robust and efficient.
However, to fully meet the PR objectives, please ensure that:
- The overlay for restricted mentor profile access is implemented (possibly in another file).
- The link to the ongoing mentorship dashboard is correctly implemented.
- The changes build without errors and function as expected in the development environment.
Given that the PR was developed under less than ideal conditions, I recommend thorough testing of the entire feature, including edge cases and different user scenarios.
To assist with verification, you can use this script to check for any TODO comments or unfinished implementations:
#!/bin/bash # Description: Check for TODO comments or unfinished implementations related to this feature echo "Searching for TODO comments or unfinished implementations:" rg -i 'TODO|FIXME|XXX|HACK|unfinished|incomplete' --glob '*.{ts,tsx,js,jsx}' echo "\nSearching for files related to mentor profile restrictions:" rg -l 'mentor.*profile.*restrict|active.*mentorship' --glob '*.{ts,tsx,js,jsx}'This script will help identify any remaining tasks or relevant files that might need attention to complete the feature implementation.
apps/redi-connect/src/pages/app/profile/Profile.tsx (2)
2-2: Import statement approvedThe addition of
LoadMyProfileQueryto the import statements is appropriate and necessary for the new functionality.
267-267: Verify the mentorship link URLPlease ensure that the URL
/app/mentorships/${activeMentorshipMatch?.mentorId}correctly navigates to the user's ongoing mentorship. Depending on the routing setup, you might need to use the mentorship match ID or adjust the path to reflect the correct route.Run the following script to check if the route exists and uses the expected parameter:
|
|
||
| if (menteeHasAnActiveMatch) { | ||
| const matchId = profile?.mentorshipMatches?.[0].id | ||
| const matchId = profile?.mentorshipMatches?.find(match => | ||
| match.status === MentorshipMatchStatus.Accepted | ||
| ).id | ||
| history.replace(`/app/mentorships/${matchId}`) |
There was a problem hiding this comment.
💡 Codebase verification
Overlay implementation for restricted mentor profile access is missing.
While the codebase includes modal components, there is no evidence of an overlay being triggered when a mentee with an active mentorship attempts to view another mentor's profile. Please implement the overlay as described in the PR objectives to restrict access and provide the necessary user feedback.
🔗 Analysis chain
Verify the overlay implementation for restricted mentor profile access.
The changes successfully implement the restriction for mentees with active mentorships. However, the PR objectives mention displaying an overlay when a mentee with an active mentorship attempts to view another mentor's profile. This overlay implementation is not visible in the current changes.
Please ensure that:
- The overlay is implemented as described in the PR objectives.
- The link labeled "an ongoing mentorship" in the overlay directs users to their current mentorship dashboard.
To verify the implementation, you can use the following script:
This script will help identify any overlay-related components or logic, as well as the routing to the mentorship dashboard, which should be present to fulfill the PR objectives.
Please run the corrected verification script below to ensure the overlay implementation is present as per the PR objectives:
Please run the corrected verification script below to ensure the overlay implementation is present as per the PR objectives:
Please run the corrected verification script below to ensure the overlay implementation is present as per the PR objectives:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for the presence of an overlay component or logic in the codebase
# Search for potential overlay components or logic
echo "Searching for overlay components or logic:"
rg -i -t tsx -t ts 'overlay|modal|popup|restriction' --glob '!*test*'
# Search for mentorship dashboard routing logic
echo "\nSearching for mentorship dashboard routing:"
rg -i -t tsx -t ts 'mentorship.*dashboard|ongoing.*mentorship' --glob '!*test*'
Length of output: 14328
Script:
#!/bin/bash
# Description: Check for the presence of an overlay component or logic in the codebase
# Search for potential overlay components or logic
echo "Searching for overlay components or logic:"
rg -i -t tsx -t ts 'overlay|modal|popup|restriction' --glob '!*test*'
# Search for mentorship dashboard routing logic
echo "\nSearching for mentorship dashboard routing:"
rg -i -t tsx -t ts 'mentorship.*dashboard|ongoing.*mentorship' --glob '!*test*'
Length of output: 405
Script:
#!/bin/bash
# Description: Check for the presence of an overlay component or logic in the codebase
# Search for potential overlay components or logic
echo "Searching for overlay components or logic:"
rg -i -t typescriptreact -t typescript 'overlay|modal|popup|restriction' --glob '!*test*'
# Search for mentorship dashboard routing logic
echo "\nSearching for mentorship dashboard routing:"
rg -i -t typescriptreact -t typescript 'mentorship.*dashboard|ongoing.*mentorship' --glob '!*test*'
Length of output: 469
Script:
#!/bin/bash
# Description: Check for the presence of an overlay component or logic in the codebase
# Search for potential overlay components or logic
echo "Searching for overlay components or logic:"
rg -i 'overlay|modal|popup|restriction' --glob '*.{ts,tsx}' --glob '!*test*'
# Search for mentorship dashboard routing logic
echo "\nSearching for mentorship dashboard routing:"
rg -i 'mentorship.*dashboard|ongoing.*mentorship' --glob '*.{ts,tsx}' --glob '!*test*'
Length of output: 30009
| currentUserProfile.userType === UserType.Mentee && | ||
| activeMentorshipMatch?.status === MentorshipMatchStatus.Accepted && | ||
| activeMentorshipMatch?.mentorId != activeMentorshipMatch.mentorId |
There was a problem hiding this comment.
Fix the logical error in mentor ID comparison
The condition activeMentorshipMatch?.mentorId != activeMentorshipMatch.mentorId is comparing the same property to itself, which will always evaluate to false. This means the intended check is not functioning correctly. To verify if the active mentorship is with a different mentor than the one currently being viewed, you should compare activeMentorshipMatch?.mentorId with profile.id.
Apply this diff to correct the condition:
if (
currentUserProfile.userType === UserType.Mentee &&
activeMentorshipMatch?.status === MentorshipMatchStatus.Accepted &&
- activeMentorshipMatch?.mentorId != activeMentorshipMatch.mentorId
+ activeMentorshipMatch?.mentorId != profile.id
) {
return 'currentUserIsMenteeAndViewsNotTheirMentor'
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| currentUserProfile.userType === UserType.Mentee && | |
| activeMentorshipMatch?.status === MentorshipMatchStatus.Accepted && | |
| activeMentorshipMatch?.mentorId != activeMentorshipMatch.mentorId | |
| currentUserProfile.userType === UserType.Mentee && | |
| activeMentorshipMatch?.status === MentorshipMatchStatus.Accepted && | |
| activeMentorshipMatch?.mentorId != profile.id |
There was a problem hiding this comment.
@ericbolikowski It seems like coderabbit is right. You are comparing two same things not being equal to each other here. Am I missing something?
Also let's use !== instead, if there's again not a good reason.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Nice find, this is wrong. Pushed a fixed.
|
Dear @ericbolikowski, as I told you in Slack - PR is not covering Case 2, please implement the same solution for Case 2. |
|
Hey @ericbolikowski,
Mentees with an active mentorship can't access the profile page of their current mentor. Instead, they see the "another mentee got to this mentor first" overlay.
When accessing the profile page (/app/find-a-mentor/profile/:profiled) of other mentors, a mentee with an active mentorship sees other overlays instead of the "looks like you already have an ongoing mentorship with another mentor" overlay.
When accessing the find-a-mentor page, a mentee with an active mentorship sees this:
The ongoing mentorship link seems to lead to the wrong mentorship ID, and instead of the dashboard of the current mentorship, an empty page is being displayed: |
|
Hello guys!
So, mentee should be able to open his mentor's page. but pages of other mentors should be blurred with a message.
no, unfortunately not. Thanks! |
|
@katamatata this error should now be fixed:
Can you please pull latest changes and test? |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
apps/redi-connect/src/pages/app/profile/Profile.tsx (2)
93-96: LGTM: NewactiveMentorshipMatchvariable is well-implemented.The new variable correctly finds the active mentorship match. However, consider using
Array.find()instead ofArray.filter()for a slight performance improvement, as we only need the first match.Consider applying this optimization:
const activeMentorshipMatch = - myProfile.mentorshipMatches.filter(match => + myProfile.mentorshipMatches.find(match => match.status === MentorshipMatchStatus.Accepted - )[0] )
323-349: LGTM:ViewModeanddetermineViewModeupdates are well-implemented.The changes to
ViewModeanddetermineViewModecorrectly implement the new requirement for restricting mentees with active mentorships. The early return for mentee profiles is a good optimization.Consider simplifying the condition for checking if the current user is a mentee with an active mentorship viewing another mentor's profile:
if ( currentUserProfile.userType === UserType.Mentee && - activeMentorshipMatch?.status === MentorshipMatchStatus.Accepted && activeMentorshipMatch?.mentor.id !== profile.userId ) { return 'currentUserIsMenteeAndViewsNotTheirMentor' }This simplification is possible because
activeMentorshipMatchis already filtered to only include accepted matches.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- apps/redi-connect/src/pages/app/find-a-mentor/FindAMentor.tsx (2 hunks)
- apps/redi-connect/src/pages/app/profile/Profile.tsx (4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/redi-connect/src/pages/app/find-a-mentor/FindAMentor.tsx
🧰 Additional context used
🔇 Additional comments (3)
apps/redi-connect/src/pages/app/profile/Profile.tsx (3)
2-5: LGTM: Import changes are consistent with new functionality.The addition of
LoadMyProfileQueryto the imports is appropriate for the new functionality implemented in this file.
98-98: LGTM:viewModedetermination updated correctly.The
viewModevariable now uses the updateddetermineViewModefunction, passing both the profile and the current user's profile. This change allows for more context-aware view mode determination, which is consistent with the new requirements.
Line range hint
1-355: Overall implementation looks good, with a few suggestions for improvement.The changes effectively implement the new restrictions for mentees with active mentorships, aligning well with the PR objectives. The code is generally well-structured and readable. Here's a summary of the review:
- The new imports and variable declarations are appropriate.
- The
ViewModetype anddetermineViewModefunction have been correctly updated to handle the new use case.- The new message for mentees viewing other mentors' profiles is well-written and informative.
However, there are a few areas that could benefit from minor improvements or verification:
- Consider using
Array.find()instead ofArray.filter()foractiveMentorshipMatchfor better performance.- Verify the correctness of the mentorship link in the new message.
- Simplify the condition in
determineViewModefor checking if a mentee with an active mentorship is viewing another mentor's profile.Please address these suggestions and verify the mentorship link to ensure the implementation is fully correct and optimized.
| currentUserIsMenteeAndViewsNotTheirMentor: ( | ||
| <> | ||
| Hey there! It looks like you already have{' '} | ||
| <a href={`/app/mentorships/${activeMentorshipMatch?.mentorId}`}>an ongoing mentorship</a> with | ||
| another mentor. Please remember that you can only have one mentor at a | ||
| time. You can save this link to check if this mentor remains available | ||
| once you complete your current mentorship match. If you have any | ||
| questions in the meantime, feel free to check out the{' '} | ||
| <a href="/faq">FAQ</a>. | ||
| </> | ||
| ) |
There was a problem hiding this comment.
💡 Codebase verification
Verify the mentorship link identifier in Profile.tsx.
The mentorship link in Profile.tsx uses activeMentorshipMatch?.mentorId, whereas in FindAMentor.tsx, it uses activeMentorshipMatch?.mentor.id. Please ensure that the correct identifier (mentor.id) is used consistently across the codebase to prevent potential issues with mentorship link navigation.
- File:
apps/redi-connect/src/pages/app/profile/Profile.tsx- Line: 264-274
🔗 Analysis chain
Message content LGTM, but verify the mentorship link.
The new message for mentees viewing other mentors' profiles is well-written and aligns with the requirements. However, there's a potential issue with the link to the ongoing mentorship.
The link to the ongoing mentorship uses activeMentorshipMatch?.mentorId, which might not be the correct ID for the mentorship dashboard. Please verify if this is the correct ID to use or if it should be a different identifier for the mentorship itself.
To help verify this, you can run the following script:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for mentorship-related route definitions and usages
# Search for route definitions related to mentorships
echo "Searching for mentorship route definitions:"
rg --type typescript 'mentorship.*route'
# Search for usages of mentorship-related links
echo "\nSearching for mentorship link usages:"
rg --type typescript 'href.*mentorship'
Length of output: 340
Script:
#!/bin/bash
# Description: Check for mentorship-related route definitions and usages
# Search for route definitions related to mentorships
echo "Searching for mentorship route definitions:"
rg --type ts 'mentorship.*route'
# Search for usages of mentorship-related links
echo "\nSearching for mentorship link usages:"
rg --type ts 'href.*mentorship'
Length of output: 577
@astkhikatredi unfortunately since the FindAMentor has a lot of vertical content the overlay centered vertically in the same manner as on the profile might lead to the message not being easily visible. I tried to "move the overlay" so that it's higher up, but couldn't find an easy fix for this. If we can't wait much longer, I suggest we deploy this change as-is and get back to figuring out a proper overlay for FindAMentor later. |
Edge case I hadn't thought of - just pushed code that should take care of it. @katamatata can you please pull & test? PS! Nice find - @katamatata you live up to your reputation finding the bugs! |
|
Hey @ericbolikowski , one more comment: could you please replace this sentence "You can save this link to check if this mentor remains available once you complete your current mentorship match." with this: "You can check available mentors once you complete your current mentorship match" THANKS |
Alright, please make a ticket about this so that we don't forget. THANK YOU! |
Tested it again.
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- apps/redi-connect/src/pages/app/find-a-mentor/FindAMentor.tsx (2 hunks)
- apps/redi-connect/src/pages/app/profile/Profile.tsx (4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/redi-connect/src/pages/app/find-a-mentor/FindAMentor.tsx
🧰 Additional context used
🔇 Additional comments (3)
apps/redi-connect/src/pages/app/profile/Profile.tsx (3)
2-2: LGTM: New imports and variable declarationsThe new import
LoadMyProfileQueryand theactiveMentorshipMatchvariable are well-implemented and necessary for the new functionality. The updatedviewModevariable now correctly uses themyProfileparameter.Also applies to: 93-97
Line range hint
1-364: Overall implementation looks good, but needs thorough testingThe changes implement the new restriction for mentees viewing other mentors' profiles, which aligns with the PR objectives. The new
ViewModetype,determineViewModefunction, and the message for mentees with active mentorships are well-implemented.However, please address the issues mentioned in the previous comments, particularly:
- Fixing the logic in the
determineViewModefunction.- Verifying the correct identifier for the mentorship link.
Additionally, as mentioned in the PR description, thorough testing is crucial, especially given the development conditions. Please ensure to:
- Test the implementation by logging in as a mentee with an active mentorship.
- Verify that the overlay is displayed when viewing other mentors' profiles.
- Confirm that the "ongoing mentorship" link correctly directs to the current mentorship dashboard.
To assist with testing, you can use the following script to simulate different user scenarios:
#!/bin/bash # Description: Simulate different user scenarios for testing # Function to simulate user login and profile viewing simulate_user() { local user_type=$1 local has_active_mentorship=$2 local viewing_profile=$3 echo "Simulating $user_type with${has_active_mentorship:+ active} mentorship viewing $viewing_profile's profile" # Add your test logic here, e.g., API calls or state changes } # Test scenarios simulate_user "mentee" "active" "other_mentor" simulate_user "mentee" "" "other_mentor" simulate_user "mentee" "active" "current_mentor" simulate_user "mentor" "" "mentee" echo "Manual verification required for UI elements and navigation"
263-274: Verify the mentorship link identifierThe new message for mentees viewing other mentors' profiles is well-written and aligns with the requirements. However, there's a potential issue with the link to the ongoing mentorship.
The link uses
activeMentorshipMatch?.mentorId, which might not be the correct ID for the mentorship dashboard. Please verify if this is the correct ID to use or if it should be a different identifier for the mentorship itself.Run the following script to check for mentorship-related route definitions and usages:
| type ViewMode = | ||
| | 'display' | ||
| | 'notActivelyMentoring' | ||
| | 'mentoringButNoFreeSpots' | ||
| | 'currentUserIsMenteeAndViewsNotTheirMentor' | ||
|
|
||
| function determineViewMode( | ||
| profile: ProfilePageQueryQuery['conProfile'] | ||
| profile: ProfilePageQueryQuery['conProfile'], | ||
| currentUserProfile: LoadMyProfileQuery['conProfile'] | ||
| ): ViewMode { | ||
| if (profile.userType !== UserType.Mentor) return 'display' | ||
| // If we're looking at a mentee, show the profile. Otherwise, the profile | ||
| // is a mentor's profile, so continue to determine if the view mode is a | ||
| // "special" one. | ||
| if (profile.userType === UserType.Mentee) return 'display' | ||
|
|
||
| const activeMentorshipMatch = currentUserProfile.mentorshipMatches.find( | ||
| (match) => match.status === MentorshipMatchStatus.Accepted | ||
| ) | ||
|
|
||
| // Is current user a mentee, does that mentee have an active match, and is | ||
| // that match with another mentor than the one we're currently looking at? | ||
| if (activeMentorshipMatch.mentorId === currentUserProfile.id) return 'display' | ||
|
|
||
| console.log(activeMentorshipMatch, profile) | ||
|
|
||
| if ( | ||
| currentUserProfile.userType === UserType.Mentee && | ||
| activeMentorshipMatch?.status === MentorshipMatchStatus.Accepted && | ||
| activeMentorshipMatch?.mentorId !== profile.userId | ||
| ) { | ||
| return 'currentUserIsMenteeAndViewsNotTheirMentor' | ||
| } |
There was a problem hiding this comment.
Fix issues in ViewMode type and determineViewMode function
The changes to the ViewMode type and determineViewMode function are good, but there are a few issues that need to be addressed:
- Remove the incorrect condition on line 344. This check is not necessary and may cause unexpected behavior.
- Remove the
console.logstatement on line 346. This should not be present in production code. - Update the condition on line 351 to use
profile.idinstead ofprofile.userIdfor consistency.
Apply this diff to fix the issues:
- if (activeMentorshipMatch.mentorId === currentUserProfile.id) return 'display'
-
- console.log(activeMentorshipMatch, profile)
-
if (
currentUserProfile.userType === UserType.Mentee &&
activeMentorshipMatch?.status === MentorshipMatchStatus.Accepted &&
- activeMentorshipMatch?.mentorId !== profile.userId
+ activeMentorshipMatch?.mentorId !== profile.id
) {
return 'currentUserIsMenteeAndViewsNotTheirMentor'
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| type ViewMode = | |
| | 'display' | |
| | 'notActivelyMentoring' | |
| | 'mentoringButNoFreeSpots' | |
| | 'currentUserIsMenteeAndViewsNotTheirMentor' | |
| function determineViewMode( | |
| profile: ProfilePageQueryQuery['conProfile'] | |
| profile: ProfilePageQueryQuery['conProfile'], | |
| currentUserProfile: LoadMyProfileQuery['conProfile'] | |
| ): ViewMode { | |
| if (profile.userType !== UserType.Mentor) return 'display' | |
| // If we're looking at a mentee, show the profile. Otherwise, the profile | |
| // is a mentor's profile, so continue to determine if the view mode is a | |
| // "special" one. | |
| if (profile.userType === UserType.Mentee) return 'display' | |
| const activeMentorshipMatch = currentUserProfile.mentorshipMatches.find( | |
| (match) => match.status === MentorshipMatchStatus.Accepted | |
| ) | |
| // Is current user a mentee, does that mentee have an active match, and is | |
| // that match with another mentor than the one we're currently looking at? | |
| if (activeMentorshipMatch.mentorId === currentUserProfile.id) return 'display' | |
| console.log(activeMentorshipMatch, profile) | |
| if ( | |
| currentUserProfile.userType === UserType.Mentee && | |
| activeMentorshipMatch?.status === MentorshipMatchStatus.Accepted && | |
| activeMentorshipMatch?.mentorId !== profile.userId | |
| ) { | |
| return 'currentUserIsMenteeAndViewsNotTheirMentor' | |
| } | |
| type ViewMode = | |
| | 'display' | |
| | 'notActivelyMentoring' | |
| | 'mentoringButNoFreeSpots' | |
| | 'currentUserIsMenteeAndViewsNotTheirMentor' | |
| function determineViewMode( | |
| profile: ProfilePageQueryQuery['conProfile'], | |
| currentUserProfile: LoadMyProfileQuery['conProfile'] | |
| ): ViewMode { | |
| // If we're looking at a mentee, show the profile. Otherwise, the profile | |
| // is a mentor's profile, so continue to determine if the view mode is a | |
| // "special" one. | |
| if (profile.userType === UserType.Mentee) return 'display' | |
| const activeMentorshipMatch = currentUserProfile.mentorshipMatches.find( | |
| (match) => match.status === MentorshipMatchStatus.Accepted | |
| ) | |
| if ( | |
| currentUserProfile.userType === UserType.Mentee && | |
| activeMentorshipMatch?.status === MentorshipMatchStatus.Accepted && | |
| activeMentorshipMatch?.mentorId !== profile.id | |
| ) { | |
| return 'currentUserIsMenteeAndViewsNotTheirMentor' | |
| } |
There was a problem hiding this comment.
@helloanil, I guess this hasn't been handled, and it now causes an error on production:
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- apps/redi-connect/src/pages/app/find-a-mentor/FindAMentor.tsx (2 hunks)
- apps/redi-connect/src/pages/app/profile/Profile.tsx (4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/redi-connect/src/pages/app/find-a-mentor/FindAMentor.tsx
🧰 Additional context used
🔇 Additional comments (4)
apps/redi-connect/src/pages/app/profile/Profile.tsx (4)
2-2: LGTM: New imports and variables added correctlyThe new import
LoadMyProfileQueryand the addition of theactiveMentorshipMatchvariable enhance the component's functionality. The update to theviewModevariable is consistent with the changes in thedetermineViewModefunction. These changes appear to be well-implemented and necessary for the new features.Also applies to: 93-97
323-327: LGTM: ViewMode type updated correctlyThe
ViewModetype has been appropriately updated to include the newcurrentUserIsMenteeAndViewsNotTheirMentoroption. This addition accurately represents the new scenario being handled in the component.
Line range hint
1-354: Overall implementation aligns with PR objectivesThe changes in this file effectively implement the new restrictions for mentees with active mentorships, as outlined in the PR objectives and linked issue #982. The new
ViewModeoption, updateddetermineViewModefunction, and additional message for mentees viewing other mentors' profiles all contribute to achieving the desired functionality.A few minor issues were identified in the
determineViewModefunction, which have been addressed in a previous comment. Once these are resolved, the implementation will be complete and aligned with the requirements.To fully meet the objectives, ensure that:
- The "find a mentor" page restrictions are also implemented (as mentioned in the linked issue).
- The mentorship link in the new message is verified to use the correct identifier.
To ensure all requirements are met, please confirm that the "find a mentor" page restrictions have been implemented in the appropriate file(s).
263-274: Verify the mentorship link identifierThe new message for mentees viewing other mentors' profiles is well-written and aligns with the requirements. However, please verify if
activeMentorshipMatch?.idis the correct identifier for linking to the mentorship dashboard. Ensure this is consistent with how mentorship links are generated elsewhere in the application.Run the following script to check for consistency in mentorship link generation:
✅ Verification successful
Run an updated script to search mentorship link patterns in TypeScript files:
Mentorship link identifier is consistent and correct
The mentorship link in
Profile.tsxusesactiveMentorshipMatch?.id, which is consistently implemented across the codebase in files likeFindAMentor.tsx. This ensures reliable linking to the mentorship dashboard.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for mentorship link generation patterns echo "Searching for mentorship link patterns:" rg --type typescript 'href.*mentorship.*id'Length of output: 169
Script:
#!/bin/bash # Description: Check for mentorship link generation patterns in TypeScript files without using --type echo "Searching for mentorship link patterns in .ts and .tsx files:" rg 'href.*mentorship.*id' --glob "*.ts" --glob "*.tsx"Length of output: 439
| function determineViewMode( | ||
| profile: ProfilePageQueryQuery['conProfile'] | ||
| profile: ProfilePageQueryQuery['conProfile'], | ||
| currentUserProfile: LoadMyProfileQuery['conProfile'] | ||
| ): ViewMode { | ||
| if (profile.userType !== UserType.Mentor) return 'display' | ||
| // If we're looking at a mentee, show the profile. Otherwise, the profile | ||
| // is a mentor's profile, so continue to determine if the view mode is a | ||
| // "special" one. | ||
| if (profile.userType === UserType.Mentee) return 'display' | ||
|
|
||
| const activeMentorshipMatch = currentUserProfile.mentorshipMatches.find( | ||
| (match) => match.status === MentorshipMatchStatus.Accepted | ||
| ) | ||
|
|
||
| // Is current user a mentee, does that mentee have an active match, and is | ||
| // that match with another mentor than the one we're currently looking at? | ||
| if (activeMentorshipMatch.mentorId === currentUserProfile.id) return 'display' | ||
|
|
||
| console.log(activeMentorshipMatch, profile) | ||
|
|
||
| if ( | ||
| currentUserProfile.userType === UserType.Mentee && | ||
| activeMentorshipMatch?.status === MentorshipMatchStatus.Accepted && | ||
| activeMentorshipMatch?.mentorId !== profile.userId | ||
| ) { | ||
| return 'currentUserIsMenteeAndViewsNotTheirMentor' | ||
| } |
There was a problem hiding this comment.
Fix issues in determineViewMode function
The updated function signature and new logic for handling mentees viewing other mentors' profiles are good additions. However, there are a few issues that need to be addressed:
- Remove the incorrect condition on line 344. This check is not necessary and may cause unexpected behavior.
- Remove the
console.logstatement on line 346. This should not be present in production code. - Update the condition on line 351 to use
profile.idinstead ofprofile.userIdfor consistency.
Apply this diff to fix the issues:
- if (activeMentorshipMatch.mentorId === currentUserProfile.id) return 'display'
-
- console.log(activeMentorshipMatch, profile)
-
if (
currentUserProfile.userType === UserType.Mentee &&
activeMentorshipMatch?.status === MentorshipMatchStatus.Accepted &&
- activeMentorshipMatch?.mentorId !== profile.userId
+ activeMentorshipMatch?.mentorId !== profile.id
) {
return 'currentUserIsMenteeAndViewsNotTheirMentor'
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function determineViewMode( | |
| profile: ProfilePageQueryQuery['conProfile'] | |
| profile: ProfilePageQueryQuery['conProfile'], | |
| currentUserProfile: LoadMyProfileQuery['conProfile'] | |
| ): ViewMode { | |
| if (profile.userType !== UserType.Mentor) return 'display' | |
| // If we're looking at a mentee, show the profile. Otherwise, the profile | |
| // is a mentor's profile, so continue to determine if the view mode is a | |
| // "special" one. | |
| if (profile.userType === UserType.Mentee) return 'display' | |
| const activeMentorshipMatch = currentUserProfile.mentorshipMatches.find( | |
| (match) => match.status === MentorshipMatchStatus.Accepted | |
| ) | |
| // Is current user a mentee, does that mentee have an active match, and is | |
| // that match with another mentor than the one we're currently looking at? | |
| if (activeMentorshipMatch.mentorId === currentUserProfile.id) return 'display' | |
| console.log(activeMentorshipMatch, profile) | |
| if ( | |
| currentUserProfile.userType === UserType.Mentee && | |
| activeMentorshipMatch?.status === MentorshipMatchStatus.Accepted && | |
| activeMentorshipMatch?.mentorId !== profile.userId | |
| ) { | |
| return 'currentUserIsMenteeAndViewsNotTheirMentor' | |
| } | |
| function determineViewMode( | |
| profile: ProfilePageQueryQuery['conProfile'], | |
| currentUserProfile: LoadMyProfileQuery['conProfile'] | |
| ): ViewMode { | |
| // If we're looking at a mentee, show the profile. Otherwise, the profile | |
| // is a mentor's profile, so continue to determine if the view mode is a | |
| // "special" one. | |
| if (profile.userType === UserType.Mentee) return 'display' | |
| const activeMentorshipMatch = currentUserProfile.mentorshipMatches.find( | |
| (match) => match.status === MentorshipMatchStatus.Accepted | |
| ) | |
| if ( | |
| currentUserProfile.userType === UserType.Mentee && | |
| activeMentorshipMatch?.status === MentorshipMatchStatus.Accepted && | |
| activeMentorshipMatch?.mentorId !== profile.id | |
| ) { | |
| return 'currentUserIsMenteeAndViewsNotTheirMentor' | |
| } |
What Github issue does this PR relate to? Insert link.
Closes #982
What should the reviewer know?
This PR implements a quick solution to add further restrictions to mentees visiting the profile page of a mentor. Besides the two existing restrictions (mentor isn't actively mentoring, or mentor has no free mentoring spots), we add a third one: mentees with an active mentor/mentorship can't view the profile of another mentor.
NOTE: PR implemented while travelling on a laptop without working dev environment - please test the implementation and error-free build:
/app/find-a-mentor/profile/:profileIdof a mentorSummary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Documentation