-
-
Notifications
You must be signed in to change notification settings - Fork 489
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
User-feedback functionality #2419
Conversation
Moving this to 3.4.2 because I want to close today 3.4.1 to provide a stable version without some relevant bugs and this is a big change to be sure.... |
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.
Added some comments, for me looks fine.
Maybe would be good that also RegisteredUser profile comments require approval, not only anonymous comments.
|
||
Metadata metadata = metadataRepository.findOne(metadataId); | ||
|
||
List<UserFeedback> list = userfeedbackRepository.findByMetadata_Uuid(metadata.getUuid()); |
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.
Can't be added a method deleteByMetadataUuid
to UserFeedbackRepository
?
final UserFeedbackRepository userFeedbackRepository = ApplicationContextHolder.get() | ||
.getBean(UserFeedbackRepository.class); | ||
|
||
// TODO: Manage UserFeedback not found |
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.
Please check this.
@@ -5,5 +5,7 @@ INSERT INTO Settings (name, value, datatype, position, internal) VALUES | |||
|
|||
INSERT INTO Settings (name, value, datatype, position, internal) VALUES ('system/metadata/validation/removeSchemaLocation', 'false', 2, 9170, 'n'); | |||
|
|||
UPDATE Settings SET name = 'system/localrating/enable', datatype = 0 WHERE id = 211; |
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.
In the UI the values seem off
, basic
, advanced
, I think the migration script should handle to change the previous values false
, true
.
Also should be added to the proper migration file, 340 is no longer valid when the PR is merged.
…les that go with it
Working angular userfeedback module structure Userfeedback API mockup integration UserFeedback Model remove file
Model and first API integration
Initial implementation for UserFeedbackDatabaseService methods user feedback Dto mapper Save fix
- user data (name, email, organisation) - comment textarea - keyword area - accordion for three part of ratings modal (categories, free input, keywords)
Splitting directive in a modular version User Feedback Api wiring Full comment list GUI-API integration Doc and code fix double comment fix To make templates on homepage work
- buttons now beside each other - comments section mockup on the homepage: now with numbers, not with good/bad
Issue with logged user Fix code without model mapper Basic publish workflow Delete userfeedback on metadata delete General fixes and multilanguage support
Font size and color changes * font size of comment increased * title of dataset decreased * score card now has 3 colors (mot only green) * scores in sidebar have color coding Improved coloring for the scores Added whitespace around the author name. Added quotes around the comment.
898c42f
to
d54af4b
Compare
@josegar74 I updated according to your review. If it's fine for you, please, approve. I just updated with last code from 3.4.x. There's a lot of code impacted for this new feature and it's expensive to keep it updated. |
@PascalLike do you think we should have this in 3.4.2 ? or make the release first, then merge ? |
I would really like to see it merged in 3.4.2, so if it's fine for you please go ahead! |
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. I added some questions about the API to be discussed. Some of my remarks are fixed in fxprunayre@df09c84 so you can cherry-pick.
Maybe this should be enabled by default ?
* @throws Exception the exception | ||
*/ | ||
@ApiOperation(value = "Provides an average rating for a metadata record", nickname = "getMetadataUserComments") | ||
@RequestMapping(value = "/metadata/{uuid}/userfeedbackrating", produces = MediaType.APPLICATION_JSON_VALUE, method = RequestMethod.GET) |
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 would renamed /metadata by /records to be consistent with the API
@ResponseStatus(value = HttpStatus.OK) | ||
@ResponseBody | ||
public RatingAverage getMetadataRating(@PathVariable(value = "uuid") | ||
final String metadataUuid, final HttpServletRequest request, final HttpServletResponse response, final HttpSession httpSession) |
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.
|
||
final IUserFeedbackService userFeedbackService = getUserFeedbackService(); | ||
|
||
final String uuid = request.getParameter("target"); |
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.
target
and maxnumber
parameters are not part of the method signature and they are not displayed in the service doc
Use @RequestParam
annotation ?
Also "target" may be renamed to "record" maybe as it is a metadata record.
At some point we may need to add paging here - but this is not part of this PR - we should maybe plan for adding this for users/groups/... API.
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.
If target is mandatory, should we move that service to /records/{uuid}/userfeedback ? would be more consistent with userfeedbackrating
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.
target is not mandatory, so maybe not. or should we have both ?
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.
None of them is mandatory. I added them (uuid, previously target, and maxnumber) as not requested parameters
|
||
$http({ | ||
method: 'GET', | ||
url: '../api/userfeedback?full=true&uuid=' + scope.metatdataUUID, |
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 see this full parameter in the server side. What is it for ?
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.
No, it's not used anymore
Thanks for the review (and the commit). Please let me know if it's ok now |
console.log('DELETED ' + id); | ||
|
||
$http.delete('../api/userfeedback/' + id).success(function(data, status) { | ||
console.log(data); |
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.
Do we need all those console.log ?
|
||
}; | ||
|
||
scope, showPopover = function(info) { |
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 odd and useless no ?
|
||
}; | ||
|
||
function validateEmail(email) { |
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.
This function is never used no ? Email check is done in line 345 ?
|
||
$http.post('../api/userfeedback', data).success(function(data, status) { | ||
console.log(data); | ||
$window.location.reload(); |
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.
When user post the feedback, the page is fully reload - we don't need that. Only refresh the list of feedback no ?
<input type="hidden" data-ng-model="uf.ratingAVG"> | ||
<button type="button" class="btn btn-default" data-dismiss="modal" | ||
data-translate="">GUFclose</button> | ||
<button type="button" class="btn btn-primary" |
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.
Would be good to add recaptcha to avoid automatic submission of the form ?
@@ -172,6 +172,12 @@ <h4> | |||
<div data-gn-info-list=""></div> | |||
</form> | |||
</tab> | |||
|
|||
<tab heading="Comments" active="infoTabs.commentsalt.active" data-ng-show="isUserFeedbackEnabled"> |
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.
Not translated
- / - <span class="fa fa-fw fa-star"> </span> | ||
</div> | ||
<div class="gn-userfeedback-reply"> | ||
<a>{{row.authorName}}</a><br>on {{row.date | date:'medium'}} |
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.
"GUFsave": "Save", | ||
"GUFfeedbackOn": "Feedback on:", | ||
"GUFnoComments": "No comments", | ||
"GUFaddRating": "Review this dataset", |
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.
"GUFfeedbackOn": "Feedback on:", | ||
"GUFnoComments": "No comments", | ||
"GUFaddRating": "Review this dataset", | ||
"GUFplusAddRating": "<span class=\"fa fa-plus\"></span> Review this dataset", |
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.
We usually put the HTML in templates not in translation to keep translation as easy as possible
Improve API doc. API / Add /records/uuid/userfeedback Add nbOfComments to load in directive Latest comments / remove link on user Latest comments / Add nb of comments parameter Author / Same layout in home page and other location Do not show no rating while loading Remove HTML from translation
Done. I really like this last improvements, now it looks better. Thank you |
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.
Will do live testing tomorrow but looks good. Nice improvement.
@PascalLike , We also need to index this depending on the settings in here https://github.com/geonetwork/core-geonetwork/blob/3.4.x/core/src/main/java/org/fao/geonet/kernel/DataManager.java#L683. Currently only the basic rating is displayed whatever the config. So this is misleading and the stars in search results does not represent the average of rating. eg. no start displayed on a record with one user feedback Do you have time to cover that ? |
String metadataTitle; | ||
try { | ||
final Element element = input.getMetadata().getXmlData(false); | ||
final XPath xpath = XPath.newInstance("gmd:identificationInfo/*/gmd:citation/gmd:CI_Citation/gmd:title/gco:CharacterString"); |
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.
This will not work for records not in ISO19139.
|
||
/** The date. */ | ||
@Column | ||
private Date date; |
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.
Would have been better to use same encoding for date as other classes ie. ISODate
Starting with https://github.com/geonetwork/core-geonetwork/wiki/Userfeedback-proposal
we created a functionality for user feedback on metadata records.
The changes are implemented starting by 4 angular directives:
The creation of user feedbacks is allowed for every user. For feedbacks from not signed in users a basic workflow will be applied (the comment will be hidden to non-reviewers until the reviewer approval).
gnUserfeedbacklasthome is added on the home page.
gnUserfeedback, gnUserfeedbackfull and gnUserfeedbacknew are integrated in the record view.
The GN API now contains new methods for UF (api/{version/}userfeedback/...): Creating, deleting, publishing and listing. Some methods returns statistic used by the different views implemented in the directives.
The feedbacks are stored in new tables in gn's database.
The functionality is disabled by default but can be activated in settings page. Is very similar to the rating functionality, so the setting for local rating now supports these 3 options:
off - no rating
basic - the current 5 star rating
advanced - the new user feedback functionality
There is another function called user feedback and it's a bit ambiguos. The official documentation should be updated.
This software has been improved under the ELISE action, which is supported by the ISA² Programme. ISA² is a EUR 131 million programme of the European Commission, supporting the modernisation of public administrations in Europe through the development of eGovernment solutions. More than 20 solutions are already available, with more to come soon. All solutions are open source and available free of charge to any interested public administration in Europe.