Feat/schema dates #94
Conversation
godchiSymbionix
requested review from
yassinedorbozgithub and
jacob-khoza-symb
WalkthroughThe pull request implements user authorization checks in the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
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 (
|
There was a problem hiding this comment.
Hello @godchiSymbionix,
You're using new Date('2024-01-01T00:00:00Z'), in several places, maybe it would be better to declare a variable for this one and then use it whenever needed.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 22
🧹 Outside diff range and nitpick comments (52)
src/entities/base-entity.ts (1)
3-4: Use consistent naming conventionThe current naming convention for the new fields (
created_at,updated_at) uses snake_case, which is inconsistent with TypeScript's typical camelCase convention. Consider updating the field names to maintain consistency with TypeScript best practices.You could update the field names like this:
export class BaseEntity { id?: string; - created_at?: Date; - updated_at?: Date; + createdAt?: Date; + updatedAt?: Date; }src/mappers/access-ticket-mapper.ts (1)
5-13: LGTM! Consider adding a type foraccessTicketEntity.The changes to
mapEntityToModellook good. The addition ofcreated_atandupdated_atfields aligns with the PR objective of adding timestamp fields to all tables. The explicit return type is a good practice for type safety.Consider adding an explicit type annotation for the
accessTicketEntityparameter to improve code readability and maintainability:export const mapEntityToModel = ( accessTicketEntity: AccessTicketEntity ): AccessTicketModel | undefined => { // ... rest of the function }src/domain/models/user.ts (1)
Line range hint
1-58: Consider adding getter and setter methods for new timestamp fieldsTo maintain consistency with the existing pattern in the class, consider adding getter and setter methods for the
createdAtandupdatedAtfields.Here's a suggested implementation:
getCreatedAt(): Date | undefined { return this.createdAt; } setCreatedAt(createdAt: Date): void { this.createdAt = createdAt; } getUpdatedAt(): Date | undefined { return this.updatedAt; } setUpdatedAt(updatedAt: Date): void { this.updatedAt = updatedAt; }Add these methods to the class to provide a consistent interface for accessing and modifying the new timestamp fields.
src/domain/models/shlink-access.ts (3)
11-12: Good addition of timestamp fields, consider addingdeletedAtThe addition of
createdAtandupdatedAtfields aligns well with the PR objectives. These optional Date parameters will allow for better tracking of record lifecycles.Consider also adding a
deletedAtparameter to fully implement the timestamp fields mentioned in the PR objectives:constructor( private shlinkId: string, private accessTime: Date, private recipient: string, private id?: string, createdAt?: Date, updatedAt?: Date, + deletedAt?: Date ) {
21-22: Correct usage of superclass constructor, consider addingdeletedAtThe
createdAtandupdatedAtparameters are correctly passed to the superclass constructor.If you add the
deletedAtparameter as suggested earlier, make sure to pass it to the superclass constructor as well:super( z.object({ shlinkId: z.string().min(1), id: z.string().optional(), access_time: z.date().default(new Date()), recipient: z.string().min(1) }), createdAt, updatedAt, + deletedAt );
Line range hint
1-58: ImplementdeletedAtfield to fully meet PR objectivesThe PR objectives mention adding three timestamp fields: createdAt, updatedAt, and deletedAt. While createdAt and updatedAt have been implemented, the deletedAt field is missing from the current implementation.
To fully meet the PR objectives and provide complete lifecycle tracking for records, please add the deletedAt field to the class. This involves:
- Adding it as an optional parameter to the constructor
- Passing it to the superclass constructor
- Including it in the validation schema
- Adding getter and setter methods for it
Here's a template for the getter and setter methods you can add:
getDeletedAt(): Date | undefined { return this.deletedAt; } setDeletedAt(deletedAt: Date): void { this.deletedAt = deletedAt; }src/mappers/user-mapper.ts (1)
Line range hint
1-50: Ensure consistency across all mapper functionsAfter reviewing all the mapper functions, there's a general lack of consistency in how the new timestamp fields (
created_atandupdated_at) are handled. To maintain data integrity and meet the PR objectives, ensure that these fields are consistently mapped across all functions:mapEntityToModel,mapModelToEntity,mapModelToDto, andmapDtoToModel.Also, consider the following points:
- Update the UserModel, UserEntity, and UserDto interfaces/classes to include these new fields.
- Decide whether the
deletedAtfield mentioned in the PR objectives should be included in these mappers.- Ensure that all places where these mapper functions are used are updated to handle the new fields correctly.
src/mappers/shlink-access-mapper.ts (4)
Line range hint
7-15: Consider adding timestamp fields to the entity mapping.The PR objectives mention adding
createdAt,updatedAt, anddeletedAtfields to all tables. However, these fields are not included in themapModelToEntityfunction. Consider updating the function to include these fields in the entity mapping.Here's a suggested implementation:
export const mapModelToEntity = ( shlinkAccessModel?: SHLinkAccessModel, ): SHLinkAccessEntity | undefined => { return shlinkAccessModel ? { access_time: shlinkAccessModel.getAccessTime(), recipient: shlinkAccessModel.getRecipient(), shlink_id: shlinkAccessModel.getSHLinkId(), id: shlinkAccessModel.getId(), + created_at: shlinkAccessModel.getCreatedAt(), + updated_at: shlinkAccessModel.getUpdatedAt(), + deleted_at: shlinkAccessModel.getDeletedAt(), } : undefined; };Note: Ensure that the corresponding getter methods are added to the
SHLinkAccessModelclass if they don't already exist.
Line range hint
20-29: Consider adding thedeleted_atfield and updating the model constructor.While
created_atandupdated_athave been added, thedeleted_atfield mentioned in the PR objectives is still missing. Additionally, ensure that theSHLinkAccessModelconstructor is updated to accept these new parameters.Here's a suggested implementation:
export const mapEntityToModel = ( shlinkAccessEntity?: SHLinkAccessEntity, ): SHLinkAccessModel | undefined => { return shlinkAccessEntity ? new SHLinkAccessModel( shlinkAccessEntity.shlink_id, shlinkAccessEntity.access_time, shlinkAccessEntity.recipient, shlinkAccessEntity.id, shlinkAccessEntity.created_at, shlinkAccessEntity.updated_at, + shlinkAccessEntity.deleted_at, ) : undefined; };Also, ensure that the
SHLinkAccessModelconstructor in the model file is updated to accept these new parameters:constructor( shlinkId: string, accessTime: Date, recipient: string, id: string, createdAt: Date, updatedAt: Date, deletedAt: Date | null ) { // ... constructor implementation }
Line range hint
34-43: Consider adding timestamp fields to the DTO mapping.The PR objectives mention adding
createdAt,updatedAt, anddeletedAtfields to all tables. However, these fields are not included in themapModelToDtofunction. Consider updating the function to include these fields in the DTO mapping.Here's a suggested implementation:
export const mapModelToDto = ( shlinkAccessModel?: SHLinkAccessModel, ): SHLinkAccessDto | undefined => { return shlinkAccessModel ? { id: shlinkAccessModel.getId(), shlinkId: shlinkAccessModel.getSHLinkId(), accessTime: shlinkAccessModel.getAccessTime(), recipient: shlinkAccessModel.getRecipient(), + createdAt: shlinkAccessModel.getCreatedAt(), + updatedAt: shlinkAccessModel.getUpdatedAt(), + deletedAt: shlinkAccessModel.getDeletedAt(), } : undefined; };Note: Ensure that the corresponding getter methods are added to the
SHLinkAccessModelclass and that theSHLinkAccessDtointerface is updated to include these new fields.
Line range hint
1-44: Summary: Ensure consistent implementation of timestamp fields across all mappingsWhile the changes in this file are a step in the right direction, there are a few inconsistencies and potential improvements to fully align with the PR objectives:
- The
mapModelToEntityfunction needs to be updated to include all three timestamp fields (createdAt, updatedAt, deletedAt).- The
mapEntityToModelfunction should include thedeleted_atfield.- The
mapModelToDtofunction needs to be updated to include all three timestamp fields.Additionally, please ensure that the following related changes are made:
- Update the
SHLinkAccessModelclass to include getter and setter methods for the new timestamp fields.- Update the
SHLinkAccessDtointerface to include the new timestamp fields.- Verify that the
SHLinkAccessEntitytype definition includes the new timestamp fields.These changes will ensure consistency across the entire mapping process and fully implement the PR objectives.
src/infrastructure/repositories/prisma/base-repository.ts (1)
Issues Found with
createdAtanddeletedAtHandlingThe repository methods do not handle the
createdAtanddeletedAtfields as required by the PR objectives:
insertandinsertManyMethods: Do not explicitly set thecreatedAtfield.deleteMethod: Does not implement soft deletes using thedeletedAtfield.🔗 Analysis chain
Line range hint
1-63: Verify handling ofcreatedAtanddeletedAtfieldsThe PR objectives mention adding
createdAt,updatedAt, anddeletedAtfields to all tables. While we've addressed theupdatedAt(orupdated_at) field in theupdatemethod, it's important to ensure that thecreatedAtanddeletedAtfields are also properly handled in the repository methods.Please verify the following:
Ensure that the
insertandinsertManymethods correctly handle thecreatedAtfield. If Prisma doesn't set this automatically, you may need to explicitly set it.Consider implementing soft deletes using the
deletedAtfield in thedeletemethod, if that's part of the intended functionality.Run the following script to check the current implementation:
Based on the results, you may need to update these methods to ensure proper handling of the new timestamp fields.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the handling of createdAt and deletedAt fields in the repository methods # Test: Search for createdAt handling in insert methods echo "Checking createdAt handling in insert methods:" rg --type typescript -A 5 'async insert' src/infrastructure/repositories rg --type typescript -A 5 'async insertMany' src/infrastructure/repositories # Test: Search for deletedAt handling in delete method echo "Checking deletedAt handling in delete method:" rg --type typescript -A 5 'async delete' src/infrastructure/repositoriesLength of output: 537
Script:
#!/bin/bash # Description: Check the handling of createdAt and deletedAt fields in the repository methods # Test: Search for createdAt handling in insert methods echo "Checking createdAt handling in insert methods:" rg --glob "*.ts" -A 5 'async insert' src/infrastructure/repositories rg --glob "*.ts" -A 5 'async insertMany' src/infrastructure/repositories # Test: Search for deletedAt handling in delete method echo "Checking deletedAt handling in delete method:" rg --glob "*.ts" -A 5 'async delete' src/infrastructure/repositoriesLength of output: 2309
src/domain/dtos/shlink-endpoint.ts (2)
50-50: Approved: Improved example forurlPathThe updated example for
urlPathprovides a clearer representation of the expected value. This change enhances the documentation and helps developers understand the purpose of this property better.Consider adding a brief explanation of what
/summaryrepresents in the context of a share link endpoint. For example:* type: string * description: An optional string representing the share link endpoint's query parameters. -* example: /summary +* description: An optional string representing the share link endpoint's path or view. +* example: /summary # Represents a summary view of the share link
Missing Timestamp Fields in DTO Classes
The PR objectives mention adding timestamp fields (
createdAt,updatedAt,deletedAt) to all DTOs. However, the following DTO classes insrc/domain/dtos/shlink-endpoint.tsare missing these fields:
CreateSHLinkEndpointDtoSHLinkEndpointDtoPlease ensure that all DTO classes include the necessary timestamp fields as per the PR objectives.
🔗 Analysis chain
Line range hint
1-54: Verify: Discrepancy between PR objectives and implemented changesThe PR objectives mention adding timestamp fields (createdAt, updatedAt, deletedAt) to all tables. However, these changes are not reflected in the current file. Could you please clarify:
- Are these timestamp fields implemented in other files not provided for this review?
- If not, are these changes still planned to be implemented as part of this PR?
- If the objectives have changed, could you update the PR description to reflect the current goals?
To help verify the implementation of timestamp fields across the project, you can run the following script:
This will help us understand if the timestamp fields have been implemented in other parts of the project.
</details> <details> <summary>🏁 Scripts executed</summary> The following scripts were executed for the analysis: Script: ```shell #!/bin/bash # Description: Search for timestamp fields in DTO and entity files # Search for createdAt, updatedAt, and deletedAt in TS files echo "Searching for timestamp fields in DTO and entity files:" rg --type typescript -e 'createdAt|updatedAt|deletedAt' src/Length of output: 218
Script:
#!/bin/bash # Description: Search for timestamp fields in DTO and entity files # Corrected search for createdAt, updatedAt, and deletedAt in TypeScript files echo "Searching for timestamp fields in DTO and entity files:" rg --type ts -e 'createdAt|updatedAt|deletedAt' src/Length of output: 5106
src/mappers/shlink-endpoint-mapper.ts (1)
Line range hint
1-57: Ensure consistent implementation of timestamp fields across all mapping functions.The changes in this file partially implement the PR objectives by adding
created_atandupdated_atfields to themapEntityToModelfunction. However, there are inconsistencies in the implementation across different mapping functions:
mapEntityToModel: Includescreated_atandupdated_at, but missingdeletedAt.mapModelToEntity: Doesn't include any of the new timestamp fields.mapModelToDto: Doesn't include any of the new timestamp fields.mapDtoToModel: Doesn't include any of the new timestamp fields.To fully align with the PR objectives and maintain consistency:
- Implement
createdAt,updatedAt, anddeletedAtfields in all mapping functions.- Ensure that the corresponding model, entity, and DTO interfaces/classes are updated to include these new fields.
- Verify that the naming convention for these fields is consistent across all usages (e.g.,
created_atvscreatedAt).Consider creating a base interface or type for the timestamp fields to ensure consistency across all entities and reduce code duplication. For example:
interface TimeStampedFields { createdAt: Date; updatedAt: Date; deletedAt?: Date; } interface SHLinkEndpointBase extends TimeStampedFields { id: string; shlinkId: string; serverConfigId: string; urlPath: string; } type SHLinkEndpointEntity = SHLinkEndpointBase; type SHLinkEndpointDto = SHLinkEndpointBase;This approach would help maintain consistency and make it easier to add these fields to new entities in the future.
src/mappers/access-ticket-mapper.test.ts (3)
6-7: Consider using a more appropriate date for testing.The
dateValueconstant is set to a future date (2024). For testing purposes, it's generally better to use a known past date or the current date to avoid potential issues with future dates.Consider modifying the date to a past or current date:
-const dateValue = new Date('2024-01-01T00:00:00Z') +const dateValue = new Date('2023-01-01T00:00:00Z') // or use new Date() for the current date
14-15: LGTM! Minor formatting suggestion.The addition of
created_atandupdated_atproperties aligns well with the PR objectives. Good job on using thedateValueconstant for consistency.Consider adding spaces after the colons for better readability:
- created_at:dateValue, - updated_at:dateValue + created_at: dateValue, + updated_at: dateValue
Line range hint
37-49: UpdatemapModelToEntitytest to include new timestamp fields.While the
AccessTicketEntityhas been updated withcreated_atandupdated_atfields, the test case formapModelToEntitydoesn't reflect these changes. To ensure complete test coverage, consider updating this test case as well.Here's a suggested update to the test case:
it('should map a valid AccessTicketModel to AccessTicketEntity', () => { const model = new AccessTicketModel('abc', '123'); model.setCreatedAt(dateValue); model.setUpdatedAt(dateValue); const entity = mapModelToEntity(model); expect(entity).toEqual({ id: '123', shlink_id: 'abc', created_at: dateValue, updated_at: dateValue }); });Note: This assumes that
AccessTicketModelhas been updated with methods to set and get the new timestamp fields. If not, you may need to update theAccessTicketModelclass as well.src/usecases/shlink-qrcode/get-shlink-qrcode.test.ts (2)
22-25: LGTM! Consider enhancing test coverage for new fields.The addition of
createdAt,updatedAt, anddeletedAtfields to themockSHLinkobject aligns well with the PR objectives. The use of a specific date for testing and settingdeletedAttonullare appropriate choices.To ensure comprehensive test coverage, consider adding assertions to verify these new fields in your test cases. For example:
it('should include the new timestamp fields in the encoded SHLink', async () => { // ... existing test setup ... expect(encodeSHLink).toHaveBeenCalledWith(expect.objectContaining({ createdAt: expect.any(Date), updatedAt: expect.any(Date), deletedAt: null })); // ... rest of the test ... });This addition would validate that the new fields are correctly passed to the
encodeSHLinkfunction, ensuring they are included in the QR code generation process.
Line range hint
31-67: Enhance test coverage for new timestamp fieldsWhile the changes to
mockSHLinkare correct, the current test cases don't explicitly verify how these new fields affect the QR code generation process. To ensure comprehensive coverage, consider the following enhancements:
- Modify the existing tests to assert that the new fields are included in the encoded data.
- Add a new test case that specifically checks the handling of different
deletedAtvalues (null vs. date).Here's an example of how you could enhance the first test case:
it('should return an image buffer when QR code generation is successful', async () => { // ... existing test setup ... const result = await getSHLinkQRCodeUseCase(mockSHLink); expect(encodeSHLink).toHaveBeenCalledWith(expect.objectContaining({ createdAt: expect.any(Date), updatedAt: expect.any(Date), deletedAt: null })); // ... rest of the test ... });And add a new test case:
it('should handle different deletedAt values correctly', async () => { const deletedSHLink = { ...mockSHLink, deletedAt: new Date('2024-02-01T00:00:00Z') }; await getSHLinkQRCodeUseCase(deletedSHLink); expect(encodeSHLink).toHaveBeenCalledWith(expect.objectContaining({ deletedAt: expect.any(Date) })); // You might want to add more assertions here to verify the QR code content });These changes will help ensure that the new timestamp fields are correctly handled throughout the QR code generation process.
src/domain/models/shlink.ts (1)
15-16: LGTM! Consider adding default values for timestamps.The addition of
createdAtandupdatedAtparameters aligns well with the PR objectives. The optional nature andDatetype are appropriate for these fields.Consider setting default values for these fields in the constructor to ensure they are always populated:
createdAt: Date = new Date(), updatedAt: Date = new Date()This would automatically set the current timestamp when a new instance is created, which is a common practice for such fields.
src/domain/models/server-config.ts (2)
36-39: Add type validation for new timestamp fields.The inclusion of
createdAtandupdatedAtin the schema is correct. However, consider adding type validation for these new fields to ensure data integrity.Consider updating the schema to include type validation:
refreshToken: z.string().optional().nullable(), +createdAt: z.date().optional().nullable(), +updatedAt: z.date().optional().nullable() }), createdAt, updatedAtThis change will ensure that the
createdAtandupdatedAtfields are properly validated as optionalDateobjects.
Line range hint
1-114: Consider additional improvements to fully meet PR objectives.While the addition of
createdAtandupdatedAtfields is a good start, there are a few points to consider for improvement:
- Add getter and setter methods for
createdAtandupdatedAtto maintain consistency with other fields in the class.- Implement the
deletedAtfield, which was mentioned in the PR objectives but is currently missing from this implementation.Here's a suggestion for adding getter and setter methods:
getCreatedAt(): Date | undefined { return this.createdAt; } setCreatedAt(createdAt: Date): void { this.createdAt = createdAt; } getUpdatedAt(): Date | undefined { return this.updatedAt; } setUpdatedAt(updatedAt: Date): void { this.updatedAt = updatedAt; }Don't forget to implement the
deletedAtfield similarly tocreatedAtandupdatedAt, including its addition to the constructor, schema, and creating getter/setter methods.src/mappers/shlink-endpoint-mapper.test.ts (3)
20-21: Add deletedAt field to mockEntityThe addition of
created_atandupdated_atfields to the mockEntity object is good and aligns with the PR objectives. However, the PR summary mentions three timestamp fields, includingdeletedAt. Consider adding this field to the mockEntity for completeness:const mockEntity: SHLinkEndpointEntity = { // ... existing fields ... created_at: dateValue, updated_at: dateValue, deleted_at: null // or dateValue, depending on your use case };This ensures that all three timestamp fields mentioned in the PR objectives are represented in the test data.
29-30: Add deletedAt parameter to mockModel constructorThe addition of
created_atandupdated_atparameters to the SHLinkEndpointModel constructor is good and aligns with the PR objectives. However, to fully implement the changes described in the PR summary, consider adding thedeletedAtparameter:const mockModel = new SHLinkEndpointModel( 'shlink-456', 'config-789', '/api/path', 'endpoint-123', dateValue, dateValue, null // deletedAt parameter, or dateValue depending on your use case );This ensures that all three timestamp fields mentioned in the PR objectives are represented in the test data.
Line range hint
1-93: Overall review: Inconsistent implementation of timestamp fieldsWhile the changes in this file partially implement the PR objectives by adding
created_atandupdated_atfields, there are several inconsistencies and omissions:
- The
deletedAtfield, mentioned in the PR objectives, is not implemented anywhere in the file.- The new timestamp fields are added inconsistently across mockEntity, mockModel, and mockDto.
- The mapModelToEntity test doesn't include the new timestamp fields in its expected result.
Recommendations:
- Add the
deletedAtfield to all relevant parts of the file (mockEntity, mockModel, mockDto, and test expectations).- Ensure consistent implementation of all three timestamp fields (createdAt, updatedAt, deletedAt) across all representations of SHLinkEndpoint in the test file.
- Update all relevant test expectations to include and validate the new timestamp fields.
These changes will ensure that the tests accurately reflect and validate the new schema changes described in the PR objectives.
src/mappers/server-config-mapper.ts (1)
Line range hint
1-79: Overall assessment: Changes align with PR objectives, but some improvements are recommended.The additions of
created_atandupdated_atfields in themapEntityToModelandmapModelToEntityfunctions align well with the PR objectives. However, consider the following recommendations:
- Verify that the database schema and ServerConfigModel have been updated to include these new timestamp fields.
- Consider including the timestamp fields in the DTO mapping (
mapModelToDto) and model creation (mapDtoToModel) if they are relevant for API responses and application logic.- There are inconsistencies between the AI-generated summary and the actual code regarding function signatures. Please review and update either the code or the PR description accordingly.
These changes will ensure a more comprehensive implementation of the timestamp fields across the entire mapping process.
src/mappers/user-mapper.test.ts (3)
12-13: LGTM! Consider using a more descriptive variable name.The addition of the
dateValueconstant is good. It provides a consistent date for testing purposes.Consider using a more descriptive name like
testTimestampormockCreationDateto better reflect its purpose in the context of the new timestamp fields being added.
60-60: LGTM! Consider adding tests for timestamp fields.The update to include
server_config_idin themapModelToEntitytest is correct and consistent with the earlier changes.Consider adding assertions for the new timestamp fields that were added to the UserModel constructor. This will ensure that these fields are correctly mapped between the model and entity.
Example:
expect(result).toEqual({ id: userModel.getId(), user_id: userModel.getUserId(), patient_id: userModel.getPatientId(), server_config_id: userModel.getServerConfigId(), created_at: userModel.getCreatedAt(), updated_at: userModel.getUpdatedAt() // Add deletedAt if it's implemented });
Line range hint
1-101: Overall good implementation, but some areas need attention.The addition of
server_config_idis consistently implemented across entity, model, and DTO. However, there are a few points that need attention:
The PR objectives mention adding createdAt, updatedAt, and deletedAt fields, but only two date fields are visible in the changes. Please clarify if this is intentional or if a field is missing.
The tests for
mapModelToEntity,mapModelToDto, andmapDtoToModelshould be updated to include assertions for the new timestamp fields.Consider adding tests to verify the correct initialization of the new fields in the UserModel constructor.
To ensure comprehensive coverage of the new fields:
- Update all relevant mapper tests to include assertions for server_config_id and timestamp fields.
- Add specific tests for the initialization and handling of the new timestamp fields in the UserModel.
- Ensure that the implementation aligns with the PR objectives regarding the three timestamp fields (createdAt, updatedAt, deletedAt).
These changes will improve the robustness of the tests and ensure that the new fields are correctly handled throughout the application.
src/usecases/shlinks/get-single-shlink.test.ts (2)
39-41: LGTM! Consider using a constant for the date value.The addition of
created_at,updated_at, anddeleted_atfields to themockSHLinkEntityis consistent with the PR objectives. The implementation looks correct.For better maintainability, consider defining a constant for the date value:
const MOCK_DATE = new Date('2024-01-01T00:00:00Z'); // Then use it like this: created_at: MOCK_DATE, updated_at: MOCK_DATE,This would make it easier to update the mock date across all fields if needed in the future.
54-56: LGTM! Consider improving consistency and type safety.The addition of timestamp fields to the
SHLinkModelconstructor is consistent with the changes made tomockSHLinkEntity. The implementation looks correct.Consider the following improvements:
- Use the
mockSHLinkEntityproperties directly for consistency:mockSHLinkEntity.created_at, mockSHLinkEntity.updated_at, mockSHLinkEntity.deleted_at
- If you implement the constant suggestion from the previous comment, use it here as well:
MOCK_DATE, MOCK_DATE, null
- To improve type safety, consider using a type assertion for
null:null as Date | nullThis ensures that the
deleted_atfield is correctly typed asDate | null.src/usecases/shlinks/activate-shlink.test.ts (3)
41-43: LGTM! Consider using a constant for the fixed date.The addition of
created_at,updated_at, anddeleted_atfields aligns well with the PR objectives. The initialization looks correct.Consider defining a constant for the fixed date
'2024-01-01T00:00:00Z'at the top of the test file. This would make it easier to update all occurrences if needed and improve readability. For example:const FIXED_DATE = new Date('2024-01-01T00:00:00Z'); // Then use it like this: created_at: FIXED_DATE, updated_at: FIXED_DATE,
60-62: LGTM! Consider using the same constant for the fixed date.The addition of timestamp fields to the
SHLinkModelconstructor is consistent with the changes in the entity. The values used are correct and maintain consistency with the mock entity data.As suggested earlier, consider using a constant for the fixed date. This would make the test data more maintainable and reduce duplication. For example:
const FIXED_DATE = new Date('2024-01-01T00:00:00Z'); // Then use it like this: FIXED_DATE, FIXED_DATE, null
Line range hint
1-105: Consider adding test cases for the new timestamp fields.While the mock data has been updated to include the new timestamp fields (
created_at,updated_at, anddeleted_at), the current test cases don't explicitly verify the behavior related to these fields.To ensure comprehensive test coverage, consider adding the following test cases:
- Verify that the
activeSHLinksUseCasepreserves thecreated_attimestamp when activating a SHLink.- Check that the
updated_attimestamp is modified when the SHLink is activated.- Ensure that the
deleted_atfield remains null after activation.Here's an example of how you might implement these test cases:
it('should preserve created_at and update updated_at when activating a SHLink', async () => { const result = await activeSHLinksUseCase(mockContext, { id: mockId }); expect(result.created_at).toEqual(mockSHLinkEntity.created_at); expect(result.updated_at).not.toEqual(mockSHLinkEntity.updated_at); expect(result.updated_at.getTime()).toBeGreaterThan(mockSHLinkEntity.updated_at.getTime()); }); it('should keep deleted_at as null after activation', async () => { const result = await activeSHLinksUseCase(mockContext, { id: mockId }); expect(result.deleted_at).toBeNull(); });These additional tests will help ensure that the
activeSHLinksUseCasecorrectly handles the new timestamp fields.src/usecases/shlinks/add-shlink.test.ts (1)
64-66: LGTM: Mock entity updated correctly with new timestamp fieldsThe
mockEntityobject has been correctly updated with the new timestamp fields, maintaining consistency with theSHLinkModel. This change aligns well with the PR objectives.For improved consistency and easier maintenance, consider using the same date constant for both the
SHLinkModelconstructor and themockEntityobject. For example:const testDate = new Date('2024-01-01T00:00:00Z'); // Use testDate in both SHLinkModel constructor and mockEntityThis approach would make it easier to update the test date in the future if needed.
src/usecases/shlinks/deactivate-shlink.test.ts (3)
45-46: LGTM! Consider using a more realistic date for testing.The addition of
created_atandupdated_atproperties aligns well with the PR objectives. However, using a future date ('2024-01-01T00:00:00Z') for testing might not be ideal.Consider using a past or present date for more realistic test scenarios. For example:
- created_at: new Date('2024-01-01T00:00:00Z'), - updated_at: new Date('2024-01-01T00:00:00Z') + created_at: new Date('2023-01-01T00:00:00Z'), + updated_at: new Date('2023-01-01T00:00:00Z')
63-64: LGTM! Ensure consistency with the previous date change.The addition of
created_atandupdated_atto theSHLinkModelconstructor is consistent with the changes in the mock entity.For consistency, update the date here as well if you change it in the mock entity:
- new Date('2024-01-01T00:00:00Z'), - new Date('2024-01-01T00:00:00Z') + new Date('2023-01-01T00:00:00Z'), + new Date('2023-01-01T00:00:00Z')
Line range hint
1-114: Consider adding test cases for the new timestamp fields.While the changes to include
created_atandupdated_atfields are good, the test suite doesn't explicitly verify any behavior related to these new fields.Consider adding new test cases that:
- Verify that the
created_atfield remains unchanged after deactivation.- Check that the
updated_atfield is modified when the SHLink is deactivated.For example:
it('should not change the created_at field when deactivating', async () => { const result = await deactivateSHLinksUseCase(mockContext, { id: mockId, user: { id: 'user-123567', name: '', email: '' }, }); expect(result.created_at).toEqual(mockSHLinkEntity.created_at); }); it('should update the updated_at field when deactivating', async () => { const result = await deactivateSHLinksUseCase(mockContext, { id: mockId, user: { id: 'user-123567', name: '', email: '' }, }); expect(result.updated_at).not.toEqual(mockSHLinkEntity.updated_at); expect(result.updated_at.getTime()).toBeGreaterThan(mockSHLinkEntity.updated_at.getTime()); });src/mappers/shlink-mapper.ts (1)
Line range hint
1-124: Summary: Consistent implementation ofcreatedAtandupdatedAt, butdeletedAtis missingThe changes in this file consistently implement the addition of
createdAtandupdatedAtfields across all mapping functions, which aligns with the PR objectives. The naming conventions are correctly maintained (snake_case for entities, camelCase for DTOs).However, there's a recurring issue: the
deletedAtfield mentioned in the PR objectives is consistently missing across all functions.Next steps:
- Add the
deletedAtfield to all mapping functions in this file.- Ensure that the
SHLinkModel,SHLinkEntity,SHLinkDto, andSHLinkMiniDtointerfaces/classes are updated to include all three timestamp fields.- Verify that these changes are consistently applied across the entire codebase, including other mapping functions and related interfaces/classes.
Consider creating a base interface or type for the timestamp fields (e.g.,
TimestampFields) and extending it in your entity, model, and DTO interfaces. This would ensure consistency and make it easier to manage these fields across your codebase.Example:
interface TimestampFields { createdAt: Date; updatedAt: Date; deletedAt?: Date; } interface SHLinkDto extends TimestampFields { // other SHLink-specific fields }This approach would make it easier to maintain consistency and add or modify timestamp-related fields in the future.
src/usecases/shlinks/get-shlink.test.ts (2)
38-40: LGTM! Consider using a dynamic date for more realistic test data.The addition of
created_at,updated_at, anddeleted_atfields aligns well with the PR objectives. The implementation is correct, withdeleted_atappropriately set to null for an active record.For more realistic test data, consider using
new Date()forcreated_atandupdated_atinstead of a hardcoded date. This would make the tests more robust against future changes. For example:created_at: new Date(), updated_at: new Date(), deleted_at: null
67-69: LGTM! Consider using dynamic dates for SHLinkModel instances as well.The addition of timestamp fields to the SHLinkModel instances is correct and consistent with the changes made to the mock entities.
As suggested for the mock entities, consider using
new Date()forcreated_atandupdated_atin these SHLinkModel instances as well. This would make the tests more realistic and robust. For example:new Date(), // created_at new Date(), // updated_at null // deleted_atAlso applies to: 80-82
src/usecases/shlinks/validate-shlink.test.ts (2)
19-21: LGTM! Consider using constants for clarity.The addition of createdAt, updatedAt, and deletedAt timestamps is consistent with the PR objectives. Good job on implementing these changes.
For improved readability and maintainability, consider defining constants for these common timestamp values:
const MOCK_CREATED_AT = new Date('2024-01-01T00:00:00Z'); const MOCK_UPDATED_AT = new Date('2024-01-01T00:00:00Z'); const MOCK_DELETED_AT = null;Then use these constants throughout the test file.
142-144: LGTM! Consistent implementation throughout the file.The changes here maintain consistency with all previous instances. Great job on implementing these timestamp fields consistently across all test cases in the file.
Consider adding a test case that specifically checks the behavior of the
validateSHLinkUseCasewith respect to these new timestamp fields. For example, you could test whether a SHLink with a non-nulldeletedAtvalue is considered invalid.src/usecases/shlinks/update-single-shlink.test.ts (1)
42-44: LGTM: New timestamp parameters added correctly to SHLinkModel constructorThe addition of timestamp parameters to the SHLinkModel constructor call is correct and consistent with the changes in mockDto.
Consider adding comments to clarify the purpose of these new parameters, especially the null value. For example:
new Date('2024-01-01T00:00:00Z'), // created_at new Date('2024-01-01T00:00:00Z'), // updated_at null // deleted_atThis would improve readability and make the purpose of each parameter more explicit.
prisma/schema.prisma (1)
Line range hint
1-129: Overall review: Timestamp fields added consistently, but one field is missingThe changes to add
created_atandupdated_atfields to all models in the Prisma schema are consistent and well-implemented. This will improve tracking of record lifecycles as intended.However, there's a discrepancy between the PR objectives and the actual changes:
- The PR summary mentions adding three timestamp fields (createdAt, updatedAt, and deletedAt), but only two (created_at and updated_at) have been implemented.
- The
deletedAtfield is missing from all models.Please address the following:
- Add the
deletedAtfield to all models, or explain why it was omitted.- If
deletedAtis not needed, update the PR description to reflect the actual changes.Additionally, some models have existing time-related fields (
access_timeinshlink_accessandrefresh_timeinserver_config) that might overlap with the new timestamp fields. Consider reviewing these fields to ensure they serve distinct purposes and are still necessary.src/mappers/server-config-mapper.test.ts (1)
25-26: LGTM: Good addition of timestamp fields. Minor formatting suggestion.The addition of
created_atandupdated_atfields aligns with the PR objective.Consider adding a space after the colon for consistency with other fields:
- created_at:dateValue, - updated_at:dateValue + created_at: dateValue, + updated_at: dateValuesrc/domain/dtos/shlink.ts (3)
93-108: LGTM: Addition ofcreatedAtandupdatedAtpropertiesThe addition of
createdAtandupdatedAtproperties to theSHLinkDtoclass is a good implementation of the PR objectives. These fields will improve tracking of the lifecycle of records in the database.The properties are well-documented with appropriate descriptions and examples, using a consistent date format (ISO 8601) across the file.
There's a minor typo in the
updatedAttype definition. Please apply this small fix:- type:datetime + type: datetimeThis will ensure consistency with the
createdAttype definition.
161-162: Addition ofcreatedAtandupdatedAtproperties needs documentationThe addition of
createdAtandupdatedAtproperties to theSHLinkMiniDtoclass is good and aligns with the PR objectives. However, these new properties are missing Swagger documentation, which is inconsistent with the documentation style inSHLinkDto.Please add Swagger documentation for these new properties to maintain consistency. You can use the following as a template:
* createdAt: * type: datetime * description: A date representing when the share link was created. * example: 2024-09-26T10:03:39.379Z * updatedAt: * type: datetime * description: A date representing when the share link was last updated. * example: 2024-09-26T10:03:39.379Z
Add
deletedAtField to DTOs for Complete Timestamp TrackingThe
deletedAtfield was not found in theSHLinkDtoandSHLinkMiniDtoclasses. While it appears in the test files, its absence in the main DTOs suggests it was unintentionally omitted. To ensure complete timestamp tracking and consistency across the codebase, please add thedeletedAtfield to both DTO classes. Additionally, include appropriate Swagger documentation for this field.
- Location to Update:
src/domain/dtos/shlink.ts🔗 Analysis chain
Line range hint
1-220: Consider addingdeletedAtfield for complete timestamp trackingThe changes implement
createdAtandupdatedAtfields as per the PR objectives, which is great for tracking the creation and last update times of share links. However, I noticed that thedeletedAtfield mentioned in the PR objectives is not present in the changes.If the omission of
deletedAtwas unintentional, consider adding it to bothSHLinkDtoandSHLinkMiniDtoclasses for complete timestamp tracking. This would align fully with the PR objectives of adding createdAt, updatedAt, and deletedAt fields to all tables.If you decide to add the
deletedAtfield, please ensure it follows the same pattern ascreatedAtandupdatedAt, including appropriate Swagger documentation.To verify if
deletedAtis implemented elsewhere or if it's intentionally omitted, we can run the following script:This will help us understand if
deletedAtis implemented elsewhere in the codebase or if it's consistently omitted.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for 'deletedAt' in all TypeScript files rg --type typescript 'deletedAt' # Search for soft delete implementation rg --type typescript -i 'soft.*delete|delete.*at'Length of output: 154
Script:
#!/bin/bash # Search for 'deletedAt' in all TypeScript files rg --type ts 'deletedAt' # Search for soft delete implementation rg --type ts -i 'soft.*delete|delete.*at'Length of output: 848
src/domain/models/base-model.ts (2)
23-26: Reconsider passingcreatedAtandupdatedAtas constructor parametersTypically,
createdAtis set when a new instance is created, andupdatedAtis updated whenever the instance is modified. Passing these as optional parameters might lead to inconsistencies or accidental overwriting of these timestamps.Consider initializing
createdAtwithin the constructor and managingupdatedAtthrough setters or within methods that modify the instance. Here's a suggested change:constructor(private _schema: ZodObject<ZodRawShape>) { - this.createdAt = createdAt; - this.updatedAt = updatedAt; + this.createdAt = new Date(); + this.updatedAt = new Date(); }This approach sets the timestamps automatically, reducing the risk of errors from external assignment.
46-61: Evaluate the necessity of explicit getters and setters for public propertiesSince
createdAtandupdatedAtare public properties, adding explicit getter and setter methods may be unnecessary unless additional logic is involved. This can make the code more concise and easier to maintain.If no extra processing is needed in the getters and setters, you might remove them and access the properties directly:
- getCreatedAt(): Date | undefined { - return this.createdAt; - } - - setCreatedAt(createdAt: Date): void { - this.createdAt = createdAt; - } - - getUpdatedAt(): Date | undefined { - return this.updatedAt; - } - - setUpdatedAt(updatedAt: Date): void { - this.updatedAt = updatedAt; - }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (33)
- prisma/schema.prisma (3 hunks)
- prisma/schema.test.prisma (4 hunks)
- src/domain/dtos/shlink-endpoint.ts (1 hunks)
- src/domain/dtos/shlink.ts (3 hunks)
- src/domain/models/access-ticket.ts (2 hunks)
- src/domain/models/base-model.ts (3 hunks)
- src/domain/models/server-config.ts (2 hunks)
- src/domain/models/shlink-access.ts (1 hunks)
- src/domain/models/shlink-endpoint.ts (1 hunks)
- src/domain/models/shlink.ts (2 hunks)
- src/domain/models/user.ts (1 hunks)
- src/entities/base-entity.ts (1 hunks)
- src/infrastructure/repositories/prisma/base-repository.ts (1 hunks)
- src/mappers/access-ticket-mapper.test.ts (2 hunks)
- src/mappers/access-ticket-mapper.ts (2 hunks)
- src/mappers/server-config-mapper.test.ts (6 hunks)
- src/mappers/server-config-mapper.ts (4 hunks)
- src/mappers/shlink-access-mapper.test.ts (3 hunks)
- src/mappers/shlink-access-mapper.ts (3 hunks)
- src/mappers/shlink-endpoint-mapper.test.ts (2 hunks)
- src/mappers/shlink-endpoint-mapper.ts (4 hunks)
- src/mappers/shlink-mapper.test.ts (14 hunks)
- src/mappers/shlink-mapper.ts (5 hunks)
- src/mappers/user-mapper.test.ts (3 hunks)
- src/mappers/user-mapper.ts (3 hunks)
- src/usecases/shlink-qrcode/get-shlink-qrcode.test.ts (1 hunks)
- src/usecases/shlinks/activate-shlink.test.ts (2 hunks)
- src/usecases/shlinks/add-shlink.test.ts (2 hunks)
- src/usecases/shlinks/deactivate-shlink.test.ts (2 hunks)
- src/usecases/shlinks/get-shlink.test.ts (4 hunks)
- src/usecases/shlinks/get-single-shlink.test.ts (2 hunks)
- src/usecases/shlinks/update-single-shlink.test.ts (3 hunks)
- src/usecases/shlinks/validate-shlink.test.ts (7 hunks)
🔇 Additional comments (71)
src/domain/models/access-ticket.ts (3)
17-18: LGTM! Verify BaseModel implementation.The
createdAtandupdatedAtparameters are correctly passed to the superclass constructor.To ensure proper implementation, verify that
BaseModelcorrectly handles these timestamp fields:#!/bin/bash # Search for BaseModel implementation rg --type typescript 'class BaseModel' # Search for createdAt and updatedAt usage in BaseModel rg --type typescript 'class BaseModel' -A 20 | rg 'createdAt|updatedAt'
Line range hint
1-38: Overall implementation looks good, with some suggestions for improvement.The changes to
AccessTicketModelalign well with the PR objectives of adding timestamp fields. However, consider the following points:
- Add the
deletedAtfield to fully meet the PR objectives.- Verify that
BaseModelcorrectly implements the handling of these timestamp fields.- Ensure consistency across other model classes in the project.
To check for consistency across the project, run:
#!/bin/bash # Search for other model classes rg --type typescript 'class \w+Model extends BaseModel' # Check if they implement similar timestamp fields rg --type typescript 'class \w+Model extends BaseModel' -A 10 | rg 'createdAt|updatedAt|deletedAt'
9-10: LGTM! Consider addingdeletedAtparameter.The addition of
createdAtandupdatedAtparameters aligns with the PR objectives. These timestamp fields will enhance the tracking of record lifecycles.However, the PR objectives mention adding three timestamp fields, including
deletedAt. Consider adding this parameter as well for consistency:constructor( private shlinkId: string, private id?: string, createdAt?: Date, - updatedAt?: Date + updatedAt?: Date, + deletedAt?: Date ) {To verify if
deletedAtis implemented in other models, run:src/mappers/access-ticket-mapper.ts (1)
Line range hint
17-25: Verify intention:created_atandupdated_atnot mapped to entityThe change from
shlink_idtogetSHLinkId()is good, as it follows the principle of encapsulation by using a getter method instead of directly accessing the property.However, I noticed that the
created_atandupdated_atfields are not being mapped back to the entity. This could potentially lead to data loss when converting from model to entity.Could you please confirm if this is intentional? If not, consider updating the function to include these fields:
export const mapModelToEntity = ( accessTicketModel: AccessTicketModel, ): AccessTicketEntity | undefined => { return accessTicketModel ? { id: accessTicketModel.getId(), shlink_id: accessTicketModel.getSHLinkId(), created_at: accessTicketModel.getCreatedAt(), updated_at: accessTicketModel.getUpdatedAt() } : undefined; };Also, to ensure consistency across the codebase, let's verify if other mapper files have been updated similarly:
src/domain/models/user.ts (1)
15-23: Verify handling of timestamp fields in BaseModelThe
createdAtandupdatedAtfields are correctly passed to thesuper()call. However, the validation schema doesn't include these new fields.Please verify that the
BaseModelclass correctly handles these timestamp fields. If it doesn't, consider updating the validation schema to include them:If
BaseModeldoesn't handle these fields, update the validation schema as follows:super( z.object({ userId: z.string().min(1), patientId: z.string().min(1), id: z.string().optional(), serverConfigId: z.string().optional(), createdAt: z.date().optional(), updatedAt: z.date().optional() }), createdAt, updatedAt );This ensures that all fields, including the new timestamp fields, are properly validated.
src/domain/models/shlink-endpoint.ts (2)
11-12: LGTM: Constructor signature updated correctly.The addition of
createdAtandupdatedAtparameters to the constructor aligns with the PR objectives. These optionalDateparameters are appropriately typed and follow the correct naming convention.
21-22: Verify BaseModel handling of timestamp fields.The
createdAtandupdatedAtparameters are correctly passed to the superclass constructor. However, please ensure that theBaseModelclass properly handles these timestamp fields.To verify the
BaseModelimplementation, please run the following script:✅ Verification successful
To continue verifying the
BaseModelimplementation, please run the following script:
To further verify how the
BaseModelhandles thecreatedAtandupdatedAtfields, please run the following script:
BaseModel correctly handles timestamp fields.
The
BaseModelclass properly manages thecreatedAtandupdatedAtfields. Passing these parameters to the superclass constructor is appropriate and correctly implemented.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the BaseModel implementation for timestamp handling # Test: Search for BaseModel class definition and its handling of timestamp fields rg --type typescript -A 10 'class BaseModel'Length of output: 81
Script:
# #!/bin/bash # Description: Check the BaseModel implementation for timestamp handling by searching within TypeScript files # Search for the BaseModel class definition in .ts and .tsx files rg -g "*.ts" -g "*.tsx" 'class BaseModel'Length of output: 109
Script:
# #!/bin/bash # Description: Check how BaseModel handles timestamp fields in base-model.ts # Search for 'createdAt' and 'updatedAt' in base-model.ts rg 'createdAt|updatedAt' src/domain/models/base-model.tsLength of output: 538
src/mappers/user-mapper.ts (2)
46-50:⚠️ Potential issueInconsistent mapping and missing timestamp fields
The formatting changes improve readability. However, there are a couple of issues to address:
The newly added timestamp fields (
created_atandupdated_at) are not included in the mapping from UserDto to UserModel. This could lead to loss of data when converting from DTO to Model.The
serverConfigIdis now included in the mapping, which wasn't visible in the previous version. Ensure that this change is intentional and consistent with the UserDto definition.Consider updating the UserModel constructor call to include the timestamp fields:
return userDto ? new UserModel( userDto.userId, userDto.patientId, userDto.id, userDto.serverConfigId, userDto.createdAt, userDto.updatedAt ) : undefined;Also, verify that the UserDto interface includes all these fields.
To verify the UserDto interface definition:
#!/bin/bash # Search for UserDto interface definition rg --type typescript 'interface UserDto' -A 10
14-15: Partial implementation of timestamp fieldsThe addition of
created_atandupdated_atfields aligns with the PR objectives. However, there are a couple of points to consider:
- The
deletedAtfield mentioned in the PR objectives is not included. Was this intentional?- The order of parameters in the UserModel constructor has changed. Ensure that this change is reflected in the UserModel class definition and all other places where UserModel is instantiated.
To verify the UserModel constructor changes:
src/mappers/shlink-access-mapper.ts (3)
13-13: LGTM: Formatting improvement.The removal of the trailing comma improves code consistency.
27-28: LGTM: Added timestamp fields to model mapping.The addition of
created_atandupdated_atfields to theSHLinkAccessModelconstructor aligns with the PR objectives.
41-41: LGTM: Formatting improvement.The removal of the trailing comma improves code consistency.
src/mappers/access-ticket-mapper.test.ts (2)
46-46: LGTM! Improved consistency.Removing the trailing comma after the
shlink_idproperty improves consistency with other object literals in the file. This is a good minor style improvement.
Line range hint
11-22: Consider wrapping the entity object into a variable.As suggested in a previous review, it would be beneficial to wrap the
AccessTicketEntityobject into a variable. This would improve readability and reduce duplication, as the same object is used later in the assertion.Here's how you could implement this suggestion:
const entity: AccessTicketEntity = { id: '123', shlink_id: 'abc', created_at: dateValue, updated_at: dateValue }; const model = mapEntityToModel(entity); expect(model).toBeInstanceOf(AccessTicketModel); expect(model?.getId()).toBe(entity.id); expect(model?.getSHLinkId()).toBe(entity.shlink_id);src/domain/models/shlink.ts (1)
15-16: Consider adding thedeletedAtfield.The PR objectives mention adding three timestamp fields: createdAt, updatedAt, and deletedAt. However, only createdAt and updatedAt have been implemented. Was the omission of deletedAt intentional?
If
deletedAtshould be included, consider adding it:private userId: string, private name: string, private passcodeFailuresRemaining?: number, private active?: boolean, private managementToken?: string, private configPasscode?: string, private configExp?: Date, private id?: string, createdAt?: Date, updatedAt?: Date, deletedAt?: DateDon't forget to include it in the validation schema as well if you add it.
src/domain/models/server-config.ts (1)
19-20: LGTM: Constructor updated with new timestamp fields.The addition of
createdAtandupdatedAtparameters to the constructor aligns with the PR objectives. These optionalDateparameters will allow for better tracking of record lifecycles.src/mappers/shlink-endpoint-mapper.test.ts (1)
12-13: LGTM: Addition of dateValue constantThe introduction of the
dateValueconstant is a good practice. It provides a consistent date value for testing the new timestamp fields, aligning with the PR objective of adding these fields to all tables.src/mappers/user-mapper.test.ts (2)
35-35: LGTM! Consistent with entity and model changes.The addition of
serverConfigIdto theuserDtoobject is correct and follows the existing naming convention for DTOs.
19-19: LGTM! Verify consistency across the codebase.The addition of
server_config_idto theuserEntityobject is correct and follows the existing naming convention.Please ensure that this new field is consistently added to all relevant user entity definitions across the codebase. Run the following script to verify:
src/usecases/shlinks/get-single-shlink.test.ts (1)
Line range hint
1-100: Overall, the changes look good. Verify test coverage for new fields.The implementation of the new timestamp fields (
created_at,updated_at, anddeleted_at) in the test file is consistent with the PR objectives. The changes have been correctly applied to both the mock entity and the model.To ensure comprehensive test coverage, please verify that the new timestamp fields are properly tested in all relevant test cases. Run the following command to check the test coverage for these new fields:
If the output doesn't show assertions for these fields, consider adding specific test cases to validate their behavior.
src/usecases/shlinks/add-shlink.test.ts (2)
50-52: LGTM: New timestamp fields added correctlyThe addition of
created_at,updated_at, anddeleted_atfields to theSHLinkModelconstructor aligns with the PR objectives. The use of a specific date for testing and settingdeleted_atto null for a new entity are appropriate choices.
Line range hint
1-105: Consider adding explicit tests for new timestamp fieldsWhile the mock objects have been updated with the new timestamp fields (
created_at,updated_at, anddeleted_at), the test cases themselves don't appear to explicitly verify the handling of these new fields.To ensure comprehensive test coverage, consider adding or updating test cases to:
- Verify that the
addShlinkUseCasecorrectly sets thecreated_atandupdated_atfields when creating a new SHLink.- Confirm that the
deleted_atfield is initially set to null for new SHLinks.- Check that the mapper functions (
mapModelToEntityandmapEntityToModel) correctly handle these new fields.Here's a script to verify the current test coverage for these fields:
This script will help identify any existing coverage for the new fields and highlight areas where additional tests might be beneficial.
src/mappers/shlink-mapper.ts (5)
39-40: LGTM! Consider addingdeleted_atfield and verify getter methods.The addition of
created_atandupdated_atfields to the entity object is consistent with the PR objectives. However, consider the following points:
The
deletedAtfield mentioned in the PR objectives is not included. Consider adding it asdeleted_atto maintain consistency with the snake_case naming convention used in the entity.The use of
getCreatedAt()andgetUpdatedAt()methods implies that these have been added to theSHLinkModelclass.Please ensure that these getter methods have been properly implemented in the
SHLinkModelclass. Run the following script to verify:#!/bin/bash # Description: Verify the implementation of new getter methods in SHLinkModel # Test: Search for SHLinkModel class definition and its methods echo "Checking SHLinkModel class definition:" rg --type typescript "class SHLinkModel" -A 30 # Test: Specifically search for the new getter methods echo "Checking for new getter methods:" rg --type typescript "getCreatedAt\(\)|getUpdatedAt\(\)" -C 2
58-59: LGTM! Consider addingdeletedAtfield.The addition of
createdAtandupdatedAtfields to the DTO object is consistent with the PR objectives and follows the correct camelCase naming convention for DTOs.However, the
deletedAtfield mentioned in the PR objectives is not included. Consider adding it to maintain consistency with the PR goals.To ensure consistency across the codebase, please run the following script:
#!/bin/bash # Description: Verify consistent implementation of timestamp fields in DTOs # Test: Search for SHLinkDto interface or type definition echo "Checking SHLinkDto definition:" rg --type typescript "interface SHLinkDto|type SHLinkDto" -A 15 # Test: Search for other DTO definitions to ensure consistency echo "Checking other DTO definitions for timestamp fields:" rg --type typescript "interface .*Dto|type .*Dto" -A 15 | rg "createdAt|updatedAt|deletedAt" -C 2
105-106: LGTM! Consider addingdeletedAtfield.The addition of
createdAtandupdatedAtfields to the SHLinkModel constructor is consistent with the PR objectives and the changes made in other mapping functions.However, the
deletedAtfield mentioned in the PR objectives is not included. Consider adding it to maintain consistency with the PR goals and other mapping functions.To ensure consistency across the codebase, please run the following script:
#!/bin/bash # Description: Verify consistent implementation of timestamp fields in DTO to Model mapping # Test: Search for SHLinkModel constructor usage in mapping functions echo "Checking SHLinkModel constructor usage in mapping functions:" rg --type typescript "new SHLinkModel\(" -A 10 # Test: Search for other DTO to Model mapping functions to ensure consistency echo "Checking other DTO to Model mapping functions for timestamp fields:" rg --type typescript "mapDtoToModel|mapEntityToModel" -A 15 | rg "createdAt|updatedAt|deletedAt" -C 2
20-21: LGTM! Consider addingdeletedAtfield.The addition of
created_atandupdated_atfields to theSHLinkModelconstructor is consistent with the PR objectives. However, thedeletedAtfield mentioned in the PR objectives is not included.Could you clarify if the
deletedAtfield should also be added to this mapping function? If so, please update the function accordingly.To verify the consistency of these changes across the codebase, run the following script:
79-80: LGTM! Consider addingdeletedAtfield and verify MiniDTO interface.The addition of
createdAtandupdatedAtfields to the MiniDTO object is consistent with the PR objectives and follows the correct camelCase naming convention for DTOs.However, the
deletedAtfield mentioned in the PR objectives is not included. Consider adding it to maintain consistency with the PR goals.To ensure consistency across the codebase, please run the following script:
src/usecases/shlinks/get-shlink.test.ts (2)
51-53: Consistent implementation of timestamp fields.The addition of timestamp fields here is consistent with the previous mock entity.
Please refer to the previous comment regarding the use of
new Date()for more realistic test data.
Line range hint
1-145: Overall, the changes look good and align with the PR objectives.The implementation of
created_at,updated_at, anddeleted_atfields is consistent across mock entities and models. The changes enhance the test data to reflect the new schema accurately.To ensure consistency across the codebase, let's verify if similar changes have been made in other test files:
This script will help identify any test files that might need similar updates and potential inconsistencies in the usage of timestamp fields.
✅ Verification successful
Verification Successful: All test files consistently implement the timestamp fields.
No inconsistencies found regarding
created_at,updated_at, anddeleted_atacross the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for consistent implementation of timestamp fields in test files. # Test: Search for test files that might need similar updates echo "Test files that might need timestamp field updates:" rg --type typescript -g '*.test.ts' -l 'new SHLinkModel' | grep -v 'get-shlink.test.ts' # Test: Check for inconsistent usage of timestamp fields echo "\nPotential inconsistencies in timestamp field usage:" rg --type typescript -g '*.test.ts' -n 'new SHLinkModel\([^)]+\)' | grep -vE '(created_at|updated_at|deleted_at)'Length of output: 503
Script:
#!/bin/bash # Description: Check for consistent implementation of timestamp fields in test files. # Test: Search for test files that might need similar updates echo "Test files that might need timestamp field updates:" rg -g '*.test.ts' -l 'new SHLinkModel' | grep -v 'get-shlink.test.ts' # Test: Check for inconsistent usage of timestamp fields echo "\nPotential inconsistencies in timestamp field usage:" rg -g '*.test.ts' -n 'new SHLinkModel\([^)]+\)' | grep -vE '(created_at|updated_at|deleted_at)'Length of output: 923
src/usecases/shlinks/validate-shlink.test.ts (6)
40-42: Consistent implementation of timestamp fields.The changes here are consistent with the previous instance, which is good for maintaining uniformity across test cases.
Please refer to the previous comment about using constants for these timestamp values to improve readability and maintainability.
62-64: LGTM! Consistent implementation.The changes here maintain consistency with the previous instances, which is excellent for code uniformity.
84-86: LGTM! Maintaining consistency.The changes continue to be consistent with previous instances, which is great for code readability and maintainability.
105-107: LGTM! Consistent implementation.The changes here continue to maintain consistency with previous instances.
123-125: LGTM! Consistency maintained.The changes continue to be consistent with previous instances, which is excellent for code uniformity.
Line range hint
1-156: Overall, excellent implementation of timestamp fields.The changes in this file consistently add
createdAt,updatedAt, anddeletedAtfields to theSHLinkModelconstructor in all test cases, which aligns well with the PR objectives. The implementation is uniform throughout the file, which is great for maintainability.To further improve the code:
- Consider using constants for the timestamp values to enhance readability.
- Add a test case specifically for validating a SHLink with a non-null
deletedAtvalue to ensure comprehensive coverage of the new fields.Great job on this implementation!
prisma/schema.test.prisma (9)
13-14: LGTM: Timestamp fields added correctlyThe
created_atandupdated_atfields have been properly added to thecas_itemmodel. The use of@default(now())forcreated_atand@updatedAt @default(now())forupdated_atensures accurate tracking of record creation and modification times.
25-26: LGTM: Timestamp fields added consistentlyThe
created_atandupdated_atfields have been added to theshlink_filemodel, maintaining consistency with thecas_itemmodel. The attributes are correctly applied to ensure proper tracking of creation and update times.
39-40: LGTM with a question: Timestamp fields added, but clarification neededThe
created_atandupdated_atfields have been correctly added to theshlink_accessmodel. However, there's an existingaccess_timefield that also defaults to the current timestamp.Could you clarify the intended difference between
access_timeandcreated_at? If they serve different purposes, consider adding a comment to explain their distinct roles. If they are redundant, we might want to consider removing one of them.Also applies to: 43-43
52-53: LGTM: Timestamp fields added correctlyThe
created_atandupdated_atfields have been properly added to theshlinkmodel, maintaining consistency with the other models. The attributes are correctly applied to ensure proper tracking of creation and update times.
73-74: LGTM: Timestamp fields added consistentlyThe
created_atandupdated_atfields have been correctly added to theusermodel, maintaining consistency with the other models. The attributes are properly applied to ensure accurate tracking of creation and update times.
87-88: LGTM with a question: Timestamp fields added, but clarification neededThe
created_atandupdated_atfields have been correctly added to theserver_configmodel. However, there's an existingrefresh_timefield that also defaults to the current timestamp.Could you clarify the intended difference between
refresh_timeandupdated_at? If they serve different purposes, consider adding a comment to explain their distinct roles. Ifrefresh_timeis meant to track a specific type of update, it might be worth considering if it should use the@updatedAtattribute instead of@default(now()).Also applies to: 94-94
104-105: LGTM: Timestamp fields added correctlyThe
created_atandupdated_atfields have been properly added to theshlink_endpointmodel, maintaining consistency with the other models. The attributes are correctly applied to ensure proper tracking of creation and update times.
119-120: LGTM: Timestamp fields added consistentlyThe
created_atandupdated_atfields have been correctly added to theaccess_ticketmodel, maintaining consistency with the other models. The attributes are properly applied to ensure accurate tracking of creation and update times.
Line range hint
1-124: Overall LGTM with minor clarifications neededThe changes in this PR successfully add
created_atandupdated_attimestamp fields to all models in the Prisma schema, which aligns with the stated objectives. The implementation is consistent across all models and follows best practices for tracking record creation and modification times.However, there are two points that require clarification:
- In the
shlink_accessmodel, the relationship between the newcreated_atfield and the existingaccess_timefield needs to be explained.- In the
server_configmodel, the purpose of therefresh_timefield in relation to the newupdated_atfield should be clarified.Once these points are addressed, the changes will be fully ready for approval.
src/usecases/shlinks/update-single-shlink.test.ts (4)
28-30: LGTM: New timestamp fields added correctlyThe addition of
created_at,updated_at, anddeleted_atfields to themockDtoobject aligns with the PR objectives. The initialization of these fields with appropriate values (dates for creation and update, null for deletion) is correct and consistent.
56-58: LGTM: New timestamp fields added correctly to mockSHLinkEntityThe addition of
created_at,updated_at, anddeleted_atfields to themockSHLinkEntityobject is correct and consistent with the changes made to other mock objects. This ensures that the test data accurately reflects the new schema changes across all levels of the application.
Line range hint
1-158: Summary: New timestamp fields successfully added, consider enhancing test coverageThe changes to add
created_at,updated_at, anddeleted_atfields to the mock objects in this test file have been implemented correctly and consistently. These additions align well with the PR objectives of adding timestamp fields to all tables.While the mock data structure has been updated appropriately, consider enhancing the test suite to explicitly verify the behavior of these new fields in various scenarios. This will ensure that the
updateSingleSHLinkUseCasecorrectly handles these fields and maintains data integrity.Overall, the changes look good, but additional test coverage would further strengthen the reliability of the code.
Line range hint
1-158: Consider adding test cases for new timestamp fieldsWhile the mock objects have been updated to include the new
created_at,updated_at, anddeleted_atfields, the existing test cases don't explicitly verify the behavior related to these fields. To ensure comprehensive test coverage:
- Add a new test case that verifies the
updated_atfield is properly set when updating a SHLink.- Modify the existing test cases to assert that
created_atremains unchanged anddeleted_atstays null after updates.- Consider adding a test case for soft deletion if that's a feature of the system, verifying that
deleted_atis set correctly.These additions would help ensure that the new fields are properly handled by the use case and maintain data integrity.
To assist in identifying areas where additional tests might be needed, you can run the following command to check for usage of the new fields in the test file:
This will help identify if there are any existing assertions for the new fields, and highlight areas where additional test coverage might be beneficial.
prisma/schema.prisma (8)
11-17: LGTM: Timestamp fields added correctly tocas_itemmodelThe
created_atandupdated_atfields have been added correctly to thecas_itemmodel. The implementation follows Prisma best practices:
created_atuses@default(now())to set the creation time.updated_atuses@updatedAtto automatically update the timestamp on record modifications.These additions will improve tracking of record lifecycles as intended.
23-25: LGTM: Timestamp fields added correctly toshlink_filemodelThe
created_atandupdated_atfields have been added correctly to theshlink_filemodel, consistent with the implementation in thecas_itemmodel. This maintains consistency across the schema and fulfills the PR objectives.
51-52: LGTM: Timestamp fields added correctly toshlinkmodelThe
created_atandupdated_atfields have been added correctly to theshlinkmodel, consistent with the implementation in other models. This maintains consistency across the schema and fulfills the PR objectives.
73-74: LGTM: Timestamp fields added correctly tousermodelThe
created_atandupdated_atfields have been added correctly to theusermodel, consistent with the implementation in other models. This maintains consistency across the schema and fulfills the PR objectives.
108-109: LGTM: Timestamp fields added correctly toshlink_endpointmodelThe
created_atandupdated_atfields have been added correctly to theshlink_endpointmodel, consistent with the implementation in other models. This maintains consistency across the schema and fulfills the PR objectives.
123-124: LGTM: Timestamp fields added correctly toaccess_ticketmodelThe
created_atandupdated_atfields have been added correctly to theaccess_ticketmodel, consistent with the implementation in other models. This maintains consistency across the schema and fulfills the PR objectives.
89-90: LGTM with a suggestion: Clarify the purpose ofrefresh_timeThe
created_atandupdated_atfields have been added correctly to theserver_configmodel, consistent with other models. However, there's an existingrefresh_timefield that might overlap withupdated_at.Could you clarify the difference between
refresh_timeandupdated_at? If they serve different purposes, please add a comment explaining their distinct roles. Ifrefresh_timeis no longer needed, consider removing it to avoid confusion.To verify the usage of
refresh_time, run the following script:#!/bin/bash # Description: Check for usage of refresh_time in the codebase # Test: Search for refresh_time usage rg --type typescript --type javascript 'refresh_time'Also applies to: 98-98
37-43: LGTM with a suggestion: Clarify the purpose ofaccess_timeThe
created_atandupdated_atfields have been added correctly to theshlink_accessmodel, consistent with other models. However, there's an existingaccess_timefield that might overlap withcreated_at.Could you clarify the difference between
access_timeandcreated_at? If they serve the same purpose, consider removingaccess_timeto avoid redundancy. If they're different, please add a comment explaining their distinct purposes.To verify the usage of
access_time, run the following script:✅ Verification successful
Verified:
access_timeis actively used across the codebaseThe
access_timefield is referenced in multiple components, ensuring it serves a distinct purpose separate from the newly addedcreated_atandupdated_atfields. No redundancy issues detected.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for usage of access_time in the codebase # Test: Search for access_time usage rg --type typescript --type javascript 'access_time'Length of output: 88
Script:
#!/bin/bash # Description: Check for usage of access_time in the codebase # Test: Search for access_time usage rg --type typescript --type javascript 'access_time'Length of output: 87
Script:
#!/bin/bash # Description: Check for usage of access_time in TypeScript and JavaScript files # Test: Search for access_time usage in .ts and .js files rg 'access_time' --glob '*.ts' --glob '*.js'Length of output: 877
src/mappers/server-config-mapper.test.ts (4)
12-12: LGTM: Good addition of a date constant for testing.The addition of
dateValueconstant with a specific UTC date is a good practice for consistent date handling in tests.
38-38: LGTM: Consistent removal of trailing comma.The removal of the trailing comma after 'entity-refresh-token' is consistent with other similar changes in the file.
50-50: LGTM: Consistent removal of trailing commas in object literals.The removal of trailing commas in object literals at lines 50, 94, and 116 is consistent with earlier changes and maintains a uniform coding style throughout the file.
Also applies to: 94-94, 116-116
Line range hint
1-146: Consider updating tests to cover new timestamp fields.While the changes align with the PR objectives and improve code consistency, the tests haven't been updated to check the new
created_atandupdated_atfields. Consider adding assertions for these fields in the relevant test cases, particularly in themapEntityToModelandmapModelToEntitytests.To verify the current test coverage for the new fields, you can run the following command:
If this command doesn't return any results, it confirms that the new fields are not being tested yet.
src/domain/dtos/shlink.ts (1)
84-84: LGTM: Improved example forconfigExpThe update to the
configExpexample with a specific date (2024-09-26T10:03:39.379Z) is a good improvement. It provides a clear, concrete example of the expected date format (ISO 8601), which enhances the API documentation and helps developers understand the expected input.src/mappers/shlink-mapper.test.ts (9)
14-15: LGTM: Good practice for test date consistencyThe introduction of the
dateValueconstant is a good practice. It ensures consistency across the test file and makes it easier to update the date value if needed in the future.
25-27: LGTM: Timestamp fields added as per PR objectiveThe addition of
config_exp,created_at, andupdated_atfields toshLinkEntityaligns with the PR objective. Using thedateValueconstant ensures consistency across the test file.
37-37: LGTM: SHLinkModel constructor updated correctlyThe
SHLinkModelconstructor has been correctly updated to include the new timestamp fields. The use ofdateValueensures consistency, and the placement of the new parameters at the end of the constructor call is correct.Also applies to: 39-40
51-53: LGTM: DTO updated with correct naming conventionThe
shLinkDtoobject has been correctly updated with the new timestamp fields. The use of camelCase for the DTO fields (configExp,createdAt,updatedAt) follows the correct naming convention, and the use ofdateValueensures consistency across the test file.
89-90: LGTM: mapModelToEntity test updated correctlyThe
mapModelToEntityfunction test has been correctly updated to include the new timestamp fields. The use of getter methods (getCreatedAt()andgetUpdatedAt()) is consistent with the other fields in the test.
112-114: LGTM: mapModelToDto test updated correctlyThe
mapModelToDtofunction test has been correctly updated to include the new timestamp fields. The use of getter methods and the camelCase naming convention for the DTO fields are correct and consistent with the rest of the test.
159-160: LGTM: Model constructors updated consistentlyThe constructors for
SHLinkModelandSHLinkEndpointModelhave been consistently updated across all test cases to include the new timestamp fields. The use ofdateValueensures consistency, and the placement of the new parameters at the end of the constructor calls is correct.Also applies to: 169-170, 177-178, 266-267, 276-277
195-196: LGTM: mapModelToMiniDto tests updated correctlyThe expected results in the
mapModelToMiniDtotests have been correctly updated to include the new timestamp fields. The use ofdateValueensures consistency, and the camelCase naming convention for the DTO fields is correct.Also applies to: 240-241, 293-294
Line range hint
1-304: Overall: Excellent implementation of timestamp fieldsThe changes in this file comprehensively address the PR objective of adding timestamp fields (createdAt, updatedAt) to all relevant objects and test cases. The implementation is consistent, using the
dateValueconstant throughout, which ensures easy maintenance. All necessary areas of the test file have been updated, including entity objects, models, DTOs, and various mapping function tests. The changes follow correct naming conventions and are well-integrated into the existing test structure.Great job on maintaining consistency and thoroughness throughout the file!
src/mappers/shlink-access-mapper.test.ts (1)
139-139: Verify ifcreatedAtandupdatedAtshould be included in the DTO.The
SHLinkAccessDtocurrently does not includecreatedAtandupdatedAt. If these fields are intended to be exposed via the DTO, update themapModelToDtofunction and the test accordingly.Do you need assistance in updating the DTO and related mappings to include these fields?
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (13)
src/usecases/shlinks/deactivate-shlink.ts (1)
11-24: Overall improvement in code structure with a minor suggestion.The refactoring to a model-based approach significantly improves the code structure and maintainability. It aligns well with the PR objectives of enhancing data management.
However, there's a minor inconsistency in error handling:
Consider adding an explicit check for the existence of the model before proceeding with the authorization check. This would provide clearer error messages and maintain consistency with the entity-based approach. Here's a suggested improvement:
const model = mapEntityToModel(await context.repo.findById(data.id)); let newShlink: SHLinkEntity; if (model) { + if (!model) { + throw new Error('SHLink not found'); + } if (data.user.id !== model.getUserId()) throw new AuthenticationError( 'User not authorized to deactivate shlink.', ); model.setActive(false); newShlink = await context.repo.update(mapModelToEntity(model)); } return mapEntityToModel(newShlink);This change would ensure that a clear error is thrown if the SHLink is not found, improving error handling and debugging.
src/usecases/shlinks/update-single-shlink.ts (1)
31-32: Good changes: Consistent use of model and proper layer separation.The modifications to use the model in the validator call and map it back to an entity for the repository update are good practices. They maintain consistency and proper separation between domain and persistence layers.
However, a minor improvement could be made:
Consider extracting the model-to-entity mapping to a separate line for better readability:
- updateShlink = await context.repo.update(mapModelToEntity(model)); + const entityToUpdate = mapModelToEntity(model); + updateShlink = await context.repo.update(entityToUpdate);This change would make the code slightly more readable and easier to debug if needed.
Also applies to: 35-35
src/usecases/shlinks/update-single-shlink.test.ts (3)
27-39: LGTM: Detailed mock entity and model setupThe mock entity and model setups are now more comprehensive, including necessary methods for testing. The mapper function mocks are correctly configured to return the mock model and entity.
Consider adding a
setActivemethod to the mock model for consistency, as it might be useful in future tests:mockModel = { id: '1', active: false, setConfigPasscode: jest.fn(), setConfigExp: jest.fn(), getConfigPasscode: jest.fn().mockReturnValue('1234'), + setActive: jest.fn(), } as unknown as SHLinkModel;
42-63: LGTM: Comprehensive test for updating passcode and expiry dateThis test case thoroughly checks the behavior of
updateSingleSHLinkUseCasewhen updating both passcode and expiry date. It verifies all expected method calls and the final result.Consider adding an assertion to check if
mapModelToEntitywas called beforerepo.update:expect(repo.update).toHaveBeenCalledWith(mockEntity); +expect(mapModelToEntity).toHaveBeenCalledWith(mockModel); expect(result).toBe(mockModel);This ensures that the model is correctly mapped back to an entity before updating the repository.
90-104: LGTM with suggestion: Error handling testThis test case effectively verifies that the
updateSingleSHLinkUseCasethrows an error when validation fails. The use ofrejects.toThrow()is appropriate for testing asynchronous error handling.Consider adding assertions to verify the behavior before the error is thrown:
// Arrange const data = { id: '1', passcode: 'invalidPasscode' }; repo.findOne.mockResolvedValue(mockEntity); mockValidator.mockImplementation(() => { throw new Error('Validation failed'); }); +// Act +const updatePromise = updateSingleSHLinkUseCase( + { repo, validator: mockValidator }, + data +); + // Act & Assert -await expect( - updateSingleSHLinkUseCase( - { repo, validator: mockValidator }, - data - ) -).rejects.toThrow('Validation failed'); +await expect(updatePromise).rejects.toThrow('Validation failed'); + +// Additional assertions +expect(repo.findOne).toHaveBeenCalledWith({ id: data.id }); +expect(mockModel.setConfigPasscode).toHaveBeenCalledWith(data.passcode); +expect(mockValidator).toHaveBeenCalled(); +expect(repo.update).not.toHaveBeenCalled();These additional assertions will ensure that the expected methods are called before the error is thrown, and that the repository update is not called after the validation fails.
src/usecases/shlinks/deactivate-shlink.test.ts (3)
19-20: Consider using a more flexible date value.While using a constant date value helps maintain consistency across test cases, setting it to a future date (2024) might not be ideal for all test scenarios. Consider using a dynamic date that's closer to the current date, or use a library like
jest-date-mockto have more control over the date in your tests.You could replace the current implementation with:
const dateValue = new Date();Or, if you want to use a fixed date that's not in the future:
const dateValue = new Date('2023-01-01T00:00:00Z');
Line range hint
22-33: LGTM: Mock setup updated correctly with a minor suggestion.The changes in the mock setup are consistent with the new fields added to the entities. The use of
jest.Mocked<ISHLinkRepository>provides better type checking for mocked methods, which is a good practice.Consider destructuring the
dateValuewhen setting multiple fields to the same value inmockSHLinkEntity:mockSHLinkEntity = { // ... other fields config_exp: dateValue, created_at: dateValue, updated_at: dateValue, };This can make the code more concise and easier to maintain.
Also applies to: 35-35, 48-50, 55-55, 67-68, 75-76
110-121: LGTM: Excellent addition of authentication error test.This new test case significantly improves the overall test coverage by checking the authentication logic. It correctly verifies that
findByIdis called butupdateis not called when the user is unauthorized.Consider adding a more specific error message check to ensure the correct error is thrown:
await expect( deactivateSHLinksUseCase(mockContext, { id: mockId, user: { id: wrongUserId, name: '', email: '' }, }), ).rejects.toThrow('User is not authorized to deactivate this SHLink');This will make the test more robust against potential changes in error messages.
prisma/migrations/20240927083507_/migration.sql (5)
1-11: LGTM! Consider adding a table comment.The
cas_itemtable creation looks good and aligns with the PR objectives. The timestamp fields are correctly included, and appropriate data types are used for each column.Consider adding a comment to explain the purpose of this table. For example:
COMMENT ON TABLE "cas_item" IS 'Stores content-addressable storage items with their hashes and reference counts';
25-35: LGTM! Consider revising access_time default.The
shlink_accesstable creation looks good and aligns with the PR objectives. The timestamp fields are correctly included.Consider removing the default value for
access_time:- "access_time" TIMESTAMP(3) NOT NULL DEFAULT CURRENT_TIMESTAMP, + "access_time" TIMESTAMP(3) NOT NULL,This allows for explicitly setting the access time, which might differ from the record creation time. If you want to keep the default, consider explaining why it's needed in addition to
created_at.Also, consider adding a table comment to explain the purpose of this table and its relationship to other tables.
85-95: LGTM! Consider adding a unique constraint.The
shlink_endpointtable creation looks good and aligns with the PR objectives. The timestamp fields are correctly included.Consider adding a unique constraint to ensure that each combination of
shlink_id,server_config_id, andurl_pathis unique:ALTER TABLE "shlink_endpoint" ADD CONSTRAINT "unique_shlink_endpoint" UNIQUE ("shlink_id", "server_config_id", "url_path");This constraint would prevent duplicate entries and maintain data integrity.
Also, add a table comment to explain the purpose of this table and its relationship to other tables.
164-186: LGTM! Consider consistent ON DELETE behavior.The foreign key constraints look good and maintain referential integrity between tables.
For consistency, consider standardizing the ON DELETE behavior across all foreign key constraints. Currently, you're using a mix of SET NULL and RESTRICT. Unless there's a specific reason for this variation, it might be clearer to use RESTRICT consistently:
-ALTER TABLE "shlink_file" ADD CONSTRAINT "shlink_file_content_hash_fkey" FOREIGN KEY ("content_hash") REFERENCES "cas_item"("hash") ON DELETE SET NULL ON UPDATE CASCADE; +ALTER TABLE "shlink_file" ADD CONSTRAINT "shlink_file_content_hash_fkey" FOREIGN KEY ("content_hash") REFERENCES "cas_item"("hash") ON DELETE RESTRICT ON UPDATE CASCADE; -ALTER TABLE "user" ADD CONSTRAINT "user_server_config_id_fkey" FOREIGN KEY ("server_config_id") REFERENCES "server_config"("id") ON DELETE SET NULL ON UPDATE CASCADE; +ALTER TABLE "user" ADD CONSTRAINT "user_server_config_id_fkey" FOREIGN KEY ("server_config_id") REFERENCES "server_config"("id") ON DELETE RESTRICT ON UPDATE CASCADE;This change would ensure that no records can be deleted if they are referenced by other tables, providing a consistent safeguard against data loss. However, if the current mixed approach is intentional based on your application's requirements, please clarify the reasoning in comments for future maintainers.
1-186: Overall, well-structured migration with some areas for improvement.This migration successfully implements the PR objectives by adding timestamp fields to all tables and creating a comprehensive database schema. Here's a summary of key points to address:
Security:
- Remove plain text storage of passwords and secrets (e.g.,
config_passcode,config_client_secret).- Consider encrypting sensitive fields like
config_refresh_tokenandaccess_token_response.Optimization:
- Review and remove redundant indexes, especially on primary key columns.
- Consider adding composite indexes based on query patterns.
Consistency:
- Standardize ON DELETE behavior for foreign key constraints.
- Ensure consistent naming conventions across all tables and constraints.
Documentation:
- Add table comments to explain the purpose of each table and its relationships.
Validation:
- Clarify the purpose of the
access_tickettable and its relationship to other tables.- Verify the necessity of both
idanduser_idin theusertable.Address these points to enhance the security, performance, and maintainability of your database schema.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (11)
- prisma/migrations/20240927083507_/migration.sql (1 hunks)
- prisma/migrations/20240927084525_/migration.sql (1 hunks)
- src/domain/models/base-model.ts (3 hunks)
- src/mappers/shlink-mapper.test.ts (13 hunks)
- src/mappers/shlink-mapper.ts (5 hunks)
- src/usecases/shlinks/activate-shlink.test.ts (4 hunks)
- src/usecases/shlinks/activate-shlink.ts (1 hunks)
- src/usecases/shlinks/deactivate-shlink.test.ts (6 hunks)
- src/usecases/shlinks/deactivate-shlink.ts (1 hunks)
- src/usecases/shlinks/update-single-shlink.test.ts (1 hunks)
- src/usecases/shlinks/update-single-shlink.ts (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/domain/models/base-model.ts
- src/mappers/shlink-mapper.test.ts
- src/mappers/shlink-mapper.ts
- src/usecases/shlinks/activate-shlink.test.ts
🔇 Additional comments (17)
prisma/migrations/20240927084525_/migration.sql (1)
1-8: Consider alternatives to dropping thedeleted_atcolumn.Dropping the
deleted_atcolumn will result in permanent data loss, as warned in the comment. This column is typically used for soft deletes, which can be beneficial for data recovery and auditing purposes.Before proceeding with this change, please consider the following:
- Is the data in this column no longer needed? If it's valuable, consider exporting it before dropping the column.
- Are there any dependencies in the application code that rely on this column? Removing it might cause runtime errors if not handled properly.
- Instead of dropping the column, could it be repurposed or renamed to fit the new schema design?
If you decide to proceed with dropping the column, ensure that all related application code is updated accordingly to prevent any potential issues.
To help assess the impact, let's check for usage of the
deleted_atcolumn in the codebase:✅ Verification successful
Dropping the
deleted_atcolumn is verified safe.No references to
deleted_atwere found in the application codebase, indicating no dependencies. You can proceed with removing the column as planned.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for usage of the deleted_at column in the codebase # Search for 'deleted_at' in TypeScript/JavaScript files echo "TypeScript/JavaScript files referencing 'deleted_at':" rg --type-add 'ts:*.{ts,js}' --type ts 'deleted_at' -l # Search for 'deleted_at' in SQL files echo "SQL files referencing 'deleted_at':" rg --type sql 'deleted_at' -l # Search for 'deletedAt' in TypeScript/JavaScript files (camelCase variant) echo "TypeScript/JavaScript files referencing 'deletedAt':" rg --type-add 'ts:*.{ts,js}' --type ts 'deletedAt' -lLength of output: 565
src/usecases/shlinks/deactivate-shlink.ts (2)
5-5: LGTM: Good separation of concerns with mapper functions.The addition of
mapEntityToModelandmapModelToEntityimports from a dedicated mapper file is a positive change. It promotes better separation of concerns and improves code maintainability.
11-12: LGTM: Improved separation of concerns with model-based approach.The changes effectively implement a model-based approach:
- Using
mapEntityToModelto work with a model instead of an entity.- Utilizing model methods like
getUserId()andsetActive()for better encapsulation.- Applying
mapModelToEntitybefore updating the repository.These changes enhance the separation between entity and model layers, improving code clarity and maintainability.
Also applies to: 15-22
src/usecases/shlinks/update-single-shlink.ts (4)
4-4: LGTM: Import statement updated correctly.The addition of
mapModelToEntityimport is consistent with the changes described in the summary and supports the bidirectional mapping between entity and model.
21-21: Excellent: Improved entity-to-model mapping.The change to map the repository result to a model (
mapEntityToModel) is a good practice. It enhances type safety, allows for the use of model-specific methods, and maintains a clear separation between the persistence layer (entities) and the domain layer (models).
24-24: Great improvement: Using model methods for updates.The changes to use
model.setConfigPasscode()andmodel.setConfigExp()instead of directly modifying properties are excellent. This approach:
- Encapsulates the update logic within the model.
- Allows for additional validation or business logic in the setter methods.
- Improves maintainability and adheres to the single responsibility principle.
Also applies to: 27-27
Line range hint
1-38: Overall excellent improvements to code structure and design.The changes in this file significantly enhance the separation of concerns between the domain and persistence layers, consistently using the
SHLinkModelthroughout the use case. This approach improves type safety, encapsulation, and maintainability.To ensure consistency across the codebase:
- Verify that similar changes have been applied to other use cases dealing with SHLinks.
- Consider creating (if not already present) or updating architecture documentation to reflect this approach of using mappers and working primarily with domain models in use cases.
To check for consistency, we can search for other files that might need similar updates:
This will help identify other areas of the codebase that might benefit from similar refactoring.
✅ Verification successful
To effectively verify the consistency of changes across the codebase, let's adjust the shell scripts to target
.tsfiles directly.
Verification Successful: The refactoring changes in
update-single-shlink.tsare consistently applied across the codebase. No direct accesses toSHLinkEntityproperties remain, and mapper functions are appropriately used in all relevant use cases.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other files that might need similar updates echo "Files potentially needing similar updates:" rg --type typescript 'export const \w+SHLinkUseCase' --files-with-matches echo "\nFiles still directly accessing SHLinkEntity properties:" rg --type typescript 'SHLinkEntity.*\.(configPasscode|configExp)' -l echo "\nFiles not using mapEntityToModel or mapModelToEntity:" rg --type typescript 'ISHLinkRepository' -l | xargs rg --type typescript -L '(mapEntityToModel|mapModelToEntity)'Length of output: 741
Script:
#!/bin/bash # Search for other files that might need similar updates echo "Files potentially needing similar updates:" rg 'export const \w+SHLinkUseCase' -g "*.ts" --files-with-matches echo "\nFiles still directly accessing SHLinkEntity properties:" rg 'SHLinkEntity.*\.(configPasscode|configExp)' -g "*.ts" -l echo "\nFiles not using mapEntityToModel or mapModelToEntity:" rg 'ISHLinkRepository' -g "*.ts" -l | xargs rg -L '(mapEntityToModel|mapModelToEntity)'Length of output: 6389
src/usecases/shlinks/update-single-shlink.test.ts (3)
4-4: LGTM: Improved import and mock setupThe addition of
mapModelToEntityimport and mocking the entire mapper module enhances the test setup, allowing for more comprehensive testing of bi-directional mapping between entities and models.Also applies to: 8-9
12-22: LGTM: Comprehensive mock setupThe mock repository and validator setups are well-structured. The repository mock includes both
findOneandupdatemethods, which are crucial for testing the use case. The validator mock is appropriately set up as a simple jest mock function.Also applies to: 24-25
66-87: LGTM: Proper handling of undefined inputsThis test case effectively verifies that the
updateSingleSHLinkUseCasedoesn't modify passcode or expiry date when they're not provided in the input. It correctly checks that the setter methods are not called and that the validator is still invoked with the existing data.src/usecases/shlinks/deactivate-shlink.test.ts (4)
5-9: LGTM: Imports and mocks updated appropriately.The new imports and mocks are consistent with the changes in the test suite. The
AuthenticationErrorimport suggests a new test case for authentication, which is a good addition for comprehensive testing.Also applies to: 16-16
78-80: LGTM: Good practice to clear mocks after each test.Adding an
afterEachblock to clear mocks after each test is an excellent practice. This ensures that each test starts with a clean slate, improving test isolation and preventing potential state pollution between tests.
125-125: LGTM: Improved error handling test.The changes in the error handling test are well-implemented. Using
mockRejectedValueis the correct way to simulate a rejected promise in Jest. The added expectation ensures that theupdatemethod is called with the correct argument before throwing an error, which improves the test's thoroughness.Also applies to: 134-134
Line range hint
1-136: Overall, excellent improvements to the test suite.The changes in this file align well with the PR objectives of adding timestamp fields (created_at, updated_at) to all tables. The test suite has been significantly improved with:
- Better type checking for mocked repositories.
- Consistent use of new timestamp fields in mock data.
- Additional test case for authentication errors.
- Improved error handling tests.
- Better test isolation with the addition of
afterEachcleanup.These changes enhance the overall quality and coverage of the
deactivateSHLinksUseCasetests. The only minor point for consideration is the use of a fixed future date fordateValue, which could be made more flexible for robust testing across different scenarios.Great job on improving the test suite!
prisma/migrations/20240927083507_/migration.sql (3)
13-23: LGTM! Clarify content_type default and nullable content_hash.The
shlink_filetable creation looks good and aligns with the PR objectives. The timestamp fields are correctly included.Please clarify the following:
- Is 'application/json' the most appropriate default for
content_type? Will most files be JSON, or should this be left NULL by default?- Why is
content_hashnullable? If it's meant to referencecas_item.hash, should it be required?Consider adding a table comment to explain the purpose of this table and its relationship to other tables.
55-65: LGTM! Clarify id/user_id redundancy and patient_id constraint.The
usertable creation looks good and aligns with the PR objectives. The timestamp fields are correctly included.Please clarify the following:
- Is there a need for both
idanduser_id? Ifuser_idis meant to be an external identifier, consider renaming it to clarify its purpose (e.g.,external_user_id).- Is
patient_idalways required? If not all users are patients, consider making this field nullable:- "patient_id" TEXT NOT NULL, + "patient_id" TEXT,Also, consider adding a table comment to explain the purpose of this table and its relationship to other tables.
97-105: LGTM! Clarify the purpose of access_ticket table.The
access_tickettable creation looks good and aligns with the PR objectives. The timestamp fields are correctly included.Please clarify the purpose of this table. It seems to be related to
shlink, but its exact function is not clear from the structure. Consider:
- Adding more fields if necessary to fully represent an "access ticket".
- Adding a table comment to explain its purpose and relationship to other tables.
- If it's meant to be a one-to-one relationship with
shlink, consider adding a unique constraint onshlink_id:ALTER TABLE "access_ticket" ADD CONSTRAINT "unique_access_ticket_shlink" UNIQUE ("shlink_id");This additional information will help in understanding the table's role in the overall schema.
| passcode: '1234', | ||
| }); | ||
|
|
||
| // Verify mapper and result | ||
| expect(mapEntityToModel).toHaveBeenCalledWith(updatedSHLinkEntity); | ||
| expect(result).toEqual(mockSHLinkModel); | ||
| expect(repo.update).toHaveBeenCalledWith(mockEntity); | ||
| expect(result).toBe(mockModel); | ||
| }); | ||
|
|
||
| it('should not update config_passcode or config_exp if they are not set in the data', async () => { | ||
| const mockSHLinkModelWithoutFields = new SHLinkModel( | ||
| mockSHLinkModel.getUserId(), | ||
| mockSHLinkModel.getName(), | ||
| mockSHLinkModel.getPasscodeFailuresRemaining(), | ||
| mockSHLinkModel.getActive(), | ||
| mockSHLinkModel.getManagementToken(), | ||
| undefined, // No configPasscode | ||
| undefined, // No configExp | ||
| '1', | ||
| ); | ||
|
|
||
| const entityWithoutConfig: SHLinkEntity = { | ||
| ...mockSHLinkEntity, | ||
| config_passcode: mockSHLinkEntity.config_passcode, // Should remain unchanged | ||
| config_exp: mockSHLinkEntity.config_exp, // Should remain unchanged | ||
| }; | ||
| it('should not set passcode or expiry date if they are not provided', async () => { | ||
| // Arrange | ||
| const data = { id: '1' }; // No passcode or expiry date | ||
| repo.findOne.mockResolvedValue(mockEntity); | ||
| repo.update.mockResolvedValue(mockEntity); | ||
|
|
||
| // Mock repository behavior | ||
| mockRepo.findOne.mockResolvedValue(mockSHLinkEntity); | ||
| mockRepo.update.mockResolvedValue(entityWithoutConfig); | ||
| (mapEntityToModel as jest.Mock).mockReturnValue( | ||
| mockSHLinkModelWithoutFields, | ||
| ); | ||
|
|
||
| // Call the use case | ||
| // Act | ||
| const result = await updateSingleSHLinkUseCase( | ||
| { repo: mockRepo, validator: jest.fn().mockResolvedValue(true) }, // Mock validator | ||
| { id: '1' }, // No passcode or expiryDate provided | ||
| { repo, validator: mockValidator }, | ||
| data | ||
| ); | ||
|
|
||
| // Verify repository interactions | ||
| expect(mockRepo.findOne).toHaveBeenCalledWith({ id: '1' }); | ||
| expect(mockRepo.update).toHaveBeenCalledWith(entityWithoutConfig); | ||
|
|
||
| // Verify result | ||
| expect(result).toEqual(mockSHLinkModelWithoutFields); | ||
| // Assert | ||
| expect(repo.findOne).toHaveBeenCalledWith({ id: data.id }); | ||
| expect(mockModel.setConfigPasscode).not.toHaveBeenCalled(); | ||
| expect(mockModel.setConfigExp).not.toHaveBeenCalled(); | ||
| expect(mockValidator).toHaveBeenCalledWith({ | ||
| shlink: mockModel, | ||
| passcode: '1234', | ||
| }); | ||
| expect(repo.update).toHaveBeenCalledWith(mockEntity); | ||
| expect(result).toBe(mockModel); | ||
| }); | ||
|
|
||
| it('should call validator before updating', async () => { | ||
| const mockValidator = jest.fn().mockResolvedValue(true); | ||
|
|
||
| // Mock repository behavior | ||
| mockRepo.findOne.mockResolvedValue(mockSHLinkEntity); | ||
| mockRepo.update.mockResolvedValue(mockSHLinkEntity); | ||
| (mapEntityToModel as jest.Mock).mockReturnValue(mockSHLinkModel); | ||
|
|
||
| // Call the use case | ||
| await updateSingleSHLinkUseCase( | ||
| { repo: mockRepo, validator: mockValidator }, | ||
| { id: '1', passcode: 'new-passcode' }, | ||
| ); | ||
|
|
||
| // Verify validator call | ||
| expect(mockValidator).toHaveBeenCalledWith({ | ||
| shlink: mockSHLinkModel, | ||
| passcode: 'new-passcode', | ||
| it('should throw an error if the validator fails', async () => { | ||
| // Arrange | ||
| const data = { id: '1', passcode: 'invalidPasscode' }; | ||
| repo.findOne.mockResolvedValue(mockEntity); | ||
| mockValidator.mockImplementation(() => { | ||
| throw new Error('Validation failed'); | ||
| }); | ||
|
|
||
| // Act & Assert | ||
| await expect( | ||
| updateSingleSHLinkUseCase( | ||
| { repo, validator: mockValidator }, | ||
| data | ||
| ) | ||
| ).rejects.toThrow('Validation failed'); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Suggestion: Consider adding more test cases for improved coverage
The current test suite covers the main scenarios well. To further improve the test coverage and robustness of the updateSingleSHLinkUseCase, consider adding the following test cases:
- Test updating only the passcode (without expiry date).
- Test updating only the expiry date (without passcode).
- Test with an invalid ID (repo.findOne returns null).
- Test with an inactive SHLink (if applicable to your use case).
Here's an example of how you might implement the first suggested test case:
it('should update only the passcode when expiry date is not provided', async () => {
// Arrange
const data = { id: '1', passcode: 'newPasscode' };
repo.findOne.mockResolvedValue(mockEntity);
repo.update.mockResolvedValue(mockEntity);
// Act
const result = await updateSingleSHLinkUseCase(
{ repo, validator: mockValidator },
data
);
// Assert
expect(repo.findOne).toHaveBeenCalledWith({ id: data.id });
expect(mockModel.setConfigPasscode).toHaveBeenCalledWith(data.passcode);
expect(mockModel.setConfigExp).not.toHaveBeenCalled();
expect(mockValidator).toHaveBeenCalledWith({
shlink: mockModel,
passcode: '1234',
});
expect(repo.update).toHaveBeenCalledWith(mockEntity);
expect(result).toBe(mockModel);
});Adding these test cases will provide more comprehensive coverage of the use case's behavior under different scenarios.
| -- CreateTable | ||
| CREATE TABLE "shlink" ( | ||
| "id" VARCHAR(43) NOT NULL, | ||
| "created_at" TIMESTAMP(3) NOT NULL DEFAULT CURRENT_TIMESTAMP, | ||
| "updated_at" TIMESTAMP(3) NOT NULL DEFAULT CURRENT_TIMESTAMP, | ||
| "deleted_at" TIMESTAMP(3), | ||
| "passcode_failures_remaining" INTEGER NOT NULL DEFAULT 5, | ||
| "config_passcode" TEXT, | ||
| "config_passcode_hash" TEXT, | ||
| "config_exp" TIMESTAMP(3), | ||
| "active" BOOLEAN NOT NULL DEFAULT true, | ||
| "management_token" VARCHAR(43) NOT NULL, | ||
| "user_id" VARCHAR(43) NOT NULL, | ||
| "name" TEXT NOT NULL DEFAULT 'nameless shlink', | ||
|
|
||
| CONSTRAINT "shlink_pkey" PRIMARY KEY ("id") | ||
| ); |
There was a problem hiding this comment.
LGTM! Address security concern with config_passcode storage.
The shlink table creation looks good and aligns with the PR objectives. The timestamp fields, including deleted_at for soft delete, are correctly implemented.
There's a potential security risk with storing both config_passcode and config_passcode_hash. Storing passwords in plain text is generally discouraged. Consider removing the config_passcode field and only keeping the hashed version:
- "config_passcode" TEXT,
"config_passcode_hash" TEXT,This change enhances security by ensuring that plain text passwords are not stored in the database.
Also, consider adding a table comment to explain the purpose of this table and its relationship to other tables.
📝 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.
| -- CreateTable | |
| CREATE TABLE "shlink" ( | |
| "id" VARCHAR(43) NOT NULL, | |
| "created_at" TIMESTAMP(3) NOT NULL DEFAULT CURRENT_TIMESTAMP, | |
| "updated_at" TIMESTAMP(3) NOT NULL DEFAULT CURRENT_TIMESTAMP, | |
| "deleted_at" TIMESTAMP(3), | |
| "passcode_failures_remaining" INTEGER NOT NULL DEFAULT 5, | |
| "config_passcode" TEXT, | |
| "config_passcode_hash" TEXT, | |
| "config_exp" TIMESTAMP(3), | |
| "active" BOOLEAN NOT NULL DEFAULT true, | |
| "management_token" VARCHAR(43) NOT NULL, | |
| "user_id" VARCHAR(43) NOT NULL, | |
| "name" TEXT NOT NULL DEFAULT 'nameless shlink', | |
| CONSTRAINT "shlink_pkey" PRIMARY KEY ("id") | |
| ); | |
| -- CreateTable | |
| CREATE TABLE "shlink" ( | |
| "id" VARCHAR(43) NOT NULL, | |
| "created_at" TIMESTAMP(3) NOT NULL DEFAULT CURRENT_TIMESTAMP, | |
| "updated_at" TIMESTAMP(3) NOT NULL DEFAULT CURRENT_TIMESTAMP, | |
| "deleted_at" TIMESTAMP(3), | |
| "passcode_failures_remaining" INTEGER NOT NULL DEFAULT 5, | |
| "config_passcode_hash" TEXT, | |
| "config_exp" TIMESTAMP(3), | |
| "active" BOOLEAN NOT NULL DEFAULT true, | |
| "management_token" VARCHAR(43) NOT NULL, | |
| "user_id" VARCHAR(43) NOT NULL, | |
| "name" TEXT NOT NULL DEFAULT 'nameless shlink', | |
| CONSTRAINT "shlink_pkey" PRIMARY KEY ("id") | |
| ); | |
| -- Add a comment to explain the purpose of the table | |
| COMMENT ON TABLE "shlink" IS 'Stores information about shortened links and their configurations'; |
| -- CreateIndex | ||
| CREATE UNIQUE INDEX "cas_item_id_key" ON "cas_item"("id"); | ||
|
|
||
| -- CreateIndex | ||
| CREATE UNIQUE INDEX "cas_item_hash_key" ON "cas_item"("hash"); | ||
|
|
||
| -- CreateIndex | ||
| CREATE UNIQUE INDEX "cas_item_content_key" ON "cas_item"("content"); | ||
|
|
||
| -- CreateIndex | ||
| CREATE INDEX "idx_cas_item_hash" ON "cas_item"("id"); | ||
|
|
||
| -- CreateIndex | ||
| CREATE UNIQUE INDEX "shlink_file_id_key" ON "shlink_file"("id"); | ||
|
|
||
| -- CreateIndex | ||
| CREATE INDEX "idx_shlink_file_shlinkId" ON "shlink_file"("shlink_id"); | ||
|
|
||
| -- CreateIndex | ||
| CREATE INDEX "idx_shlink_file_content_hash" ON "shlink_file"("content_hash"); | ||
|
|
||
| -- CreateIndex | ||
| CREATE INDEX "idx_shlink_access_shlinkId" ON "shlink_access"("shlink_id"); | ||
|
|
||
| -- CreateIndex | ||
| CREATE UNIQUE INDEX "shlink_id_key" ON "shlink"("id"); | ||
|
|
||
| -- CreateIndex | ||
| CREATE UNIQUE INDEX "shlink_management_token_key" ON "shlink"("management_token"); | ||
|
|
||
| -- CreateIndex | ||
| CREATE INDEX "idx_shlink_id" ON "shlink"("id"); | ||
|
|
||
| -- CreateIndex | ||
| CREATE UNIQUE INDEX "user_user_id_key" ON "user"("user_id"); | ||
|
|
||
| -- CreateIndex | ||
| CREATE INDEX "idx_user_id" ON "user"("id"); | ||
|
|
||
| -- CreateIndex | ||
| CREATE UNIQUE INDEX "server_config_id_key" ON "server_config"("id"); | ||
|
|
||
| -- CreateIndex | ||
| CREATE INDEX "idx_server_config_id" ON "server_config"("id"); | ||
|
|
||
| -- CreateIndex | ||
| CREATE UNIQUE INDEX "shlink_endpoint_id_key" ON "shlink_endpoint"("id"); | ||
|
|
||
| -- CreateIndex | ||
| CREATE INDEX "idx_shlink_endpoint_shlinkId" ON "shlink_endpoint"("shlink_id"); | ||
|
|
||
| -- CreateIndex | ||
| CREATE INDEX "idx_shlink_endpoint_serverConfigID" ON "shlink_endpoint"("server_config_id"); | ||
|
|
||
| -- CreateIndex | ||
| CREATE INDEX "idx_access_ticket_id" ON "access_ticket"("id"); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Review and optimize indexing strategy.
The index creations generally look good, but there are some potential optimizations:
-
Remove redundant indexes on primary key columns:
cas_item_id_key(line 108)shlink_file_id_key(line 120)shlink_id_key(line 132)server_config_id_key(line 147)shlink_endpoint_id_key(line 153)idx_access_ticket_id(line 162)
Primary key columns are automatically indexed by the database.
-
Consider adding a composite index on
shlink_access(shlink_id, access_time)if you frequently query access records for a specific shlink within a time range. -
Evaluate the need for
idx_cas_item_hash(line 117). It seems to be indexing theidcolumn instead ofhash. If it's meant to indexhash, correct it:
-CREATE INDEX "idx_cas_item_hash" ON "cas_item"("id");
+CREATE INDEX "idx_cas_item_hash" ON "cas_item"("hash");- Consider adding an index on
server_config(endpoint_url)if you frequently look up configurations by URL.
Review these suggestions and adjust based on your specific query patterns and performance requirements.
Committable suggestion was skipped due to low confidence.
| -- CreateTable | ||
| CREATE TABLE "server_config" ( | ||
| "id" VARCHAR(43) NOT NULL, | ||
| "created_at" TIMESTAMP(3) NOT NULL DEFAULT CURRENT_TIMESTAMP, | ||
| "updated_at" TIMESTAMP(3) NOT NULL DEFAULT CURRENT_TIMESTAMP, | ||
| "endpoint_url" TEXT NOT NULL, | ||
| "config_key" VARCHAR(43), | ||
| "config_client_id" TEXT, | ||
| "config_client_secret" TEXT, | ||
| "config_client_secret_hash" TEXT, | ||
| "config_token_endpoint" TEXT, | ||
| "config_refresh_token" TEXT, | ||
| "refresh_time" TIMESTAMP(3) NOT NULL DEFAULT CURRENT_TIMESTAMP, | ||
| "access_token_response" TEXT NOT NULL, | ||
|
|
||
| CONSTRAINT "server_config_pkey" PRIMARY KEY ("id") | ||
| ); |
There was a problem hiding this comment.
LGTM! Address security concerns and improve default values.
The server_config table creation looks good and aligns with the PR objectives. The timestamp fields are correctly included.
- There's a potential security risk with storing both
config_client_secretandconfig_client_secret_hash. Consider removing theconfig_client_secretfield and only keeping the hashed version:
- "config_client_secret" TEXT,
"config_client_secret_hash" TEXT,- The default value for
refresh_timemight not be appropriate. Consider removing the default or setting it to a future time:
- "refresh_time" TIMESTAMP(3) NOT NULL DEFAULT CURRENT_TIMESTAMP,
+ "refresh_time" TIMESTAMP(3) NOT NULL,- Consider encrypting sensitive fields like
config_refresh_tokenandaccess_token_response.
Also, add a table comment to explain the purpose of this table and its relationship to other tables.
Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
prisma/migrations/20240927144350_test/migration.sql (1)
9-31: LGTM: Consistent timestamp columns added to all remaining tablesThe addition of
created_atandupdated_atcolumns is correctly implemented and consistent across all remaining tables (server_config, shlink, shlink_access, shlink_endpoint, shlink_file, and user). This change aligns well with the PR objectives and will improve record tracking capabilities throughout the database.For future migrations involving repetitive changes across multiple tables, consider using a more programmatic approach or a migration tool that supports looping or templating. This could make the migration script more concise and easier to maintain, especially if more tables need to be modified in the future.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- prisma/migrations/20240912115114_initial/migration.sql (1 hunks)
- prisma/migrations/20240927144350_test/migration.sql (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- prisma/migrations/20240912115114_initial/migration.sql
🔇 Additional comments (3)
prisma/migrations/20240927144350_test/migration.sql (3)
1-3: LGTM: Appropriate timestamp columns added to "access_ticket" tableThe addition of
created_atandupdated_atcolumns to the "access_ticket" table is well-implemented. These columns will effectively track the creation and last modification times of each record, enhancing data management capabilities.
5-7: LGTM: Timestamp columns correctly added to "cas_item" tableThe addition of
created_atandupdated_atcolumns to the "cas_item" table is correctly implemented, maintaining consistency with the previous table modifications.
1-31: Consider adding the "deletedAt" column as mentioned in the PR objectivesThe migration script successfully adds
created_atandupdated_atcolumns to all specified tables, which aligns with part of the PR objectives. However, the PR summary also mentions adeletedAtfield, which is not included in this migration.If the
deletedAtcolumn is still part of the requirements, consider adding it to the migration script. This column would typically be defined as a nullable TIMESTAMP to indicate when a record was soft-deleted.Here's a shell script to verify if
deletedAtis used elsewhere in the project:If the
deletedAtcolumn is indeed required, you might want to add it to each table like this:ADD COLUMN "deleted_at" TIMESTAMP(3);Please clarify if the
deletedAtcolumn should be included in this migration or if it's planned for a separate migration.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
prisma/migrations/20240927150538_added_created_updated_dates_to_schema/migration.sql (3)
1-3: LGTM! Consider adding an ON UPDATE clause for theupdated_atcolumn.The addition of
created_atandupdated_atcolumns to theaccess_tickettable is well-implemented. The use ofTIMESTAMP(3)provides millisecond precision, which is suitable for most applications. Setting the columns asNOT NULLwith a default value ofCURRENT_TIMESTAMPensures data integrity for both existing and new rows.Consider adding an
ON UPDATE CURRENT_TIMESTAMPclause to theupdated_atcolumn. This would automatically update the column whenever a row is modified, ensuring thatupdated_atalways reflects the latest change without requiring manual updates in your application code.Example:
ADD COLUMN "updated_at" TIMESTAMP(3) NOT NULL DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP
5-7: LGTM! Consider the same improvement as suggested for theaccess_tickettable.The addition of
created_atandupdated_atcolumns to thecas_itemtable is consistent with the changes made to theaccess_tickettable and aligns with the PR objectives.As suggested for the
access_tickettable, consider adding anON UPDATE CURRENT_TIMESTAMPclause to theupdated_atcolumn for automatic updates.
9-31: LGTM! Consider theON UPDATEclause improvement for all remaining tables.The addition of
created_atandupdated_atcolumns is consistent across all remaining tables:server_config,shlink,shlink_access,shlink_endpoint,shlink_file, anduser. This consistency is excellent for maintainability and aligns well with the PR objectives.As suggested earlier, consider adding an
ON UPDATE CURRENT_TIMESTAMPclause to theupdated_atcolumn for all these tables. This would ensure automatic updates of theupdated_atcolumn whenever a row is modified, without requiring manual updates in your application code.Example for all remaining tables:
ADD COLUMN "updated_at" TIMESTAMP(3) NOT NULL DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP
prisma/migrations/20240927150538_added_created_updated_dates_to_schema/migration.sql
Show resolved
Hide resolved
Added createdAt, updatedAt and deletedAt fields to all tables
Summary by CodeRabbit
New Features
Bug Fixes
AuthenticationErroris thrown for unauthorized users attempting to activate a short link.Chores