-
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
implement use case for sending information to contacts #232
base: develop
Are you sure you want to change the base?
implement use case for sending information to contacts #232
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.
The design, layering, and approach are correct, but we should use a different API endpoint for the reasons I mentioned in one of my comments. If there isn't an alternative endpoint in the API, we will need to implement one.
}) | ||
|
||
/* ... */ | ||
``` |
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.
Typically, we add the phrase 'See use case definition.' right below the code. Please take a look at other examples. Additionally, it would be helpful to provide a high-level description of the DTO or at least mention the type of identifier (targetId) it handles (numeric database identifier vs persistent identifiers). I assume it uses the latter, since it is numeric and is the common one for all DV objects, but it would be good to clarify.
On the other hand, the syntax in the block is incorrect. You need to first create an instance of the test DTO and then pass it as a parameter. (contact: ContactDTO) is how the parameter is defined in the method, not how it is invoked.
|
||
export class ContactRepository extends ApiRepository implements IContactRepository { | ||
public async submitContactInfo(contactDTO: ContactDTO): Promise<Contact[]> { | ||
return this.doPost(`/admin/feedback`, contactDTO) |
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 don't think we should use this API endpoint. It's an endpoint under the 'admin' basepath. These calls are restricted to administrators only, and AFAIK, they are limited to calls within the same host (they would fail if the consumer and the backend are not on the same host). Additionally, the call returns data that could be sensitive, such as the recipient's email address. Also, please take a look at the related comment in the endpoint definition https://github.com/IQSS/dataverse/blob/develop/src/main/java/edu/harvard/iq/dataverse/api/FeedbackApi.java#L33.
What this PR does / why we need it:
Implement the use case allowing users to send feedback to contacts of collection or dataset.
The use case should:
Which issue(s) this PR closes:
Related Dataverse PRs:
Special notes for your reviewer:
Suggestions on how to test this:
visually inspect and run tests on it
Is there a release notes update needed for this change?:
Additional documentation: