Skip to content
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

Fix/comments bug #359

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Fix/comments bug #359

wants to merge 7 commits into from

Conversation

Mxchaeltrxn
Copy link
Collaborator

@Mxchaeltrxn Mxchaeltrxn commented Oct 11, 2020

Addresses #304

Changes made
Frontend

  • Change 'Reply' to 'Comment in Thread'.
  • Add 'Delete' and 'Block User' button to thread comments.
  • Update 'Delete' button pop up for thread comments.
  • Number of comments decreases immediately when you delete a comment (not sure if this is possible).
  • Hide the 'Block User' button for a user's own comments.

Backend

  • Delete thread comment functionality.
  • Setting blocked users.

Bugs noted:

  1. When a user is blocked, it's not possible to unblock them. There's some code that sets isBlocked in generateCommentsObjects.js but it has been commented out. I didn't want to fix this because I'm not sure what the deal is with the commented code.
  2. Should line 731 in internalApi.js be
// Note the '||' instead of '&&'
function deleteComment() {
    ...
    if(!userIsUploader || !userIsCommenter){
      throw new Error('not the proper user');
    }
    ...
}

@Mxchaeltrxn
Copy link
Collaborator Author

I've updated the threadComment variable to be called commentInThread in my latest commit @mayeaux

@BassOfBass
Copy link
Collaborator

In regards to:

Number of comments decreases immediately

There are Node.removeChild() and ChildNode.remove() methods which allow to nuke DOM nodes and rerender the tree. Ofc it should wait for the server response before the actual removal.

So it would go like this:

  • user pushes the delete button
  • all relevant validation checks go through
  • the server returns a response
  • node removal is ran in case of successful response

As for

Hide the 'Block User' button for a user's own comments.

Wouldn't an _id comparison between the person deleting the comment and the _id of the comment suffice for that? Though sending _id to the front is kinda sketchy to begin with, but that's what the code does.

Also use the unbuffered comments please.

@Mxchaeltrxn
Copy link
Collaborator Author

@BassOfBass Thanks for the comment.

I'm not too familiar with server side rendering but the number of comments is a variable (commentCount) that gets passed into the pug template from the backend. It doesn't seem to change even when I remove the node. Only a page refresh seems to work (e.g. location.reload()) which is not ideal because it brings the user back to the top of the page.

The block user functionality is there but commented so I'm not sure what I should be doing with the commented code.

I'll use unbuffered comments after your pr gets merged. That way, it'll be easy for me to know which buffered comments are mine

@BassOfBass
Copy link
Collaborator

I'm not too familiar with server side rendering but the number of comments is a variable (commentCount) that gets passed into the pug template from the backend.

That's because media.pug is a big mess. A ton of inline <script> inserts (a lot of them are done through partials too, so it's even harder to get a hang of) and even more global variables inserted through them.

commentCount is just a number, you can't rely on it for DOM manipulation. comments is the one you'd need, though if it isn't stored, you'll have to collect them off the page in a NodeList.

There are 3 files responsible for comment section:

/api/comment/delete is the end-point responsible for comment deletion, which invokes deleteComment() method.
It looks like the route already performs all needed auth checks, so you only need to fetch() a "POST" response to it as a result of pressing the button and, depending on the outcome, either remove the comment from the the page or return an error.

The main hurdle there is you'll also need to surgically remove related jQuery logic, which is all over the place.

@Mxchaeltrxn
Copy link
Collaborator Author

Thanks for the detailed breakdown.

I will leave it for now because it just seems too complicated for me to handle right now but this information will be useful for anyone who decides to implement it in the future anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants