-
Notifications
You must be signed in to change notification settings - Fork 89
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
🍰 1062 notification about comment on post #1270
Conversation
…nnection into 1062-Notification-about-comment-on-post
…//github.com/Human-Connection/Human-Connection into 1062-notification-about-comment-on-post
Used this for displaying in the mentions menu in frontend. Rewrite backend test.
…nnection into 1062-notification-about-comment-on-post # Conflicts: # backend/src/middleware/handleNotifications/handleNotifications.spec.js
…//github.com/Human-Connection/Human-Connection into 1062-notification-about-comment-on-post # Conflicts: # backend/src/middleware/handleNotifications/handleNotifications.js # backend/src/middleware/handleNotifications/handleNotifications.spec.js # webapp/components/notifications/Notification/Notification.vue # webapp/components/notifications/NotificationMenu/NotificationMenu.vue
Rename files. Co-Authored-By: mattwr18 <mattwr18@gmail.com>
…nnection into 1062-notification-about-comment-on-post
…nnection into 1062-notification-about-comment-on-post # Conflicts: # backend/src/middleware/handleHtmlContent/handleContentData.js
…nnection into 1062-notification-about-comment-on-post # Conflicts: # backend/src/middleware/handleNotifications/handleNotificationsMiddleware.spec.js Refactored there tests a little
const session = context.driver.session() | ||
const createdAt = new Date().toISOString() | ||
let cypher | ||
switch (reason) { |
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.
Now, this cypher statement is getting messy. Shall I jump in an try to refactor?
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.
that would be great @roschaefer, but @Tirokk has changes to the branch to push up, could you wait some hours?
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 the code is pushed up. @roschaefer
I and @mattwr18 would like to have pairing with you about this.
May @ogerly and @alina-beck like to join it as well?
…nnection into 1062-notification-about-comment-on-post
backend/src/middleware/handleNotifications/handleNotificationsMiddleware.js
Outdated
Show resolved
Hide resolved
backend/src/middleware/handleNotifications/handleNotificationsMiddleware.js
Outdated
Show resolved
Hide resolved
AND NOT (author)<-[:BLOCKED]-(user) | ||
CREATE (notification: Notification {id: apoc.create.uuid(), read: false, reason: $reason, createdAt: $createdAt }) | ||
MERGE (comment)-[:NOTIFIED]->(notification)-[:NOTIFIED]->(user) | ||
` |
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.
Are the mentioned_in_comment
and comment_on_post
cypher statements the same, or am I missing something?
the $reason
would be different, right? but since it's passed in as a variable that would be ok to use the same cypher statement, no?
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.
oh, this is different, no
AND NOT (user)<-[:BLOCKED]-(author)
AND NOT (author)<-[:BLOCKED]-(user)
right?
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.
Yes, this is different. 😉
…nnection into 1062-notification-about-comment-on-post # Conflicts: # backend/src/middleware/index.js
Co-Authored-By: mattwr18 <mattwr18@gmail.com>
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.
hey this is great @Tirokk!! as we've talked about, and you agree with, it would be great to refactor the cypher statement, but since the functionality works well, I'm approving this, and we can refactor in a separate ticket. We also talked about refactoring the notifications.spec.js as well.
Job on ya for adding component tests that somehow didn't get added in the last PR
about notifications 😺
return record.get('user') | ||
}) | ||
if (context.user.id !== postAuthor.id) { | ||
await notifyUsers('Comment', comment.id, [postAuthor.id], 'comment_on_post', context) |
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.
Is it possible that you cannot mention another user when you create a comment?
backend/src/middleware/handleNotifications/handleNotificationsMiddleware.js
Outdated
Show resolved
Hide resolved
…nnection into 1062-notification-about-comment-on-post
…d `hashtagsMiddleware`
🍰 Pullrequest
Create Notifications if a User writes a Comment to your Post.
Issues
Todo