Allow roleSet invitation to an extra Role #4587
Conversation
WalkthroughThe pull request introduces several changes across multiple files, primarily focusing on renaming the Changes
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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (14)
src/domain/access/invitation/dto/invitation.dto.create.ts (2)
15-19: Approved: Field renaming and description update improve clarity.The changes to this field enhance its clarity and purpose:
- Renaming to
invitedContributorIDclearly indicates it's an identifier.- Removing
@IsOptional()aligns with the non-nullable GraphQL field definition.- The updated description provides more context.
Consider further improving the description for consistency:
- 'The identifier for the contributor being invited to join in the entry Role.', + 'The identifier for the contributor being invited to join in the entry role.',This change makes "role" lowercase to match typical naming conventions.
31-38: Approved: NewextraRolefield is well-defined and documented.The new
extraRolefield is correctly implemented:
- It uses the
CommunityRoleTypeenum for type safety.- It's properly marked as optional in both GraphQL and class-validator.
- The
@MaxLengthdecorator uses theSMALL_TEXT_LENGTHconstant for consistency.- The description clearly explains the field's purpose.
Consider a minor improvement to the description for clarity:
- 'An additional role to assign to the Contributor, in addition to the entry Role.', + 'An additional role to assign to the contributor, in addition to the entry role.',This change makes "contributor" and "role" lowercase for consistency with typical naming conventions.
src/domain/access/invitation/invitation.interface.ts (3)
10-10: Approved: Property renamed for clarity.The renaming of
invitedContributortoinvitedContributorIDimproves clarity by explicitly indicating that it's an ID. This change aligns with the PR objectives.Consider adding a
@Fielddecorator to this property for consistency with other properties in the class:@Field(() => String, { description: 'ID of the invited contributor' }) invitedContributorID!: string;
40-45: Approved: NewextraRoleproperty added correctly.The
extraRoleproperty is correctly implemented with appropriate typing, optionality, and GraphQL field decoration. This addition aligns with the PR objectives.Consider adding a default value of
undefinedto make the optionality more explicit:extraRole?: CommunityRoleType = undefined;
Issues Found: Inconsistent Usage of
invitedContributorPropertyThe renaming from
invitedContributortoinvitedContributorIDappears incomplete. The following files still referenceinvitedContributor, which may lead to inconsistencies and potential bugs:
src/migrations/1727930288139-invitationToRole.tssrc/migrations/1724417095448-convergance.tssrc/migrations/1721897303394-fixPhantomVCInvitations.tssrc/migrations/1718174556242-invitationContributor.tssrc/services/api/roles/roles.service.tssrc/services/api/registration/registration.service.tssrc/domain/access/invitation/invitation.service.tssrc/domain/access/invitation/invitation.resolver.fields.tssrc/domain/access/invitation/invitation.interface.tssrc/domain/access/role-set/role.set.service.tssrc/services/adapters/notification-adapter/notification.adapter.tssrc/services/adapters/notification-adapter/dto/notification.dto.input.community.invitation.tsPlease review and update these references to ensure consistency across the codebase.
🔗 Analysis chain
Line range hint
1-45: Verify impact on related components.The changes to the
IInvitationinterface improve clarity and functionality, aligning well with the PR objectives. However, it's important to ensure that all components using this interface are updated accordingly.Please run the following script to identify potential areas that might need updates:
Retry: Find usages of
IInvitationandinvitedContributorin TypeScript filesThe previous script failed due to an unrecognized file type. Please run the following corrected script to locate all usages of
IInvitationandinvitedContributorwithin.tsfiles:🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find usages of IInvitation and invitedContributor echo "Files using IInvitation:" rg --type typescript "IInvitation" echo "\nPotential uses of old 'invitedContributor' property:" rg --type typescript "invitedContributor" echo "\nVerify these files and update as necessary."Length of output: 431
Script:
# #!/bin/bash # Description: Find usages of IInvitation and invitedContributor in TypeScript files echo "Files using IInvitation:" rg "IInvitation" --glob "*.ts" echo "\nPotential uses of old 'invitedContributor' property:" rg "invitedContributor" --glob "*.ts" echo "\nVerify these files and update as necessary."Length of output: 9862
src/domain/access/invitation/invitation.entity.ts (2)
42-46: Approve newextraRoleproperty and suggest documentation.The new
extraRoleproperty is well-defined and consistent with the entity's structure. Its optional nature is correctly reflected in both the TypeScript type and the database column definition.Consider adding a brief comment explaining the purpose and usage of the
extraRoleproperty to improve code documentation. For example:/** * Additional role that can be assigned to the invited user. * This is used when the invitation is for a role beyond the standard set. */ @Column('varchar', { length: ENUM_LENGTH, nullable: true, }) extraRole?: CommunityRoleType;
Line range hint
1-46: Overall impact: Ensure thorough testing and documentation updates.The changes to the
Invitationentity align well with the PR objectives. The renamedinvitedContributorIDimproves clarity, and the newextraRoleproperty extends the invitation functionality as intended.To ensure a smooth integration:
- Update all references to
invitedContributoracross the codebase.- Modify invitation creation and processing logic to handle the new
extraRoleproperty.- Update relevant documentation, including API docs if this entity is exposed via API.
- Conduct thorough testing, especially around invitation creation, acceptance, and role assignment processes.
Consider reviewing and updating the following areas:
- Invitation service logic
- API endpoints handling invitations
- Unit and integration tests for the invitation flow
src/domain/access/role-set/role.set.lifecycle.invitation.options.provider.ts (1)
130-139: LGTM with a suggestion: Consider refactoring for improved maintainabilityThe addition of the
extraRolelogic successfully implements the new functionality as per the PR objectives. The code is well-structured and follows the existing patterns in the file.To further improve maintainability, consider extracting the common
assignContributorToRolelogic into a separate method. This would reduce code duplication and make future changes easier.Here's a suggested refactoring:
private async assignContributorToRoleSet( roleSet: RoleSet, roleType: CommunityRoleType, contributorID: string, contributorType: string, agentInfo: AgentInfo, isMainRole: boolean ): Promise<void> { await this.roleSetService.assignContributorToRole( roleSet, roleType, contributorID, contributorType, agentInfo, isMainRole ); } // Then in the communityAddMember action: await this.assignContributorToRoleSet( roleSet, CommunityRoleType.MEMBER, contributorID, invitation.contributorType, event.agentInfo, true ); if (invitation.extraRole) { await this.assignContributorToRoleSet( roleSet, invitation.extraRole, contributorID, invitation.contributorType, event.agentInfo, false ); }This refactoring would make the code more DRY and easier to maintain.
src/domain/access/invitation/invitation.service.ts (1)
156-156: LGTM with suggestion: Consistent property naming and TypeScript improvementThe change from
invitedContributortoinvitedContributorIDin thewhereclause is consistent with the PR objectives and maintains consistency with the previous changes. This ensures that the query will work correctly with the updated property name.To further improve the code, consider using TypeScript's type system to define the shape of the
whereclause:const where: Partial<Pick<Invitation, 'invitedContributorID'>> = { invitedContributorID: contributorID }; findOpts.where = where;This change would provide better type checking and autocompletion for the
whereclause properties.src/core/authorization/authorization.service.ts (3)
90-93: Improved error message, consider using template literals.The addition of the authorization type to the error message is a good improvement. It provides more context for debugging and understanding which type of authorization policy is causing the issue.
Consider using template literals for better readability:
throw new AuthorizationInvalidPolicyException( `AuthorizationPolicy without credential rules provided: ${authorization.id}, type: ${authorization.type}`, LogContext.AUTH );
131-135: Improved log message, consider consistent formatting.The addition of the authorization type to the log message is a good improvement. It provides more context for understanding which type of authorization policy is granting the privilege.
For consistency with the error message in
validateAuthorization, consider formatting the log message like this:this.logger.verbose?.( `[CredentialRule] Granted privilege '${privilegeRequired}' using rule '${rule.name}' on authorization ${authorization.id}, type: ${authorization.type}`, LogContext.AUTH_POLICY );This change makes the formatting consistent across different parts of the code.
Line range hint
1-324: Overall improvements in error and log messages.The changes in this file enhance the clarity of error and log messages by adding more context about the authorization type. This aligns well with the PR objectives and improves the debuggability of the system.
Consider reviewing other error and log messages throughout this file for similar opportunities to add context. For example, in methods like
logCredentialCheckFailDetails,getGrantedPrivileges, and others where authorization type information could be valuable.Additionally, to improve consistency and reduce duplication, you might want to consider creating a helper method for formatting these messages. This could ensure that all messages follow the same pattern and make future updates easier.
src/domain/community/virtual-contributor/virtual.contributor.service.ts (1)
594-596: LGTM! Consider using object shorthand for consistency.The change from
invitedContributortoinvitedContributorIDimproves clarity and aligns with the PR objectives. Well done!For consistency with modern JavaScript/TypeScript practices, consider using object shorthand notation:
- where: { invitedContributorID: contributorID }, + where: { invitedContributorID },This assumes the parameter name matches the property name, which it does in this case.
src/domain/access/role-set/role.set.resolver.mutations.ts (1)
Line range hint
309-341: LGTM! Consider enhancing error handling.The new
joinRoleSetfunction is well-implemented and follows NestJS best practices. It correctly checks for existing invitations and verifies user privileges before allowing a join.Consider creating a custom exception for the case when a user tries to join with a pending invitation. This would improve error management and make it easier to handle this specific case in the client. For example:
export class PendingInvitationException extends Error { constructor(message: string) { super(message); this.name = 'PendingInvitationException'; } } // Then in the joinRoleSet function: if (membershipStatus === CommunityMembershipStatus.INVITATION_PENDING) { throw new PendingInvitationException( `Unable to join RoleSet (${roleSet.id}): invitation to join is pending.` ); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (12)
- src/core/authorization/authorization.service.ts (2 hunks)
- src/domain/access/invitation/dto/invitation.dto.create.ts (3 hunks)
- src/domain/access/invitation/invitation.entity.ts (3 hunks)
- src/domain/access/invitation/invitation.interface.ts (2 hunks)
- src/domain/access/invitation/invitation.service.ts (3 hunks)
- src/domain/access/role-set/role.set.lifecycle.invitation.options.provider.ts (2 hunks)
- src/domain/access/role-set/role.set.resolver.mutations.ts (2 hunks)
- src/domain/access/role-set/role.set.service.ts (3 hunks)
- src/domain/community/virtual-contributor/virtual.contributor.service.ts (1 hunks)
- src/migrations/1727930288139-invitationToRole.ts (1 hunks)
- src/services/api/registration/registration.service.ts (1 hunks)
- src/services/api/roles/roles.service.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (12)
src/core/authorization/authorization.service.ts (1)
Pattern
src/**/*.{ts,js}: Review the TypeScript/JavaScript code for NestJS best practices, dependency injection, module structure, and potential bugs. Ensure that the code adheres to TypeScript's typing system and modern standards.src/domain/access/invitation/dto/invitation.dto.create.ts (1)
Pattern
src/**/*.{ts,js}: Review the TypeScript/JavaScript code for NestJS best practices, dependency injection, module structure, and potential bugs. Ensure that the code adheres to TypeScript's typing system and modern standards.src/domain/access/invitation/invitation.entity.ts (1)
Pattern
src/**/*.{ts,js}: Review the TypeScript/JavaScript code for NestJS best practices, dependency injection, module structure, and potential bugs. Ensure that the code adheres to TypeScript's typing system and modern standards.src/domain/access/invitation/invitation.interface.ts (1)
Pattern
src/**/*.{ts,js}: Review the TypeScript/JavaScript code for NestJS best practices, dependency injection, module structure, and potential bugs. Ensure that the code adheres to TypeScript's typing system and modern standards.src/domain/access/invitation/invitation.service.ts (1)
Pattern
src/**/*.{ts,js}: Review the TypeScript/JavaScript code for NestJS best practices, dependency injection, module structure, and potential bugs. Ensure that the code adheres to TypeScript's typing system and modern standards.src/domain/access/role-set/role.set.lifecycle.invitation.options.provider.ts (1)
Pattern
src/**/*.{ts,js}: Review the TypeScript/JavaScript code for NestJS best practices, dependency injection, module structure, and potential bugs. Ensure that the code adheres to TypeScript's typing system and modern standards.src/domain/access/role-set/role.set.resolver.mutations.ts (1)
Pattern
src/**/*.{ts,js}: Review the TypeScript/JavaScript code for NestJS best practices, dependency injection, module structure, and potential bugs. Ensure that the code adheres to TypeScript's typing system and modern standards.src/domain/access/role-set/role.set.service.ts (1)
Pattern
src/**/*.{ts,js}: Review the TypeScript/JavaScript code for NestJS best practices, dependency injection, module structure, and potential bugs. Ensure that the code adheres to TypeScript's typing system and modern standards.src/domain/community/virtual-contributor/virtual.contributor.service.ts (1)
Pattern
src/**/*.{ts,js}: Review the TypeScript/JavaScript code for NestJS best practices, dependency injection, module structure, and potential bugs. Ensure that the code adheres to TypeScript's typing system and modern standards.src/migrations/1727930288139-invitationToRole.ts (1)
Pattern
src/**/*.{ts,js}: Review the TypeScript/JavaScript code for NestJS best practices, dependency injection, module structure, and potential bugs. Ensure that the code adheres to TypeScript's typing system and modern standards.src/services/api/registration/registration.service.ts (1)
Pattern
src/**/*.{ts,js}: Review the TypeScript/JavaScript code for NestJS best practices, dependency injection, module structure, and potential bugs. Ensure that the code adheres to TypeScript's typing system and modern standards.src/services/api/roles/roles.service.ts (1)
Pattern
src/**/*.{ts,js}: Review the TypeScript/JavaScript code for NestJS best practices, dependency injection, module structure, and potential bugs. Ensure that the code adheres to TypeScript's typing system and modern standards.
🔇 Additional comments (16)
src/domain/access/invitation/dto/invitation.dto.create.ts (1)
1-6: LGTM: Import changes are appropriate.The new imports for
SMALL_TEXT_LENGTHandCommunityRoleTypeare correctly added and align with the newextraRolefield. These changes follow NestJS best practices for importing constants and enums.src/domain/access/invitation/invitation.interface.ts (1)
6-6: LGTM: New import statement added correctly.The import for
CommunityRoleTypeis properly added and follows the existing import style. This import is necessary for the newextraRoleproperty.src/domain/access/invitation/invitation.entity.ts (1)
8-8: LGTM: New import statement is correctly placed and follows best practices.The import for
CommunityRoleTypeis properly added and consistent with the existing import style in the file.src/domain/access/role-set/role.set.lifecycle.invitation.options.provider.ts (1)
97-97: LGTM: Improved property namingThe renaming of
invitedContributortoinvitedContributorIDenhances code clarity by explicitly indicating that it's an ID. This change aligns well with the PR objectives and improves code readability.src/domain/access/invitation/invitation.service.ts (3)
114-114: LGTM: Improved property namingThe change from
invitedContributortoinvitedContributorIDis consistent with the PR objectives and improves code clarity by accurately reflecting that it's an ID.
140-140: LGTM: Consistent property naming in queryThe change from
invitedContributortoinvitedContributorIDin thewhereclause is consistent with the PR objectives and maintains consistency with the previous change. This ensures that the query will work correctly with the updated property name.
Line range hint
1-224: Overall assessment: Approved with minor suggestionThe changes in this file consistently rename
invitedContributortoinvitedContributorID, which aligns with the PR objectives. This improves code clarity by accurately reflecting that it's an ID. The changes are applied consistently across multiple methods, ensuring that queries and logic continue to work correctly with the updated property name.A minor suggestion was made to improve TypeScript usage in the
findInvitationsForContributormethod, which would further enhance type safety and developer experience.Overall, these changes are well-implemented and improve the codebase. Great job!
src/services/api/roles/roles.service.ts (1)
244-244: LGTM! Verify usage across the codebase.The change from
invitedContributortoinvitedContributorIDimproves clarity and aligns with the PR objectives. This modification accurately reflects that the property represents an identifier for the invited contributor.To ensure consistency across the codebase, please run the following script to check for any remaining occurrences of
invitedContributor:If any occurrences are found, please update them accordingly to maintain consistency with this change.
src/domain/access/role-set/role.set.resolver.mutations.ts (1)
525-525: LGTM! Parameter rename improves clarity.The change from
invitedContributortoinvitedContributorIDin theCreateInvitationInputobject is a good improvement. It clearly indicates that the parameter is an ID, which enhances code readability and aligns with the PR objectives.src/domain/access/role-set/role.set.service.ts (1)
1110-1112: LGTM! Error message updated consistently.The error message has been correctly updated to use
invitedContributorID, maintaining consistency with the earlier parameter name change.src/migrations/1727930288139-invitationToRole.ts (6)
1-2: Import statements are appropriate.The import of
MigrationInterfaceandQueryRunnerfromtypeormis correct and necessary for the migration.
3-4: Migration class is well-defined.The class
InvitationToRole1727930288139correctly implementsMigrationInterface, and thenameproperty is appropriately set.
7-9: Adding 'extraRole' column to 'invitation' table.The SQL statement properly adds a new nullable
varchar(128)columnextraRoleto theinvitationtable, which aligns with the PR objective to recognize and utilize an additional parameter upon invitation acceptance.
21-34: Ensure the 'down' method fully reverses the 'up' method.The
downmethod should accurately revert all changes made in theupmethod. Please check the following:
- The
extraRolecolumn is dropped from theinvitationtable, which correctly undoes its addition.- The foreign key constraint
FK_562dce4a08bb214f08107b3631eis removed fromplatform_invitation, and the index with the same name is recreated. Ensure that the recreation of the index uses the correct syntax and properties.- The column
invitedContributorIDis renamed back toinvitedContributorwith the original data type and constraints.Confirm that the
downmethod restores the database schema to its exact state before theupmethod was applied, ensuring consistency and reliability in schema migrations.
14-15:⚠️ Potential issueChanging column to 'NOT NULL' may introduce issues with existing data.
The
ALTER TABLEstatement renamesinvitedContributortoinvitedContributorIDand sets it aschar(36) NOT NULL. This change introduces aNOT NULLconstraint that wasn't explicitly specified before. If existing records haveNULLvalues ininvitedContributor, this migration will fail or cause data integrity issues.Please verify that the
invitedContributorcolumn does not contain anyNULLvalues before applying this migration. You might need to update existing records to have validinvitedContributorIDvalues or adjust the migration to handleNULLentries.
16-18:⚠️ Potential issueAdding foreign key constraint may affect existing records in 'platform_invitation'.
The addition of the foreign key constraint
FK_562dce4a08bb214f08107b3631eonplatform_invitation.roleSetIdreferencingrole_set(id)withON DELETE CASCADEcould lead to issues if there areroleSetIdvalues inplatform_invitationthat do not exist inrole_set. This would cause the migration to fail due to foreign key violations.To ensure a smooth migration, verify that all
roleSetIdvalues inplatform_invitationcorrespond to existingidvalues inrole_set. Consider cleaning up or updating orphaned records prior to applying this migration.
…ugh the extraRole
techsmyth
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/domain/access/role-set/dto/role.set.dto.entry.role.invite.ts (2)
28-35: LGTM: NewextraRolefield is well-implemented.The
extraRolefield is correctly added with appropriate decorators and constraints. The use ofCommunityRoleTypeenum ensures type safety, and the field is properly marked as optional. The description is clear and informative for GraphQL schema documentation.Consider adding a comment explaining why
SMALL_TEXT_LENGTHis used as the maximum length for an enum value. This might help future developers understand the reasoning behind this constraint.
Line range hint
1-41: Summary: Changes successfully implement the PR objectives.The modifications to
InviteForEntryRoleOnRoleSetInputclass effectively implement the PR objectives by adding theextraRolefield. This new field allows for assigning an additional role to invited contributors, enhancing the flexibility of the invitation process. The implementation follows NestJS and GraphQL best practices, maintaining consistency with the existing codebase.As the invitation process becomes more complex with additional roles, consider reviewing the overall role management system to ensure it remains scalable and maintainable. This might involve creating a separate service for role assignment and management if it doesn't already exist.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- src/domain/access/role-set/dto/role.set.dto.entry.role.invite.ts (2 hunks)
- src/domain/access/role-set/role.set.resolver.mutations.ts (3 hunks)
- src/migrations/1727930288139-invitationToRole.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/domain/access/role-set/role.set.resolver.mutations.ts
🧰 Additional context used
📓 Path-based instructions (2)
src/domain/access/role-set/dto/role.set.dto.entry.role.invite.ts (1)
Pattern
src/**/*.{ts,js}: Review the TypeScript/JavaScript code for NestJS best practices, dependency injection, module structure, and potential bugs. Ensure that the code adheres to TypeScript's typing system and modern standards.src/migrations/1727930288139-invitationToRole.ts (1)
Pattern
src/**/*.{ts,js}: Review the TypeScript/JavaScript code for NestJS best practices, dependency injection, module structure, and potential bugs. Ensure that the code adheres to TypeScript's typing system and modern standards.
🔇 Additional comments (5)
src/migrations/1727930288139-invitationToRole.ts (4)
1-5: LGTM: Class structure follows TypeORM migration standards.The migration class is correctly implemented, following TypeORM best practices. The class name includes a timestamp, which is good for maintaining order in migrations.
6-14: LGTM:upmethod correctly implements required changes.The
upmethod successfully adds the newextraRolecolumn and renamesinvitedContributortoinvitedContributorID. The use ofNULLforextraRoleensures backward compatibility, while maintainingNOT NULLforinvitedContributorIDpreserves data integrity.
16-24: LGTM:downmethod correctly reverts changes.The
downmethod successfully reverses the changes made in theupmethod. It drops theextraRolecolumn and renamesinvitedContributorIDback toinvitedContributor. The order of operations is correct, ensuring a smooth rollback if needed.
1-25: Excellent implementation of the migration file.This migration file is well-structured, follows TypeORM best practices, and correctly implements the changes outlined in the PR objectives. Both
upanddownmethods are properly implemented, ensuring the migration is reversible. The addition of theextraRolecolumn and the renaming ofinvitedContributortoinvitedContributorIDare executed correctly.src/domain/access/role-set/dto/role.set.dto.entry.role.invite.ts (1)
4-9: LGTM: New imports are correctly added and grouped.The new imports for constants and the
CommunityRoleTypeenum are necessary for the newextraRolefield. The grouping of imports is consistent with the existing style in the file.
|
I have tested with the playground and looks good:
I have been able to send invitations to organizations but I haven't been able to accept them |
|
@Comoque1 please take a look to my previous comment and test invitations. I think it works alright, but I haven't been able to test invites for organizations, I don't see them in the UI and I think that's a gap |
Additional parameter added to the create invitation DTO, that is then honored when the invitation is accepted.
Renamed invitedContributor on invitation to be invitedContributorID as it is an ID
Note: this update does not require client changes.
Summary by CodeRabbit
Release Notes
New Features
extraRolefield for invitations to assign additional roles.Improvements
invitedContributorIDfor consistency.Summary by CodeRabbit
Release Notes
New Features
joinRoleSetallowing users to join a RoleSet without approval.extraRolefield for invitations to assign additional roles.Improvements
invitedContributorIDfor consistency.Bug Fixes