Feat/con/link mentoring session to parent match #991
Conversation
WalkthroughThis pull request introduces several modifications across multiple files to incorporate a new property, Changes
Possibly related issues
Possibly related PRs
Suggested reviewers
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: 1
🧹 Outside diff range and nitpick comments (6)
libs/common-types/src/lib/con-mentoring-session/con-mentoring-session.recordprops.ts (1)
13-13: Consider adding validation for Master-Detail relationship.The addition of
Mentorship_Match__caligns with the PR objectives. However, since this will be a Master-Detail relationship in Salesforce, consider adding validation to ensure this field is always populated.Consider using class-validator to enforce the required field:
+import { IsNotEmpty } from 'class-validator' export class ConMentoringSessionRecordProps implements RecordProps { // ... + @IsNotEmpty({ message: 'Mentorship Match is required' }) Mentorship_Match__c: string // ... }libs/common-types/src/lib/con-mentoring-session/con-mentoring-session.entityprops.ts (1)
18-19: Consider adding JSDoc documentation.To improve code maintainability, consider adding JSDoc documentation to describe the relationship between the mentoring session and its parent match.
+ /** ID of the parent Mentorship Match this session belongs to */ @Field((type) => ID) mentorshipMatchId: stringlibs/common-types/src/lib/con-mentoring-session/con-mentoring-session.record.ts (1)
Line range hint
16-29: Consider documenting field requirements in metadata.Since this represents a significant relationship change, consider enhancing the metadata documentation to indicate whether this is a required field and its relationship type (lookup vs master-detail).
Example enhancement:
public static metadata: RecordMetadata = { SALESFORCE_OBJECT_NAME: 'Mentoring_Session__c', SALESFORCE_OBJECT_FIELDS: [ 'Id', 'Date__c', 'Durations_in_Minutes__c', 'Mentee__c', 'Mentor__c', 'Mentorship_Match__c', 'CreatedDate', 'LastModifiedDate', ], SALESFORCE_ORDER_BY: ['Date__c', 'ASC'], + FIELD_REQUIREMENTS: { + 'Mentorship_Match__c': { + required: false, // Update based on actual requirement + relationship: 'Master-Detail', + description: 'Links mentoring session to parent Mentorship Match' + } + } }apps/nestjs-api/src/salesforce-api/sf-api-con-mentoring-sessions.service.ts (1)
Validation for mentorship match is recommended
Based on the codebase analysis:
- The
ConMentoringSessionRecordPropsshows thatMentorship_Match__cis typed as a required string field- The repository layer (
SfApiRepository) provides methods to query Salesforce objects- No existing validation is implemented for the mentorship match relationship
Recommended implementation in
sf-api-con-mentoring-sessions.service.ts:async create(record: ConMentoringSessionRecord) { // Verify mentorship match exists and matches mentor/mentee const matchRecord = await this.repository.findRecordsOfObject({ objectName: 'Mentorship_Match__c', objectFields: ['Id', 'Mentor__c', 'Mentee__c'], filter: { Id: record.props.Mentorship_Match__c } }); if (!matchRecord.length) { throw new Error('Mentorship match not found'); } if (matchRecord[0].Mentor__c !== record.props.Mentor__c || matchRecord[0].Mentee__c !== record.props.Mentee__c) { throw new Error('Mentor/Mentee mismatch with the mentorship match'); } // Existing creation logic... const cleanProps = pick(record.props, [ 'Date__c', 'Durations_in_Minutes__c', 'Mentor__c', 'Mentee__c', 'Mentorship_Match__c', ]); return this.repository.createRecord( ConMentoringSessionRecord.metadata.SALESFORCE_OBJECT_NAME, cleanProps ); }🔗 Analysis chain
Line range hint
34-42: Consider adding validation for mentorship match.To ensure data integrity, consider adding validation to:
- Verify that the mentorship match ID is provided
- Confirm that the mentor and mentee in the session match those in the referenced mentorship match
Let's verify the type safety in the record props:
Consider implementing these validations:
async create(record: ConMentoringSessionRecord) { // Validate mentorship match exists and contains the same mentor/mentee const match = await this.repository.findRecordsOfObject({ objectName: 'Mentorship_Match__c', objectFields: ['Id', 'Mentor__c', 'Mentee__c'], filter: { Id: record.props.Mentorship_Match__c } }); if (!match.length || match[0].Mentor__c !== record.props.Mentor__c || match[0].Mentee__c !== record.props.Mentee__c) { throw new Error('Invalid mentorship match'); } // Rest of the existing code... }🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if Mentorship_Match__c is properly typed in ConMentoringSessionRecord ast-grep --pattern 'class ConMentoringSessionRecord { $$$ Mentorship_Match__c: $_ $$$ }' # Check if there are any existing validations rg -A 5 "Mentorship_Match__c.*validate"Length of output: 138
Script:
#!/bin/bash # Let's first find the ConMentoringSessionRecord class definition fd ConMentoringSessionRecord --exec cat {} # Check for any existing validation logic in the service fd sf-api-con-mentoring-sessions.service --exec cat {} # Look for any validation patterns in similar services rg -A 5 "validate.*Mentorship_Match"Length of output: 1645
Script:
#!/bin/bash # Let's check the common-types for ConMentoringSessionRecord definition rg -A 10 "class ConMentoringSessionRecord" # Check if there's any validation in the repository layer fd sf-api.repository --exec cat {} # Look for any existing validation patterns in similar services rg -A 5 "validate.*Match"Length of output: 13189
apps/redi-connect/src/pages/app/mentorship/Mentorship.tsx (1)
Line range hint
1-124: Consider implementing session filtering by match.While the PR objectives mention keeping the existing query unchanged to show all sessions, consider adding an optional filter capability to MSessions. This would allow users to toggle between viewing all sessions or only those related to the current match.
Would you like me to propose a design for implementing this filtering capability?
apps/redi-connect/src/components/organisms/modules/MSessions.tsx (1)
Line range hint
94-98: Consider implementing a more robust caching strategy.The TODO comment raises a valid concern about query invalidation. While the current approach works, implementing a normalized cache (like Apollo) would provide more intelligent cache invalidation and better data consistency.
Options to consider:
- Migrate to Apollo Client for automatic cache normalization
- Implement a custom cache normalization layer with React Query
- Use React Query's built-in cache manipulation utilities for more granular control
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (10)
- apps/nestjs-api/src/salesforce-api/sf-api-con-mentoring-sessions.service.ts (1 hunks)
- apps/redi-connect/src/components/organisms/modules/MSessions.tsx (2 hunks)
- apps/redi-connect/src/pages/app/mentorship/Mentorship.tsx (1 hunks)
- libs/common-types/src/lib/con-mentoring-session/con-mentoring-session.entityprops.ts (1 hunks)
- libs/common-types/src/lib/con-mentoring-session/con-mentoring-session.mapper.ts (2 hunks)
- libs/common-types/src/lib/con-mentoring-session/con-mentoring-session.record.ts (1 hunks)
- libs/common-types/src/lib/con-mentoring-session/con-mentoring-session.recordprops.ts (1 hunks)
- libs/common-types/src/lib/con-mentoring-session/entity-dtos/create-con-mentoring-session.entityinput.ts (2 hunks)
- libs/data-access/src/lib/types/types.ts (2 hunks)
- schema.graphql (2 hunks)
🔇 Additional comments (16)
libs/common-types/src/lib/con-mentoring-session/entity-dtos/create-con-mentoring-session.entityinput.ts (2)
1-1: LGTM! Clean import statement.The import statement has been simplified by removing unused imports, improving code cleanliness.
8-11: Verify GraphQL clients for breaking change.Adding
mentorshipMatchIdas a required field in the input type is a breaking change that could affect existing GraphQL clients. This aligns with the PR objective of linking sessions to matches, but requires careful deployment.Let's verify the impact:
libs/common-types/src/lib/con-mentoring-session/con-mentoring-session.recordprops.ts (1)
Line range hint
20-22: Verify data transformation handling.The
createmethod usesplainToClassfor transformation. Let's verify that other components properly handle the new field during record creation.✅ Verification successful
Based on the results, I can generate the final response as we have sufficient information about how the field is handled in the codebase.
Field transformation is properly handled across components
The verification shows that:
- The
Mentorship_Match__cfield is correctly mapped in both directions inConMentoringSessionMapper:
- From raw to entity:
props.mentorshipMatchId = raw.props.Mentorship_Match__c- From entity to record:
props.Mentorship_Match__c = srcProps.mentorshipMatchId- The create method chain is consistent:
ConMentoringSessionRecordProps.create()transforms raw dataConMentoringSessionRecord.create()uses the transformed propsConMentoringSessionMapperhandles the field mapping properly🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if the new field is properly handled in related services and mappers # Look for related service implementations rg -l "ConMentoringSessionRecordProps" | xargs rg -A 5 "create.*Mentoring.*Session" # Check for field mapping in related mappers rg -l "ConMentoringSession.*Mapper" | xargs rg -A 5 "Mentorship_Match__c"Length of output: 2187
libs/common-types/src/lib/con-mentoring-session/con-mentoring-session.entityprops.ts (1)
18-19: Implementation looks good, but verify deployment sequence.The addition of
mentorshipMatchIdas a required field aligns with the PR objectives. However, since this field will be populated by an APEX script for existing records, we should ensure proper deployment sequencing.Let's verify if there are any existing sessions that might be affected:
✅ Verification successful
Let me gather more specific information about the GraphQL mutations and input types related to creating mentoring sessions.
Let me try a different approach to find the input type and mutation definitions using ripgrep.
No deployment sequencing concerns - field already required in mutations
Based on the evidence from the codebase:
- The
mentorshipMatchIdfield is already a required field in theCreateConMentoringSessionInputtype as shown in the schema- The mutation
createConMentoringSessionis the only way to create new sessions and it already requires this field- Existing sessions will be handled by the APEX script as planned
Therefore, making the field required in
con-mentoring-session.entityprops.tsaligns with the existing GraphQL schema and won't affect the application's functionality.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any GraphQL queries or mutations that might fail # due to the new required field before the APEX script runs # Look for GraphQL operations involving ConMentoringSession rg -l "ConMentoringSession" | xargs rg "query|mutation" -A 5Length of output: 12025
Script:
#!/bin/bash # Check the CreateConMentoringSessionInput type definition and usage ast-grep --pattern 'input CreateConMentoringSessionInput { $$$ }' # Also check for any existing session creation mutations in the resolver ast-grep --pattern '@Mutation() $$$createConMentoringSession$$$'Length of output: 138
Script:
#!/bin/bash # Search for CreateConMentoringSessionInput definition rg "CreateConMentoringSessionInput" -A 5 # Search for createConMentoringSession mutation implementation rg "createConMentoringSession" -A 5 "apps/nestjs-api"Length of output: 8782
libs/common-types/src/lib/con-mentoring-session/con-mentoring-session.record.ts (1)
24-24: Verify Salesforce field configuration.The addition of 'Mentorship_Match__c' aligns with the PR objective to link mentoring sessions with their parent match.
Let's verify the field configuration across the codebase:
✅ Verification successful
Field mapping and usage is consistent across the codebase
The verification shows that:
- The field
Mentorship_Match__cis properly mapped tomentorshipMatchIdin the entity layer- The field is consistently used in record props, mappers, and services
- The field is part of the core data model as evidenced by its inclusion in
ConMentoringSessionRecordProps- The field is actively used in mentorship-related operations (accept, decline, complete)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify consistent usage of Mentorship_Match__c field # Expected: Field should be referenced consistently in related files # Check for field references in TypeScript/JavaScript files rg -t ts -t js "Mentorship_Match__c" # Check for potential schema definitions fd -e schema.graphql -x cat {} \; | rg -A 5 "MentoringSession|MentorshipMatch"Length of output: 978
Script:
#!/bin/bash # Let's verify the field mapping and usage consistency # Check for mentorshipMatchId in TypeScript files rg -t ts "mentorshipMatchId" -A 2 # Check the complete ConMentoringSession record definition fd -e ts "con-mentoring-session.recordprops.ts" -x cat {} # Check for any validation or required field definitions rg -t ts "required.*Mentorship_Match__c|Mentorship_Match__c.*required" # Check GraphQL type definitions for MentoringSession fd -g "*.graphql" -x cat {} | rg -A 5 "type.*MentoringSession"Length of output: 12190
apps/nestjs-api/src/salesforce-api/sf-api-con-mentoring-sessions.service.ts (1)
34-34: LGTM! The implementation aligns with PR objectives.The addition of
Mentorship_Match__cto the picked properties correctly implements the new requirement to link mentoring sessions with their parent mentorship match.libs/common-types/src/lib/con-mentoring-session/con-mentoring-session.mapper.ts (3)
47-47: LGTM! Correct Salesforce field mappingThe mapping correctly preserves the mentorship match reference when persisting to Salesforce, following proper field naming conventions.
27-27: Verify nullability requirements for mentorshipMatchIdSince this represents a Salesforce Master-Detail relationship to Mentorship Match, we should ensure proper handling of null values. Consider adding null checks or making the field required in the entity props if it maps to a Master-Detail relationship.
26-28: Consider adding data consistency validationGiven that an APEX script will backfill existing records, consider adding validation in the mapper to ensure data consistency:
- Verify that the mentor/mentee pair in the session matches those in the referenced mentorship match
- Log any mismatches for monitoring during the transition period
Also applies to: 46-48
apps/redi-connect/src/pages/app/mentorship/Mentorship.tsx (1)
105-105: LGTM! Verify MSessions component implementation.The addition of the
mentorshipMatchIdprop correctly passes the route'smatchIdto the MSessions component, supporting the new feature of linking sessions to parent matches.Let's verify the MSessions component implementation:
✅ Verification successful
Let me gather more information about the MSessions component interface and implementation.
Let me check one more aspect to ensure the mutation type and interface are properly defined.
Based on the evidence gathered from the shell scripts, I can now provide the final response:
✅ MSessions component correctly implements mentorshipMatchId
The verification confirms that:
- The component properly uses the
mentorshipMatchIdprop in the session creation mutation- The mutation correctly includes
mentorshipMatchIdin its input payload- The implementation aligns with the PR's objective of linking sessions to parent matches
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify MSessions component properly handles mentorshipMatchId # Check MSessions component interface and implementation ast-grep --pattern 'interface MSessionsProps { $$$ mentorshipMatchId: $_ $$$ }' # Check session creation mutation usage rg -A 5 'createSessionMutation.mutateAsync'Length of output: 655
Script:
#!/bin/bash # Check MSessions component definition and props usage ast-grep --pattern 'const MSessions = ({ $$ }: MSessionsProps) => { $$$ }' # Check MSessions component type definition rg -B 5 -A 10 'interface MSessionsProps' # Check createSession mutation type rg -B 2 -A 5 'createSession.*Mutation'Length of output: 2459
Script:
#!/bin/bash # Check mutation type definition and interface rg -B 2 -A 2 'useCreateMentoringSessionMutation' # Check if mentorshipMatchId is used in the mutation type rg -B 5 -A 5 'CreateMentoringSessionInput'Length of output: 1649
apps/redi-connect/src/components/organisms/modules/MSessions.tsx (2)
71-76: LGTM! Component signature properly updated.The component signature has been correctly updated to include the new required prop, maintaining consistency with the interface changes.
91-91: LGTM! Mutation input correctly updated.The mentorshipMatchId is properly included in the mutation input object.
schema.graphql (2)
37-37: LGTM! The new field aligns with the Master-Detail relationship requirement.The addition of the non-nullable
mentorshipMatchIdfield to theConMentoringSessiontype correctly implements the requirement to link mentoring sessions to their parent mentorship matches.
170-170: LGTM! The input type change ensures data consistency.The addition of the required
mentorshipMatchIdfield toCreateConMentoringSessionInputensures that new mentoring sessions are always linked to a parent mentorship match, maintaining data integrity.libs/data-access/src/lib/types/types.ts (2)
58-58: LGTM: Added required mentorship match relationship.The new
mentorshipMatchIdfield is correctly typed as a non-nullable ID scalar, properly establishing the relationship between mentoring sessions and their parent mentorship matches.
197-197: LGTM: Input type updated to require mentorship match.The
CreateConMentoringSessionInputtype has been correctly updated to require amentorshipMatchId, ensuring that all new mentoring sessions are properly linked to a parent mentorship match.
|
@ericbolikowski, I just have one question. So, the |
Yes, that's right. The ticket requirements stated it's necessary, but I found it overall better not to do it, mainly for the reason mentioned in the PR description: The query that loads mentoring sessions ... |
ericbolikowski
deleted the
feat/con/link-mentoring-session-to-parent-match
branch
What Github issue does this PR relate to? Insert link.
#949
What should the reviewer know?
This PR makes use of the new
Mentorship_Match__cfield onMentoring Sessionrecords in Salesforce when sessions are logged.When a mentoring session is logged, it's always been linked to a
Mentorand aMentee. With this PR, sessions will also be linked to a parentMentorship Match.The query that loads mentoring sessions and display them on the "Mentorship Dashboard" page, as seen here ...
... was not updated, since it lets the mentor and mentee see their entire history of logged sessions, even if they have more than one match (e.g. a completed one and a currently active one.
The other tasks that are part of #949 will be taken care of on deployment to production:
Summary by CodeRabbit
Release Notes
New Features
mentorshipMatchIdto enhance the data flow in mentoring sessions, linking them directly to mentorship matches.mentorshipMatchIdproperty during session creation.Bug Fixes
Mentorship_Match__cfield.Documentation
mentorshipMatchIdin relevant types and input structures.