Conversation
Oaphi
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files
☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
cellio
left a comment
cellio
left a comment
There was a problem hiding this comment.
Looks good so far, based on inspection. (I won't be able to test for a few days.) I see that this is marked as a draft so I assume more changes are coming (hence "so far" and not hitting the approval button yet).
Yup, at least one more change is coming - I closed off the ability to self-review suggested edits in restricted categories client-side, but not server-side yet, which is more important - will switch out of draft mode afterwards. |
…ted edit review access control checks
…pprove? & can_reject? (fixes users being able to act on their own edits where they shouldn't)
Oaphi
force-pushed
the
0valt/1605/restricted-edits
branch
from
d3cecb3 to
e95977e
Compare
|
I'll leave the missing coverage line be as it's on the code path that normally should not even possible to reach - not worth the effort (especially given it just fixes the notification). |
| edit = suggested_edits(:pending_high_trust) | ||
|
|
||
| post :approve, params: { id: edit.id, format: 'json' } | ||
| assert_response(:bad_request) |
There was a problem hiding this comment.
Q: why is this a bad request? Would a comment help or is it just my inexperience? (Non-blocking.)
There was a problem hiding this comment.
No reason why as far as I am aware, wondered about that myself. It's not correct to return 400 for access control errors (should be 401, 403 [the most appropriate candidate here], or 404 when AC prevents the client from even knowing the requested resource exists), yes.
I chose not to change that to avoid making unrelated changes just for the sake of correctness - not opposed to doing so, though
There was a problem hiding this comment.
I agree with not doing that here. That's a separate issue and might affect other tests too. Thanks for explaining.
This PR looks good to me by inspection. I'm not currently in a position to test; approving with that caveat and I see Art has also reviewed.
There was a problem hiding this comment.
@cellio I didn't mean I won't fix them right here xD
…ded against missing redirect_url
3 participants
closes #1605
With this change, even users with the ability to edit the post will only be able to submit suggested edits unless they meet category requirements (if it's present) [both screenshots show a post with a pending suggested edit made by the user lacking access]:
And this is how the same post looks like for a user with appropriate access:
This PR also adds a couple of partials to make it easier to work with our rather complex view logic when it comes to editing.
Additionally, the PR fixes broken error notice when attempting to reject a suggested edit without access ("Failure: undefined" due to what is likely just a typo).
Finally, it ensures user trust level is recalculated when manually granting / revoking user roles (currently, it is not updated).
On a tangent: we should review whether we still need the
check_your_post_privilegehelper (it's legacy and significantly differs from server-side checks at this point)