Flags: Implement Labels, bugfixes and improvements #971
Conversation
Bugfixes: - Toxic comment flag was not added - GraphQL warnings for missing fields
graph/loaders/comments.js
Outdated
| query = query.where({id: {$in: ids}}); | ||
| if ( | ||
| (context.user != null && context.user.can(SEARCH_NON_NULL_OR_ACCEPTED_COMMENTS)) || | ||
| statuses && statuses.every((status) => ['NONE', 'ACCEPTED'].includes(status)) |
There was a problem hiding this comment.
If statuses is null/undefined, no comments will be returned. Is that desired?
There was a problem hiding this comment.
It'll return all comments if permissions are there, look inside the if clause
There was a problem hiding this comment.
For these if statements, if you can write a short comment above that describes how/why we're doing this. It'll make it super clear.
graph/mutators/comment.js
Outdated
| * @param {String} [asset_id] asset for the comment | ||
| * @param {Object} [wordlist={}] the results of the wordlist scan | ||
| * @return {Promise} resolves to the comment's status | ||
| * @return {Object} resolves to the comment's status and actions |
There was a problem hiding this comment.
Every async function returns a Promise
|
|
||
| // Then we actually create the comment with the new status. | ||
| comment = await createComment(context, comment); | ||
| const result = await createComment(context, comment); |
There was a problem hiding this comment.
Why can't we just assign this to the original comment variable? We don't use the old one here, and truly, it's realized the new comment here instead.
There was a problem hiding this comment.
I think I added this because I used to have more changes here.
But I think it is good to have a different variable here.
Because comment in my mind was the input and
result is the created comment. Different concepts and as such I would assign them to different variables just to make it more clear.
graph/resolvers/asset.js
Outdated
|
|
||
| // Include the asset id in the search. | ||
| query.asset_id = id; | ||
| query.statuses = ['NONE', 'ACCEPTED']; |
There was a problem hiding this comment.
Should not be applied at the resolver level, instead should apply at the schema level. This prevents any future query to support a statuses field, which currently, the graph schema says it does allow.
There was a problem hiding this comment.
Mixed this up with the RepliesQuery as that one does not allow specifying statuses.
|
|
||
| query.asset_id = asset_id; | ||
| query.parent_id = id; | ||
| query.statuses = ['NONE', 'ACCEPTED']; |
There was a problem hiding this comment.
Reply query currently does not allow specifying statuses.
graph/resolvers/root_mutation.js
Outdated
| const actions = await Actions.getByID.load(comment.id); | ||
| const flags = actions.filter(({action_type}) => action_type === 'FLAG'); | ||
|
|
||
| return {comment, flags}; |
There was a problem hiding this comment.
Too specific, this should instead be actions, don't filter out the flags.
There was a problem hiding this comment.
You said something about filter in the call, so I was thinking what possibly could I filter and ended up filtering out the flags 😅
graph/typeDefs.graphql
Outdated
| comment: Comment | ||
|
|
||
| # Flags that was assigned during creation of the comment. | ||
| flags: [FlagAction] |
There was a problem hiding this comment.
See above, should return actions: [Action] instead.
What does this PR do?
adminCommentLabelsallcomments queue anduserDetailalltab not showing all comments.loadMorefeature not considering tags.CreateCommentMutationHow do I test this PR?
allmoderation tab containsSYSTEM_WITHHELDcomments