-
Notifications
You must be signed in to change notification settings - Fork 143
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
GraphQL - Post Lock #875
GraphQL - Post Lock #875
Conversation
Will need to be adjusted for #874 |
Yup, will let @NuckChorris merge in first and then fix mine up. |
345d7e4
to
84ee233
Compare
post = ::Post.find(value.id) | ||
post.assign_attributes(value.to_model) | ||
post |
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 distinctly not a fan of this thing where you get the changes from some other file and then shove them in over here... It seems like indirection without purpose
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.
Funny enough this is how it was originally done (by Matt). I liked it so I kinda just started using it also lol.
post.save | ||
|
||
if post.errors.any? | ||
Errors::RailsModel.graphql_error(post) | ||
else | ||
{ | ||
post: post | ||
} | ||
end |
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.
FWIW I added an Errors::ActiveRecord::RecordInvalid
in #880 that should do this for us in future with post.save!
We can change it once both are merged
def to_model | ||
to_h.merge(locked_at: DateTime.current, locked_by: current_user) | ||
end |
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 feels way too magical for me
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.
fix typo Co-authored-by: Peter Lejeck <[email protected]>
…u-server into feature/post-locking
Code Climate has analyzed commit 5e9c031 and detected 3 issues on this pull request. Here's the issue category breakdown:
View more on Code Climate. |
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.
LGTM from a feature perspective for frontend. Will need Emma's review for code quality
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.
My comments still stand, I'd like to see improvements to the way we do GraphQL — but for now, this works
What
Allow posts to be locked. When a post is locked only admin will be able to make comments.
Locking/Unlocking a post will be done through a GraphQL Mutation.
Why
Certain people don't know how to be nice to others. 🖕 them.
Checklist