GET and DELETE a template Use Case and Restructure#906
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request restructures template-related functionality by creating a dedicated /templates folder with new use cases for template management (getTemplate, deleteTemplate, createTemplate) and moving the existing getTemplatesByCollectionId functionality. The changes update the codebase to use a new TemplateRepository interface and rename DatasetTemplate to Template for better alignment with API conventions.
Changes:
- Introduced new template use cases: getTemplate, deleteTemplate, and createTemplate in a dedicated templates folder
- Renamed DatasetTemplate interface to Template and moved template-related logic from DatasetRepository to TemplateRepository
- Updated package dependency to @iqss/dataverse-client-javascript version 2.1.0-pr406.31ac368 to support new template APIs
Reviewed changes
Copilot reviewed 28 out of 29 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/templates/domain/useCases/*.ts | New use case functions for template operations (get, delete, create) |
| src/templates/domain/repositories/TemplateRepository.ts | New repository interface for template operations |
| src/templates/infrastructure/repositories/TemplateJSDataverseRepository.ts | Implementation of TemplateRepository using js-dataverse client |
| src/templates/domain/models/Template.ts | Duplicate Template interface definition (also in DatasetTemplate.ts) |
| src/dataset/domain/models/DatasetTemplate.ts | Renamed DatasetTemplate to Template interface |
| src/dataset/domain/useCases/getTemplatesByCollectionId.ts | Moved use case that should be in templates folder |
| src/dataset/domain/repositories/DatasetRepository.ts | Removed getTemplates method |
| src/sections/create-dataset/*.tsx | Updated to use TemplateRepository and renamed interfaces |
| tests/**/*.spec.tsx | Updated test files to use TemplateRepository and new naming |
| package.json | Updated js-dataverse dependency to PR build version |
| CHANGELOG.md | Added entry for template refactoring |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "@dnd-kit/utilities": "3.2.2", | ||
| "@faker-js/faker": "7.6.0", | ||
| "@iqss/dataverse-client-javascript": "2.0.0-alpha.79", | ||
| "@iqss/dataverse-client-javascript": "2.1.0-pr406.31ac368", |
There was a problem hiding this comment.
The package version "2.1.0-pr406.31ac368" appears to be a pre-release PR build rather than a stable version. PR builds are typically used for testing and should not be committed to the main branch. According to the PR description, package.json was intended to be updated once the js-dataverse gets a new alpha version with template APIs. Consider updating this to a stable or alpha release version (e.g., "2.1.0-alpha.80" or similar) before merging, or ensure the corresponding PR in js-dataverse is merged first.
| "@iqss/dataverse-client-javascript": "2.1.0-pr406.31ac368", | |
| "@iqss/dataverse-client-javascript": "2.1.0-alpha.80", |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 30 out of 31 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (3)
src/dataset/domain/hooks/useGetTemplatesByCollectionId.ts:12
- The interface name uses lowercase "useGetTemplatesByCollectionIdProps" but TypeScript convention typically uses PascalCase for interface names. Consider renaming to "UseGetTemplatesByCollectionIdProps" for consistency with TypeScript naming conventions.
src/dataset/domain/hooks/useGetTemplatesByCollectionId.ts:62 - The hook still uses "dataset" prefixes in variable and function names (datasetTemplates, isLoadingDatasetTemplates, errorGetDatasetTemplates, fetchDatasetTemplates) despite the refactoring to a generic Template model. Consider renaming these to use "template" prefixes instead (templates, isLoadingTemplates, errorGetTemplates, fetchTemplates) for better consistency with the new architecture where templates are decoupled from datasets.
src/dataset/domain/hooks/useGetTemplatesByCollectionId.ts:18 - The hook is located in the dataset domain folder but now deals with templates from a separate domain. Consider moving this hook to a templates/domain/hooks folder to better align with domain-driven design principles and the separation of concerns established by the new templates domain.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Copilot suggested adding tests for the use cases, which I chose not to do at that time, since I would add tests once the use cases were actually in use. |
ekraffmiller
left a comment
There was a problem hiding this comment.
Looks good! Just need to change the js-dataverse version, now that dependent PR has been merged
What this PR does / why we need it:
Which issue(s) this PR closes:
Special notes for your reviewer:
package.jsonneeds to be updated once the js-dataverse gets new alpha version with template apisSuggestions on how to test this:
Does this PR introduce a user interface change? If mockups are available, please link/include them here:
no ui change
Is there a release notes or changelog update needed for this change?:
Additional documentation: