-
Notifications
You must be signed in to change notification settings - Fork 495
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
add new feedback api #11162
base: develop
Are you sure you want to change the base?
add new feedback api #11162
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
JsonNumber jsonNumber = jsonObject.getJsonNumber("targetId"); | ||
DvObject feedbackTarget = null; | ||
if (jsonNumber != null) { | ||
feedbackTarget = dvObjSvc.findDvObject(jsonNumber.longValue()); |
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.
Probably out of scope, but how hard would it be to support finding Dataverses by alias and datasets/files by persistent id?
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.
Probably not hard but it would require a documentation change that makes the api under /admin different from this new one. Would it be more confusing to have the json files differ between the 2 APIs?
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'll make the change
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.
Done
@POST | ||
@AuthRequired | ||
@Consumes("application/json") | ||
public Response submitFeedback(@Context ContainerRequestContext crc, JsonObject jsonObject) { |
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 think it might be helpful to get the json as a string and then parse it within the try so that you can give the user better feedback. I tried it with invalid json and I get a 500 error and an empty response.
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.
good idea
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 made the change to pass in a string and validate it
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
} | ||
|
||
// feedbackTarget and idtf are both null this is a support feedback and is ok | ||
if (feedbackTarget == null && idtf != null) { |
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 think we need to say && (idtf != null || jsonNumber != null) because now if the jsonNumber doesn't return a dvObject it will go to general support, which we probably don't want.
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.
Good catch. the idtf used to have a string version of the jsonNumber but now it doesn't
// idtf will hold the "targetId" or the "identifier". If neither is set then this is a general feedback to support
String idtf = jsonNumber != null ? jsonNumber.toString() : jsonObject.containsKey("identifier") ? jsonObject.getString("identifier") : null;
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.
Just one more small logic change (when there's no feedback target) and I think we're good to go.
📦 Pushed preview images as
🚢 See on GHCR. Use by referencing with full name as printed above, mind the registry name. |
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.
OK. Looks good. Thanks for the updates.
What this PR does / why we need it: In SPA, users are allowed to send feedback to contacts of a collection, dataset or file.
Input would include subject, message, from (email autocomplete if login, or filled out by guest users).
In Native API, it has a similar API endpoint sending feedback to contact, but it is under admin path, and it returns contacts' email that could be sensitive. Send Feedback To Contact(s)
Which issue(s) this PR closes: #11129
Special notes for your reviewer:
Suggestions on how to test this:
Does this PR introduce a user interface change? If mockups are available, please link/include them here:
Is there a release notes update needed for this change?: Included
Additional documentation: config and native-api additions