-
Notifications
You must be signed in to change notification settings - Fork 8
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Get collection metadata blocks #141
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall it looks great, I have looked at the code and tested the use case while working on the issue IQSS/dataverse-frontend#338 and it works as expected.
I think at this point we should only change .env and thats it.
import { IMetadataBlocksRepository } from '../repositories/IMetadataBlocksRepository' | ||
|
||
export class GetCollectionMetadataBlocks implements UseCase<MetadataBlock[]> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To adhere to our naming conventions, should we consider renaming the method to GetMetadataBlocksByCollection
? Given that we are in the MetadataBlocks
domain, this name explicitly reflects that we're retrieving metadata blocks.
Additionally, this naming strategy enhances usability in IDEs. When developers start typing getMetadataBlocks
, the IDE can autocomplete with all related fetching methods like byName
, byCollection
, etc. This could streamline finding the right method compared to starting with getCollection
, which might not immediately suggest that it pertains to metadata blocks.
Alternatively, we could consider moving this use case to the collection domain. In that context, naming the method getCollectionMetadataBlocks
would be more semantically appropriate, as it clearly indicates that the method retrieves metadata blocks specific to collections.
QA police 🚨 😆
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the naming convention you describe, and makes sense to me.
In any case, the naming convention that we have mostly followed so far in the package is the one I have used. If you notice, it is used in almost all use cases that return a property "by a parameter".
For example, see: GetDatasetFiles, GetFileCitation, GetFileDataTables, etc.
To be consistent with the naming convention, we must rename all or none. We can create a separate issue for general renaming and make all use cases follow the same naming strategy (And mention this requirement in the dev guidelines). Let me know what you think. @MellyGray
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's fine for me to create a separate issue.
However, based on the use cases you describe, I don't see the same problem with GetFileCitation
and GetFileDataTables
. They are in the file domain, and as they are attributes of the file, they sound fine, I wouldn't change those.
In the case of GetDatasetFiles
, it might be more similar to the metadatablock case, because you are in the files domain and you want some files. However, you have to start typing getDataset
, which might make more sense as getFilesByDatasetId
or simply getFiles
, as the necessary datasetId could be specified through the method parameters.
In any case, I think it makes sense to choose a naming convention that aligns well with IDE autocomplete and to create a separate issue for that
test/environment/.env
Outdated
DATAVERSE_IMAGE_REGISTRY=ghcr.io | ||
DATAVERSE_IMAGE_TAG=10389-metadata-field-display-on-create-api-ext |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the Dataverse PR is already merged I think we can restore the .env variables
… into 135-collections-get-metadata-blocks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
@GPortas I leave to you the creation of the issue for the naming convention
What this PR does / why we need it:
Adds new use case to the package for retrieving the metadata blocks configured in a collection.
Extends the current MetadataBlock model to include the new displayOnCreate boolean flag and new enum types.
Refactored the ApiRepository class a bit, as it included collection-specific logic that should be placed in CollectionsRepository.
Which issue(s) this PR closes:
Related Dataverse PRs:
Special notes for your reviewer:
Suggestions on how to test this:
Visual inspection of the code and review GitHub actions.