-
-
Notifications
You must be signed in to change notification settings - Fork 925
Hide community v2 #2055
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
Hide community v2 #2055
Conversation
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.
Sry I'm slow on this, been busy with a few other things.
Just a few changes then I'll do testing.
…ommunity_view. Dont force non null for reason
|
Think I got all these requested changes in the latest commit. I'll look over it later a bit more before I request another review. |
|
Also lint is failing, run |
|
Think I straightened out all those things. Unless something minor was left in a match statement. Went ahead and requested re review. I'll look over it a bit later if no one reviews it tomorrow. |
|
changed column and tested still working. |
|
@dessalines Bump |
|
Thanks @dessalines Those are fixed. New checks running |
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.
Looks good, but I'd like nutomic to do a pass over it as well.
| updated: Some(naive_now()), | ||
| hidden: Some(data.hidden), | ||
| ..CommunityForm::default() | ||
| }; |
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 dont think there is any reason to update icon/banner/nsfw here.
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.
These are mainly my fault, we need to make sure all the nullable fields in every form are Option<Option<..., but that can be a separate PR.
| let op = UserOperationCrud::EditCommunity; | ||
| send_community_ws_message(data.community_id, op, websocket_id, None, context).await | ||
| } | ||
| } |
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.
Unrelated to this PR, but later we really need to find a diffent (shorter) way to handle all this. If changing a single field needs so much code, it creates way too much bloat.
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 can always create specific helper functions rather than use the full update version, but I'd rather just do defaults correctly and do the thing above.
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.
Sorry i missed the notification somehow, just got a minor note. Thanks for the contribution!
Wanted a cleaner PR because I messed up prev branch with a rebase.
Fixing problems found with,
#2017
And then #2032
For issue #1980
This PR does the following,
Still reviewing it so I will ask for a code Review in the chat. Might make some cleanups but ran it through a few simple test casesRan it over test cases looked over the code, asked some questions below but I think this is mostly ready for review.