-
Notifications
You must be signed in to change notification settings - Fork 10
Implement Use Case for dataset download count #274
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
Conversation
ekraffmiller
left a comment
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.
Looks good! I just have some comments on allowing datasetId to be number | string and small text changes.
Co-authored-by: Ellen Kraffmiller <[email protected]>
…hub.com/IQSS/dataverse-client-javascript into 269-get-dataset-download-count-use-case
|
need an approval before this gets merged. |
|
@ekraffmiller hi ellen, the datasetid changes applied. please check again:) Thanks |
ekraffmiller
left a comment
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.
question about the change to docker-compose.yml
test/environment/docker-compose.yml
Outdated
| -Ddataverse.files.localstack1.access-key=default | ||
| -Ddataverse.files.localstack1.secret-key=default | ||
| ports: | ||
| - '8081:8080' |
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.
was this change just for your local environment? Can it be reverted?
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 forgot remove it, sorry. fixing now
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 seems like I reverted back to '8080:8080', pushed yesterday. I had a few commits yesterday, the latest one did this revert
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.
Sorry @ChengShi-1, my mistake! Approving now
ekraffmiller
left a comment
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.
looks good, approving
|
merging PR, tests are passing. |
What this PR does / why we need it:
Which issue(s) this PR closes:
Related Dataverse PRs:
Special notes for your reviewer:
Suggestions on how to test this:
Inspect code and run tests
Is there a release notes update needed for this change?:
Additional documentation: