Thread Spam UI + Cleanup/Refactor Thread Components

mzparacha commented 1 year ago

Link to Issue

Closes: https://github.com/hicommonwealth/commonwealth/issues/3977

Description of Changes

Added thread spam ui on the discussion page with spam toggle and integrated api
Added thread spam ui on the view threads page with spam toggle for comments and integrated api for marking thread/comment as spam
Fixed minor ui bugs along the process
Dried out multiple components that rendered a similar ui
Move complex business logic / react state to components higher in the tree that most need it -- made small components presentational

Test Plan

On the /discussions page

[ ] Verify the thread card and spam filter toggle ui matches figma
[ ] Verify the thread card actions work as they did before
[ ] Verify that marking the thread as spam will actually mark it as spam and thread card will update ui with spam label, the 'flag as spam' option should not be there in the card options any more
[ ] Verify that toggling the 'Include spam' filter will show/hide spam threads
[ ] Some options were not available before on this page like 'edit', 'edit collaborators', 'snapshot proposal from thread', 'update status', 'change topic' - these were only on the details page - Verify that these options work as on the details page.
[ ] Hovering over the admin icon and name did not show the 'ban user' tooltip, verify that it shows and works as an admin.
[ ] Verify that pinning/locking and subscribing adds/updates icons in the thread.

On the /discussions/:threadId page

[ ] Verify the thread header, comment tree and other components related to spam ui matches figma
[ ] Verify the thread actions work as they did before
[ ] Verify that marking the thread as spam will actually mark it as spam and thread card will update ui with spam label, the 'flag as spam' option should not be there in the options any more
[ ] Verify that toggling the 'Include spam' filter will show/hide spam comments
[ ] Some options were not available before on this page like 'update status' and 'pin thread' - these were only on the main discussion page - Verify that these options work as on the main discussions page.
[ ] Verify that pinning/locking and subscribing adds/updates icons in the thread.
[ ] Verify that marking a nested comment as spam will hide that specific comment and the comments nested inside it.

[Future] Deployment Plan

I would suggest we do proper component refactoring to any components we touch. Basically not pass extra state setters/getters to multiple component directly but via properly named props. Also dry out code where ever possible, if there are uncommon parts to a frequently common component we can use props to handle these things instead of replicating components. In the PR we did some refactoring to move the complex state up to components that most need it, so the smaller the component is the less complex it is.

Other Considerations

N/A

mzparacha commented 1 year ago

@jnaviask Api integrated.. Now adding the frontend filters, these things

mzparacha commented 1 year ago

@jnaviask @CowMuon @meeshlin Ready for review!

jnaviask commented 1 year ago

cc @mzparacha

No way to unflag threads or comments as spam? This could be a server-related issue. But is it part of the designs?
Not sure how to put this clearly but the new dropdown feels... laggy? On my machine at least. The hover states don't appear immediately.
Question for @meeshlin @zakhap: should we show the "include comments/threads marked as spam" button if there aren't any to view? Or, how will a viewing user know that marked-as-spam content exists?

mzparacha commented 1 year ago

@jnaviask, Thanks for the review.

No way to unflag threads or comments as spam? This could be a server-related issue. But is it part of the designs?

Actually the design does have unflag option, I have added that option back in UI, minor api adjustments needed to unflag, lmk if i should go ahead for those...

Not sure how to put this clearly but the new dropdown feels... laggy? On my machine at least. The hover states don't appear immediately.

Thanks for the raising the concern. I tried again but seems to work fine for me, maybe it only happens in specific conditions. My guess is the browser was using a lot of memory, maybe a big community with a large list of threads...? Can you please confirm if it happens in a small community?

https://github.com/hicommonwealth/commonwealth/assets/51641047/362cf1e2-d72a-4320-85b3-570ab16db985

jnaviask commented 1 year ago

@mzparacha I'm concerned the server routes don't permit unflagging -- so maybe we should confirm unflagging works. Will test performance again later on.

mzparacha commented 1 year ago

@jnaviask atm yes, api doesn't have the unflag logic. I believe these will be simple changes like removing the date here on some parameters, like a isSpam bool etc..

jnaviask commented 1 year ago

@rbennettcw when you get a minute, would it be possible to add the server-side support for unflagging spam? We'll work off this PR as the feature branch for the time being; the update is theoretically so small that I don't think it merits another PR.

rbennettcw commented 1 year ago

Done– I'm just wrestling with an expired github token issue. Can push branch, but doesn't show up on the origin branch.

rbennettcw commented 1 year ago

Alright, pushed the new endpoints to unmark threads/comments as spam.

jnaviask commented 1 year ago

cc @mzparacha to finish up the front-end

mzparacha commented 1 year ago

@jnaviask Done!

jnaviask commented 1 year ago

Looks like some pagination issues on e.g. /dydx, where the slowdown is because it's loading ALL threads regardless of my scroll position... Can't even really test it because of that (for reference, I have occasionally seen similar behavior elsewhere. Need to make sure our pagination patterns are correct).

meeshlin commented 1 year ago

Question for @meeshlin @zakhap: should we show the "include comments/threads marked as spam" button if there aren't any to view? Or, how will a viewing user know that marked-as-spam content exists?

We should keep things consistent and it should always be present even if there's no spam.

jnaviask commented 12 months ago

cc @mzparacha to fix merge conflicts

mzparacha commented 12 months ago

seems like tests unrelated to the pr are breaking. @jnaviask are we in the process of updating unit tests somewhere?

jnaviask commented 12 months ago

@mzparacha it's a flaky e2e test, cc @kurtisassad

