Fix/con/898 ensure exclusive acceptance of mentors #914
Conversation
…etails) this prevents a scenario where both Mentor A and B, both of them having received a mentorship application from Mentee C, go to the Applications page, and Mentor A his application first, and thereafter Mentor B then accepts his application. The acceptance by Mentor A of the first mentorship match will invalidate the second mentorship match sent to Mentor B
WalkthroughThe recent changes primarily focus on enhancing the mentorship matching workflow. A critical check was added to ensure mentorships are only accepted when in the "APPLIED" state. Error handling improvements and periodic data refetch mechanisms were also introduced across various components. Additionally, unnecessary logging was removed from an unrelated file. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as ConfirmMentorship UI
participant APICall as acceptMentorship API Call
participant Service as ConMentorshipMatchesService
User->>UI: Click Accept Mentorship
UI->>APICall: Send Accept Request
APICall->>Service: Handle Request
Service->>Service: Check if match is in "APPLIED" state
alt State is "APPLIED"
Service->>APICall: Confirm Success
APICall->>UI: Success Response
UI->>User: Show Success Message and Reload Page
else State is not "APPLIED"
Service->>APICall: Reject with Error
APICall->>UI: Error Response
UI->>User: Show Error Message and Reload Page
end
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
|
@ericbolikowski, I think the better way could be to inform them about application invalidation on the platform as well and not let them accept it. But I don't understand why the mentor sees the |
|
Hey @ericbolikowski, thanks for the PR and questions. Dear @katamatata, thanks for the comment. Here are my thoughts, what do you think? 1. Move Application to Thanks! |
|
Ah, we're getting into the nasty territory of concurrency and simultaneity here. First, a brainfuck: we're already doing what @astkhikatredi suggests in her points (1) and (2). And @katamatata, yes, we don't show Accept/Decline buttons when application is Here's why - let me illustrate with the example again:
|
|
Writing this made me think about a simple fix: the refresh. We can very easily update our code so that Kate's browser asks the server every five or ten seconds to give it an up-to-date application, so that it can become aware of a change from We don't give a lot of context to tell the mentor in the browser what "Cancelled" means (that's done in a lot of detail in the email they receive), but we can quite easily also add some info bubble/card that says something like "This application was cancelled since another mentor accepted an application from this mentee". |
I thought about the refresh, too, and thought we were already doing it. Probably not that often. |
|
We do all our data loading using react-query. React-query does its queries using a query client what we configure and give it. Here's how we do it in both CON and TP:
Note We can set these options on a per-query level as well. At the moment we've got that could be changed to: ... which will probably have this effect: while user is on the applications page, aggressively refetch the data every five seconds. We'd need to recheck the react-query docs (I just did so very briefly right now), but this is easy to put in place. @katamatata, maybe a nice learning opportunity? Wanna dig in and then we talk more in next mentoring session? |
|
Dear @ericbolikowski, any timeline for the implementation? |
|
Hi @astkhikatredi, @katamatata and I aligned this morning:
Most work is already done, so I don't think we're far from completing this ticket. |
@ericbolikowski, I apologize for the delayed response and thank you for providing more context on the query implementation! Upon reviewing the react-query docs, I found that since We indeed can use only the Do you agree? |
|
@katamatata sounds good - thanks for digging into it. I agree with the higher Can you test this locally? If it works as intended, you should see a new request every minute (or whatever you set, for testing) in the |
|
@ericbolikowski, this solution works fine 👍 I tested it & pushed the commit. |
|
@katamatata thank you! And - agreed. Let's talk more about this on Monday morning. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (4)
apps/redi-connect/src/pages/app/applications/Applications.tsx (2)
Line range hint
45-45: Replace the anchor tag with a button for better accessibility.Using an anchor tag for actions that perform operations other than navigation is not recommended as it can lead to poor accessibility. Consider changing this to a button element to improve accessibility and semantic correctness.
- <a onClick={() => history.push('/app/find-a-mentor')}> + <button onClick={() => history.push('/app/find-a-mentor')}>
Line range hint
51-55: Remove unnecessary React fragment.The React fragment here is redundant since it wraps a single child. Removing it can clean up the code and reduce unnecessary nesting.
- <> {profile.userType === 'MENTOR' && ( <> You currently have no mentee applications. To increase your chances of receiving an application, make sure that your profile is up-to-date, informative and friendly. </> )} - </> + {profile.userType === 'MENTOR' && ( You currently have no mentee applications. To increase your chances of receiving an application, make sure that your profile is up-to-date, informative and friendly. )}apps/redi-connect/src/pages/app/applications/ApplicationsFilterContext.tsx (1)
Line range hint
64-76: Remove unnecessary else clauses to simplify logic.The else clauses are unnecessary as all previous branches in the filter function return early. Removing them can simplify the code and increase readability.
- else if (activeFilter === 'accepted') { + if (activeFilter === 'accepted') { - else if (activeFilter === 'declined') { + if (activeFilter === 'declined') { - else if (activeFilter === 'cancelled') { + if (activeFilter === 'cancelled') {apps/nestjs-api/src/con-mentorship-matches/con-mentorship-matches.service.ts (1)
Line range hint
45-47: Remove unnecessary else clause to simplify logic.The else clause after the if condition can be omitted as it is unnecessary and can simplify the control flow of the code.
- else { + {
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- apps/nestjs-api/src/con-mentorship-matches/con-mentorship-matches.service.ts (1 hunks)
- apps/redi-connect/src/components/organisms/ConfirmMentorship.tsx (1 hunks)
- apps/redi-connect/src/pages/app/applications/Applications.tsx (1 hunks)
- apps/redi-connect/src/pages/app/applications/ApplicationsFilterContext.tsx (1 hunks)
- apps/redi-talent-pool/src/components/organisms/company-profile-editables/EditableNamePhotoLocation.tsx (1 hunks)
Files skipped from review due to trivial changes (2)
- apps/redi-connect/src/components/organisms/ConfirmMentorship.tsx
- apps/redi-talent-pool/src/components/organisms/company-profile-editables/EditableNamePhotoLocation.tsx
Additional context used
Biome
apps/redi-connect/src/pages/app/applications/Applications.tsx
[error] 45-45: Use a button element instead of an a element. (lint/a11y/useValidAnchor)
Anchor elements should only be used for default sections or page navigation
Check this thorough explanation to better understand the context.
[error] 51-55: Avoid using unnecessary Fragment. (lint/complexity/noUselessFragments)
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragmentapps/redi-connect/src/pages/app/applications/ApplicationsFilterContext.tsx
[error] 64-76: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 69-76: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
[error] 71-76: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
apps/nestjs-api/src/con-mentorship-matches/con-mentorship-matches.service.ts
[error] 45-47: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
Additional comments not posted (3)
apps/redi-connect/src/pages/app/applications/Applications.tsx (1)
16-19: Ensure periodic data refresh with a refetch interval of one minute.The
refetchIntervalis set to 60 seconds, which should help keep the mentorship match data up-to-date and address the concurrency issue described in the PR.apps/redi-connect/src/pages/app/applications/ApplicationsFilterContext.tsx (1)
47-50: Ensure periodic data refresh with a refetch interval of one minute.The
refetchIntervalis consistently set to 60 seconds across different components, which aligns with the PR objectives to keep data fresh and mitigate race conditions.apps/nestjs-api/src/con-mentorship-matches/con-mentorship-matches.service.ts (1)
76-81: Ensure mentorship acceptance is contingent on the correct status.The added condition ensures that a mentorship can only be accepted if it is in the 'APPLIED' state, addressing the concurrency issue highlighted in the PR.
|
Hey @katamatata and @ericbolikowski , could you maybe review point 3 in my message above? What do you think about showing it to a mentor with an invalid mentorship application? 3. Insert Notification: |
What Github issue does this PR relate to? Insert link.
#898
This PR fixes bug #898 which allowed mentees in special cases to have two simultaneously active/accepted Mentorship Matches:
The case could happen when for example:
AppliedAcceptedInvalidated/CancelledAcceptedThe fix was obvious: in the code that handles the "accept mentorship" mutation, only change the Mentorship Match state into
Acceptedif the current state isApplied.What should the reviewer know?
The backend issue is solved, and we likely won't get into a state of one mentee + two active mentorships. But an unpleasant UX issue came up:
If the above steps happened again, but with the fix in this PR, Kate would in step 6 be prevented from accepting the mentorship request. Nestjs would return an error handled by the CON frontend in the
catch (error) {clause you'll see. I've implemented a crude alert() shown to the user and then the page refreshes.That scenario is illustrated in this Loom:
https://www.loom.com/share/47fb8764f8d14c619092a3c8a6775b6d?sid=1dd21bcc-d13f-4020-a133-7a91d5bbb265
This leads to the loss of the acceptance message that Kate wrote - which is annoying, but the message isn't relevant any longer since Eric has a mentorship with Anil and isn't supposed to have one with Kate.
The question is: are there any easy ways to improve this UX?
Summary by CodeRabbit
New Features
Enhancements
Chores
console.logstatement from the company profile editables component.