-
Notifications
You must be signed in to change notification settings - Fork 355
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
Flag username #133
Flag username #133
Conversation
@@ -272,8 +274,8 @@ const mapDispatchToProps = (dispatch) => ({ | |||
getStream: (rootId) => dispatch(getStream(rootId)), | |||
addNotification: (type, text) => dispatch(addNotification(type, text)), | |||
clearNotification: () => dispatch(clearNotification()), | |||
postAction: (item, action, itemType, field, detail) => dispatch(postAction(item, action, itemType, field, detail)), |
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.
At this point we should use an object instead of positional arguments
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'm using onClickOutside or something like that in Permalink if you want to check it out @davidgljay
@@ -217,11 +217,12 @@ export function postItem (item, type, id) { | |||
* | |||
*/ | |||
|
|||
export function postAction (item_id, action_type, user_id, item_type) { | |||
export function postAction (item_id, action_type, item_type, field, detail) { |
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.
same as above, too many positional params
header: lang.t('step-1-header'), | ||
options: [ | ||
{val: 'user', text: lang.t('flag-username')}, | ||
{val: 'comments', text: lang.t('flag-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.
remove extra comma
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 there are extra commas in every array, if this is intended that's ok
@@ -35,12 +37,14 @@ ActionSchema.statics.findById = function(id) { | |||
* @param {String} action the new action to the comment | |||
* @return {Promise} | |||
*/ | |||
ActionSchema.statics.insertUserAction = ({item_id, item_type, user_id, action_type}) => { | |||
ActionSchema.statics.insertUserAction = ({item_id, item_type, user_id, action_type, field, detail}) => { |
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.
Too many params
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 alternative is to pass in a whole object, which seems messy (and frought, see @wyatt's comments below.) I'm leaving this as is.
Comment | ||
.addAction(req.params.comment_id, req.user.id, action_type) | ||
.addAction(req.params.comment_id, req.user.id, req.body) |
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 should extract only the parameters we need from the body, not the whole one, this would result in anyone being able to stuff whatever they want in our db.
item_id, | ||
item_type: 'comment', | ||
user_id, | ||
action_type | ||
action |
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 style (hiding fields inside this new action
value) makes this difficult to read.
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.
Agreed, @dan requested this change. I can revert.
item_id, | ||
item_type: 'comment', | ||
user_id, | ||
action |
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.
Same as above.
router.post('/:user_id/actions', authorization.needed(), (req, res, next) => { | ||
|
||
User | ||
.addAction(req.params.comment_id, req.user.id, req.body) |
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.
Same as above, we shouldn't just pull the entire body.
…n the backend." This reverts commit 9d28577.
I still can't get rid of the popup |
Functionality is there :) |
@@ -158,9 +158,15 @@ router.put('/:user_id/bio', (req, res, next) => { | |||
}); | |||
|
|||
router.post('/:user_id/actions', authorization.needed(), (req, res, next) => { | |||
console.log('Hit action endpoint'); |
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.
Seems this one snuck through
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.
Oops! My apologies.
@@ -350,7 +350,7 @@ describe('/api/v1/comments/:comment_id/actions', () => { | |||
return chai.request(app) | |||
.post('/api/v1/comments/abc/actions') | |||
.set(passport.inject({id: '456', roles: ['admin']})) | |||
.send({'action_type': 'flag'}) | |||
.send({'user_id': '456', 'action_type': 'flag'}) |
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 shouldn't actually do anything, should revert this line.
@losowsky confirmed the need to close: We should always have the ability to close the box (currently if you're in the flow, there's not way to get out.) Clicking off the box should always close it (it doesn't close now if you're on step 2.) |
What does this PR do?
Allows a user to flag a username or comment and provide details.
How do I test this PR?
/v1/api/comments/:comment_id/actions
or/v1/api/users/:user_id/actions