jnaviask commented 12 months ago

@mzparacha please fix merge conflicts -- I do want to get this PR in soon

jnaviask commented 12 months ago

Tried doing the merge conflicts -- looks like they caused some trouble. Couldn't flag comments as spam anymore, discussions page layout got messed up when I flagged threads as spam, etc.

mzparacha commented 12 months ago

@jnaviask merge conflicts fixed. Please lmk if any issues.

jnaviask commented 12 months ago

A few more issues:

TypeError: onProposalStageChange is not a function when trying to change stage via while on a single discussion page
changing stages doesn't move the thread immediately when filtering by stage

mzparacha commented 12 months ago

These are fixed but i found another issue (maybe also a merge conflict), -> using any option for a thread list from the list page causes a bunch of bulkThreads call. Will fix it

jnaviask commented 12 months ago

cc @mzparacha to fix merge conflicts again :(

mzparacha commented 12 months ago

Will do this today cc: @jnaviask

mzparacha commented 11 months ago

Working on this

mzparacha commented 11 months ago

Just noticed this issue is also on main, so probably not related to this PR

Reproduction steps.

Create a community (open devtools network tab)
Create a single thread
Now mark that thread as pinned/locked or any option from the 3-dots menu
Notice the network tab - tons of unnecessary non-stop api calls

Alternate steps to reproduce

Go to a community with a lot of threads (you must be admin of that community)
Now scroll to the bottom until no more threads are left
Mark the last threads as pinned
Notice the network tab - tons of unnecessary non-stop api calls

I believe this is because we loaded all the threads and its trying to load more when there aren't any more. Finding a way to fix it in this pr...

cc: @CowMuon

mzparacha commented 11 months ago

I think an ideal solution to fix this bug would be to introduce a new proper pagination param to bulkThreads for the applied filter, like lets say we have this filter

?limit=20&page=0&chain=dydx&includePinnedThreads=true&to_date=2023-06-16T19%3A20%3A17.463Z&orderBy=createdAt%3Adesc

we and return this as response

{
    results: [
        // ... thread objects here
    ],
    page: 0, // the current page
    per_page: 10, // how many records are we returning per page
    total_results: 1000, // how many results we have in total for the applied filter
    total_pages: 100, // how many pages there can be for the total thread if page_page is x
}

Maybe a followup ticket would be best here since this issue is also on production. Thoughts?

We will solve the bug by using total_results or total_pages in frontend and when we have reached this number further api calls for the current filter will be canceled

cc: @CowMuon @rbennettcw

rbennettcw commented 11 months ago

@mzparacha Here's some context on what the refactor will look like for the "get threads" endpoints: https://github.com/hicommonwealth/commonwealth/issues/4282#issuecomment-1595205743

Ultimately, you can do whatever you want to the bulkThreads endpoint because I'm not changing the logic for it– just moving it to the controller. You can add whatever query params you need as well.

mzparacha commented 11 months ago

Thank you for the update @rbennettcw.

@CowMuon I will do the changes in this pr

mzparacha commented 11 months ago

Ok so i was working on this but this got a little complex as it involved some state update for threads, we have to basically restructure the frontend thread state to match the updated bulkThreads response for use of proper pagination params and thread state.

The core issue here is that the Virtuoso list will trigger unnecessary api calls when any option from thread card is used (from the three dots menu in thread card) which ideally should be fixed in Virtuoso but we still needed pagination params anyway so I added those to /bulkThreads api, which can fix the Virtuoso bug -- we can look for total_results param of pagination and cancel the endReached callback by Virtuoso that causes the bug.

The current status is to refactor the frontend thread state to also hold pagination params which would need update in every other place that uses threads.

This could be fixed in this PR but its totally unrelated as its also in production. I suggest we make a separate ticket to deal with this issue just to not keep this feature waiting but it could be done in this pr as well (will need more time).

I am a little undecided now if i should fix it in this pr or a followup ticket, please lmk what you think @CowMuon.

mzparacha commented 11 months ago

@CowMuon test are now passing. The issue was with the amount of bundles in the project build. Apparently pull/4276 introduced new tests. In the tests it had hard coded value client_scripts_views_pages_landing_index_tsx.5d36c013.chunk.js while the new chuck name after my pr is client_scripts_views_pages_landing_index_tsx.fa2f849a.chunk.js. I updated the test to use regex instead of a hard coded value, now it will work fine in our case and in future scenarios too

mzparacha commented 11 months ago

Tests are passing locally for me, by adding a console log, so I think the failing test is due to some cache somewhere

mzparacha commented 11 months ago

Re-ran the tests and now they pass.

CowMuon commented 11 months ago

Tests are looking good when I run them, so that's fine. I do want to make sure we are fully aligned on this approach (looking for sign off here from @rbennettcw) as our long term solution. Assuming there's nothing but yes in that regard, would like to get this in today.

rbennettcw commented 11 months ago

Pagination in this PR looks fine to me. Having the API return pagination metadata (page num, total num items, etc.) helps simplify UI state, and is something we should do across all API endpoints.

We should create a wiki to document our standard approach to pagination terminology + data structures.

CowMuon commented 11 months ago

This should be ready to go in as soon as we catch this conflict with threads refactor (oops, bulkthreads.ts)

mzparacha commented 11 months ago

@CowMuon Ready to go....

For context: Apparently, the threads API refactor (great work by Ryan - kudos) consolidated thread APIs, I had updated the /bulkThreads to return a new column in this PR. Now updated the new threads API (refactored one) with the changes I did in this pr.

hicommonwealth / commonwealth